From 24acddae18dedf44730f0db00a2d25dc6568eff0 Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Wed, 17 Oct 2018 18:01:55 -0700 Subject: Code review feedback --- src/debug/ee/CMakeLists.txt | 10 ------ src/debug/ee/controller.cpp | 77 ++++++++++----------------------------------- src/debug/ee/controller.h | 31 +++++++++--------- src/debug/ee/debugger.cpp | 2 +- 4 files changed, 32 insertions(+), 88 deletions(-) (limited to 'src/debug') 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 $<$: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. -- cgit v1.2.3