From 5f388cfe53163d75a2889c9e976093673840211b Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Thu, 13 Dec 2018 22:29:46 -0800 Subject: [PATCH] Setting the m_willBlockOnGarbageCollectionEvent earlier and only when we will block to avoid the async break deadlock (dotnet/coreclr#21319) Setting the m_willBlockOnGarbageCollectionEvent earlier and only when we will block to avoid the async break deadlock Commit migrated from https://github.com/dotnet/coreclr/commit/b093822a4dacfe6bd36970e270f0a6d720732a04 --- src/coreclr/src/debug/ee/debugger.cpp | 30 +++++++++++++++++++++--------- src/coreclr/src/debug/ee/debugger.h | 6 ++++-- src/coreclr/src/vm/dbginterface.h | 5 +++-- src/coreclr/src/vm/gcenv.ee.cpp | 6 ++++-- 4 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/coreclr/src/debug/ee/debugger.cpp b/src/coreclr/src/debug/ee/debugger.cpp index 5796a36..7031b55 100644 --- a/src/coreclr/src/debug/ee/debugger.cpp +++ b/src/coreclr/src/debug/ee/debugger.cpp @@ -951,6 +951,7 @@ Debugger::Debugger() m_pLazyData(NULL), m_defines(_defines), m_isBlockedOnGarbageCollectionEvent(FALSE), + m_willBlockOnGarbageCollectionEvent(FALSE), m_isGarbageCollectionEventsEnabled(FALSE), m_isGarbageCollectionEventsEnabledLatch(FALSE) { @@ -5989,7 +5990,7 @@ bool Debugger::ThreadsAtUnsafePlaces(void) return (m_threadsAtUnsafePlaces != 0); } -void Debugger::BeforeGarbageCollection() +void Debugger::SuspendForGarbageCollectionStarted() { CONTRACTL { @@ -5997,13 +5998,25 @@ void Debugger::BeforeGarbageCollection() GC_NOTRIGGER; } CONTRACTL_END; - + this->m_isGarbageCollectionEventsEnabledLatch = this->m_isGarbageCollectionEventsEnabled; + this->m_willBlockOnGarbageCollectionEvent = this->m_isGarbageCollectionEventsEnabledLatch; +} + +void Debugger::SuspendForGarbageCollectionCompleted() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + } + CONTRACTL_END; if (!CORDebuggerAttached() || !this->m_isGarbageCollectionEventsEnabledLatch) { return; } + this->m_isBlockedOnGarbageCollectionEvent = TRUE; Thread* pThread = GetThread(); @@ -6013,8 +6026,6 @@ void Debugger::BeforeGarbageCollection() { Debugger::DebuggerLockHolder dbgLockHolder(this); - this->m_isBlockedOnGarbageCollectionEvent = true; - DebuggerIPCEvent* ipce1 = m_pRCThread->GetIPCEventSendBuffer(); InitIPCEvent(ipce1, DB_IPCE_BEFORE_GARBAGE_COLLECTION, @@ -6029,7 +6040,7 @@ void Debugger::BeforeGarbageCollection() ResetEvent(this->GetGarbageCollectionBlockerEvent()); } -void Debugger::AfterGarbageCollection() +void Debugger::ResumeForGarbageCollectionStarted() { CONTRACTL { @@ -6063,7 +6074,8 @@ void Debugger::AfterGarbageCollection() WaitForSingleObject(this->GetGarbageCollectionBlockerEvent(), INFINITE); ResetEvent(this->GetGarbageCollectionBlockerEvent()); - this->m_isBlockedOnGarbageCollectionEvent = false; + this->m_isBlockedOnGarbageCollectionEvent = FALSE; + this->m_willBlockOnGarbageCollectionEvent = FALSE; } #ifdef FEATURE_DATABREAKPOINT @@ -10738,9 +10750,9 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent) if ((pEvent->type & DB_IPCE_TYPE_MASK) == DB_IPCE_ASYNC_BREAK || (pEvent->type & DB_IPCE_TYPE_MASK) == DB_IPCE_ATTACHING || - this->m_isBlockedOnGarbageCollectionEvent) + this->m_willBlockOnGarbageCollectionEvent) { - if (!this->m_isBlockedOnGarbageCollectionEvent) + if (!this->m_willBlockOnGarbageCollectionEvent && !this->m_stopped) { lockedThreadStore = true; ThreadSuspend::LockThreadStore(ThreadSuspend::SUSPEND_FOR_DEBUGGER); @@ -10807,7 +10819,7 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent) _ASSERTE(ThreadHoldsLock()); // Simply trap all Runtime threads if we're not already trying to. - if (!m_isBlockedOnGarbageCollectionEvent && !m_trappingRuntimeThreads) + if (!m_willBlockOnGarbageCollectionEvent && !m_trappingRuntimeThreads) { // If the RS sent an Async-break, then that's an explicit request. m_RSRequestedSync = TRUE; diff --git a/src/coreclr/src/debug/ee/debugger.h b/src/coreclr/src/debug/ee/debugger.h index 746f914..f2271d2 100644 --- a/src/coreclr/src/debug/ee/debugger.h +++ b/src/coreclr/src/debug/ee/debugger.h @@ -2954,10 +2954,12 @@ public: DWORD m_defines; DWORD m_mdDataStructureVersion; #ifndef DACCESS_COMPILE - virtual void BeforeGarbageCollection(); - virtual void AfterGarbageCollection(); + virtual void SuspendForGarbageCollectionStarted(); + virtual void SuspendForGarbageCollectionCompleted(); + virtual void ResumeForGarbageCollectionStarted(); #endif BOOL m_isBlockedOnGarbageCollectionEvent; + BOOL m_willBlockOnGarbageCollectionEvent; BOOL m_isGarbageCollectionEventsEnabled; // this latches m_isGarbageCollectionEventsEnabled in BeforeGarbageCollection so we can // guarantee the corresponding AfterGC event is sent even if the events are disabled during GC. diff --git a/src/coreclr/src/vm/dbginterface.h b/src/coreclr/src/vm/dbginterface.h index d0be137..9c5ae0d 100644 --- a/src/coreclr/src/vm/dbginterface.h +++ b/src/coreclr/src/vm/dbginterface.h @@ -409,8 +409,9 @@ public: virtual void EnumMemoryRegionsIfFuncEvalFrame(CLRDataEnumMemoryFlags flags, Frame * pFrame) = 0; #endif #ifndef DACCESS_COMPILE - virtual void BeforeGarbageCollection() = 0; - virtual void AfterGarbageCollection() = 0; + virtual void SuspendForGarbageCollectionStarted() = 0; + virtual void SuspendForGarbageCollectionCompleted() = 0; + virtual void ResumeForGarbageCollectionStarted() = 0; #endif virtual BOOL IsSynchronizing() = 0; }; diff --git a/src/coreclr/src/vm/gcenv.ee.cpp b/src/coreclr/src/vm/gcenv.ee.cpp index ff8f109..9898542 100644 --- a/src/coreclr/src/vm/gcenv.ee.cpp +++ b/src/coreclr/src/vm/gcenv.ee.cpp @@ -20,16 +20,18 @@ void GCToEEInterface::SuspendEE(SUSPEND_REASON reason) _ASSERTE(reason == SUSPEND_FOR_GC || reason == SUSPEND_FOR_GC_PREP); + g_pDebugInterface->SuspendForGarbageCollectionStarted(); + ThreadSuspend::SuspendEE((ThreadSuspend::SUSPEND_REASON)reason); - g_pDebugInterface->BeforeGarbageCollection(); + g_pDebugInterface->SuspendForGarbageCollectionCompleted(); } void GCToEEInterface::RestartEE(bool bFinishedGC) { WRAPPER_NO_CONTRACT; - g_pDebugInterface->AfterGarbageCollection(); + g_pDebugInterface->ResumeForGarbageCollectionStarted(); ThreadSuspend::RestartEE(bFinishedGC, TRUE); } -- 2.7.4