Fix code version table lock / coop GC mode switch ordering (#25129)

Fixes https://github.com/dotnet/coreclr/issues/25086
- The lock is taken inside other unsafe locks in coop mode, in some paths it may not be legal to switch to preemptive mode before taking the lock
- Iterating over the cross loader allocator hash table to backpatch entry point slots needs coop GC mode, and was being done inside the lock
- Moved the switch to coop GC mode to before acquiring the lock to maintain consistent ordering between the two

diff --git a/src/vm/codeversion.cpp b/src/vm/codeversion.cpp
index 71ed248..6d7cbe8 100644 (file)
--- a/src/vm/codeversion.cpp
+++ b/src/vm/codeversion.cpp
@@ -2123,7 +2123,13 @@ HRESULT CodeVersionManager::SetActiveILCodeVersions(ILCodeVersion* pActiveVersio
 
     // step 3 - update each pre-existing method instantiation
     {
+        // Backpatching entry point slots requires cooperative GC mode, see
+        // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that
+        // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering
+        // must be used here to prevent deadlock.
+        GCX_COOP();
         TableLockHolder lock(this);
+
         for (DWORD i = 0; i < cActiveVersions; i++)
         {
             // Its possible the active IL version has changed if
@@ -2235,7 +2241,8 @@ PCODE CodeVersionManager::PublishVersionableCodeIfNecessary(MethodDesc* pMethodD
             pCode = pMethodDesc->PrepareCode(activeVersion);
         }
 
-        MethodDescBackpatchInfoTracker::ConditionalLockHolder lockHolder(pMethodDesc->MayHaveEntryPointSlotsToBackpatch());
+        bool mayHaveEntryPointSlotsToBackpatch = pMethodDesc->MayHaveEntryPointSlotsToBackpatch();
+        MethodDescBackpatchInfoTracker::ConditionalLockHolder lockHolder(mayHaveEntryPointSlotsToBackpatch);
 
         // suspend in preparation for publishing if needed
         if (fEESuspend)
@@ -2244,7 +2251,13 @@ PCODE CodeVersionManager::PublishVersionableCodeIfNecessary(MethodDesc* pMethodD
         }
 
         {
+            // Backpatching entry point slots requires cooperative GC mode, see
+            // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that
+            // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering
+            // must be used here to prevent deadlock.
+            GCX_MAYBE_COOP(mayHaveEntryPointSlotsToBackpatch);
             TableLockHolder lock(this);
+
             // The common case is that newActiveCode == activeCode, however we did leave the lock so there is
             // possibility that the active version has changed. If it has we need to restart the compilation
             // and publishing process with the new active version instead.
@@ -2298,7 +2311,7 @@ PCODE CodeVersionManager::PublishVersionableCodeIfNecessary(MethodDesc* pMethodD
                     break;
                 }
             }
-        } // exit lock
+        } // exit lock, revert GC mode
 
         if (fEESuspend)
         {
@@ -2318,9 +2331,29 @@ HRESULT CodeVersionManager::PublishNativeCodeVersion(MethodDesc* pMethod, Native
 {
     // TODO: This function needs to make sure it does not change the precode's target if call counting is in progress. Track
     // whether call counting is currently being done for the method, and use a lock to ensure the expected precode target.
-    WRAPPER_NO_CONTRACT;
+
+    CONTRACTL
+    {
+        GC_NOTRIGGER;
+
+        // Backpatching entry point slots requires cooperative GC mode, see MethodDescBackpatchInfoTracker::Backpatch_Locked().
+        // The code version manager's table lock is an unsafe lock that may be taken in any GC mode. The lock is taken in
+        // cooperative GC mode on other paths, so the caller must use the same ordering to prevent deadlock (switch to
+        // cooperative GC mode before taking the lock).
+        if (pMethod->MayHaveEntryPointSlotsToBackpatch())
+        {
+            MODE_COOPERATIVE;
+        }
+        else
+        {
+            MODE_ANY;
+        }
+    }
+    CONTRACTL_END;
+
     _ASSERTE(LockOwnedByCurrentThread());
     _ASSERTE(pMethod->IsVersionable());
+
     HRESULT hr = S_OK;
     PCODE pCode = nativeCodeVersion.IsNull() ? NULL : nativeCodeVersion.GetNativeCode();
     if (pMethod->IsVersionableWithoutJumpStamp())

diff --git a/src/vm/tieredcompilation.cpp b/src/vm/tieredcompilation.cpp
index d8f90e5..8cdfc48 100644 (file)
--- a/src/vm/tieredcompilation.cpp
+++ b/src/vm/tieredcompilation.cpp
@@ -745,12 +745,19 @@ void TieredCompilationManager::ActivateCodeVersion(NativeCodeVersion nativeCodeV
     // code version will activate then.
     ILCodeVersion ilParent;
     HRESULT hr = S_OK;
-    MethodDescBackpatchInfoTracker::ConditionalLockHolder lockHolder;
+    bool mayHaveEntryPointSlotsToBackpatch = pMethod->MayHaveEntryPointSlotsToBackpatch();
+    MethodDescBackpatchInfoTracker::ConditionalLockHolder lockHolder(mayHaveEntryPointSlotsToBackpatch);
 
     {
-        // As long as we are exclusively using precode publishing for tiered compilation
-        // methods this first attempt should succeed
+        // Backpatching entry point slots requires cooperative GC mode, see
+        // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that
+        // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering
+        // must be used here to prevent deadlock.
+        GCX_MAYBE_COOP(mayHaveEntryPointSlotsToBackpatch);
         CodeVersionManager::TableLockHolder lock(pCodeVersionManager);
+
+        // As long as we are exclusively using any non-JumpStamp publishing for tiered compilation
+        // methods this first attempt should succeed
         ilParent = nativeCodeVersion.GetILCodeVersion();
         hr = ilParent.SetActiveNativeCodeVersion(nativeCodeVersion, FALSE);
         LOG((LF_TIEREDCOMPILATION, LL_INFO10000, "TieredCompilationManager::ActivateCodeVersion Method=0x%pM (%s::%s), code version id=0x%x. SetActiveNativeCodeVersion ret=0x%x\n",
@@ -768,7 +775,13 @@ void TieredCompilationManager::ActivateCodeVersion(NativeCodeVersion nativeCodeV
         // viable. This fallback path is just here as a proof-of-concept.
         ThreadSuspend::SuspendEE(ThreadSuspend::SUSPEND_FOR_REJIT);
         {
+            // Backpatching entry point slots requires cooperative GC mode, see
+            // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that
+            // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering
+            // must be used here to prevent deadlock.
+            GCX_MAYBE_COOP(mayHaveEntryPointSlotsToBackpatch);
             CodeVersionManager::TableLockHolder lock(pCodeVersionManager);
+
             hr = ilParent.SetActiveNativeCodeVersion(nativeCodeVersion, TRUE);
             LOG((LF_TIEREDCOMPILATION, LL_INFO10000, "TieredCompilationManager::ActivateCodeVersion Method=0x%pM (%s::%s), code version id=0x%x. [Suspended] SetActiveNativeCodeVersion ret=0x%x\n",
                 pMethod, pMethod->m_pszDebugClassName, pMethod->m_pszDebugMethodName,