From 33242624ae30141fb1ea7de473a4f9bc02992308 Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Thu, 24 May 2018 11:43:54 -0700 Subject: [PATCH] Completed the lock reversal work Commit migrated from https://github.com/dotnet/coreclr/commit/8d9206829c5ae9f2efcf70c5ee19977f0130bae8 --- src/coreclr/src/debug/ee/debugger.cpp | 122 ++++++++++++++++++++++------------ src/coreclr/src/debug/ee/debugger.h | 13 ++-- src/coreclr/src/debug/ee/rcthread.cpp | 39 +++++------ src/coreclr/src/vm/threads.cpp | 14 +--- src/coreclr/src/vm/threads.h | 2 - src/coreclr/src/vm/threadsuspend.cpp | 22 ++++-- 6 files changed, 124 insertions(+), 88 deletions(-) diff --git a/src/coreclr/src/debug/ee/debugger.cpp b/src/coreclr/src/debug/ee/debugger.cpp index 8ef3378..1099032 100644 --- a/src/coreclr/src/debug/ee/debugger.cpp +++ b/src/coreclr/src/debug/ee/debugger.cpp @@ -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: { diff --git a/src/coreclr/src/debug/ee/debugger.h b/src/coreclr/src/debug/ee/debugger.h index 1db799c..ac0f7be 100644 --- a/src/coreclr/src/debug/ee/debugger.h +++ b/src/coreclr/src/debug/ee/debugger.h @@ -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(); \ diff --git a/src/coreclr/src/debug/ee/rcthread.cpp b/src/coreclr/src/debug/ee/rcthread.cpp index 06cc17e..200e91e 100644 --- a/src/coreclr/src/debug/ee/rcthread.cpp +++ b/src/coreclr/src/debug/ee/rcthread.cpp @@ -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; diff --git a/src/coreclr/src/vm/threads.cpp b/src/coreclr/src/vm/threads.cpp index 15e36ca..7a638a9 100644 --- a/src/coreclr/src/vm/threads.cpp +++ b/src/coreclr/src/vm/threads.cpp @@ -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; diff --git a/src/coreclr/src/vm/threads.h b/src/coreclr/src/vm/threads.h index 3d9db05..3aa1681 100644 --- a/src/coreclr/src/vm/threads.h +++ b/src/coreclr/src/vm/threads.h @@ -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. diff --git a/src/coreclr/src/vm/threadsuspend.cpp b/src/coreclr/src/vm/threadsuspend.cpp index abf9597..6122b04 100644 --- a/src/coreclr/src/vm/threadsuspend.cpp +++ b/src/coreclr/src/vm/threadsuspend.cpp @@ -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; } -- 2.7.4