Fail FuncEval if slot backpatching lock is held by any thread

src/coreclr/src/debug/ee/debugger.cpp

@@ -15309,6 +15309,15 @@ HRESULT Debugger::FuncEvalSetup(DebuggerIPCE_FuncEvalInfo *pEvalInfo,
         return CORDBG_E_FUNC_EVAL_BAD_START_POINT;
     }
 
+    if (MethodDescBackpatchInfoTracker::IsLockOwnedByAnyThread())
+    {
+        // A thread may have suspended for the debugger while holding the slot backpatching lock while trying to enter
+        // cooperative GC mode. If the FuncEval calls a method that is eligible for slot backpatching (virtual or interface
+        // methods that are eligible for tiering), the FuncEval may deadlock on trying to acquire the same lock. Fail the
+        // FuncEval to avoid the issue.
+        return CORDBG_E_FUNC_EVAL_BAD_START_POINT;
+    }
+
     // Create a DebuggerEval to hold info about this eval while its in progress. Constructor copies the thread's
     // CONTEXT.
     DebuggerEval *pDE = new (interopsafe, nothrow) DebuggerEval(filterContext, pEvalInfo, fInException);

src/coreclr/src/vm/callcounting.cpp

@@ -817,6 +817,7 @@ void CallCountingManager::CompleteCallCounting()
     {
         CodeVersionManager *codeVersionManager = appDomain->GetCodeVersionManager();
 
+        MethodDescBackpatchInfoTracker::PollForDebuggerSuspension();
         MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder;
 
         // Backpatching entry point slots requires cooperative GC mode, see
@@ -993,6 +994,7 @@ void CallCountingManager::StopAndDeleteAllCallCountingStubs()
     TieredCompilationManager *tieredCompilationManager = GetAppDomain()->GetTieredCompilationManager();
     bool scheduleTieringBackgroundWork = false;
     {
+        MethodDescBackpatchInfoTracker::PollForDebuggerSuspension();
         MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder;
 
         ThreadSuspend::SuspendEE(ThreadSuspend::SUSPEND_OTHER);

src/coreclr/src/vm/methoddescbackpatchinfo.cpp

@@ -66,6 +66,7 @@ void EntryPointSlots::Backpatch_Locked(TADDR slot, SlotType slotType, PCODE entr
 // MethodDescBackpatchInfoTracker
 
 CrstStatic MethodDescBackpatchInfoTracker::s_lock;
+bool MethodDescBackpatchInfoTracker::s_isLocked = false;
 
 #ifndef DACCESS_COMPILE
 
@@ -111,7 +112,6 @@ void MethodDescBackpatchInfoTracker::AddSlotAndPatch_Locked(MethodDesc *pMethodD
 #endif // DACCESS_COMPILE
 
 #ifdef _DEBUG
-
 bool MethodDescBackpatchInfoTracker::IsLockOwnedByCurrentThread()
 {
     WRAPPER_NO_CONTRACT;
@@ -122,16 +122,32 @@ bool MethodDescBackpatchInfoTracker::IsLockOwnedByCurrentThread()
     return true;
 #endif
 }
+#endif // _DEBUG
 
-bool MethodDescBackpatchInfoTracker::MayHaveEntryPointSlotsToBackpatch(PTR_MethodDesc methodDesc)
+#ifndef DACCESS_COMPILE
+void MethodDescBackpatchInfoTracker::PollForDebuggerSuspension()
 {
-    // The only purpose of this method is to allow asserts in inline functions defined in the .h file, by which time MethodDesc
-    // is not fully defined
+    CONTRACTL
+    {
+        NOTHROW;
+        GC_TRIGGERS;
+        MODE_PREEMPTIVE;
+    }
+    CONTRACTL_END;
 
-    WRAPPER_NO_CONTRACT;
-    return methodDesc->MayHaveEntryPointSlotsToBackpatch();
-}
+    _ASSERTE(!IsLockOwnedByCurrentThread());
 
-#endif // _DEBUG
+    // If suspension is pending for the debugger, pulse the GC mode to suspend the thread here. Following this call, typically
+    // the lock is acquired and the GC mode is changed, and suspending there would cause FuncEvals to fail (see
+    // Debugger::FuncEvalSetup() at the reference to IsLockOwnedByAnyThread()). Since this thread is in preemptive mode, the
+    // debugger may think it's already suspended and it would be unfortunate to suspend the thread with the lock held.
+    Thread *thread = GetThread();
+    _ASSERTE(thread != nullptr);
+    if (thread->HasThreadState(Thread::TS_DebugSuspendPending))
+    {
+        GCX_COOP();
+    }
+}
+#endif
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

src/coreclr/src/vm/methoddescbackpatchinfo.h

@@ -66,6 +66,7 @@ class MethodDescBackpatchInfoTracker
 {
 private:
     static CrstStatic s_lock;
+    static bool s_isLocked;
 
     class BackpatchInfoTrackerHashTraits : public NoRemoveDefaultCrossLoaderAllocatorHashTraits<MethodDesc *, UINT_PTR>
     {
@@ -97,9 +98,23 @@ class MethodDescBackpatchInfoTracker
     static bool IsLockOwnedByCurrentThread();
 #endif
 
+#ifndef DACCESS_COMPILE
+public:
+    static bool IsLockOwnedByAnyThread()
+    {
+        LIMITED_METHOD_CONTRACT;
+        return VolatileLoadWithoutBarrier(&s_isLocked);
+    }
+
+    static void PollForDebuggerSuspension();
+#endif
+
 public:
     class ConditionalLockHolder : private CrstHolderWithState
     {
+    private:
+        bool m_isLocked;
+
     public:
         ConditionalLockHolder(bool acquireLock = true)
             : CrstHolderWithState(
@@ -108,13 +123,37 @@ class MethodDescBackpatchInfoTracker
 #else
                 nullptr
 #endif
-                )
+                ),
+            m_isLocked(false)
         {
-            LIMITED_METHOD_CONTRACT;
+            WRAPPER_NO_CONTRACT;
+
+        #ifndef DACCESS_COMPILE
+            if (acquireLock)
+            {
+                _ASSERTE(IsLockOwnedByCurrentThread());
+                _ASSERTE(!s_isLocked);
+                m_isLocked = true;
+                s_isLocked = true;
+            }
+        #endif
         }
 
-        ConditionalLockHolder(const ConditionalLockHolder &) = delete;
-        ConditionalLockHolder &operator =(const ConditionalLockHolder &) = delete;
+        ~ConditionalLockHolder()
+        {
+            WRAPPER_NO_CONTRACT;
+
+        #ifndef DACCESS_COMPILE
+            if (m_isLocked)
+            {
+                _ASSERTE(IsLockOwnedByCurrentThread());
+                _ASSERTE(s_isLocked);
+                s_isLocked = false;
+            }
+        #endif
+        }
+
+        DISABLE_COPY(ConditionalLockHolder);
     };
 
 public:
@@ -123,16 +162,10 @@ class MethodDescBackpatchInfoTracker
         LIMITED_METHOD_CONTRACT;
     }
 
-#ifdef _DEBUG
-public:
-    static bool MayHaveEntryPointSlotsToBackpatch(PTR_MethodDesc methodDesc);
-#endif
-
 #ifndef DACCESS_COMPILE
 public:
     void Backpatch_Locked(MethodDesc *pMethodDesc, PCODE entryPoint);
     void AddSlotAndPatch_Locked(MethodDesc *pMethodDesc, LoaderAllocator *pLoaderAllocatorOfSlot, TADDR slot, EntryPointSlots::SlotType slotType, PCODE currentEntryPoint);
-public:
 #endif
 
     DISABLE_COPY(MethodDescBackpatchInfoTracker);

src/coreclr/src/vm/tieredcompilation.cpp

@@ -450,6 +450,7 @@ void TieredCompilationManager::DeactivateTieringDelay()
         COUNT_T methodCount = methodsPendingCounting->GetCount();
         CodeVersionManager *codeVersionManager = GetAppDomain()->GetCodeVersionManager();
 
+        MethodDescBackpatchInfoTracker::PollForDebuggerSuspension();
         MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder;
 
         // Backpatching entry point slots requires cooperative GC mode, see
@@ -815,6 +816,10 @@ void TieredCompilationManager::ActivateCodeVersion(NativeCodeVersion nativeCodeV
     HRESULT hr = S_OK;
     {
         bool mayHaveEntryPointSlotsToBackpatch = pMethod->MayHaveEntryPointSlotsToBackpatch();
+        if (mayHaveEntryPointSlotsToBackpatch)
+        {
+            MethodDescBackpatchInfoTracker::PollForDebuggerSuspension();
+        }
         MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder(mayHaveEntryPointSlotsToBackpatch);
 
         // Backpatching entry point slots requires cooperative GC mode, see