Code review feedback

4 files changed, 32 insertions, 88 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.