Fix data breakpoint issues (#46763)

* Ignore data breakpoint during GC * Simplify disabling DebuggerDataBreakpoint during GC * Fix destroy pinned handle * Fix CordbModule::IsMappedLayout assert to allow using VS with debug/checked runtime builds Co-authored-by: Chuck Ries <chuckr@microsoft.com>
dotnet · Jan 14, 2021 · 1d1a0f2 · 1d1a0f2
1 parent 863b326
commit 1d1a0f2
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 22 deletions.
diff --git a/src/coreclr/debug/di/divalue.cpp b/src/coreclr/debug/di/divalue.cpp
@@ -4420,14 +4420,7 @@ HRESULT CordbHandleValue::Dispose()
                           m_appdomain->GetADToken());
 
     event.DisposeHandle.vmObjectHandle = vmObjHandle;
-    if (m_handleType == HANDLE_STRONG)
-    {
-        event.DisposeHandle.fStrong = TRUE;
-    }
-    else
-    {
-        event.DisposeHandle.fStrong = FALSE;
-    }
+    event.DisposeHandle.handleType = m_handleType;
 
     // Note: one-way event here...
     hr = process->SendIPCEvent(&event, sizeof(DebuggerIPCEvent));

diff --git a/src/coreclr/debug/di/module.cpp b/src/coreclr/debug/di/module.cpp
@@ -2758,18 +2758,20 @@ HRESULT CordbModule::GetJITCompilerFlags(DWORD *pdwFlags )
 
 HRESULT CordbModule::IsMappedLayout(BOOL *isMapped)
 {
+    PUBLIC_API_ENTRY(this);
     VALIDATE_POINTER_TO_OBJECT(isMapped, BOOL*);
     FAIL_IF_NEUTERED(this);
 
     HRESULT hr = S_OK;
     CordbProcess *pProcess = GetProcess();
 
     ATT_REQUIRE_STOPPED_MAY_FAIL(pProcess);
-    PUBLIC_API_BEGIN(pProcess);
+
+    EX_TRY
     {
         hr = pProcess->GetDAC()->IsModuleMapped(m_vmModule, isMapped);
     }
-    PUBLIC_API_END(hr);
+    EX_CATCH_HRESULT(hr);
 
     return hr;
 }

diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp
@@ -2708,10 +2708,21 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address,
 #ifdef FEATURE_DATABREAKPOINT
     if (stWhat & ST_SINGLE_STEP &&
         tpr != TPR_TRIGGER_ONLY_THIS &&
-        DebuggerDataBreakpoint::TriggerDataBreakpoint(thread, context))
+        DebuggerDataBreakpoint::IsDataBreakpoint(thread, context))
     {
-        DebuggerDataBreakpoint *pDataBreakpoint = new (interopsafe) DebuggerDataBreakpoint(thread);
-        pDcq->dcqEnqueue(pDataBreakpoint, FALSE);
+        if (g_pDebugger->m_isSuspendedForGarbageCollection)
+        {
+            // The debugger is not interested in Data Breakpoints during garbage collection
+            // We can safely ignore them since the Data Breakpoints are now on pinned objects
+            LOG((LF_CORDB, LL_INFO10000, "D:DDBP: Ignoring data breakpoint while suspended for GC \n"));
+
+            used = DPOSS_USED_WITH_NO_EVENT;
+        }
+        else if(DebuggerDataBreakpoint::TriggerDataBreakpoint(thread, context))
+        {
+            DebuggerDataBreakpoint *pDataBreakpoint = new (interopsafe) DebuggerDataBreakpoint(thread);
+            pDcq->dcqEnqueue(pDataBreakpoint, FALSE);
+        }
     }
 #endif
 
@@ -8977,12 +8988,9 @@ bool DebuggerContinuableExceptionBreakpoint::SendEvent(Thread *thread, bool fIpC
 
 #ifdef FEATURE_DATABREAKPOINT
 
-/* static */ bool DebuggerDataBreakpoint::TriggerDataBreakpoint(Thread *thread, CONTEXT * pContext)
+/* static */ bool DebuggerDataBreakpoint::IsDataBreakpoint(Thread *thread, CONTEXT * pContext)
 {
-    LOG((LF_CORDB, LL_INFO10000, "D::DDBP: Doing TriggerDataBreakpoint...\n"));
-
     bool hitDataBp = false;
-    bool result = false;
 #ifdef TARGET_UNIX
     #error Not supported
 #endif // TARGET_UNIX
@@ -8996,6 +9004,15 @@ bool DebuggerContinuableExceptionBreakpoint::SendEvent(Thread *thread, bool fIpC
 #else // defined(TARGET_X86) || defined(TARGET_AMD64)
     #error Not supported
 #endif // defined(TARGET_X86) || defined(TARGET_AMD64)
+    return hitDataBp;
+}
+
+/* static */ bool DebuggerDataBreakpoint::TriggerDataBreakpoint(Thread *thread, CONTEXT * pContext)
+{
+    LOG((LF_CORDB, LL_INFO10000, "D::DDBP: Doing TriggerDataBreakpoint...\n"));
+
+    bool hitDataBp = IsDataBreakpoint(thread, pContext);
+    bool result = false;
     if (hitDataBp)
     {
         if (g_pDebugger->IsThreadAtSafePlace(thread))

diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h
@@ -1811,6 +1811,7 @@ class DebuggerDataBreakpoint : public DebuggerController
         return true;
     }
 
+    static bool IsDataBreakpoint(Thread *thread, CONTEXT * pContext);
     static bool TriggerDataBreakpoint(Thread *thread, CONTEXT * pContext);
 };
 

diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp
@@ -935,6 +935,7 @@ Debugger::Debugger()
     m_forceNonInterceptable(FALSE),
     m_pLazyData(NULL),
     m_defines(_defines),
+    m_isSuspendedForGarbageCollection(FALSE),
     m_isBlockedOnGarbageCollectionEvent(FALSE),
     m_willBlockOnGarbageCollectionEvent(FALSE),
     m_isGarbageCollectionEventsEnabled(FALSE),
@@ -5944,6 +5945,8 @@ void Debugger::SuspendForGarbageCollectionCompleted()
     }
     CONTRACTL_END;
 
+    this->m_isSuspendedForGarbageCollection = FALSE;
+
     if (!CORDebuggerAttached() || !this->m_isGarbageCollectionEventsEnabledLatch)
     {
         return;
@@ -5981,6 +5984,7 @@ void Debugger::ResumeForGarbageCollectionStarted()
     }
     CONTRACTL_END;
 
+    this->m_isSuspendedForGarbageCollection = TRUE;
     if (!CORDebuggerAttached() || !this->m_isGarbageCollectionEventsEnabledLatch)
     {
         return;
@@ -11345,14 +11349,21 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent)
         {
             // DISPOSE an object handle
             OBJECTHANDLE objectHandle = pEvent->DisposeHandle.vmObjectHandle.GetRawPtr();
+            CorDebugHandleType handleType = pEvent->DisposeHandle.handleType;
 
-            if (pEvent->DisposeHandle.fStrong == TRUE)
+            switch (handleType)
             {
+            case HANDLE_STRONG:
                 DestroyStrongHandle(objectHandle);
-            }
-            else
-            {
+                break;
+            case HANDLE_WEAK_TRACK_RESURRECTION:
                 DestroyLongWeakHandle(objectHandle);
+                break;
+            case HANDLE_PINNED:
+                DestroyPinningHandle(objectHandle);
+                break;
+            default:
+                pEvent->hr = E_INVALIDARG;
             }
             break;
         }

diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h
@@ -2918,6 +2918,7 @@ class Debugger : public DebugInterface
     virtual void SuspendForGarbageCollectionCompleted();
     virtual void ResumeForGarbageCollectionStarted();
 #endif
+    BOOL m_isSuspendedForGarbageCollection;
     BOOL m_isBlockedOnGarbageCollectionEvent;
     BOOL m_willBlockOnGarbageCollectionEvent;
     BOOL m_isGarbageCollectionEventsEnabled;

diff --git a/src/coreclr/debug/inc/dbgipcevents.h b/src/coreclr/debug/inc/dbgipcevents.h
@@ -2309,7 +2309,7 @@ struct MSLAYOUT DebuggerIPCEvent
         struct MSLAYOUT
         {
             VMPTR_OBJECTHANDLE vmObjectHandle;
-            BOOL            fStrong;
+            CorDebugHandleType handleType;
         } DisposeHandle;
 
         struct MSLAYOUT
-Original file line number
+Diff line change
@@ Expand Up / @@ -1811,6 +1811,7 @@ class DebuggerDataBreakpoint : public DebuggerController @@
             return true;
         }
+        static bool IsDataBreakpoint(Thread *thread, CONTEXT * pContext);
         static bool TriggerDataBreakpoint(Thread *thread, CONTEXT * pContext);
     };
@@ Expand Down @@