Completed the lock reversal work
authorAndrew Au <andrewau@microsoft.com>
Thu, 24 May 2018 18:43:54 +0000 (11:43 -0700)
committerAndrew Au <cshung@gmail.com>
Wed, 7 Nov 2018 02:34:47 +0000 (18:34 -0800)
Commit migrated from https://github.com/dotnet/coreclr/commit/8d9206829c5ae9f2efcf70c5ee19977f0130bae8

src/coreclr/src/debug/ee/debugger.cpp
src/coreclr/src/debug/ee/debugger.h
src/coreclr/src/debug/ee/rcthread.cpp
src/coreclr/src/vm/threads.cpp
src/coreclr/src/vm/threads.h
src/coreclr/src/vm/threadsuspend.cpp

index 8ef3378..1099032 100644 (file)
@@ -707,8 +707,6 @@ HRESULT __cdecl CorDBGetInterface(DebugInterface** rcInterface)
 //-----------------------------------------------------------------------------
 void Debugger::SendSimpleIPCEventAndBlock()
 {
-    // TODO, databp, enable these contracts
-    /*
     CONTRACTL
     {
         SO_NOT_MAINLINE;
@@ -716,7 +714,6 @@ void Debugger::SendSimpleIPCEventAndBlock()
         MAY_DO_HELPER_THREAD_DUTY_GC_TRIGGERS_CONTRACT;
     }
     CONTRACTL_END;
-    */
 
     // BEGIN will acquire the lock (END will release it). While blocking, the
     // debugger may have detached though, so we need to check for that.
@@ -954,7 +951,8 @@ Debugger::Debugger()
     m_pIDbgThreadControl(NULL),
     m_forceNonInterceptable(FALSE),
     m_pLazyData(NULL),
-    m_defines(_defines)
+    m_defines(_defines),
+    m_isBlockedOnGarbageCollectionEvent(FALSE)
 {
     CONTRACTL
     {
@@ -2377,7 +2375,8 @@ DebuggerLazyInit::DebuggerLazyInit() :
     m_CtrlCMutex(NULL),
     m_exAttachEvent(NULL),
     m_exUnmanagedAttachEvent(NULL),
-    m_DebuggerHandlingCtrlC(NULL)
+    m_DebuggerHandlingCtrlC(NULL),
+    m_garbageCollectionBlockerEvent(NULL)
 {
 }
 
@@ -2415,6 +2414,8 @@ void DebuggerLazyInit::Init()
     m_CtrlCMutex             = CreateWin32EventOrThrow(NULL, kAutoResetEvent, FALSE);
     m_DebuggerHandlingCtrlC  = FALSE;
 
+    m_garbageCollectionBlockerEvent = CreateEventW(NULL, TRUE, FALSE, NULL);
+
     // Let the helper thread lazy init stuff too.
     m_RCThread.Init();
 }
@@ -2452,6 +2453,11 @@ DebuggerLazyInit::~DebuggerLazyInit()
     {
         CloseHandle(m_exUnmanagedAttachEvent);
     }
+
+    if (m_garbageCollectionBlockerEvent != NULL)
+    {
+        CloseHandle(m_garbageCollectionBlockerEvent);
+    }
 }
 
 
@@ -5451,8 +5457,6 @@ DebuggerModule* Debugger::AddDebuggerModule(DomainFile * pDomainFile)
 // Neither pDbgLockHolder nor pAppDomain are used.
 void Debugger::TrapAllRuntimeThreads()
 {
-    // TODO, databp, enable these contracts
-    /*
     CONTRACTL
     {
         SO_NOT_MAINLINE;
@@ -5468,8 +5472,7 @@ void Debugger::TrapAllRuntimeThreads()
                      !g_pEEInterface->IsPreemptiveGCDisabled());
     }
     CONTRACTL_END;
-    */
-
 #if !defined(FEATURE_DBGIPC_TRANSPORT_VM)
     // Only sync if RS requested it.
     if (!m_RSRequestedSync)
@@ -5503,9 +5506,13 @@ void Debugger::TrapAllRuntimeThreads()
         m_trappingRuntimeThreads = TRUE;
 
         // Take the thread store lock.
-        STRESS_LOG0(LF_CORDB,LL_INFO1000, "About to lock thread Store\n");
-        ThreadSuspend::LockThreadStore(ThreadSuspend::SUSPEND_FOR_DEBUGGER);
-        STRESS_LOG0(LF_CORDB,LL_INFO1000, "Locked thread store\n");
+        bool alreadyHeldThreadStoreLock = ThreadStore::HoldingThreadStore();
+        if (!alreadyHeldThreadStoreLock)
+        {
+            STRESS_LOG0(LF_CORDB, LL_INFO1000, "About to lock thread Store\n");
+            ThreadSuspend::LockThreadStore(ThreadSuspend::SUSPEND_FOR_DEBUGGER);
+            STRESS_LOG0(LF_CORDB, LL_INFO1000, "Locked thread store\n");
+        }
 
         // We start the suspension here, and let the helper thread finish it.
         // If there's no helper thread, then we need to do helper duty.
@@ -5554,9 +5561,12 @@ void Debugger::TrapAllRuntimeThreads()
             // from the RS now that we're stopped.
             // We need to release the TSL which we acquired above. (The helper will
             // likely take this lock while doing stuff).
-            STRESS_LOG0(LF_CORDB,LL_INFO1000, "About to unlock thread store!\n");
-            ThreadSuspend::UnlockThreadStore(FALSE, ThreadSuspend::SUSPEND_FOR_DEBUGGER);
-            STRESS_LOG0(LF_CORDB,LL_INFO1000, "TART: Unlocked thread store!\n");
+            if (!alreadyHeldThreadStoreLock)
+            {
+                STRESS_LOG0(LF_CORDB, LL_INFO1000, "About to unlock thread store!\n");
+                ThreadSuspend::UnlockThreadStore(FALSE, ThreadSuspend::SUSPEND_FOR_DEBUGGER);
+                STRESS_LOG0(LF_CORDB, LL_INFO1000, "TART: Unlocked thread store!\n");
+            }
         }
         _ASSERTE(ThreadHoldsLock()); // still hold the lock. (though it may have been toggled)
     }
@@ -6014,21 +6024,31 @@ void Debugger::BeforeGarbageCollection()
 
     Thread* pThread = GetThread();
 
-    SENDIPCEVENT_BEGIN(this, pThread)
-
     if (CORDBUnrecoverableError(this))
         return;
 
-    // Send an event to the Right Side
-    DebuggerIPCEvent* ipce = m_pRCThread->GetIPCEventSendBuffer();
-    InitIPCEvent(ipce,
-        DB_IPCE_BEFORE_GARBAGE_COLLECTION,
-        pThread,
-        pThread->GetDomain());
+    {
+        Debugger::DebuggerLockHolder dbgLockHolder(this);
 
-    SendSimpleIPCEventAndBlock();
+        this->m_stopped = true;
+        this->m_isBlockedOnGarbageCollectionEvent = true;
 
-    SENDIPCEVENT_END;
+        DebuggerIPCEvent* ipce1 = m_pRCThread->GetIPCEventSendBuffer();
+        InitIPCEvent(ipce1,
+            DB_IPCE_BEFORE_GARBAGE_COLLECTION,
+            pThread,
+            pThread->GetDomain());
+
+        m_pRCThread->SendIPCEvent();
+
+        DebuggerIPCEvent* ipce2 = m_pRCThread->GetIPCEventSendBuffer();
+        InitIPCEvent(ipce2, DB_IPCE_SYNC_COMPLETE);
+
+        m_pRCThread->SendIPCEvent();
+    }
+
+    WaitForSingleObject(this->GetGarbageCollectionBlockerEvent(), INFINITE);
+    ResetEvent(this->GetGarbageCollectionBlockerEvent());
 }
 
 void Debugger::AfterGarbageCollection()
@@ -6040,9 +6060,6 @@ void Debugger::AfterGarbageCollection()
     }
     CONTRACTL_END;
 
-    // TODO, databp, ideally, remove this.
-    CONTRACT_VIOLATION(GCViolation);
-
     if (!CORDebuggerAttached())
     {
         return;
@@ -6050,21 +6067,31 @@ void Debugger::AfterGarbageCollection()
 
     Thread* pThread = GetThread();
 
-    SENDIPCEVENT_BEGIN(this, pThread)
-
     if (CORDBUnrecoverableError(this))
         return;
 
-    // Send an event to the Right Side
-    DebuggerIPCEvent* ipce = m_pRCThread->GetIPCEventSendBuffer();
-    InitIPCEvent(ipce,
-        DB_IPCE_AFTER_GARBAGE_COLLECTION,
-        pThread,
-        pThread->GetDomain());
+    {
+        Debugger::DebuggerLockHolder dbgLockHolder(this);
 
-    SendSimpleIPCEventAndBlock();
+        this->m_stopped = true;
+        this->m_isBlockedOnGarbageCollectionEvent = true;
 
-    SENDIPCEVENT_END;
+        DebuggerIPCEvent* ipce1 = m_pRCThread->GetIPCEventSendBuffer();
+        InitIPCEvent(ipce1,
+            DB_IPCE_AFTER_GARBAGE_COLLECTION,
+            pThread,
+            pThread->GetDomain());
+
+        m_pRCThread->SendIPCEvent();
+
+        DebuggerIPCEvent* ipce2 = m_pRCThread->GetIPCEventSendBuffer();
+        InitIPCEvent(ipce2, DB_IPCE_SYNC_COMPLETE);
+
+        m_pRCThread->SendIPCEvent();
+    }
+
+    WaitForSingleObject(this->GetGarbageCollectionBlockerEvent(), INFINITE);
+    ResetEvent(this->GetGarbageCollectionBlockerEvent());
 }
 
 void Debugger::SendDataBreakpoint(Thread *thread, CONTEXT *context,
@@ -10733,7 +10760,8 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent)
     DebuggerLockHolder dbgLockHolder(this, FALSE);
 
     if ((pEvent->type & DB_IPCE_TYPE_MASK) == DB_IPCE_ASYNC_BREAK ||
-        (pEvent->type & DB_IPCE_TYPE_MASK) == DB_IPCE_ATTACHING)
+        (pEvent->type & DB_IPCE_TYPE_MASK) == DB_IPCE_ATTACHING ||
+        this->m_isBlockedOnGarbageCollectionEvent)
     {
         dbgLockHolder.Acquire();
     }
@@ -10809,18 +10837,26 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent)
 
     case DB_IPCE_CONTINUE:
         {
-            GetCanary()->ClearCache();
+            if (this->m_isBlockedOnGarbageCollectionEvent)
+            {
+                this->m_isBlockedOnGarbageCollectionEvent = false;
+                this->m_stopped = false;
+                SetEvent(this->GetGarbageCollectionBlockerEvent());
+            }
+            else
+            {
+                GetCanary()->ClearCache();
 
-            fContinue = ResumeThreads(pEvent->vmAppDomain.GetRawPtr());
+                fContinue = ResumeThreads(pEvent->vmAppDomain.GetRawPtr());
 
                 //
                 // Go ahead and release the TSL now that we're continuing. This ensures that we've held
                 // the thread store lock the entire time the Runtime was just stopped.
                 //
                 ThreadSuspend::UnlockThreadStore(FALSE, ThreadSuspend::SUSPEND_FOR_DEBUGGER);
-
-            break;
             }
+            break;
+        }
 
     case DB_IPCE_BREAKPOINT_ADD:
         {
index 1db799c..ac0f7be 100644 (file)
@@ -666,6 +666,7 @@ protected:
     HANDLE                m_CtrlCMutex;
     HANDLE                m_exAttachEvent;
     HANDLE                m_exUnmanagedAttachEvent;
+    HANDLE                m_garbageCollectionBlockerEvent;
 
     BOOL                  m_DebuggerHandlingCtrlC;
 
@@ -2954,7 +2955,11 @@ public:
 #ifndef DACCESS_COMPILE
     virtual void BeforeGarbageCollection();
     virtual void AfterGarbageCollection();
+    BOOL m_isBlockedOnGarbageCollectionEvent;
 #endif
+private:
+    HANDLE GetGarbageCollectionBlockerEvent() { return  GetLazyData()->m_garbageCollectionBlockerEvent; }
+    
 };
 
 
@@ -3791,14 +3796,14 @@ HANDLE OpenWin32EventOrThrow(
 
 #define SENDIPCEVENT_RAW_BEGIN_EX(pDbgLockHolder, gcxStmt)      \
   {                                                             \
-    ThreadStore::EnterThreadStoreLockOnly();                             \
+    ThreadStore::LockThreadStore();                             \
     Debugger::DebuggerLockHolder *__pDbgLockHolder = pDbgLockHolder; \
     gcxStmt;                                                    \
     g_pDebugger->LockForEventSending(__pDbgLockHolder);
 
 #define SENDIPCEVENT_RAW_END_EX                                 \
     g_pDebugger->UnlockFromEventSending(__pDbgLockHolder);      \
-    ThreadStore::LeaveThreadStoreLockOnly();                             \
+    ThreadStore::UnlockThreadStore();                             \
   }
 
 #define SENDIPCEVENT_RAW_BEGIN(pDbgLockHolder)                  \
@@ -3823,7 +3828,7 @@ HANDLE OpenWin32EventOrThrow(
         Debugger::DebuggerLockHolder __dbgLockHolder(pDebugger, FALSE);                   \
         Debugger::DebuggerLockHolder *__pDbgLockHolder = &__dbgLockHolder;                \
         gcxStmt;                                                                          \
-        ThreadStore::EnterThreadStoreLockOnly();                                          \
+        ThreadStore::LockThreadStore();                                                   \
         g_pDebugger->LockForEventSending(__pDbgLockHolder);                               \
         /* Check if the thread has been suspended by the debugger via SetDebugState(). */ \
         if (thread != NULL && thread->HasThreadStateNC(Thread::TSNC_DebuggerUserSuspend)) \
@@ -3838,7 +3843,7 @@ HANDLE OpenWin32EventOrThrow(
             ;                                                   \
         }                                                       \
         g_pDebugger->UnlockFromEventSending(__pDbgLockHolder);  \
-        ThreadStore::LeaveThreadStoreLockOnly();                \
+        ThreadStore::UnlockThreadStore();                       \
       } /* ~gcxStmt & ~DebuggerLockHolder */                    \
     } while (__fRetry);                                         \
     FireEtwDebugIPCEventEnd();                                  \
index 06cc17e..200e91e 100644 (file)
@@ -1226,7 +1226,6 @@ void DebuggerRCThread::MainLoop()
 
                 // Let's release the lock here since runtime is resumed.
                 debugLockHolderSuspended.Release();
-                ThreadStore::LeaveThreadStoreLockOnly();
 
                 // This debugger thread shoud not be holding debugger locks anymore
                 _ASSERTE(!g_pDebugger->ThreadHoldsLock());
@@ -1249,7 +1248,7 @@ void DebuggerRCThread::MainLoop()
         {
             LOG((LF_CORDB, LL_INFO1000, "DRCT::ML:: straggler event set.\n"));
             
-            ThreadStore::EnterThreadStoreLockOnly();
+            ThreadStore::LockThreadStore();
             Debugger::DebuggerLockHolder debugLockHolder(m_debugger);
             // Make sure that we're still synchronizing...
             if (m_debugger->IsSynchronizing())
@@ -1262,14 +1261,14 @@ void DebuggerRCThread::MainLoop()
                 // Skip waiting the first time and just give it a go.  Note: Implicit
                 // release of the lock, because we are leaving its scope.
                 //
-                ThreadStore::LeaveThreadStoreLockOnly();
+                ThreadStore::UnlockThreadStore();
                 goto LWaitTimedOut;
             }
 #ifdef LOGGING
             else
                 LOG((LF_CORDB, LL_INFO1000, "DRCT::ML:: told to wait, but not syncing anymore.\n"));
 #endif
-            ThreadStore::LeaveThreadStoreLockOnly();
+            ThreadStore::UnlockThreadStore();
             // dbgLockHolder goes out of scope - implicit Release
          }
         else if (dwWaitResult == WAIT_TIMEOUT)
@@ -1279,7 +1278,7 @@ LWaitTimedOut:
 
             LOG((LF_CORDB, LL_INFO1000, "DRCT::ML:: wait timed out.\n"));
 
-            ThreadStore::EnterThreadStoreLockOnly();
+            ThreadStore::LockThreadStore();
             // Debugger::DebuggerLockHolder debugLockHolder(m_debugger);
             // Explicitly get the lock here since we try to check to see if
             // have suspended.  We will release the lock if we are not suspended yet.
@@ -1334,7 +1333,7 @@ LWaitTimedOut:
                 // And so the sweep should always succeed.
                 STRESS_LOG0(LF_CORDB, LL_INFO1000, "DRCT::ML:: threads still syncing after sweep.\n");
                 debugLockHolderSuspended.Release();
-                ThreadStore::LeaveThreadStoreLockOnly();
+                ThreadStore::UnlockThreadStore();
             }
             // debugLockHolderSuspended does not go out of scope. It has to be either released explicitly on the line above or
             // we intend to hold the lock till we hit continue event.
@@ -1778,22 +1777,24 @@ HRESULT DebuggerRCThread::SendIPCEvent()
         NOTHROW;
         GC_NOTRIGGER; // duh, we're in preemptive..
 
-        if (ThisIsHelperThreadWorker())
+        if (!m_debugger->m_isBlockedOnGarbageCollectionEvent)
         {
-            // When we're stopped, the helper could actually be contracted as either mode-cooperative
-            // or mode-preemptive!
-            // If we're the helper thread, we're only sending events while we're stopped.
-            // Our callers will be mode-cooperative, so call this mode_cooperative to avoid a bunch
-            // of unncessary contract violations.
-            MODE_COOPERATIVE;
-        }
-        else
-        {
-            // Managed threads sending debug events should always be in preemptive mode.
-            MODE_PREEMPTIVE;
+            if (ThisIsHelperThreadWorker())
+            {
+                // When we're stopped, the helper could actually be contracted as either mode-cooperative
+                // or mode-preemptive!
+                // If we're the helper thread, we're only sending events while we're stopped.
+                // Our callers will be mode-cooperative, so call this mode_cooperative to avoid a bunch
+                // of unncessary contract violations.
+                MODE_COOPERATIVE;
+            }
+            else
+            {
+                // Managed threads sending debug events should always be in preemptive mode.
+                MODE_PREEMPTIVE;
+            }
         }
 
-
         PRECONDITION(ThisMaybeHelperThread());
     }
     CONTRACTL_END;
index 15e36ca..7a638a9 100644 (file)
@@ -5349,7 +5349,7 @@ Thread::ApartmentState Thread::SetApartment(ApartmentState state, BOOL fFireMDAO
 //----------------------------------------------------------------------------
 
 ThreadStore::ThreadStore()
-           : m_Crst(CrstThreadStore, (CrstFlags) (CRST_UNSAFE_ANYMODE | CRST_REENTRANCY | CRST_DEBUGGER_THREAD)),
+           : m_Crst(CrstThreadStore, (CrstFlags) (CRST_UNSAFE_ANYMODE | CRST_DEBUGGER_THREAD)),
              m_ThreadCount(0),
              m_MaxThreadCount(0),
              m_UnstartedThreadCount(0),
@@ -5434,18 +5434,6 @@ DEBUG_NOINLINE void ThreadStore::Leave()
     m_Crst.Leave();
 }
 
-void ThreadStore::EnterThreadStoreLockOnly()
-{
-    WRAPPER_NO_CONTRACT;
-    s_pThreadStore->Enter();
-}
-
-void ThreadStore::LeaveThreadStoreLockOnly()
-{
-    WRAPPER_NO_CONTRACT;
-    s_pThreadStore->Leave();
-}
-
 void ThreadStore::LockThreadStore()
 {
     WRAPPER_NO_CONTRACT;
index 3d9db05..3aa1681 100644 (file)
@@ -5321,8 +5321,6 @@ public:
     static void InitThreadStore();
     static void LockThreadStore();
     static void UnlockThreadStore();
-    static void EnterThreadStoreLockOnly();
-    static void LeaveThreadStoreLockOnly();
 
     // Add a Thread to the ThreadStore
     // WARNING : only GC calls this with bRequiresTSL set to FALSE.
index abf9597..6122b04 100644 (file)
@@ -2741,6 +2741,7 @@ void ThreadSuspend::UnlockThreadStore(BOOL bThreadDestroyed, ThreadSuspend::SUSP
         ThreadStore::s_pThreadStore->m_HoldingThread = NULL;
         ThreadStore::s_pThreadStore->m_holderthreadid.Clear();
         ThreadStore::s_pThreadStore->Leave();
+        LOG((LF_SYNC, INFO3, "Unlocked thread store\n"));
 
         // We're out of the critical area for managed/unmanaged debugging.
         if (!bThreadDestroyed && pCurThread)
@@ -5512,12 +5513,16 @@ bool Thread::SysSweepThreadsForDebug(bool forceSync)
     // This function has parallel logic in SuspendRuntime.  Please make
     // sure to make appropriate changes there as well.
 
-    // We use ThreadSuspend::SUSPEND_FOR_DEBUGGER_SWEEP here to avoid a deadlock which
-    // can occur due to the s_hAbortEvt event.  This event causes any thread trying
-    // to take the ThreadStore lock to wait for a GC to complete.  If a thread is
-    // in SuspendEE for a GC and suspends for the debugger, then this thread will
-    // deadlock if we do not pass in SUSPEND_FOR_DEBUGGER_SWEEP here.
-    ThreadSuspend::LockThreadStore(ThreadSuspend::SUSPEND_FOR_DEBUGGER_SWEEP);
+    bool alreadyHeldThreadStoreLock = ThreadStore::HoldingThreadStore();
+    if (!alreadyHeldThreadStoreLock)
+    {
+        // We use ThreadSuspend::SUSPEND_FOR_DEBUGGER_SWEEP here to avoid a deadlock which
+        // can occur due to the s_hAbortEvt event.  This event causes any thread trying
+        // to take the ThreadStore lock to wait for a GC to complete.  If a thread is
+        // in SuspendEE for a GC and suspends for the debugger, then this thread will
+        // deadlock if we do not pass in SUSPEND_FOR_DEBUGGER_SWEEP here.
+        ThreadSuspend::LockThreadStore(ThreadSuspend::SUSPEND_FOR_DEBUGGER_SWEEP);
+    }
 
     // From this point until the end of the function, consider all active thread
     // suspension to be in progress.  This is mainly to give the profiler API a hint
@@ -5663,7 +5668,10 @@ Label_MarkThreadAsSynced:
 
     // The CLR is not yet synced. We release the threadstore lock and return false.
     hldSuspendRuntimeInProgress.Release();
-    ThreadSuspend::UnlockThreadStore();
+    if (!alreadyHeldThreadStoreLock)
+    {
+        ThreadSuspend::UnlockThreadStore();
+    }
 
     RETURN false;
 }