[NativeAOT] Allow reverse pinvoke in DoNotTriggerGc thread state regardless of coop mode. #85435
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsFixes: #85377 GC makes no promises about coop mode when GC callouts are invoked.
|
| @@ -1133,16 +1133,19 @@ FORCEINLINE bool Thread::InlineTryFastReversePInvoke(ReversePInvokeFrame * pFram | |||
| // a do not trigger mode. The exception to the rule allows us to have [UnmanagedCallersOnly] methods that are called via | |||
| // the "restricted GC callouts" as well as from native, which is necessary because the methods are CCW vtable | |||
| // methods on interfaces passed to native. | |||
| if (IsCurrentThreadInCooperativeMode()) | |||
| // We will allow threads in DoNotTriggerGc mode to do reverse PInvoke regardless of their coop state. | |||
| if (IsDoNotTriggerGcSet()) | |||
There was a problem hiding this comment.
I think we want to do this before the TSF_Attached check. We do not want to attach untached GC threads as a side-effect.
There was a problem hiding this comment.
Right, on server GC this could be unattached thread.
There was a problem hiding this comment.
is there a way to pass ServerGarbageCollection=true to src\tests\build.cmd ?
There was a problem hiding this comment.
Turns out src\tests\build.cmd can pass through paramters to msbuild, but src/tests/build.proj was not propagating some parameters that could be useful.
| ASSERT(ThreadStore::IsTrapThreadsRequested()); | ||
| // make transition to coop mode without waiting for GC (noop if already in coop mode) | ||
| pFrame->m_savedPInvokeTransitionFrame = m_pTransitionFrame; | ||
| m_pTransitionFrame = NULL; |
There was a problem hiding this comment.
| m_pTransitionFrame = NULL; |
This should not be need.
| // We expect this scenario only when EE is stopped. | ||
| ASSERT(ThreadStore::IsTrapThreadsRequested()); | ||
| // make transition to coop mode without waiting for GC (noop if already in coop mode) | ||
| pFrame->m_savedPInvokeTransitionFrame = m_pTransitionFrame; |
There was a problem hiding this comment.
We can do this as very first thing in the method to avoid duplication. It will make the code a bit smaller.
src/tests/build.proj
Outdated
| @@ -581,6 +581,8 @@ | |||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:PackageOS=$(PackageOS)"</GroupBuildCmd> | |||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:RuntimeFlavor=$(RuntimeFlavor)"</GroupBuildCmd> | |||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:RuntimeVariant=$(RuntimeVariant)"</GroupBuildCmd> | |||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:ServerGarbageCollection=$(ServerGarbageCollection)"</GroupBuildCmd> | |||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:StripSymbols=$(StripSymbols)"</GroupBuildCmd> | |||
There was a problem hiding this comment.
Looks like this makes the dwarf warnings test to fail
There was a problem hiding this comment.
Something up the stack sets StripSymbols and once propagated it breaks the test.
StripSymbols is a very useful setting if someone needs to debug tests, but we can deal with that later.
|
Thanks! |
Fixes: #85377
GC makes no promises about coop mode when GC callouts are invoked.
Background GC, for example, may invoke callouts in preemptive mode. Foreground GC with WKS will do it in coop.