Fix calling convention mismatch in GC callouts #110685
Conversation
Fixes dotnet#110607. The native side expects fastcall. Filed dotnet#110684 on the test hole. We would have caught it during x86 bringup if this had _any_ tests since this is a guaranteed stack corruption and crash.
There was a problem hiding this comment.
Okay I went looking through the code and found something very interesting. It looks like when I did the unmanaged calling-convention support rewrite in the JIT, I implemented fastcall correctly.
However, as we have never supported it for P/Invokes, our exceptions that block fastcall are P/Invoke-only. Additionally, the exceptions are only thrown in CoreCLR (NativeAOT likely fails in a more inscrutible way).
In any case, looks like Fastcall + UnmanagedCallersOnly should work here just fine.
.../System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs
Outdated
Show resolved
Hide resolved
I did test this and it worked. Looking through our test tree, I don't see any test exercising Fastcall though. Do we have any testing for this? Should we have testing? It doesn't look like CoreCLR VM blocks UnmanagedCallersOnly with fastcall either. Either way, I made this use not-fastcall per Jan's request. |
Do any of those call into managed code? We're not discussing test coverage of fastcall in the Visual C++ compiler but |
|
This is fixed in main. I consider backport to 9.0 blocked on #110684 - I don't want to be laughed out of the shiproom when I say "no we don't have test coverage, but we do have a concept of a plan for it". |
|
Would it be sufficient to validate this on both x86 and x64 with the Store? We won't be able to ship to x86 with Native AOT otherwise. In theory we could keep that target on .NET Native, but it'd be simpler for us if we could migrate all 3 targets. And would the fact x86 right now is flat out broken on NAOT in XAML scenario help make the case for servicing? |
|
If you can validate the fix with Store, it should be good enough test coverage for servicing backport. |
|
I tested this on x64 and it works fine (except for #110823, but that's unrelated). Currently blocked by other dependencies on testing with x86, but I hope to get that resolved by end of this week, hopefully. Will report back as soon as I can also validate on x86 and confirm that things work correctly, and then hopefully we can backport this too 🙂 |
The bug can only be observed on x86. There's no fastcall or stdcall on x64, there's only one calling convention. This PR is essentially no-op on x64 or arm64. |
|
Oh I know, but I figured it wouldn't hurt to validate everything end to end too just in case, to make things a bit simpler for tactics to backport this. Also until we unblock x86 in the Store it was the only thing I could do 😅 |
Fixes #110607.
The native side expected fastcall, managed side wasn't fastcall.
Filed #110684 on the test hole. We would have caught it during native AOT x86 bringup if this had any tests since this is a guaranteed stack corruption and crash.
Cc @dotnet/interop-contrib