[NativeAOT] Allow reverse pinvoke in DoNotTriggerGc thread state regardless of coop mode. (#85435)

* Allow reverse pinvoke in DoNotTriggerGc thread state regardless of coop mode.

* do not attach DoNotTriggerGc threads in reverse pinvoke

* propagate useful msbuild arguments to the build

* more PR feedback.

* a nit - more consistent formatting

* Do not pass through StripSymbols

Author: VSadov

src/coreclr/nativeaot/Runtime/EHHelpers.cpp

@@ -202,11 +202,10 @@ EXTERN_C int32_t __stdcall RhpPInvokeExceptionGuard(PEXCEPTION_RECORD       pExc
 
     Thread * pThread = ThreadStore::GetCurrentThread();
 
-    // If the thread is currently in the "do not trigger GC" mode, we must not allocate, we must not reverse pinvoke, or
-    // return from a pinvoke.  All of these things will deadlock with the GC and they all become increasingly likely as
-    // exception dispatch kicks off.  So we just address this as early as possible with a FailFast.  The most
-    // likely case where this occurs is in our GC-callouts for Jupiter lifetime management -- in that case, we have
-    // managed code that calls to native code (without pinvoking) which might have a bug that causes an AV.
+    // A thread in DoNotTriggerGc mode has many restrictions that will become increasingly likely to be violated as
+    // exception dispatch kicks off. So we just address this as early as possible with a FailFast.
+    // The most likely case where this occurs is in GC-callouts -- in that case, we have
+    // managed code that runs on behalf of GC, which might have a bug that causes an AV.
     if (pThread->IsDoNotTriggerGcSet())
         RhFailFast();
 

src/coreclr/nativeaot/Runtime/thread.cpp

@@ -1125,34 +1125,33 @@ EXTERN_C NATIVEAOT_API uint32_t __cdecl RhCompatibleReentrantWaitAny(UInt32_BOOL
 
 FORCEINLINE bool Thread::InlineTryFastReversePInvoke(ReversePInvokeFrame * pFrame)
 {
-    // Do we need to attach the thread?
-    if (!IsStateSet(TSF_Attached))
-        return false; // thread is not attached
+    // remember the current transition frame, so it will be restored when we return from reverse pinvoke
+    pFrame->m_savedPInvokeTransitionFrame = m_pTransitionFrame;
 
     // If the thread is already in cooperative mode, this is a bad transition that will be a fail fast unless we are in
     // 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())
     {
-        if (IsDoNotTriggerGcSet())
-        {
-            // RhpTrapThreads will always be set in this case, so we must skip that check.  We must be sure to
-            // zero-out our 'previous transition frame' state first, however.
-            pFrame->m_savedPInvokeTransitionFrame = NULL;
-            return true;
-        }
+        // We expect this scenario only when EE is stopped.
+        ASSERT(ThreadStore::IsTrapThreadsRequested());
+        // no need to do anything
+        return true;
+    }
 
+    // Do we need to attach the thread?
+    if (!IsStateSet(TSF_Attached))
+        return false; // thread is not attached
+
+    if (IsCurrentThreadInCooperativeMode())
         return false; // bad transition
-    }
 
     // this is an ordinary transition to managed code
     // GC threads should not do that
     ASSERT(!IsGCSpecial());
 
-    // save the previous transition frame
-    pFrame->m_savedPInvokeTransitionFrame = m_pTransitionFrame;
-
     // must be in cooperative mode when checking the trap flag
     VolatileStoreWithoutBarrier(&m_pTransitionFrame, NULL);
 
@@ -1233,6 +1232,7 @@ FORCEINLINE void Thread::InlineReversePInvokeReturn(ReversePInvokeFrame * pFrame
 
 FORCEINLINE void Thread::InlinePInvoke(PInvokeTransitionFrame * pFrame)
 {
+    ASSERT(!IsDoNotTriggerGcSet() || ThreadStore::IsTrapThreadsRequested());
     pFrame->m_pThread = this;
     // set our mode to preemptive
     VolatileStoreWithoutBarrier(&m_pTransitionFrame, pFrame);

src/tests/build.proj

@@ -581,6 +581,7 @@
       <GroupBuildCmd>$(GroupBuildCmd) "/p:PackageOS=$(PackageOS)"</GroupBuildCmd>
       <GroupBuildCmd>$(GroupBuildCmd) "/p:RuntimeFlavor=$(RuntimeFlavor)"</GroupBuildCmd>
       <GroupBuildCmd>$(GroupBuildCmd) "/p:RuntimeVariant=$(RuntimeVariant)"</GroupBuildCmd>
+      <GroupBuildCmd Condition="'$(ServerGarbageCollection)' != ''">$(GroupBuildCmd) "/p:ServerGarbageCollection=$(ServerGarbageCollection)"</GroupBuildCmd>
       <GroupBuildCmd>$(GroupBuildCmd) "/p:CLRTestBuildAllTargets=$(CLRTestBuildAllTargets)"</GroupBuildCmd>
       <GroupBuildCmd>$(GroupBuildCmd) "/p:UseCodeFlowEnforcement=$(UseCodeFlowEnforcement)"</GroupBuildCmd>
       <GroupBuildCmd>$(GroupBuildCmd) "/p:__TestGroupToBuild=$(__TestGroupToBuild)"</GroupBuildCmd>