Code review feedback

10 files changed, 87 insertions, 90 deletions

diff --git a/src/debug/ee/CMakeLists.txt b/src/debug/ee/CMakeLists.txt
index 91ecbee65c..11a670285b 100644
--- a/src/debug/ee/CMakeLists.txt
+++ b/src/debug/ee/CMakeLists.txt
@@ -2,16 +2,6 @@ set(CMAKE_INCLUDE_CURRENT_DIR ON)
 
 add_definitions(-DFEATURE_NO_HOST)
 
-if(CMAKE_CONFIGURATION_TYPES) # multi-configuration generator?
-  foreach (Config DEBUG CHECKED)
-    set_property(DIRECTORY APPEND PROPERTY COMPILE_DEFINITIONS $<$<CONFIG:${Config}>:WRITE_BARRIER_CHECK=1>)
-  endforeach (Config)
-else()
-  if(UPPERCASE_CMAKE_BUILD_TYPE STREQUAL DEBUG OR UPPERCASE_CMAKE_BUILD_TYPE STREQUAL CHECKED)
-    add_definitions(-DWRITE_BARRIER_CHECK=1)
-  endif(UPPERCASE_CMAKE_BUILD_TYPE STREQUAL DEBUG OR UPPERCASE_CMAKE_BUILD_TYPE STREQUAL CHECKED)
-endif(CMAKE_CONFIGURATION_TYPES)
-
 include_directories(BEFORE ${VM_DIR})
 include_directories(BEFORE ${VM_DIR}/${ARCH_SOURCES_DIR})
 include_directories(BEFORE ${CMAKE_CURRENT_SOURCE_DIR})
diff --git a/src/debug/ee/controller.cpp b/src/debug/ee/controller.cpp
index 611b71c1ad..79979291cd 100644
--- a/src/debug/ee/controller.cpp
+++ b/src/debug/ee/controller.cpp
@@ -2568,8 +2568,7 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address,
                                          CONTEXT *context,
                                          DebuggerControllerQueue *pDcq,
                                          SCAN_TRIGGER stWhat,
-                                         TP_RESULT *pTpr,
-                                         bool* pHitDataBp)
+                                         TP_RESULT *pTpr)
 {
     CONTRACTL
     {
@@ -2586,7 +2585,6 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address,
         PRECONDITION(CheckPointer(context));
         PRECONDITION(CheckPointer(pDcq));
         PRECONDITION(CheckPointer(pTpr));
-        PRECONDITION(CheckPointer(pHitDataBp));
     }
     CONTRACTL_END;
 
@@ -2732,7 +2730,19 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address,
         tpr != TPR_TRIGGER_ONLY_THIS && 
         DebuggerDataBreakpoint::TriggerDataBreakpoint(thread, context))
     {
-        *pHitDataBp = true;
+        CONTEXT contextToAdjust;
+        bool adjustedContext = false;
+        memcpy(&contextToAdjust, context, sizeof(CONTEXT));
+        adjustedContext = g_pEEInterface->AdjustContextForWriteBarrierForDebugger(&contextToAdjust);
+        DebuggerDataBreakpoint *pDataBreakpoint = new (interopsafe) DebuggerDataBreakpoint(thread);
+        if (adjustedContext)
+        {
+            pDataBreakpoint->AddAndActivateNativePatchForAddress((CORDB_ADDRESS_TYPE*)GetIP(&contextToAdjust), FramePointer::MakeFramePointer(GetFP(&contextToAdjust)), true, DPT_DEFAULT_TRACE_TYPE);
+        }
+        else
+        {
+            pDcq->dcqEnqueue(pDataBreakpoint, FALSE);
+        }
     }
 #endif
 
@@ -2944,57 +2954,7 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE
 
     TP_RESULT tpr;
 
-    CONTEXT stash;
-    bool hitDataBp = false;
-    bool stashedContext = false;
-
-    used = ScanForTriggers((CORDB_ADDRESS_TYPE *)address, thread, context, &dcq, which, &tpr, &hitDataBp);
-
-    if (hitDataBp)
-    {
-        PCODE ip = GetIP(context);
-        LOG((LF_CORDB|LF_ENC, LL_EVERYTHING, "DataBreakpoint: My current IP is %p.\n", ip));
-#if defined(_TARGET_X86_)
-        bool withinWriteBarrierGroup = ((ip >= (PCODE) JIT_WriteBarrierGroup) && (ip <= (PCODE) JIT_WriteBarrierGroup_End));
-        bool withinPatchedWriteBarrierGroup = ((ip >= (PCODE) JIT_PatchedWriteBarrierGroup) && (ip <= (PCODE) JIT_PatchedWriteBarrierGroup_End));
-        if (withinWriteBarrierGroup || withinPatchedWriteBarrierGroup)
-        {
-            memcpy(&stash, context, sizeof(CONTEXT));
-            DWORD* esp = (DWORD*)context->Esp;
-            if (withinWriteBarrierGroup) {
-#if defined(WRITE_BARRIER_CHECK)
-                context->Ebp = *esp; esp++;
-                context->Ecx = *esp; esp++;
-#endif
-            }
-            context->Eip = *esp; esp++;
-            context->Esp = (DWORD)esp;
-            stashedContext = true;
-        }
-#elif defined(_TARGET_AMD64_)
-        if (IsIPInMarkedJitHelper((UINT_PTR)ip))
-        {
-            memcpy(&stash, context, sizeof(CONTEXT));
-            Thread::VirtualUnwindToFirstManagedCallFrame(context);
-            stashedContext = true;
-        }
-#else
-        // TODO - ARM/ARM64
-#endif
-        LOG((LF_CORDB|LF_ENC, LL_EVERYTHING, "DataBreakpoint: Unwound IP is %p.\n", GetIP(context)));
-        DebuggerDataBreakpoint *pDataBreakpoint = new (interopsafe) DebuggerDataBreakpoint(thread);
-        if (!stashedContext)
-        {
-            dcq.dcqEnqueue(pDataBreakpoint, FALSE);
-        }
-        else
-        {
-            pDataBreakpoint->AddAndActivateNativePatchForAddress((CORDB_ADDRESS_TYPE*)GetIP(context), FramePointer::MakeFramePointer(GetFP(context)), true, DPT_DEFAULT_TRACE_TYPE);
-            memcpy(context, &stash, sizeof(CONTEXT));
-            stashedContext = false;
-        }
-        LOG((LF_CORDB|LF_ENC, LL_EVERYTHING, "DataBreakpoint: Rewound IP is %p.\n", GetIP(context)));
-    }
+    used = ScanForTriggers((CORDB_ADDRESS_TYPE *)address, thread, context, &dcq, which, &tpr);
 
     LOG((LF_CORDB|LF_ENC, LL_EVERYTHING, "DC::DPOSS ScanForTriggers called and returned.\n"));
 
@@ -3069,12 +3029,6 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE
         reabort = thread->m_StateNC & Thread::TSNC_DebuggerReAbort;
 
         SENDIPCEVENT_END;
-        
-        if (stashedContext)
-        {
-            memcpy(context, &stash, sizeof(CONTEXT));
-            stashedContext = false;
-        }
 
         if (!atSafePlace)
             g_pDebugger->DecThreadsAtUnsafePlaces();
@@ -8634,6 +8588,7 @@ TP_RESULT DebuggerFuncEvalComplete::TriggerPatch(DebuggerControllerPatch *patch,
     // Restore the thread's context to what it was before we hijacked it for this func eval.
     CONTEXT *pCtx = GetManagedLiveCtx(thread);
     // TODO: Support other architectures
+    // If a data breakpoint is set while we hit a breakpoint inside a FuncEval, this will make sure the data breakpoint stays
 #ifdef _TARGET_X86_
     m_pDE->m_context.Dr0 = pCtx->Dr0;
     m_pDE->m_context.Dr1 = pCtx->Dr1;
diff --git a/src/debug/ee/controller.h b/src/debug/ee/controller.h
index 1710001d10..051817bb50 100644
--- a/src/debug/ee/controller.h
+++ b/src/debug/ee/controller.h
@@ -1079,8 +1079,7 @@ class DebuggerController
                                 CONTEXT *context,
                                 DebuggerControllerQueue *pDcq,
                                 SCAN_TRIGGER stWhat,
-                                TP_RESULT *pTpr,
-                                bool* pHitDataBp);
+                                TP_RESULT *pTpr);
 
 
     static DebuggerPatchSkip *ActivatePatchSkip(Thread *thread, 
@@ -1775,12 +1774,12 @@ private:
 class DebuggerDataBreakpoint : public DebuggerController
 {
 private:
-    CONTEXT context;
+    CONTEXT m_context;
 public:
     DebuggerDataBreakpoint(Thread* pThread) : DebuggerController(pThread, NULL)
     {
         LOG((LF_CORDB, LL_INFO10000, "D:DDBP: Data Breakpoint event created\n"));
-        memcpy(&context, g_pEEInterface->GetThreadFilterContext(pThread), sizeof(CONTEXT));
+        memcpy(&m_context, g_pEEInterface->GetThreadFilterContext(pThread), sizeof(CONTEXT));
     }
     
     virtual DEBUGGER_CONTROLLER_TYPE GetDCType(void)
@@ -1796,19 +1795,19 @@ public:
 #if defined(_TARGET_X86_) || defined(_TARGET_AMD64_)
         CONTEXT *context = g_pEEInterface->GetThreadFilterContext(thread);
 #ifdef _TARGET_X86_
-        context->Dr0 = this->context.Dr0;
-        context->Dr1 = this->context.Dr1;
-        context->Dr2 = this->context.Dr2;
-        context->Dr3 = this->context.Dr3;
-        context->Dr6 = this->context.Dr6;
-        context->Dr7 = this->context.Dr7;
+        context->Dr0 = this->m_context.Dr0;
+        context->Dr1 = this->m_context.Dr1;
+        context->Dr2 = this->m_context.Dr2;
+        context->Dr3 = this->m_context.Dr3;
+        context->Dr6 = this->m_context.Dr6;
+        context->Dr7 = this->m_context.Dr7;
 #elif defined(_TARGET_AMD64_)
-        context->Dr0 = this->context.Dr0;
-        context->Dr1 = this->context.Dr1;
-        context->Dr2 = this->context.Dr2;
-        context->Dr3 = this->context.Dr3;
-        context->Dr6 = this->context.Dr6;
-        context->Dr7 = this->context.Dr7;
+        context->Dr0 = this->m_context.Dr0;
+        context->Dr1 = this->m_context.Dr1;
+        context->Dr2 = this->m_context.Dr2;
+        context->Dr3 = this->m_context.Dr3;
+        context->Dr6 = this->m_context.Dr6;
+        context->Dr7 = this->m_context.Dr7;
 #endif
 #endif
 #endif
diff --git a/src/debug/ee/debugger.cpp b/src/debug/ee/debugger.cpp
index 44b3764118..a1eed00136 100644
--- a/src/debug/ee/debugger.cpp
+++ b/src/debug/ee/debugger.cpp
@@ -5214,7 +5214,7 @@ void Debugger::SendSyncCompleteIPCEvent(bool isEESuspendedForGC)
     {
         SO_NOT_MAINLINE;
         NOTHROW;
-        if (isEESuspendedForGC) { GC_NOTRIGGER; } else { GC_TRIGGERS; }
+        GC_NOTRIGGER;
         PRECONDITION(ThreadHoldsLock());
 
         // Anyone sending the synccomplete must hold the TSL.
diff --git a/src/vm/eedbginterface.h b/src/vm/eedbginterface.h
index 38b43b729a..f25884e001 100644
--- a/src/vm/eedbginterface.h
+++ b/src/vm/eedbginterface.h
@@ -373,6 +373,10 @@ public:
     virtual void ObjectRefFlush(Thread *pThread) = 0;
 #endif
 #endif
+
+#ifndef DACCESS_COMPILE
+    virtual BOOL AdjustContextForWriteBarrierForDebugger(CONTEXT* context) = 0;
+#endif
 };
 
 #endif // _eedbginterface_h_
diff --git a/src/vm/eedbginterfaceimpl.cpp b/src/vm/eedbginterfaceimpl.cpp
index d00d4386f9..cd5964352d 100644
--- a/src/vm/eedbginterfaceimpl.cpp
+++ b/src/vm/eedbginterfaceimpl.cpp
@@ -1630,4 +1630,14 @@ void EEDbgInterfaceImpl::ObjectRefFlush(Thread *pThread)
 #endif
 #endif
 
+#ifndef DACCESS_COMPILE
+
+BOOL AdjustContextForWriteBarrier(EXCEPTION_RECORD *pExceptionRecord, CONTEXT *pContext);
+BOOL EEDbgInterfaceImpl::AdjustContextForWriteBarrierForDebugger(CONTEXT* context)
+{
+    WRAPPER_NO_CONTRACT;
+    return AdjustContextForWriteBarrier(nullptr, context);
+}
+#endif
+
 #endif // DEBUGGING_SUPPORTED
diff --git a/src/vm/eedbginterfaceimpl.h b/src/vm/eedbginterfaceimpl.h
index da05169e1f..6fee904eef 100644
--- a/src/vm/eedbginterfaceimpl.h
+++ b/src/vm/eedbginterfaceimpl.h
@@ -343,6 +343,10 @@ public:
     virtual void ObjectRefFlush(Thread *pThread);
 #endif
 #endif
+
+#ifndef DACCESS_COMPILE
+    virtual BOOL AdjustContextForWriteBarrierForDebugger(CONTEXT* context);
+#endif
 };
 
 #endif // DEBUGGING_SUPPORTED
diff --git a/src/vm/excep.cpp b/src/vm/excep.cpp
index 013c8fe286..b95563a1d1 100644
--- a/src/vm/excep.cpp
+++ b/src/vm/excep.cpp
@@ -6966,6 +6966,37 @@ AdjustContextForWriteBarrier(
 {
     WRAPPER_NO_CONTRACT;
 
+    if (pExceptionRecord == nullptr)
+    {
+        PCODE ip = GetIP(pContext);
+#if defined(_TARGET_X86_)
+        bool withinWriteBarrierGroup = ((ip >= (PCODE) JIT_WriteBarrierGroup) && (ip <= (PCODE) JIT_WriteBarrierGroup_End));
+        bool withinPatchedWriteBarrierGroup = ((ip >= (PCODE) JIT_PatchedWriteBarrierGroup) && (ip <= (PCODE) JIT_PatchedWriteBarrierGroup_End));
+        if (withinWriteBarrierGroup || withinPatchedWriteBarrierGroup)
+        {
+            DWORD* esp = (DWORD*)pContext->Esp;
+            if (withinWriteBarrierGroup) {
+#if defined(WRITE_BARRIER_CHECK)
+                pContext->Ebp = *esp; esp++;
+                pContext->Ecx = *esp; esp++;
+#endif
+            }
+            pContext->Eip = *esp; esp++;
+            pContext->Esp = (DWORD)esp;
+            return TRUE;
+        }
+#elif defined(_TARGET_AMD64_)
+        if (IsIPInMarkedJitHelper((UINT_PTR)ip))
+        {
+            Thread::VirtualUnwindToFirstManagedCallFrame(pContext);
+            return TRUE;
+        }
+#else
+        // TODO - ARM/ARM64
+#endif
+        return FALSE;
+    }
+
 #if defined(_TARGET_X86_) && !defined(PLATFORM_UNIX)
     void* f_IP = (void *)GetIP(pContext);
 
diff --git a/src/vm/i386/jithelp.asm b/src/vm/i386/jithelp.asm
index 09b3800fa9..27b866881a 100644
--- a/src/vm/i386/jithelp.asm
+++ b/src/vm/i386/jithelp.asm
@@ -130,8 +130,6 @@ ENDM
 
 ; The code here is tightly coupled with AdjustContextForWriteBarrier, if you change
 ; anything here, you might need to change AdjustContextForWriteBarrier as well
-; The code here is tightly coupled with DebuggerController::DispatchPatchOrSingleStep, if you change
-; anything here, you might need to change DebuggerController::DispatchPatchOrSingleStep as well
 WriteBarrierHelper MACRO rg
         ALIGN 4
 
diff --git a/src/vm/threadsuspend.cpp b/src/vm/threadsuspend.cpp
index 1902d82ba6..fd070a666d 100644
--- a/src/vm/threadsuspend.cpp
+++ b/src/vm/threadsuspend.cpp
@@ -7108,6 +7108,12 @@ retry_for_debugger:
             || Thread::ThreadsAtUnsafePlaces()
 #ifdef DEBUGGING_SUPPORTED  // seriously?  When would we want to disable debugging support? :)
              || (CORDebuggerAttached() && 
+            // When the debugger is synchronizing, trying to perform a GC could deadlock. The GC has the
+            // threadstore lock and synchronization cannot complete until the debugger can get the 
+            // threadstore lock. However the GC can not complete until it sends the BeforeGarbageCollection
+            // event, and the event can not be sent until the debugger is synchronized. In order to break
+            // this deadlock cycle the GC must give up the threadstore lock, allow the debugger to synchronize, 
+            // then try again.
                  (g_pDebugInterface->ThreadsAtUnsafePlaces() || g_pDebugInterface->IsSynchronizing()))
 #endif // DEBUGGING_SUPPORTED
             )