From ae2dd97d54deedf72f5f2cc9c6186b3fd6c7a8f2 Mon Sep 17 00:00:00 2001
From: vsadov <8218165+VSadov@users.noreply.github.com>
Date: Wed, 26 Apr 2023 17:43:43 -0700
Subject: [PATCH 1/6] Allow reverse pinvoke in DoNotTriggerGc thread state
regardless of coop mode.
---
src/coreclr/nativeaot/Runtime/EHHelpers.cpp | 9 ++++-----
src/coreclr/nativeaot/Runtime/thread.cpp | 20 ++++++++++++--------
2 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp b/src/coreclr/nativeaot/Runtime/EHHelpers.cpp
index 37d593b6c7afc5..e9370948d541a3 100644
--- a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp
+++ b/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();
diff --git a/src/coreclr/nativeaot/Runtime/thread.cpp b/src/coreclr/nativeaot/Runtime/thread.cpp
index cd470e479e3882..ec4685517c4214 100644
--- a/src/coreclr/nativeaot/Runtime/thread.cpp
+++ b/src/coreclr/nativeaot/Runtime/thread.cpp
@@ -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())
{
- 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());
+ // make transition to coop mode without waiting for GC (noop if already in coop mode)
+ pFrame->m_savedPInvokeTransitionFrame = m_pTransitionFrame;
+ m_pTransitionFrame = NULL;
+ return true;
+ }
+ if (IsCurrentThreadInCooperativeMode())
+ {
return false; // bad transition
}
@@ -1233,6 +1236,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);
From 8da1db5774861e8779abbfef8707a61d4455accc Mon Sep 17 00:00:00 2001
From: vsadov <8218165+VSadov@users.noreply.github.com>
Date: Wed, 26 Apr 2023 23:43:31 -0700
Subject: [PATCH 2/6] do not attach DoNotTriggerGc threads in reverse pinvoke
---
src/coreclr/nativeaot/Runtime/thread.cpp | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/coreclr/nativeaot/Runtime/thread.cpp b/src/coreclr/nativeaot/Runtime/thread.cpp
index ec4685517c4214..fae458bb0c3e13 100644
--- a/src/coreclr/nativeaot/Runtime/thread.cpp
+++ b/src/coreclr/nativeaot/Runtime/thread.cpp
@@ -1125,10 +1125,6 @@ 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
-
// 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
@@ -1144,6 +1140,10 @@ FORCEINLINE bool Thread::InlineTryFastReversePInvoke(ReversePInvokeFrame * pFram
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
From 0a93e25d0dedf045b2c9d37826489b78810e9b0c Mon Sep 17 00:00:00 2001
From: vsadov <8218165+VSadov@users.noreply.github.com>
Date: Wed, 26 Apr 2023 23:44:17 -0700
Subject: [PATCH 3/6] propagate useful msbuild arguments to the build
---
src/tests/build.proj | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/tests/build.proj b/src/tests/build.proj
index 7f980b6cf4c2fb..7a5f19d9ec95b8 100644
--- a/src/tests/build.proj
+++ b/src/tests/build.proj
@@ -581,6 +581,8 @@
$(GroupBuildCmd) "/p:PackageOS=$(PackageOS)"
$(GroupBuildCmd) "/p:RuntimeFlavor=$(RuntimeFlavor)"
$(GroupBuildCmd) "/p:RuntimeVariant=$(RuntimeVariant)"
+ $(GroupBuildCmd) "/p:ServerGarbageCollection=$(ServerGarbageCollection)"
+ $(GroupBuildCmd) "/p:StripSymbols=$(StripSymbols)"
$(GroupBuildCmd) "/p:CLRTestBuildAllTargets=$(CLRTestBuildAllTargets)"
$(GroupBuildCmd) "/p:UseCodeFlowEnforcement=$(UseCodeFlowEnforcement)"
$(GroupBuildCmd) "/p:__TestGroupToBuild=$(__TestGroupToBuild)"
From 1295aa3fdac16e8a41ec2473fde9f1b5e4270d93 Mon Sep 17 00:00:00 2001
From: vsadov <8218165+VSadov@users.noreply.github.com>
Date: Thu, 27 Apr 2023 00:03:37 -0700
Subject: [PATCH 4/6] more PR feedback.
---
src/coreclr/nativeaot/Runtime/thread.cpp | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/coreclr/nativeaot/Runtime/thread.cpp b/src/coreclr/nativeaot/Runtime/thread.cpp
index fae458bb0c3e13..5a007cefef99d5 100644
--- a/src/coreclr/nativeaot/Runtime/thread.cpp
+++ b/src/coreclr/nativeaot/Runtime/thread.cpp
@@ -1125,6 +1125,9 @@ EXTERN_C NATIVEAOT_API uint32_t __cdecl RhCompatibleReentrantWaitAny(UInt32_BOOL
FORCEINLINE bool Thread::InlineTryFastReversePInvoke(ReversePInvokeFrame * pFrame)
{
+ // 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
@@ -1134,9 +1137,7 @@ FORCEINLINE bool Thread::InlineTryFastReversePInvoke(ReversePInvokeFrame * pFram
{
// 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;
- m_pTransitionFrame = NULL;
+ // no need to do anything
return true;
}
@@ -1153,9 +1154,6 @@ FORCEINLINE bool Thread::InlineTryFastReversePInvoke(ReversePInvokeFrame * pFram
// 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);
@@ -1236,7 +1234,7 @@ FORCEINLINE void Thread::InlineReversePInvokeReturn(ReversePInvokeFrame * pFrame
FORCEINLINE void Thread::InlinePInvoke(PInvokeTransitionFrame * pFrame)
{
- _ASSERT(!IsDoNotTriggerGcSet() || ThreadStore::IsTrapThreadsRequested());
+ ASSERT(!IsDoNotTriggerGcSet() || ThreadStore::IsTrapThreadsRequested());
pFrame->m_pThread = this;
// set our mode to preemptive
VolatileStoreWithoutBarrier(&m_pTransitionFrame, pFrame);
From 8e08e04343804ee5516a75088d7479844eb19f7b Mon Sep 17 00:00:00 2001
From: vsadov <8218165+VSadov@users.noreply.github.com>
Date: Thu, 27 Apr 2023 00:10:12 -0700
Subject: [PATCH 5/6] a nit - more consistent formatting
---
src/coreclr/nativeaot/Runtime/thread.cpp | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/coreclr/nativeaot/Runtime/thread.cpp b/src/coreclr/nativeaot/Runtime/thread.cpp
index 5a007cefef99d5..6f599b97c9eb25 100644
--- a/src/coreclr/nativeaot/Runtime/thread.cpp
+++ b/src/coreclr/nativeaot/Runtime/thread.cpp
@@ -1146,9 +1146,7 @@ FORCEINLINE bool Thread::InlineTryFastReversePInvoke(ReversePInvokeFrame * pFram
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
From 44254ed568508ac946f3057c84ab38c11199b597 Mon Sep 17 00:00:00 2001
From: vsadov <8218165+VSadov@users.noreply.github.com>
Date: Thu, 27 Apr 2023 09:03:19 -0700
Subject: [PATCH 6/6] Do not pass through StripSymbols
---
src/tests/build.proj | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/tests/build.proj b/src/tests/build.proj
index 7a5f19d9ec95b8..e0b7398f1d7a5f 100644
--- a/src/tests/build.proj
+++ b/src/tests/build.proj
@@ -581,8 +581,7 @@
$(GroupBuildCmd) "/p:PackageOS=$(PackageOS)"
$(GroupBuildCmd) "/p:RuntimeFlavor=$(RuntimeFlavor)"
$(GroupBuildCmd) "/p:RuntimeVariant=$(RuntimeVariant)"
- $(GroupBuildCmd) "/p:ServerGarbageCollection=$(ServerGarbageCollection)"
- $(GroupBuildCmd) "/p:StripSymbols=$(StripSymbols)"
+ $(GroupBuildCmd) "/p:ServerGarbageCollection=$(ServerGarbageCollection)"
$(GroupBuildCmd) "/p:CLRTestBuildAllTargets=$(CLRTestBuildAllTargets)"
$(GroupBuildCmd) "/p:UseCodeFlowEnforcement=$(UseCodeFlowEnforcement)"
$(GroupBuildCmd) "/p:__TestGroupToBuild=$(__TestGroupToBuild)"