From 7ff15c7ce327ecbcaced599ca90ced603c878fcc Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Sun, 29 Sep 2024 15:41:27 -0400 Subject: [PATCH 01/26] Initial changes to support in-place single-stepping over call instructions from out of process --- src/coreclr/debug/di/process.cpp | 28 +++-- src/coreclr/debug/ee/controller.cpp | 178 ++++++++++++++++++++-------- src/coreclr/debug/ee/controller.h | 10 +- src/coreclr/debug/ee/debugger.cpp | 34 +++++- src/coreclr/debug/ee/debugger.h | 5 +- src/coreclr/debug/ee/walker.h | 6 + src/coreclr/vm/dbginterface.h | 4 +- 7 files changed, 192 insertions(+), 73 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index fffd6df484825f..075c23db7d3f66 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11224,10 +11224,9 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) TADDR lsContextAddr = (TADDR)context.Rcx; DWORD contextSize = (DWORD)context.Rdx; - // Read the expected Rip and Rsp from the thread context. This is used to - // validate the context read from the left-side. - TADDR expectedRip = (TADDR)context.R8; - TADDR expectedRsp = (TADDR)context.R9; + CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)context.R8; + bool fIsInPlaceSingleStep = (bool)((context.R9>>8)&0x1); + PRD_TYPE opcode = (PRD_TYPE)(context.R9&0xFF); if (contextSize == 0 || contextSize > sizeof(CONTEXT) + 25000) { @@ -11259,15 +11258,6 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) ThrowHR(ERROR_PARTIAL_COPY); } - if (pContext->Rip != expectedRip || pContext->Rsp != expectedRsp) - { - _ASSERTE(!"ReadVirtual unexpectedly returned mismatched Rip and Rsp registers"); - - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - ReadVirtual unexpectedly returned mismatched Rip and Rsp registers\n")); - - ThrowHR(E_UNEXPECTED); - } - // TODO: Ideally we would use ICorDebugMutableDataTarget::SetThreadContext however this API currently only handles the legacy context. // We should combine the following code with the shared implementation @@ -11346,6 +11336,18 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from SetThreadContext\n")); ThrowHR(HRESULT_FROM_WIN32(lastError)); } + + if (fIsInPlaceSingleStep) + { + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - address=0x%p opcode=0x%x\n", patchSkipAddr, opcode)); + EX_TRY + { + TargetBuffer tb((void*)patchSkipAddr, sizeof(BYTE)); + SafeWriteBuffer(tb, (const BYTE*)&opcode); // throws + } + EX_CATCH_HRESULT(hr); + SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); + } #else #error Platform not supported #endif diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 4ce03c05704f75..e00052b48d217b 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -2838,7 +2838,7 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, // DebuggerController's TriggerPatch). Put any DCs that are interested into a queue and then calls // SendEvent on each. // Note that control will not return from this function in the case of EnC remap -DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTEXT *context, CORDB_ADDRESS_TYPE *address, SCAN_TRIGGER which) +DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTEXT *context, CORDB_ADDRESS_TYPE *address, SCAN_TRIGGER which, DebuggerPatchSkip **ppDebuggerPatchSkip) { CONTRACT(DPOSS_ACTION) { @@ -3032,7 +3032,11 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE } #endif - ActivatePatchSkip(thread, dac_cast(GetIP(pCtx)), FALSE); + DebuggerPatchSkip* pDebuggerPatchSkip = ActivatePatchSkip(thread, dac_cast(GetIP(pCtx)), FALSE); + if (ppDebuggerPatchSkip != nullptr) + { + *ppDebuggerPatchSkip = pDebuggerPatchSkip; + } lockController.Release(); @@ -4132,7 +4136,8 @@ void ThisFunctionMayHaveTriggerAGC() bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException, CONTEXT *pContext, DWORD dwCode, - Thread *pCurThread) + Thread *pCurThread, + DebuggerPatchSkip **ppDebuggerPatchSkip) { CONTRACTL { @@ -4303,7 +4308,8 @@ bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException, result = DebuggerController::DispatchPatchOrSingleStep(pCurThread, pContext, ip, - ST_PATCH); + ST_PATCH, + ppDebuggerPatchSkip); LOG((LF_CORDB, LL_EVERYTHING, "DC::DNE DispatchPatch call returned\n")); // If we detached, we should remove all our breakpoints. So if we try @@ -4434,12 +4440,23 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, NativeWalker::DecodeInstructionForPatchSkip(patchBypassRX, &(m_instrAttrib)); +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + if (g_pDebugInterface->IsOutOfProcessSetContextEnabled() && m_instrAttrib.m_fIsCall) + { + m_instrAttrib.m_fInPlaceSS = true; + } +#endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + #if defined(TARGET_AMD64) // The code below handles RIP-relative addressing on AMD64. The original implementation made the assumption that // we are only using RIP-relative addressing to access read-only data (see VSW 246145 for more information). This // has since been expanded to handle RIP-relative writes as well. - if (m_instrAttrib.m_dwOffsetToDisp != 0) + if (m_instrAttrib.m_dwOffsetToDisp != 0 +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + && (!m_instrAttrib.m_fInPlaceSS || !m_instrAttrib.m_fIsCall) +#endif + ) { _ASSERTE(m_instrAttrib.m_cbInstr != 0); @@ -4545,13 +4562,18 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, patchBypassRX = NativeWalker::SetupOrSimulateInstructionForPatchSkip(context, m_pSharedPatchBypassBuffer, (const BYTE *)patch->address, patch->opcode); #endif //TARGET_ARM64 - //set eip to point to buffer... - SetIP(context, (PCODE)patchBypassRX); +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + if (!m_instrAttrib.m_fInPlaceSS || !m_instrAttrib.m_fIsCall) +#endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + { + //set eip to point to buffer... + SetIP(context, (PCODE)patchBypassRX); - if (context ==(T_CONTEXT*) &c) - thread->SetThreadContext(&c); + if (context ==(T_CONTEXT*) &c) + thread->SetThreadContext(&c); - LOG((LF_CORDB, LL_INFO10000, "DPS::DPS Bypass at %p for opcode 0x%zx \n", patchBypassRX, patch->opcode)); + LOG((LF_CORDB, LL_INFO10000, "DPS::DPS Bypass at %p for opcode 0x%zx \n", patchBypassRX, patch->opcode)); + } // // Turn on single step (if the platform supports it) so we can @@ -4770,14 +4792,20 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont { // Fixup return address on stack #if defined(TARGET_X86) || defined(TARGET_AMD64) - SIZE_T *sp = (SIZE_T *) GetSP(context); +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + if (!m_instrAttrib.m_fInPlaceSS) +#endif + { + // Fixup return address on stack + SIZE_T *sp = (SIZE_T *) GetSP(context); - LOG((LF_CORDB, LL_INFO10000, - "Bypass call return address redirected from %p\n", *sp)); + LOG((LF_CORDB, LL_INFO10000, + "Bypass call return address redirected from %p\n", *sp)); - *sp -= patchBypass - (BYTE*)m_address; + *sp -= patchBypass - (BYTE*)m_address; - LOG((LF_CORDB, LL_INFO10000, "to %p\n", *sp)); + LOG((LF_CORDB, LL_INFO10000, "to %p\n", *sp)); + } #else PORTABILITY_ASSERT("DebuggerPatchSkip::TriggerExceptionHook -- return address fixup NYI"); #endif @@ -4797,10 +4825,17 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont ((size_t)GetIP(context) > (size_t)patchBypass && (size_t)GetIP(context) <= (size_t)(patchBypass + MAX_INSTRUCTION_LENGTH + 1))) { - LOG((LF_CORDB, LL_INFO10000, "Bypass instruction redirected because still in skip area.\n" - "\tm_fIsCall = %s, patchBypass = %p, m_address = %p\n", - (m_instrAttrib.m_fIsCall ? "true" : "false"), patchBypass, m_address)); - SetIP(context, (PCODE)((BYTE *)GetIP(context) - (patchBypass - (BYTE *)m_address))); +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + if (!m_instrAttrib.m_fInPlaceSS || !m_instrAttrib.m_fIsCall) + { +#endif + LOG((LF_CORDB, LL_INFO10000, "Bypass instruction redirected because still in skip area.\n" + "\tm_fIsCall = %s, patchBypass = %p, m_address = %p\n", + (m_instrAttrib.m_fIsCall ? "true" : "false"), patchBypass, m_address)); + SetIP(context, (PCODE)((BYTE *)GetIP(context) - (patchBypass - (BYTE *)m_address))); +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + } +#endif } else { @@ -4865,47 +4900,90 @@ bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) LOG((LF_CORDB,LL_INFO10000, "DPS::TSS: basically a no-op\n")); #if defined(TARGET_AMD64) - // Dev11 91932: for RIP-relative writes we need to copy the value that was written in our buffer to the actual address - _ASSERTE(m_pSharedPatchBypassBuffer); - if (m_pSharedPatchBypassBuffer->RipTargetFixup) +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + if (!m_instrAttrib.m_fInPlaceSS || !m_instrAttrib.m_fIsCall) +#endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) { - _ASSERTE(m_pSharedPatchBypassBuffer->RipTargetFixupSize); + // Dev11 91932: for RIP-relative writes we need to copy the value that was written in our buffer to the actual address + _ASSERTE(m_pSharedPatchBypassBuffer); + if (m_pSharedPatchBypassBuffer->RipTargetFixup) + { + _ASSERTE(m_pSharedPatchBypassBuffer->RipTargetFixupSize); - BYTE* bufferBypass = m_pSharedPatchBypassBuffer->BypassBuffer; - BYTE fixupSize = m_pSharedPatchBypassBuffer->RipTargetFixupSize; - UINT_PTR targetFixup = m_pSharedPatchBypassBuffer->RipTargetFixup; + BYTE* bufferBypass = m_pSharedPatchBypassBuffer->BypassBuffer; + BYTE fixupSize = m_pSharedPatchBypassBuffer->RipTargetFixupSize; + UINT_PTR targetFixup = m_pSharedPatchBypassBuffer->RipTargetFixup; - switch (fixupSize) - { - case 1: - *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); - break; + switch (fixupSize) + { + case 1: + *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); + break; - case 2: - *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); - break; + case 2: + *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); + break; - case 4: - *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); - break; + case 4: + *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); + break; - case 8: - *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); - break; + case 8: + *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); + break; - case 16: - case 32: - case 64: - memcpy(reinterpret_cast(targetFixup), bufferBypass, fixupSize); - break; + case 16: + case 32: + case 64: + memcpy(reinterpret_cast(targetFixup), bufferBypass, fixupSize); + break; - default: - _ASSERTE(!"bad operand size. If you hit this and it was because we need to process instructions with larger \ - relative immediates, make sure to update the SharedPatchBypassBuffer size, the DebuggerHeapExecutableMemoryAllocator, \ - and structures depending on DBG_MAX_EXECUTABLE_ALLOC_SIZE."); + default: + _ASSERTE(!"bad operand size. If you hit this and it was because we need to process instructions with larger \ + relative immediates, make sure to update the SharedPatchBypassBuffer size, the DebuggerHeapExecutableMemoryAllocator, \ + and structures depending on DBG_MAX_EXECUTABLE_ALLOC_SIZE."); + } } } -#endif +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + else if (m_instrAttrib.m_fInPlaceSS && m_instrAttrib.m_fIsCall) + { + LOG((LF_CORDB, LL_INFO10000, "This is an in-place single-step, all we want to do is back-patch the breakpoint\n")); + { + LPVOID baseAddress = (LPVOID)m_address; + DWORD oldProt; + + if (!VirtualProtect(baseAddress, + CORDbg_BREAK_INSTRUCTION_SIZE, + PAGE_EXECUTE_READWRITE, &oldProt)) + { + // we may be seeing unwriteable directly mapped executable memory. + // let's try copy-on-write instead, + if (!VirtualProtect(baseAddress, + CORDbg_BREAK_INSTRUCTION_SIZE, + PAGE_EXECUTE_WRITECOPY, &oldProt)) + { + _ASSERTE(!"VirtualProtect of code page failed"); + goto Error; + } + } + + CORDbgInsertBreakpoint(m_address); + LOG((LF_CORDB, LL_EVERYTHING, "DPS::TriggerSingleStep Breakpoint was re-inserted at %p\n", + m_address)); + + if (!VirtualProtect(baseAddress, + CORDbg_BREAK_INSTRUCTION_SIZE, + oldProt, &oldProt)) + { + _ASSERTE(!"VirtualProtect of code page failed"); + goto Error; + } + } + } +Error: +#endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) +#endif // defined(TARGET_AMD64) LOG((LF_CORDB,LL_INFO10000, "DPS::TSS: triggered, about to delete\n")); TRACE_FREE(this); diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 5c14bb201554d9..55268d6452c914 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -1072,7 +1072,8 @@ class DebuggerController static bool DispatchNativeException(EXCEPTION_RECORD *exception, CONTEXT *context, DWORD code, - Thread *thread); + Thread *thread, + DebuggerPatchSkip **ppDebuggerPatchSkip = nullptr); static bool DispatchUnwind(Thread *thread, MethodDesc *fd, DebuggerJitInfo * pDJI, SIZE_T offset, @@ -1120,7 +1121,8 @@ class DebuggerController static DPOSS_ACTION DispatchPatchOrSingleStep(Thread *thread, CONTEXT *context, CORDB_ADDRESS_TYPE *ip, - SCAN_TRIGGER which); + SCAN_TRIGGER which, + DebuggerPatchSkip **ppDebuggerPatchSkip = nullptr); static int GetNumberOfPatches() @@ -1497,6 +1499,10 @@ class DebuggerPatchSkip : public DebuggerController BYTE* patchBypass = m_pSharedPatchBypassBuffer->PatchBypass; return (CORDB_ADDRESS_TYPE *)patchBypass; } +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + BOOL IsInPlaceSingleStep() { return m_instrAttrib.m_fIsCall && m_instrAttrib.m_fInPlaceSS; } + CORDB_ADDRESS_TYPE* GetAddress() { return m_address; } +#endif #endif // !FEATURE_EMULATE_SINGLESTEP }; diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 695f48752c16c3..b547fa109667ac 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -5462,6 +5462,7 @@ bool Debugger::FirstChanceNativeException(EXCEPTION_RECORD *exception, } bool retVal; + DebuggerPatchSkip *pDebuggerPatchSkip = nullptr; { // Don't stop for native debugging anywhere inside our inproc-Filters. @@ -5470,7 +5471,7 @@ bool Debugger::FirstChanceNativeException(EXCEPTION_RECORD *exception, if (!CORDBUnrecoverableError(this)) { retVal = DebuggerController::DispatchNativeException(exception, context, - code, thread); + code, thread, &pDebuggerPatchSkip); } else { @@ -5481,7 +5482,14 @@ bool Debugger::FirstChanceNativeException(EXCEPTION_RECORD *exception, #if defined(OUT_OF_PROCESS_SETTHREADCONTEXT) && !defined(DACCESS_COMPILE) if (retVal && fIsVEH) { - SendSetThreadContextNeeded(context); + if (pDebuggerPatchSkip != nullptr && pDebuggerPatchSkip->IsInPlaceSingleStep()) + { + SendSetThreadContextNeeded(context, pDebuggerPatchSkip); + } + else + { + SendSetThreadContextNeeded(context); + } } #endif return retVal; @@ -16691,7 +16699,7 @@ void Debugger::StartCanaryThread() #ifndef DACCESS_COMPILE #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT -void Debugger::SendSetThreadContextNeeded(CONTEXT *context) +void Debugger::SendSetThreadContextNeeded(CONTEXT *context, DebuggerPatchSkip *patchSkip) { STATIC_CONTRACT_NOTHROW; STATIC_CONTRACT_GC_NOTRIGGER; @@ -16742,11 +16750,29 @@ void Debugger::SendSetThreadContextNeeded(CONTEXT *context) // adjust context size if the context pointer is not aligned with the buffer we allocated contextSize -= (DWORD)((BYTE*)pContext-(BYTE*)pBuffer); + bool fIsInPlaceSingleStep = patchSkip != nullptr && patchSkip->IsInPlaceSingleStep(); + CORDB_ADDRESS_TYPE *patchSkipAddr = patchSkip != nullptr && fIsInPlaceSingleStep ? patchSkip->GetAddress() : nullptr; + _ASSERTE(!fIsInPlaceSingleStep || pContext->Rip == patchSkipAddr); + + PRD_TYPE opcode = 0xcc; + if (fIsInPlaceSingleStep) + { + DebuggerControllerPatch *patch = DebuggerController::GetPatchTable()->GetPatch((CORDB_ADDRESS_TYPE *)pContext->Rip); + if (patch != NULL) + { + LOG((LF_CORDB, LL_INFO10000, "D::SSTCN Patch found at address 0x%p opcode=0x%x\n", pContext->Rip, patch->opcode)); + opcode = patch->opcode; + } + } + // send the context to the right side LOG((LF_CORDB, LL_INFO10000, "D::SSTCN ContextFlags=0x%X contextSize=%d..\n", contextFlags, contextSize)); EX_TRY { - SetThreadContextNeededFlare((TADDR)pContext, contextSize, pContext->Rip, pContext->Rsp); + SetThreadContextNeededFlare((TADDR)pContext, + contextSize, + patchSkipAddr, + (((fIsInPlaceSingleStep&0x1)<<8)|(opcode&0xFF))); } EX_CATCH { diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index d9513cac8f5abc..c76235aea22f43 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -1873,7 +1873,7 @@ extern "C" void __stdcall NotifyRightSideOfSyncCompleteFlare(void); extern "C" void __stdcall NotifySecondChanceReadyForDataFlare(void); #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) -extern "C" void __stdcall SetThreadContextNeededFlare(TADDR pContext, DWORD size, TADDR Rip, TADDR Rsp); +extern "C" void __stdcall SetThreadContextNeededFlare(TADDR pContext, DWORD size, CORDB_ADDRESS_TYPE* patchSkipAddr, DWORD opcode); #else #error Platform not supported #endif @@ -3058,7 +3058,8 @@ class Debugger : public DebugInterface private: BOOL m_fOutOfProcessSetContextEnabled; public: - void SendSetThreadContextNeeded(CONTEXT *context); + // Used by Debugger::FirstChanceNativeException to update the context from out of process + void SendSetThreadContextNeeded(CONTEXT *context, DebuggerPatchSkip *patchSkip = nullptr); BOOL IsOutOfProcessSetContextEnabled(); }; diff --git a/src/coreclr/debug/ee/walker.h b/src/coreclr/debug/ee/walker.h index 0c901cab466570..7dfd9f2c39f297 100644 --- a/src/coreclr/debug/ee/walker.h +++ b/src/coreclr/debug/ee/walker.h @@ -41,6 +41,9 @@ struct InstructionAttribute bool m_fIsRelBranch; // is this a relative branch (either a call or a jump)? bool m_fIsWrite; // does the instruction write to an address? +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + bool m_fInPlaceSS; // is this an in-place single-step instruction? +#endif DWORD m_cbInstr; // the size of the instruction DWORD m_cbDisp; // the size of the displacement @@ -55,6 +58,9 @@ struct InstructionAttribute m_fIsAbsBranch = false; m_fIsRelBranch = false; m_fIsWrite = false; +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + m_fInPlaceSS = false; +#endif m_cbInstr = 0; m_cbDisp = 0; m_dwOffsetToDisp = 0; diff --git a/src/coreclr/vm/dbginterface.h b/src/coreclr/vm/dbginterface.h index 9a58eea52ad256..17680ff249153d 100644 --- a/src/coreclr/vm/dbginterface.h +++ b/src/coreclr/vm/dbginterface.h @@ -192,8 +192,8 @@ class DebugInterface SIZE_T *nativeOffset) = 0; - // Used by FixContextAndResume - virtual void SendSetThreadContextNeeded(CONTEXT *context) = 0; + // Used by EditAndContinueModule::FixContextAndResume + virtual void SendSetThreadContextNeeded(CONTEXT *context, DebuggerPatchSkip *patchSkip = nullptr) = 0; virtual BOOL IsOutOfProcessSetContextEnabled() = 0; #endif // FEATURE_METADATA_UPDATER From 81070ee2d762b2860b06c0f479664581369756fe Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Mon, 30 Sep 2024 23:34:47 -0400 Subject: [PATCH 02/26] ThreadSuspension WIP --- src/coreclr/debug/di/process.cpp | 61 ++++++++++++++++++++++++++++++-- src/coreclr/debug/di/rspriv.h | 9 ++++- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 075c23db7d3f66..e5597dd8b9e7e9 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -934,6 +934,9 @@ CordbProcess::CordbProcess(ULONG64 clrInstanceId, m_syncCompleteReceived(false), m_pShim(pShim), m_userThreads(11), +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + m_SuspendedThreads(11), +#endif m_oddSync(false), #ifdef FEATURE_INTEROP_DEBUGGING m_unmanagedThreads(11), @@ -1301,6 +1304,9 @@ void CordbProcess::NeuterChildren() m_ContinueNeuterList.NeuterAndClear(this); m_userThreads.NeuterAndClear(GetProcessLock()); +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + m_SuspendedThreads.NeuterAndClear(GetProcessLock()); +#endif m_pDefaultAppDomain = NULL; @@ -11174,14 +11180,65 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // 2. Duplicate the handle to the current process // lookup the CordbThread by thread ID, so that we can access the left-side thread handle - CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); - IDacDbiInterface* pDAC = GetDAC(); if (pDAC == NULL) { LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - DAC not initialized\n")); ThrowHR(E_UNEXPECTED); } + CordbThread * pThread = nullptr; + + { + RSLockHolder lockHolder(GetProcessLock()); + //pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); + PrepopulateThreadsOrThrow(); + //pThread = TryLookupThreadByVolatileOSId(dwThreadId); + HASHFIND find; + for (CordbThread * pThreadIter = m_userThreads.FindFirst(&find); + pThreadIter != NULL; + pThreadIter = m_userThreads.FindNext(&find)) + { + _ASSERTE(pThreadIter != NULL); + + // Get the OS tid. This returns 0 if the thread is switched out. + DWORD dwThreadId2 = GetDAC()->TryGetVolatileOSThreadID(pThreadIter->m_vmThreadToken); + if (dwThreadId2 == dwThreadId) + { + pThread = pThreadIter; + } + else + { + HANDLE hOutOfProcThreadIter = pDAC->GetThreadHandle(pThreadIter->m_vmThreadToken); + if (hOutOfProcThreadIter != NULL) + { + HandleHolder hThreadIter; + BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThreadIter, ::GetCurrentProcess(), &hThreadIter, 0, FALSE, DUPLICATE_SAME_ACCESS); + if (fSuccess) + { + if (::SuspendThread(hThreadIter) != (DWORD)-1) + { + CordbThread *tmpThread = m_SuspendedThreads.GetBase(VmPtrToCookie(pThreadIter->m_vmThreadToken)); + if (tmpThread != NULL) + { + _ASSERTE(tmpThread == pThreadIter); + } + else + { + m_SuspendedThreads.AddBaseOrThrow(pThreadIter); + } + pThreadIter->m_dwInternalSuspendCount++; + } + } + } + } + } + } + + if (pThread == nullptr) + { + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Failed to find thread\n")); + ThrowHR(E_UNEXPECTED); + } HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); if (hOutOfProcThread == NULL) diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index 4f5acbbdc1c0b6..f11dc6897c2a45 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -3846,7 +3846,9 @@ class CordbProcess : RSExtSmartPtr m_pShim; CordbSafeHashTable m_userThreads; - +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + CordbSafeHashTable m_SuspendedThreads; +#endif public: ShimProcess* GetShim(); @@ -6372,6 +6374,11 @@ class CordbThread : public CordbBase, public ICorDebugThread, // offload to the shim to support V2 scenarios. HANDLE m_hCachedThread; HANDLE m_hCachedOutOfProcThread; + +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT +public: + DWORD m_dwInternalSuspendCount; +#endif }; /* ------------------------------------------------------------------------- * From 877ab0e60873718580d7825ba3ef595797f11977 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Tue, 1 Oct 2024 00:04:50 -0400 Subject: [PATCH 03/26] Change how in-place single step is tracked --- src/coreclr/debug/ee/controller.cpp | 82 +++++++++++++++-------------- src/coreclr/debug/ee/controller.h | 7 ++- src/coreclr/debug/ee/debugger.cpp | 24 ++++++--- src/coreclr/debug/ee/walker.h | 7 --- 4 files changed, 65 insertions(+), 55 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index e00052b48d217b..26da0ece6d532a 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -4443,7 +4443,7 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, #if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) if (g_pDebugInterface->IsOutOfProcessSetContextEnabled() && m_instrAttrib.m_fIsCall) { - m_instrAttrib.m_fInPlaceSS = true; + m_fInPlaceSS = true; } #endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) @@ -4454,7 +4454,7 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, // has since been expanded to handle RIP-relative writes as well. if (m_instrAttrib.m_dwOffsetToDisp != 0 #if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - && (!m_instrAttrib.m_fInPlaceSS || !m_instrAttrib.m_fIsCall) + && (!IsInPlaceSingleStep()) #endif ) { @@ -4485,7 +4485,7 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, // Copy the data into our buffer. memcpy(bufferBypassRW, patch->address + m_instrAttrib.m_cbInstr + dwOldDisp, m_instrAttrib.m_cOperandSize); - if (m_instrAttrib.m_fIsWrite) + if (m_instrAttrib.m_fIsWrite /*&& !m_instrAttrib.m_fIsCall*/) { // save the actual destination address and size so when we TriggerSingleStep() we can update the value pSharedPatchBypassBufferRW->RipTargetFixup = (UINT_PTR)(patch->address + m_instrAttrib.m_cbInstr + dwOldDisp); @@ -4563,7 +4563,7 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, #endif //TARGET_ARM64 #if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - if (!m_instrAttrib.m_fInPlaceSS || !m_instrAttrib.m_fIsCall) + if (!IsInPlaceSingleStep()) #endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) { //set eip to point to buffer... @@ -4793,7 +4793,7 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont // Fixup return address on stack #if defined(TARGET_X86) || defined(TARGET_AMD64) #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - if (!m_instrAttrib.m_fInPlaceSS) + if (!IsInPlaceSingleStep()) #endif { // Fixup return address on stack @@ -4826,7 +4826,7 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont (size_t)GetIP(context) <= (size_t)(patchBypass + MAX_INSTRUCTION_LENGTH + 1))) { #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - if (!m_instrAttrib.m_fInPlaceSS || !m_instrAttrib.m_fIsCall) + if (!IsInPlaceSingleStep()) { #endif LOG((LF_CORDB, LL_INFO10000, "Bypass instruction redirected because still in skip area.\n" @@ -4901,7 +4901,7 @@ bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) #if defined(TARGET_AMD64) #if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - if (!m_instrAttrib.m_fInPlaceSS || !m_instrAttrib.m_fIsCall) + if (!IsInPlaceSingleStep()) #endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) { // Dev11 91932: for RIP-relative writes we need to copy the value that was written in our buffer to the actual address @@ -4946,42 +4946,44 @@ bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) } } #if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - else if (m_instrAttrib.m_fInPlaceSS && m_instrAttrib.m_fIsCall) + else if (IsInPlaceSingleStep()) { LOG((LF_CORDB, LL_INFO10000, "This is an in-place single-step, all we want to do is back-patch the breakpoint\n")); - { - LPVOID baseAddress = (LPVOID)m_address; - DWORD oldProt; - - if (!VirtualProtect(baseAddress, - CORDbg_BREAK_INSTRUCTION_SIZE, - PAGE_EXECUTE_READWRITE, &oldProt)) - { - // we may be seeing unwriteable directly mapped executable memory. - // let's try copy-on-write instead, - if (!VirtualProtect(baseAddress, - CORDbg_BREAK_INSTRUCTION_SIZE, - PAGE_EXECUTE_WRITECOPY, &oldProt)) - { - _ASSERTE(!"VirtualProtect of code page failed"); - goto Error; - } - } - - CORDbgInsertBreakpoint(m_address); - LOG((LF_CORDB, LL_EVERYTHING, "DPS::TriggerSingleStep Breakpoint was re-inserted at %p\n", - m_address)); - - if (!VirtualProtect(baseAddress, - CORDbg_BREAK_INSTRUCTION_SIZE, - oldProt, &oldProt)) - { - _ASSERTE(!"VirtualProtect of code page failed"); - goto Error; - } - } + // { + // LPVOID baseAddress = (LPVOID)m_address; + // DWORD oldProt; + + // if (!VirtualProtect(baseAddress, + // CORDbg_BREAK_INSTRUCTION_SIZE, + // PAGE_EXECUTE_READWRITE, &oldProt)) + // { + // // we may be seeing unwriteable directly mapped executable memory. + // // let's try copy-on-write instead, + // if (!VirtualProtect(baseAddress, + // CORDbg_BREAK_INSTRUCTION_SIZE, + // PAGE_EXECUTE_WRITECOPY, &oldProt)) + // { + // _ASSERTE(!"VirtualProtect of code page failed"); + // goto Error; + // } + // } + + // CORDbgInsertBreakpoint(m_address); + // LOG((LF_CORDB, LL_EVERYTHING, "DPS::TSS Breakpoint was re-inserted at %p\n", + // m_address)); + + // if (!VirtualProtect(baseAddress, + // CORDbg_BREAK_INSTRUCTION_SIZE, + // oldProt, &oldProt)) + // { + // _ASSERTE(!"VirtualProtect of code page failed"); + // goto Error; + // } + // } + m_fSSCompleted = true; + return true; } -Error: +//Error: #endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) #endif // defined(TARGET_AMD64) LOG((LF_CORDB,LL_INFO10000, "DPS::TSS: triggered, about to delete\n")); diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 55268d6452c914..99dc050fe58837 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -1487,6 +1487,10 @@ class DebuggerPatchSkip : public DebuggerController CORDB_ADDRESS_TYPE *m_address; int m_iOrigDisp; // the original displacement of a relative call or jump InstructionAttribute m_instrAttrib; // info about the instruction being skipped over +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + bool m_fInPlaceSS; // is this an in-place single-step instruction? + bool m_fSSCompleted; // true if the single step has completed +#endif #ifndef FEATURE_EMULATE_SINGLESTEP // this is shared among all the skippers and the controller. see the comments // right before the definition of SharedPatchBypassBuffer for lifetime info. @@ -1500,7 +1504,8 @@ class DebuggerPatchSkip : public DebuggerController return (CORDB_ADDRESS_TYPE *)patchBypass; } #if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - BOOL IsInPlaceSingleStep() { return m_instrAttrib.m_fIsCall && m_instrAttrib.m_fInPlaceSS; } + BOOL IsInPlaceSingleStep() { return m_instrAttrib.m_fIsCall && m_fInPlaceSS; } // only in-place single steps over call intructions are supported at this time + BOOL IsSingleStepCompleted() { return m_fSSCompleted; } CORDB_ADDRESS_TYPE* GetAddress() { return m_address; } #endif #endif // !FEATURE_EMULATE_SINGLESTEP diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index b547fa109667ac..c70ed5a68fc89b 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -16751,17 +16751,27 @@ void Debugger::SendSetThreadContextNeeded(CONTEXT *context, DebuggerPatchSkip *p contextSize -= (DWORD)((BYTE*)pContext-(BYTE*)pBuffer); bool fIsInPlaceSingleStep = patchSkip != nullptr && patchSkip->IsInPlaceSingleStep(); + bool fSSCompleted = patchSkip != nullptr && patchSkip->IsSingleStepCompleted(); CORDB_ADDRESS_TYPE *patchSkipAddr = patchSkip != nullptr && fIsInPlaceSingleStep ? patchSkip->GetAddress() : nullptr; - _ASSERTE(!fIsInPlaceSingleStep || pContext->Rip == patchSkipAddr); + _ASSERTE(!fIsInPlaceSingleStep || fSSCompleted || pContext->Rip == patchSkipAddr); - PRD_TYPE opcode = 0xcc; + PRD_TYPE opcode = CORDbg_BREAK_INSTRUCTION; if (fIsInPlaceSingleStep) { - DebuggerControllerPatch *patch = DebuggerController::GetPatchTable()->GetPatch((CORDB_ADDRESS_TYPE *)pContext->Rip); - if (patch != NULL) + if (!fSSCompleted) { - LOG((LF_CORDB, LL_INFO10000, "D::SSTCN Patch found at address 0x%p opcode=0x%x\n", pContext->Rip, patch->opcode)); - opcode = patch->opcode; + DebuggerControllerPatch *patch = DebuggerController::GetPatchTable()->GetPatch((CORDB_ADDRESS_TYPE *)pContext->Rip); + if (patch != NULL) + { + LOG((LF_CORDB, LL_INFO10000, "D::SSTCN Patch found at address 0x%p opcode=0x%x\n", pContext->Rip, patch->opcode)); + opcode = patch->opcode; + } + } + else + { + LOG((LF_CORDB, LL_INFO10000, "D::SSTCN Patch found at address 0x%p opcode=0x%x deleting patch\n", pContext->Rip, patch->opcode)); + TRACE_FREE(patchSkip); + patchSkip->Delete(); } } @@ -16772,7 +16782,7 @@ void Debugger::SendSetThreadContextNeeded(CONTEXT *context, DebuggerPatchSkip *p SetThreadContextNeededFlare((TADDR)pContext, contextSize, patchSkipAddr, - (((fIsInPlaceSingleStep&0x1)<<8)|(opcode&0xFF))); + (((fSSCompleted&0x1)<<9)|((fIsInPlaceSingleStep&0x1)<<8)|(opcode&0xFF))); } EX_CATCH { diff --git a/src/coreclr/debug/ee/walker.h b/src/coreclr/debug/ee/walker.h index 7dfd9f2c39f297..3695ffd0726a36 100644 --- a/src/coreclr/debug/ee/walker.h +++ b/src/coreclr/debug/ee/walker.h @@ -41,10 +41,6 @@ struct InstructionAttribute bool m_fIsRelBranch; // is this a relative branch (either a call or a jump)? bool m_fIsWrite; // does the instruction write to an address? -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - bool m_fInPlaceSS; // is this an in-place single-step instruction? -#endif - DWORD m_cbInstr; // the size of the instruction DWORD m_cbDisp; // the size of the displacement DWORD m_dwOffsetToDisp; // the offset from the beginning of the instruction @@ -58,9 +54,6 @@ struct InstructionAttribute m_fIsAbsBranch = false; m_fIsRelBranch = false; m_fIsWrite = false; -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - m_fInPlaceSS = false; -#endif m_cbInstr = 0; m_cbDisp = 0; m_dwOffsetToDisp = 0; From fa1529b568f4013b165404b12948fda0594e4f8b Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Tue, 1 Oct 2024 00:43:12 -0400 Subject: [PATCH 04/26] Pass fSSCompleted to the right side --- src/coreclr/debug/di/process.cpp | 1 + src/coreclr/debug/di/rsthread.cpp | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index e5597dd8b9e7e9..ae43cc147d611d 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11283,6 +11283,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)context.R8; bool fIsInPlaceSingleStep = (bool)((context.R9>>8)&0x1); + bool fSSCompleted = (bool)((context.R9>>9)&0x1); PRD_TYPE opcode = (PRD_TYPE)(context.R9&0xFF); if (contextSize == 0 || contextSize > sizeof(CONTEXT) + 25000) diff --git a/src/coreclr/debug/di/rsthread.cpp b/src/coreclr/debug/di/rsthread.cpp index 619b37c87ef8b8..83956c8b3158ef 100644 --- a/src/coreclr/debug/di/rsthread.cpp +++ b/src/coreclr/debug/di/rsthread.cpp @@ -85,6 +85,10 @@ CordbThread::CordbThread(CordbProcess * pProcess, VMPTR_Thread vmThread) : m_userState(kInvalidUserState), m_hCachedThread(INVALID_HANDLE_VALUE), m_hCachedOutOfProcThread(INVALID_HANDLE_VALUE) +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + m_dwInternalSuspendCount(0) +#endif { m_fHasUnhandledException = FALSE; m_pExceptionRecord = NULL; @@ -162,6 +166,10 @@ void CordbThread::Neuter() m_pContext = NULL; } +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + m_dwInternalSuspendCount = 0; +#endif + ClearStackFrameCache(); CordbBase::Neuter(); From 6c87a988941cfcd52c6e4d27fa6ac1ff3a12f082 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Tue, 1 Oct 2024 00:52:22 -0400 Subject: [PATCH 05/26] Only suspend other threads for in-place single step --- src/coreclr/debug/di/process.cpp | 92 ++++++++++++++++---------------- 1 file changed, 45 insertions(+), 47 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index ae43cc147d611d..224a235662ebc7 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11186,53 +11186,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - DAC not initialized\n")); ThrowHR(E_UNEXPECTED); } - CordbThread * pThread = nullptr; - - { - RSLockHolder lockHolder(GetProcessLock()); - //pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); - PrepopulateThreadsOrThrow(); - //pThread = TryLookupThreadByVolatileOSId(dwThreadId); - HASHFIND find; - for (CordbThread * pThreadIter = m_userThreads.FindFirst(&find); - pThreadIter != NULL; - pThreadIter = m_userThreads.FindNext(&find)) - { - _ASSERTE(pThreadIter != NULL); - - // Get the OS tid. This returns 0 if the thread is switched out. - DWORD dwThreadId2 = GetDAC()->TryGetVolatileOSThreadID(pThreadIter->m_vmThreadToken); - if (dwThreadId2 == dwThreadId) - { - pThread = pThreadIter; - } - else - { - HANDLE hOutOfProcThreadIter = pDAC->GetThreadHandle(pThreadIter->m_vmThreadToken); - if (hOutOfProcThreadIter != NULL) - { - HandleHolder hThreadIter; - BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThreadIter, ::GetCurrentProcess(), &hThreadIter, 0, FALSE, DUPLICATE_SAME_ACCESS); - if (fSuccess) - { - if (::SuspendThread(hThreadIter) != (DWORD)-1) - { - CordbThread *tmpThread = m_SuspendedThreads.GetBase(VmPtrToCookie(pThreadIter->m_vmThreadToken)); - if (tmpThread != NULL) - { - _ASSERTE(tmpThread == pThreadIter); - } - else - { - m_SuspendedThreads.AddBaseOrThrow(pThreadIter); - } - pThreadIter->m_dwInternalSuspendCount++; - } - } - } - } - } - } + CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); if (pThread == nullptr) { @@ -11405,6 +11359,50 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) } EX_CATCH_HRESULT(hr); SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); + + if (!fSSCompleted) + { + RSLockHolder lockHolder(GetProcessLock()); + HASHFIND find; + for (CordbThread * pThreadIter = m_userThreads.FindFirst(&find); + pThreadIter != NULL; + pThreadIter = m_userThreads.FindNext(&find)) + { + _ASSERTE(pThreadIter != NULL); + + // Get the OS tid. This returns 0 if the thread is switched out. + DWORD dwThreadId2 = GetDAC()->TryGetVolatileOSThreadID(pThreadIter->m_vmThreadToken); + if (dwThreadId2 == dwThreadId) + { + pThread = pThreadIter; + } + else + { + HANDLE hOutOfProcThreadIter = pDAC->GetThreadHandle(pThreadIter->m_vmThreadToken); + if (hOutOfProcThreadIter != NULL) + { + HandleHolder hThreadIter; + BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThreadIter, ::GetCurrentProcess(), &hThreadIter, 0, FALSE, DUPLICATE_SAME_ACCESS); + if (fSuccess) + { + if (::SuspendThread(hThreadIter) != (DWORD)-1) + { + CordbThread *tmpThread = m_SuspendedThreads.GetBase(VmPtrToCookie(pThreadIter->m_vmThreadToken)); + if (tmpThread != NULL) + { + _ASSERTE(tmpThread == pThreadIter); + } + else + { + m_SuspendedThreads.AddBaseOrThrow(pThreadIter); + } + pThreadIter->m_dwInternalSuspendCount++; + } + } + } + } + } + } } #else #error Platform not supported From 232c7d5d87c76741522ddebc1cdd3b92b5311a3f Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Tue, 1 Oct 2024 01:06:21 -0400 Subject: [PATCH 06/26] Add thread resume in HandleSetThreadContextNeeded --- src/coreclr/debug/di/process.cpp | 53 ++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 224a235662ebc7..9a7fc048dee657 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11360,7 +11360,6 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) EX_CATCH_HRESULT(hr); SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); - if (!fSSCompleted) { RSLockHolder lockHolder(GetProcessLock()); HASHFIND find; @@ -11374,32 +11373,40 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) DWORD dwThreadId2 = GetDAC()->TryGetVolatileOSThreadID(pThreadIter->m_vmThreadToken); if (dwThreadId2 == dwThreadId) { - pThread = pThreadIter; + continue; } - else + HANDLE hOutOfProcThreadIter = pDAC->GetThreadHandle(pThreadIter->m_vmThreadToken); + if (hOutOfProcThreadIter == NULL) + { + continue; + } + CordbThread *tmpThread = m_SuspendedThreads.GetBase(VmPtrToCookie(pThreadIter->m_vmThreadToken)); + if (tmpThread == NULL) { - HANDLE hOutOfProcThreadIter = pDAC->GetThreadHandle(pThreadIter->m_vmThreadToken); - if (hOutOfProcThreadIter != NULL) + if (fSSCompleted) { - HandleHolder hThreadIter; - BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThreadIter, ::GetCurrentProcess(), &hThreadIter, 0, FALSE, DUPLICATE_SAME_ACCESS); - if (fSuccess) - { - if (::SuspendThread(hThreadIter) != (DWORD)-1) - { - CordbThread *tmpThread = m_SuspendedThreads.GetBase(VmPtrToCookie(pThreadIter->m_vmThreadToken)); - if (tmpThread != NULL) - { - _ASSERTE(tmpThread == pThreadIter); - } - else - { - m_SuspendedThreads.AddBaseOrThrow(pThreadIter); - } - pThreadIter->m_dwInternalSuspendCount++; - } - } + continue; } + m_SuspendedThreads.AddBaseOrThrow(pThreadIter); + } + + HandleHolder hThreadIter; + BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThreadIter, ::GetCurrentProcess(), &hThreadIter, 0, FALSE, DUPLICATE_SAME_ACCESS); + if (!fSuccess) + { + continue; + } + if (!fSSCompleted) + { + if (::SuspendThread(hThreadIter) != (DWORD)-1) + { + pThreadIter->m_dwInternalSuspendCount++; + } + } + else + { + pThreadIter->m_dwInternalSuspendCount--; + ::ResumeThread(hThreadIter); } } } From e70d2a910b9559f4e846f24ea9ff165126419f71 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Tue, 1 Oct 2024 12:27:52 -0400 Subject: [PATCH 07/26] ScanForTriggers should return DebuggerPatchSkip --- src/coreclr/debug/ee/controller.cpp | 72 ++++++++++++++++++++--------- src/coreclr/debug/ee/controller.h | 14 ++++-- src/coreclr/debug/ee/walker.h | 1 + 3 files changed, 61 insertions(+), 26 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 26da0ece6d532a..2384b03efcbd10 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -2537,7 +2537,11 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, CONTEXT *context, DebuggerControllerQueue *pDcq, SCAN_TRIGGER stWhat, - TP_RESULT *pTpr) + TP_RESULT *pTpr, +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + DebuggerPatchSkip **ppDps +#endif + ) { CONTRACTL { @@ -2739,7 +2743,8 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, used = DPOSS_USED_WITH_NO_EVENT; } - if (p->TriggerSingleStep(thread, (const BYTE *)address)) + DPOSS_ACTION tmpAction = p->TriggerSingleStep(thread, (const BYTE *)address); + if (tmpAction == DPOSS_USED_WITH_EVENT) { // by now, we should already know that we care for this exception. _ASSERTE(IsInUsedAction(used) == true); @@ -2749,6 +2754,13 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, pDcq->dcqEnqueue(p, FALSE); // @todo Return value } +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + else if (tmpAction == DPOSS_USED_WITH_NO_EVENT) + { + *ppDps = (DebuggerPatchSkip*)p; + used = DPOSS_USED_WITH_NO_EVENT; + } +#endif } p = pNext; @@ -2877,6 +2889,11 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE CrstHolderWithState lockController(&g_criticalSection); + if (ppDebuggerPatchSkip != nullptr) + { + *ppDebuggerPatchSkip = nullptr; + } + TADDR originalAddress = 0; #ifdef FEATURE_METADATA_UPDATER @@ -2923,8 +2940,20 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE #endif // FEATURE_METADATA_UPDATER TP_RESULT tpr; - - used = ScanForTriggers((CORDB_ADDRESS_TYPE *)address, thread, context, &dcq, which, &tpr); +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + DebuggerPatchSkip *dps = nullptr; +#endif + used = ScanForTriggers((CORDB_ADDRESS_TYPE *)address, thread, context, &dcq, which, &tpr, +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + &dps +#endif + ); +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + if (dps) + { + *ppDebuggerPatchSkip = dps; + } +#endif LOG((LF_CORDB|LF_ENC, LL_EVERYTHING, "DC::DPOSS ScanForTriggers called and returned.\n")); @@ -3033,7 +3062,7 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE #endif DebuggerPatchSkip* pDebuggerPatchSkip = ActivatePatchSkip(thread, dac_cast(GetIP(pCtx)), FALSE); - if (ppDebuggerPatchSkip != nullptr) + if (pDebuggerPatchSkip != nullptr && ppDebuggerPatchSkip != nullptr) { *ppDebuggerPatchSkip = pDebuggerPatchSkip; } @@ -4008,11 +4037,11 @@ TP_RESULT DebuggerController::TriggerPatch(DebuggerControllerPatch *patch, return TPR_IGNORE; } -bool DebuggerController::TriggerSingleStep(Thread *thread, +DPOSS_ACTION DebuggerController::TriggerSingleStep(Thread *thread, const BYTE *ip) { LOG((LF_CORDB, LL_INFO10000, "DC::TP: in default TriggerSingleStep\n")); - return false; + return DPOSS_DONT_CARE; } void DebuggerController::TriggerUnwind(Thread *thread, @@ -4326,7 +4355,8 @@ bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException, result = DebuggerController::DispatchPatchOrSingleStep(pCurThread, pContext, ip, - (SCAN_TRIGGER)(ST_PATCH|ST_SINGLE_STEP)); + (SCAN_TRIGGER)(ST_PATCH|ST_SINGLE_STEP), + ppDebuggerPatchSkip); // We pass patch | single step since single steps actually // do both (eg, you SS onto a breakpoint). break; @@ -4895,7 +4925,7 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont return TPR_TRIGGER; } -bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) +DPOSS_ACTION DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) { LOG((LF_CORDB,LL_INFO10000, "DPS::TSS: basically a no-op\n")); @@ -4981,7 +5011,7 @@ bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) // } // } m_fSSCompleted = true; - return true; + return DPOSS_USED_WITH_NO_EVENT; } //Error: #endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) @@ -4990,7 +5020,7 @@ bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) TRACE_FREE(this); Delete(); - return false; + return DPOSS_DONT_CARE; } // * ------------------------------------------------------------------------- @@ -7493,7 +7523,7 @@ void DebuggerStepper::TriggerMethodEnter(Thread * thread, // We may have single-stepped over a return statement to land us up a frame. // Or we may have single-stepped through a method. // We never single-step into calls (we place a patch at the call destination). -bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) +DPOSS_ACTION DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) { LOG((LF_CORDB,LL_INFO10000,"DS::TSS this:0x%p, @ ip:0x%p\n", this, ip)); @@ -7514,7 +7544,7 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) { LOG((LF_CORDB,LL_INFO10000, "DS::TSS: not in managed code, Returning false (case 0)!\n")); DisableSingleStep(); - return false; + return DPOSS_DONT_CARE; } // If we EnC the method, we'll blast the function address, @@ -7552,7 +7582,7 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) EnableMethodEnter(); } DisableSingleStep(); - return false; + return DPOSS_DONT_CARE; } DisableAll(); @@ -7562,7 +7592,7 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) if (DetectHandleLCGMethods((PCODE)ip, fd, &info)) { - return false; + return DPOSS_DONT_CARE; } if (IsInRange(offset, m_range, m_rangeCount, &info) || @@ -7574,7 +7604,7 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) EnableUnwind(m_fp); LOG((LF_CORDB,LL_INFO10000, "DS::TSS: Returning false Case 1!\n")); - return false; + return DPOSS_DONT_CARE; } else { @@ -7587,10 +7617,10 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) DebuggerMethodInfo * dmi = g_pDebugger->GetOrCreateMethodInfo(fd->GetModule(), fd->GetMemberDef()); if ((dmi != NULL) && DetectHandleNonUserCode(&info, dmi)) - return false; + return DPOSS_DONT_CARE; PrepareForSendEvent(ticket); - return true; + return DPOSS_USED_WITH_EVENT; } } @@ -9206,18 +9236,18 @@ TP_RESULT DebuggerDataBreakpoint::TriggerPatch(DebuggerControllerPatch *patch, T } } -bool DebuggerDataBreakpoint::TriggerSingleStep(Thread *thread, const BYTE *ip) +DPOSS_ACTION DebuggerDataBreakpoint::TriggerSingleStep(Thread *thread, const BYTE *ip) { if (g_pDebugger->IsThreadAtSafePlace(thread)) { LOG((LF_CORDB, LL_INFO10000, "D:DDBP: Finally safe for stopping, stop stepping\n")); this->DisableSingleStep(); - return true; + return DPOSS_USED_WITH_EVENT; } else { LOG((LF_CORDB, LL_INFO10000, "D:DDBP: Still not safe for stopping, continue stepping\n")); - return false; + return DPOSS_DONT_CARE; } } diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 99dc050fe58837..f7c642de76cde0 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -1110,7 +1110,11 @@ class DebuggerController CONTEXT *context, DebuggerControllerQueue *pDcq, SCAN_TRIGGER stWhat, - TP_RESULT *pTpr); + TP_RESULT *pTpr, +#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + DebuggerPatchSkip **ppDps +#endif + ); static DebuggerPatchSkip *ActivatePatchSkip(Thread *thread, @@ -1371,7 +1375,7 @@ class DebuggerController // Dispatched when we get a SingleStep exception on this thread. // Return true if we want SendEvent to get called. - virtual bool TriggerSingleStep(Thread *thread, const BYTE *ip); + virtual DPOSS_ACTION TriggerSingleStep(Thread *thread, const BYTE *ip); // Dispatched to notify the controller when we are going to a filter/handler @@ -1465,7 +1469,7 @@ class DebuggerPatchSkip : public DebuggerController ~DebuggerPatchSkip(); - bool TriggerSingleStep(Thread *thread, + DPOSS_ACTION TriggerSingleStep(Thread *thread, const BYTE *ip); TP_RESULT TriggerExceptionHook(Thread *thread, CONTEXT * pContext, @@ -1646,7 +1650,7 @@ class DebuggerStepper : public DebuggerController TP_RESULT TriggerPatch(DebuggerControllerPatch *patch, Thread *thread, TRIGGER_WHY tyWhy); - bool TriggerSingleStep(Thread *thread, const BYTE *ip); + DPOSS_ACTION TriggerSingleStep(Thread *thread, const BYTE *ip); void TriggerUnwind(Thread *thread, MethodDesc *fd, DebuggerJitInfo * pDJI, SIZE_T offset, FramePointer fp, CorDebugStepReason unwindReason); @@ -1833,7 +1837,7 @@ class DebuggerDataBreakpoint : public DebuggerController virtual TP_RESULT TriggerPatch(DebuggerControllerPatch *patch, Thread *thread, TRIGGER_WHY tyWhy); - virtual bool TriggerSingleStep(Thread *thread, const BYTE *ip); + virtual DPOSS_ACTION TriggerSingleStep(Thread *thread, const BYTE *ip); bool SendEvent(Thread *thread, bool fInterruptedBySetIp) { diff --git a/src/coreclr/debug/ee/walker.h b/src/coreclr/debug/ee/walker.h index 3695ffd0726a36..0c901cab466570 100644 --- a/src/coreclr/debug/ee/walker.h +++ b/src/coreclr/debug/ee/walker.h @@ -41,6 +41,7 @@ struct InstructionAttribute bool m_fIsRelBranch; // is this a relative branch (either a call or a jump)? bool m_fIsWrite; // does the instruction write to an address? + DWORD m_cbInstr; // the size of the instruction DWORD m_cbDisp; // the size of the displacement DWORD m_dwOffsetToDisp; // the offset from the beginning of the instruction From 8b6413cb533179a702be1de3bbd339b7c1d1e413 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Tue, 1 Oct 2024 15:54:28 -0400 Subject: [PATCH 08/26] Refactor out of proc thread suspension into CordbThread --- src/coreclr/debug/di/process.cpp | 60 ++------------ src/coreclr/debug/di/rspriv.h | 10 ++- src/coreclr/debug/di/rsthread.cpp | 125 +++++++++++++++++++++++++++++- 3 files changed, 133 insertions(+), 62 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 9a7fc048dee657..2d1f8d5e328176 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -934,9 +934,6 @@ CordbProcess::CordbProcess(ULONG64 clrInstanceId, m_syncCompleteReceived(false), m_pShim(pShim), m_userThreads(11), -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - m_SuspendedThreads(11), -#endif m_oddSync(false), #ifdef FEATURE_INTEROP_DEBUGGING m_unmanagedThreads(11), @@ -1304,9 +1301,6 @@ void CordbProcess::NeuterChildren() m_ContinueNeuterList.NeuterAndClear(this); m_userThreads.NeuterAndClear(GetProcessLock()); -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - m_SuspendedThreads.NeuterAndClear(GetProcessLock()); -#endif m_pDefaultAppDomain = NULL; @@ -11360,55 +11354,13 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) EX_CATCH_HRESULT(hr); SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); + if (!fSSCompleted) { - RSLockHolder lockHolder(GetProcessLock()); - HASHFIND find; - for (CordbThread * pThreadIter = m_userThreads.FindFirst(&find); - pThreadIter != NULL; - pThreadIter = m_userThreads.FindNext(&find)) - { - _ASSERTE(pThreadIter != NULL); - - // Get the OS tid. This returns 0 if the thread is switched out. - DWORD dwThreadId2 = GetDAC()->TryGetVolatileOSThreadID(pThreadIter->m_vmThreadToken); - if (dwThreadId2 == dwThreadId) - { - continue; - } - HANDLE hOutOfProcThreadIter = pDAC->GetThreadHandle(pThreadIter->m_vmThreadToken); - if (hOutOfProcThreadIter == NULL) - { - continue; - } - CordbThread *tmpThread = m_SuspendedThreads.GetBase(VmPtrToCookie(pThreadIter->m_vmThreadToken)); - if (tmpThread == NULL) - { - if (fSSCompleted) - { - continue; - } - m_SuspendedThreads.AddBaseOrThrow(pThreadIter); - } - - HandleHolder hThreadIter; - BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThreadIter, ::GetCurrentProcess(), &hThreadIter, 0, FALSE, DUPLICATE_SAME_ACCESS); - if (!fSuccess) - { - continue; - } - if (!fSSCompleted) - { - if (::SuspendThread(hThreadIter) != (DWORD)-1) - { - pThreadIter->m_dwInternalSuspendCount++; - } - } - else - { - pThreadIter->m_dwInternalSuspendCount--; - ::ResumeThread(hThreadIter); - } - } + pThread->InternalSuspendOtherThreads(&m_userThreads); + } + else + { + pThread->InternalResumeOtherThreads(); } } #else diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index f11dc6897c2a45..d30641a44409a3 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -3846,9 +3846,7 @@ class CordbProcess : RSExtSmartPtr m_pShim; CordbSafeHashTable m_userThreads; -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - CordbSafeHashTable m_SuspendedThreads; -#endif + public: ShimProcess* GetShim(); @@ -6376,8 +6374,12 @@ class CordbThread : public CordbBase, public ICorDebugThread, HANDLE m_hCachedOutOfProcThread; #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT -public: +private: + CordbSafeHashTable m_SuspendedThreads; DWORD m_dwInternalSuspendCount; +public: + HRESULT InternalSuspendOtherThreads(CordbSafeHashTable *pThreads); + HRESULT InternalResumeOtherThreads(); #endif }; diff --git a/src/coreclr/debug/di/rsthread.cpp b/src/coreclr/debug/di/rsthread.cpp index 83956c8b3158ef..fdb459c91d64d1 100644 --- a/src/coreclr/debug/di/rsthread.cpp +++ b/src/coreclr/debug/di/rsthread.cpp @@ -87,6 +87,7 @@ CordbThread::CordbThread(CordbProcess * pProcess, VMPTR_Thread vmThread) : m_hCachedOutOfProcThread(INVALID_HANDLE_VALUE) #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT , + m_SuspendedThreads(11), m_dwInternalSuspendCount(0) #endif { @@ -147,6 +148,28 @@ void CordbThread::Neuter() _ASSERTE(GetProcess()->ThreadHoldsProcessLock()); +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + InternalResumeOtherThreads(); // ensure that if we had suspended other threads, they are resumed. + if (m_dwInternalSuspendCount > 0) + { + HANDLE hOutOfProcThread = GetProcess()->GetDAC()->GetThreadHandle(m_vmThreadToken); + if (hOutOfProcThread != NULL) + { + HandleHolder hThread; + BOOL fSuccess = DuplicateHandle(GetProcess()->UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); + if (fSuccess) + { + while (m_dwInternalSuspendCount > 0) + { + m_dwInternalSuspendCount--; + ::ResumeThread(hThread); + } + } + } + m_dwInternalSuspendCount = 0; + } +#endif + delete m_pExceptionRecord; m_pExceptionRecord = NULL; @@ -166,10 +189,6 @@ void CordbThread::Neuter() m_pContext = NULL; } -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - m_dwInternalSuspendCount = 0; -#endif - ClearStackFrameCache(); CordbBase::Neuter(); @@ -2795,6 +2814,104 @@ HRESULT CordbThread::GetBlockingObjects(ICorDebugBlockingObjectEnum **ppBlocking return hr; } +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT +HRESULT CordbThread::InternalSuspendOtherThreads(CordbSafeHashTable *pThreads) +{ + RSLockHolder lockHolder(GetProcess()->GetProcessLock()); + IDacDbiInterface* pDAC = GetProcess()->GetDAC(); + if (pDAC == NULL) + { + return E_UNEXPECTED; + } + + InternalResumeOtherThreads(); // clear m_SuspendedThreads + + DWORD dwThreadId = pDAC->TryGetVolatileOSThreadID(m_vmThreadToken); + HASHFIND find; + for (CordbThread * pThread = pThreads->FindFirst(&find); + pThread != NULL; + pThread = pThreads->FindNext(&find)) + { + _ASSERTE(pThread != NULL); + + // Get the OS tid. This returns 0 if the thread is switched out. + DWORD dwThreadId2 = pDAC->TryGetVolatileOSThreadID(pThread->m_vmThreadToken); + if (dwThreadId2 == dwThreadId) + { + continue; + } + + HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); + if (hOutOfProcThread == NULL) + { + continue; + } + + HandleHolder hThread; + BOOL fSuccess = DuplicateHandle(GetProcess()->UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); + if (!fSuccess) + { + continue; + } + if (::SuspendThread(hThread) != (DWORD)-1) + { + pThread->m_dwInternalSuspendCount++; + CordbThread *tmpThread = m_SuspendedThreads.GetBase(VmPtrToCookie(pThread->m_vmThreadToken)); + _ASSERTE(tmpThread == NULL); + if (tmpThread == NULL) + { + m_SuspendedThreads.AddBaseOrThrow(pThread); + } + } + } + + return S_OK; +} + +HRESULT CordbThread::InternalResumeOtherThreads() +{ + RSLockHolder lockHolder(GetProcess()->GetProcessLock()); + + IDacDbiInterface* pDAC = GetProcess()->GetDAC(); + if (pDAC == NULL) + { + return E_UNEXPECTED; + } + + UINT32 count = m_SuspendedThreads.GetCount(); + UINT32 idx = 0; + HASHFIND find; + while (idx++ < count) + { + CordbThread * pThread = m_SuspendedThreads.FindFirst(&find); + _ASSERTE(pThread != NULL); + if (pThread == NULL) + { + break; + } + + if (pThread->m_dwInternalSuspendCount > 0) + { + HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); + if (hOutOfProcThread != NULL) + { + HandleHolder hThread; + BOOL fSuccess = DuplicateHandle(GetProcess()->UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); + if (fSuccess) + { + pThread->m_dwInternalSuspendCount--; + ::ResumeThread(hThread); + } + + } + } + m_SuspendedThreads.RemoveBase(VmPtrToCookie(pThread->m_vmThreadToken)); + } + + return S_OK; +} +#endif // OUT_OF_PROCESS_SETTHREADCONTEXT + #ifdef FEATURE_INTEROP_DEBUGGING /* ------------------------------------------------------------------------- * * Unmanaged Thread classes From 44431bdb1269c54f69efd5bcd8194b403f2cf3d6 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Fri, 4 Oct 2024 16:51:50 -0400 Subject: [PATCH 09/26] Avoid using DebuggerPatchSkip for tracking in-place single step on left side, and resume threads on the right side before the left side is notified --- src/coreclr/debug/di/process.cpp | 61 +++++-- src/coreclr/debug/di/rspriv.h | 5 + src/coreclr/debug/ee/controller.cpp | 245 ++++++++++++---------------- src/coreclr/debug/ee/controller.h | 69 +++++--- src/coreclr/debug/ee/debugger.cpp | 59 +++---- src/coreclr/debug/ee/debugger.h | 5 +- src/coreclr/vm/dbginterface.h | 4 +- 7 files changed, 225 insertions(+), 223 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 2d1f8d5e328176..f357d05038cb6b 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -981,6 +981,11 @@ CordbProcess::CordbProcess(ULONG64 clrInstanceId, m_fAssertOnTargetInconsistency(false), m_runtimeOffsetsInitialized(false), m_writableMetadataUpdateMode(LegacyCompatPolicy) +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + m_fExpectingSingleStep(false), + m_patchSkipAddr(NULL) +#endif { _ASSERTE((m_id == 0) == (pShim == NULL)); @@ -11174,17 +11179,17 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // 2. Duplicate the handle to the current process // lookup the CordbThread by thread ID, so that we can access the left-side thread handle - IDacDbiInterface* pDAC = GetDAC(); - if (pDAC == NULL) + CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); + if (pThread == NULL) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - DAC not initialized\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Failed to find thread\n")); ThrowHR(E_UNEXPECTED); } - CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); - if (pThread == nullptr) + IDacDbiInterface* pDAC = GetDAC(); + if (pDAC == NULL) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Failed to find thread\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - DAC not initialized\n")); ThrowHR(E_UNEXPECTED); } @@ -11229,10 +11234,8 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) TADDR lsContextAddr = (TADDR)context.Rcx; DWORD contextSize = (DWORD)context.Rdx; - CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)context.R8; - bool fIsInPlaceSingleStep = (bool)((context.R9>>8)&0x1); - bool fSSCompleted = (bool)((context.R9>>9)&0x1); - PRD_TYPE opcode = (PRD_TYPE)(context.R9&0xFF); + bool fIsInPlaceSingleStep = (bool)context.R8; + PRD_TYPE opcode = (PRD_TYPE)context.R9&0xFF; if (contextSize == 0 || contextSize > sizeof(CONTEXT) + 25000) { @@ -11345,6 +11348,11 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) if (fIsInPlaceSingleStep) { + CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)pFrameContext->Rip; + + this->m_patchSkipAddr = patchSkipAddr; + this->m_fExpectingSingleStep = true; + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - address=0x%p opcode=0x%x\n", patchSkipAddr, opcode)); EX_TRY { @@ -11354,14 +11362,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) EX_CATCH_HRESULT(hr); SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); - if (!fSSCompleted) - { - pThread->InternalSuspendOtherThreads(&m_userThreads); - } - else - { - pThread->InternalResumeOtherThreads(); - } + pThread->InternalSuspendOtherThreads(&m_userThreads); } #else #error Platform not supported @@ -11623,6 +11624,30 @@ HRESULT CordbProcess::Filter( HandleSetThreadContextNeeded(dwThreadId); *pContinueStatus = DBG_CONTINUE; } + else if (dwFirstChance && pRecord->ExceptionCode == STATUS_SINGLE_STEP && m_fExpectingSingleStep) + { + // we are expecting a single step exception + CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); + if (pThread != NULL) + { + EX_TRY + { + BYTE opcode = CORDbg_BREAK_INSTRUCTION; + TargetBuffer tb((void*)m_patchSkipAddr, sizeof(BYTE)); + SafeWriteBuffer(tb, (const BYTE*)&opcode); // throws + } + EX_CATCH_HRESULT(hr); + SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); + + m_fExpectingSingleStep = false; + m_patchSkipAddr = NULL; + + pThread->InternalResumeOtherThreads(); + } + + // let the normal debugger stepper logic execute for this single step + *pContinueStatus = DBG_EXCEPTION_NOT_HANDLED; + } #endif } PUBLIC_API_END(hr); diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index d30641a44409a3..48a38353eb372f 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -4125,6 +4125,11 @@ class CordbProcess : WriteableMetadataUpdateMode m_writableMetadataUpdateMode; COM_METHOD GetObjectInternal(CORDB_ADDRESS addr, CordbAppDomain* pAppDomainOverride, ICorDebugObjectValue **pObject); + +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + bool m_fExpectingSingleStep; + CORDB_ADDRESS_TYPE * m_patchSkipAddr; +#endif }; // Some IMDArocess APIs are supported as interop-only. diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 2384b03efcbd10..329976050e3d32 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -2481,7 +2481,12 @@ bool DebuggerController::MatchPatch(Thread *thread, DebuggerPatchSkip *DebuggerController::ActivatePatchSkip(Thread *thread, const BYTE *PC, - BOOL fForEnC) + BOOL fForEnC +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + DebuggerSteppingInfo *pDebuggerSteppingInfo +#endif + ) { #ifdef _DEBUG BOOL shouldBreak = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_ActivatePatchSkip); @@ -2527,6 +2532,14 @@ DebuggerPatchSkip *DebuggerController::ActivatePatchSkip(Thread *thread, LOG((LF_CORDB,LL_INFO10000, "DC::APS: About to skip from PC=0x%p\n", PC)); skip = new (interopsafe) DebuggerPatchSkip(thread, patch); TRACE_ALLOC(skip); + +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + if (pDebuggerSteppingInfo != NULL && skip != NULL && skip->IsInPlaceSingleStep()) + { + // send the opcode to the right side so it can be restored during single-step + pDebuggerSteppingInfo->EnableInPlaceSingleStepOverCall(patch->opcode); + } +#endif } return skip; @@ -2537,11 +2550,7 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, CONTEXT *context, DebuggerControllerQueue *pDcq, SCAN_TRIGGER stWhat, - TP_RESULT *pTpr, -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - DebuggerPatchSkip **ppDps -#endif - ) + TP_RESULT *pTpr) { CONTRACTL { @@ -2743,8 +2752,7 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, used = DPOSS_USED_WITH_NO_EVENT; } - DPOSS_ACTION tmpAction = p->TriggerSingleStep(thread, (const BYTE *)address); - if (tmpAction == DPOSS_USED_WITH_EVENT) + if (p->TriggerSingleStep(thread, (const BYTE *)address)) { // by now, we should already know that we care for this exception. _ASSERTE(IsInUsedAction(used) == true); @@ -2754,13 +2762,6 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, pDcq->dcqEnqueue(p, FALSE); // @todo Return value } -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - else if (tmpAction == DPOSS_USED_WITH_NO_EVENT) - { - *ppDps = (DebuggerPatchSkip*)p; - used = DPOSS_USED_WITH_NO_EVENT; - } -#endif } p = pNext; @@ -2850,7 +2851,11 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, // DebuggerController's TriggerPatch). Put any DCs that are interested into a queue and then calls // SendEvent on each. // Note that control will not return from this function in the case of EnC remap -DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTEXT *context, CORDB_ADDRESS_TYPE *address, SCAN_TRIGGER which, DebuggerPatchSkip **ppDebuggerPatchSkip) +DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTEXT *context, CORDB_ADDRESS_TYPE *address, SCAN_TRIGGER which +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT +,DebuggerSteppingInfo *pDebuggerSteppingInfo +#endif +) { CONTRACT(DPOSS_ACTION) { @@ -2889,11 +2894,6 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE CrstHolderWithState lockController(&g_criticalSection); - if (ppDebuggerPatchSkip != nullptr) - { - *ppDebuggerPatchSkip = nullptr; - } - TADDR originalAddress = 0; #ifdef FEATURE_METADATA_UPDATER @@ -2940,20 +2940,8 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE #endif // FEATURE_METADATA_UPDATER TP_RESULT tpr; -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - DebuggerPatchSkip *dps = nullptr; -#endif - used = ScanForTriggers((CORDB_ADDRESS_TYPE *)address, thread, context, &dcq, which, &tpr, -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - &dps -#endif - ); -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - if (dps) - { - *ppDebuggerPatchSkip = dps; - } -#endif + + 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")); @@ -3061,11 +3049,11 @@ DPOSS_ACTION DebuggerController::DispatchPatchOrSingleStep(Thread *thread, CONTE } #endif - DebuggerPatchSkip* pDebuggerPatchSkip = ActivatePatchSkip(thread, dac_cast(GetIP(pCtx)), FALSE); - if (pDebuggerPatchSkip != nullptr && ppDebuggerPatchSkip != nullptr) - { - *ppDebuggerPatchSkip = pDebuggerPatchSkip; - } + ActivatePatchSkip(thread, dac_cast(GetIP(pCtx)), FALSE +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , pDebuggerSteppingInfo +#endif + ); lockController.Release(); @@ -4037,11 +4025,11 @@ TP_RESULT DebuggerController::TriggerPatch(DebuggerControllerPatch *patch, return TPR_IGNORE; } -DPOSS_ACTION DebuggerController::TriggerSingleStep(Thread *thread, +bool DebuggerController::TriggerSingleStep(Thread *thread, const BYTE *ip) { LOG((LF_CORDB, LL_INFO10000, "DC::TP: in default TriggerSingleStep\n")); - return DPOSS_DONT_CARE; + return false; } void DebuggerController::TriggerUnwind(Thread *thread, @@ -4165,8 +4153,12 @@ void ThisFunctionMayHaveTriggerAGC() bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException, CONTEXT *pContext, DWORD dwCode, - Thread *pCurThread, - DebuggerPatchSkip **ppDebuggerPatchSkip) + Thread *pCurThread +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + DebuggerSteppingInfo *pDebuggerSteppingInfo +#endif + ) { CONTRACTL { @@ -4337,8 +4329,12 @@ bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException, result = DebuggerController::DispatchPatchOrSingleStep(pCurThread, pContext, ip, - ST_PATCH, - ppDebuggerPatchSkip); + ST_PATCH +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + pDebuggerSteppingInfo +#endif + ); LOG((LF_CORDB, LL_EVERYTHING, "DC::DNE DispatchPatch call returned\n")); // If we detached, we should remove all our breakpoints. So if we try @@ -4355,8 +4351,12 @@ bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException, result = DebuggerController::DispatchPatchOrSingleStep(pCurThread, pContext, ip, - (SCAN_TRIGGER)(ST_PATCH|ST_SINGLE_STEP), - ppDebuggerPatchSkip); + (SCAN_TRIGGER)(ST_PATCH|ST_SINGLE_STEP) +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + pDebuggerSteppingInfo +#endif + ); // We pass patch | single step since single steps actually // do both (eg, you SS onto a breakpoint). break; @@ -4483,8 +4483,8 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, // we are only using RIP-relative addressing to access read-only data (see VSW 246145 for more information). This // has since been expanded to handle RIP-relative writes as well. if (m_instrAttrib.m_dwOffsetToDisp != 0 -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - && (!IsInPlaceSingleStep()) +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + && !IsInPlaceSingleStep() #endif ) { @@ -4515,7 +4515,7 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, // Copy the data into our buffer. memcpy(bufferBypassRW, patch->address + m_instrAttrib.m_cbInstr + dwOldDisp, m_instrAttrib.m_cOperandSize); - if (m_instrAttrib.m_fIsWrite /*&& !m_instrAttrib.m_fIsCall*/) + if (m_instrAttrib.m_fIsWrite) { // save the actual destination address and size so when we TriggerSingleStep() we can update the value pSharedPatchBypassBufferRW->RipTargetFixup = (UINT_PTR)(patch->address + m_instrAttrib.m_cbInstr + dwOldDisp); @@ -4592,9 +4592,9 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, patchBypassRX = NativeWalker::SetupOrSimulateInstructionForPatchSkip(context, m_pSharedPatchBypassBuffer, (const BYTE *)patch->address, patch->opcode); #endif //TARGET_ARM64 -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT if (!IsInPlaceSingleStep()) -#endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) +#endif { //set eip to point to buffer... SetIP(context, (PCODE)patchBypassRX); @@ -4857,15 +4857,13 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont { #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT if (!IsInPlaceSingleStep()) - { #endif + { LOG((LF_CORDB, LL_INFO10000, "Bypass instruction redirected because still in skip area.\n" "\tm_fIsCall = %s, patchBypass = %p, m_address = %p\n", (m_instrAttrib.m_fIsCall ? "true" : "false"), patchBypass, m_address)); SetIP(context, (PCODE)((BYTE *)GetIP(context) - (patchBypass - (BYTE *)m_address))); -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT } -#endif } else { @@ -4925,102 +4923,61 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont return TPR_TRIGGER; } -DPOSS_ACTION DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) +bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) { LOG((LF_CORDB,LL_INFO10000, "DPS::TSS: basically a no-op\n")); #if defined(TARGET_AMD64) -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - if (!IsInPlaceSingleStep()) -#endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) + // Dev11 91932: for RIP-relative writes we need to copy the value that was written in our buffer to the actual address + _ASSERTE(m_pSharedPatchBypassBuffer); + if (m_pSharedPatchBypassBuffer->RipTargetFixup +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + && !IsInPlaceSingleStep() +#endif + ) { - // Dev11 91932: for RIP-relative writes we need to copy the value that was written in our buffer to the actual address - _ASSERTE(m_pSharedPatchBypassBuffer); - if (m_pSharedPatchBypassBuffer->RipTargetFixup) - { - _ASSERTE(m_pSharedPatchBypassBuffer->RipTargetFixupSize); + _ASSERTE(m_pSharedPatchBypassBuffer->RipTargetFixupSize); - BYTE* bufferBypass = m_pSharedPatchBypassBuffer->BypassBuffer; - BYTE fixupSize = m_pSharedPatchBypassBuffer->RipTargetFixupSize; - UINT_PTR targetFixup = m_pSharedPatchBypassBuffer->RipTargetFixup; + BYTE* bufferBypass = m_pSharedPatchBypassBuffer->BypassBuffer; + BYTE fixupSize = m_pSharedPatchBypassBuffer->RipTargetFixupSize; + UINT_PTR targetFixup = m_pSharedPatchBypassBuffer->RipTargetFixup; - switch (fixupSize) - { - case 1: - *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); - break; + switch (fixupSize) + { + case 1: + *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); + break; - case 2: - *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); - break; + case 2: + *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); + break; - case 4: - *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); - break; + case 4: + *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); + break; - case 8: - *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); - break; + case 8: + *(reinterpret_cast(targetFixup)) = *(reinterpret_cast(bufferBypass)); + break; - case 16: - case 32: - case 64: - memcpy(reinterpret_cast(targetFixup), bufferBypass, fixupSize); - break; + case 16: + case 32: + case 64: + memcpy(reinterpret_cast(targetFixup), bufferBypass, fixupSize); + break; - default: - _ASSERTE(!"bad operand size. If you hit this and it was because we need to process instructions with larger \ - relative immediates, make sure to update the SharedPatchBypassBuffer size, the DebuggerHeapExecutableMemoryAllocator, \ - and structures depending on DBG_MAX_EXECUTABLE_ALLOC_SIZE."); - } + default: + _ASSERTE(!"bad operand size. If you hit this and it was because we need to process instructions with larger \ + relative immediates, make sure to update the SharedPatchBypassBuffer size, the DebuggerHeapExecutableMemoryAllocator, \ + and structures depending on DBG_MAX_EXECUTABLE_ALLOC_SIZE."); } } -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - else if (IsInPlaceSingleStep()) - { - LOG((LF_CORDB, LL_INFO10000, "This is an in-place single-step, all we want to do is back-patch the breakpoint\n")); - // { - // LPVOID baseAddress = (LPVOID)m_address; - // DWORD oldProt; - - // if (!VirtualProtect(baseAddress, - // CORDbg_BREAK_INSTRUCTION_SIZE, - // PAGE_EXECUTE_READWRITE, &oldProt)) - // { - // // we may be seeing unwriteable directly mapped executable memory. - // // let's try copy-on-write instead, - // if (!VirtualProtect(baseAddress, - // CORDbg_BREAK_INSTRUCTION_SIZE, - // PAGE_EXECUTE_WRITECOPY, &oldProt)) - // { - // _ASSERTE(!"VirtualProtect of code page failed"); - // goto Error; - // } - // } - - // CORDbgInsertBreakpoint(m_address); - // LOG((LF_CORDB, LL_EVERYTHING, "DPS::TSS Breakpoint was re-inserted at %p\n", - // m_address)); - - // if (!VirtualProtect(baseAddress, - // CORDbg_BREAK_INSTRUCTION_SIZE, - // oldProt, &oldProt)) - // { - // _ASSERTE(!"VirtualProtect of code page failed"); - // goto Error; - // } - // } - m_fSSCompleted = true; - return DPOSS_USED_WITH_NO_EVENT; - } -//Error: -#endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) -#endif // defined(TARGET_AMD64) +#endif LOG((LF_CORDB,LL_INFO10000, "DPS::TSS: triggered, about to delete\n")); TRACE_FREE(this); Delete(); - return DPOSS_DONT_CARE; + return false; } // * ------------------------------------------------------------------------- @@ -7523,7 +7480,7 @@ void DebuggerStepper::TriggerMethodEnter(Thread * thread, // We may have single-stepped over a return statement to land us up a frame. // Or we may have single-stepped through a method. // We never single-step into calls (we place a patch at the call destination). -DPOSS_ACTION DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) +bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) { LOG((LF_CORDB,LL_INFO10000,"DS::TSS this:0x%p, @ ip:0x%p\n", this, ip)); @@ -7544,7 +7501,7 @@ DPOSS_ACTION DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) { LOG((LF_CORDB,LL_INFO10000, "DS::TSS: not in managed code, Returning false (case 0)!\n")); DisableSingleStep(); - return DPOSS_DONT_CARE; + return false; } // If we EnC the method, we'll blast the function address, @@ -7582,7 +7539,7 @@ DPOSS_ACTION DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) EnableMethodEnter(); } DisableSingleStep(); - return DPOSS_DONT_CARE; + return false; } DisableAll(); @@ -7592,7 +7549,7 @@ DPOSS_ACTION DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) if (DetectHandleLCGMethods((PCODE)ip, fd, &info)) { - return DPOSS_DONT_CARE; + return false; } if (IsInRange(offset, m_range, m_rangeCount, &info) || @@ -7604,7 +7561,7 @@ DPOSS_ACTION DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) EnableUnwind(m_fp); LOG((LF_CORDB,LL_INFO10000, "DS::TSS: Returning false Case 1!\n")); - return DPOSS_DONT_CARE; + return false; } else { @@ -7617,10 +7574,10 @@ DPOSS_ACTION DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) DebuggerMethodInfo * dmi = g_pDebugger->GetOrCreateMethodInfo(fd->GetModule(), fd->GetMemberDef()); if ((dmi != NULL) && DetectHandleNonUserCode(&info, dmi)) - return DPOSS_DONT_CARE; + return false; PrepareForSendEvent(ticket); - return DPOSS_USED_WITH_EVENT; + return true; } } @@ -9236,18 +9193,18 @@ TP_RESULT DebuggerDataBreakpoint::TriggerPatch(DebuggerControllerPatch *patch, T } } -DPOSS_ACTION DebuggerDataBreakpoint::TriggerSingleStep(Thread *thread, const BYTE *ip) +bool DebuggerDataBreakpoint::TriggerSingleStep(Thread *thread, const BYTE *ip) { if (g_pDebugger->IsThreadAtSafePlace(thread)) { LOG((LF_CORDB, LL_INFO10000, "D:DDBP: Finally safe for stopping, stop stepping\n")); this->DisableSingleStep(); - return DPOSS_USED_WITH_EVENT; + return true; } else { LOG((LF_CORDB, LL_INFO10000, "D:DDBP: Still not safe for stopping, continue stepping\n")); - return DPOSS_DONT_CARE; + return false; } } diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index f7c642de76cde0..301b87205afc95 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -1072,8 +1072,12 @@ class DebuggerController static bool DispatchNativeException(EXCEPTION_RECORD *exception, CONTEXT *context, DWORD code, - Thread *thread, - DebuggerPatchSkip **ppDebuggerPatchSkip = nullptr); + Thread *thread +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + DebuggerSteppingInfo *pDebuggerSteppingInfo = NULL +#endif + ); static bool DispatchUnwind(Thread *thread, MethodDesc *fd, DebuggerJitInfo * pDJI, SIZE_T offset, @@ -1110,23 +1114,28 @@ class DebuggerController CONTEXT *context, DebuggerControllerQueue *pDcq, SCAN_TRIGGER stWhat, - TP_RESULT *pTpr, -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - DebuggerPatchSkip **ppDps -#endif - ); + TP_RESULT *pTpr); static DebuggerPatchSkip *ActivatePatchSkip(Thread *thread, const BYTE *eip, - BOOL fForEnC); + BOOL fForEnC +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + DebuggerSteppingInfo *pDebuggerSteppingInfo = NULL +#endif + ); static DPOSS_ACTION DispatchPatchOrSingleStep(Thread *thread, CONTEXT *context, CORDB_ADDRESS_TYPE *ip, - SCAN_TRIGGER which, - DebuggerPatchSkip **ppDebuggerPatchSkip = nullptr); + SCAN_TRIGGER which +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + DebuggerSteppingInfo *pDebuggerSteppingInfo = NULL +#endif + ); static int GetNumberOfPatches() @@ -1375,7 +1384,7 @@ class DebuggerController // Dispatched when we get a SingleStep exception on this thread. // Return true if we want SendEvent to get called. - virtual DPOSS_ACTION TriggerSingleStep(Thread *thread, const BYTE *ip); + virtual bool TriggerSingleStep(Thread *thread, const BYTE *ip); // Dispatched to notify the controller when we are going to a filter/handler @@ -1456,6 +1465,23 @@ class DebuggerController #if !defined(DACCESS_COMPILE) +// this structure stores useful information about single-stepping over a call instruction +// it is used to communicate the patch skip opcode and current state between the controller on left side and HandleSetThreadContextNeeded on the right side +class DebuggerSteppingInfo +{ + bool fIsInPlaceSingleStep = false; + PRD_TYPE opcode = 0; + +public: + bool IsInPlaceSingleStep() { return fIsInPlaceSingleStep; } + PRD_TYPE GetOpcode() { return opcode; } + void EnableInPlaceSingleStepOverCall(PRD_TYPE opcode) + { + this->fIsInPlaceSingleStep = true; + this->opcode = opcode; + } +}; + /* ------------------------------------------------------------------------- * * DebuggerPatchSkip routines * ------------------------------------------------------------------------- */ @@ -1469,7 +1495,7 @@ class DebuggerPatchSkip : public DebuggerController ~DebuggerPatchSkip(); - DPOSS_ACTION TriggerSingleStep(Thread *thread, + bool TriggerSingleStep(Thread *thread, const BYTE *ip); TP_RESULT TriggerExceptionHook(Thread *thread, CONTEXT * pContext, @@ -1493,7 +1519,6 @@ class DebuggerPatchSkip : public DebuggerController InstructionAttribute m_instrAttrib; // info about the instruction being skipped over #if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) bool m_fInPlaceSS; // is this an in-place single-step instruction? - bool m_fSSCompleted; // true if the single step has completed #endif #ifndef FEATURE_EMULATE_SINGLESTEP // this is shared among all the skippers and the controller. see the comments @@ -1507,10 +1532,16 @@ class DebuggerPatchSkip : public DebuggerController BYTE* patchBypass = m_pSharedPatchBypassBuffer->PatchBypass; return (CORDB_ADDRESS_TYPE *)patchBypass; } -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) - BOOL IsInPlaceSingleStep() { return m_instrAttrib.m_fIsCall && m_fInPlaceSS; } // only in-place single steps over call intructions are supported at this time - BOOL IsSingleStepCompleted() { return m_fSSCompleted; } - CORDB_ADDRESS_TYPE* GetAddress() { return m_address; } +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + BOOL IsInPlaceSingleStep() + { + // only in-place single steps over call intructions are supported at this time +#ifndef FEATURE_EMULATE_SINGLESTEP + return m_instrAttrib.m_fIsCall && m_fInPlaceSS; +#else +#error single step emulation not supported with out of process set thread context +#endif + } #endif #endif // !FEATURE_EMULATE_SINGLESTEP }; @@ -1650,7 +1681,7 @@ class DebuggerStepper : public DebuggerController TP_RESULT TriggerPatch(DebuggerControllerPatch *patch, Thread *thread, TRIGGER_WHY tyWhy); - DPOSS_ACTION TriggerSingleStep(Thread *thread, const BYTE *ip); + bool TriggerSingleStep(Thread *thread, const BYTE *ip); void TriggerUnwind(Thread *thread, MethodDesc *fd, DebuggerJitInfo * pDJI, SIZE_T offset, FramePointer fp, CorDebugStepReason unwindReason); @@ -1837,7 +1868,7 @@ class DebuggerDataBreakpoint : public DebuggerController virtual TP_RESULT TriggerPatch(DebuggerControllerPatch *patch, Thread *thread, TRIGGER_WHY tyWhy); - virtual DPOSS_ACTION TriggerSingleStep(Thread *thread, const BYTE *ip); + virtual bool TriggerSingleStep(Thread *thread, const BYTE *ip); bool SendEvent(Thread *thread, bool fInterruptedBySetIp) { diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index c70ed5a68fc89b..889a01b8426260 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -5462,7 +5462,9 @@ bool Debugger::FirstChanceNativeException(EXCEPTION_RECORD *exception, } bool retVal; - DebuggerPatchSkip *pDebuggerPatchSkip = nullptr; +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + DebuggerSteppingInfo debuggerSteppingInfo; +#endif { // Don't stop for native debugging anywhere inside our inproc-Filters. @@ -5471,7 +5473,11 @@ bool Debugger::FirstChanceNativeException(EXCEPTION_RECORD *exception, if (!CORDBUnrecoverableError(this)) { retVal = DebuggerController::DispatchNativeException(exception, context, - code, thread, &pDebuggerPatchSkip); + code, thread +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + ,&debuggerSteppingInfo +#endif + ); } else { @@ -5482,14 +5488,7 @@ bool Debugger::FirstChanceNativeException(EXCEPTION_RECORD *exception, #if defined(OUT_OF_PROCESS_SETTHREADCONTEXT) && !defined(DACCESS_COMPILE) if (retVal && fIsVEH) { - if (pDebuggerPatchSkip != nullptr && pDebuggerPatchSkip->IsInPlaceSingleStep()) - { - SendSetThreadContextNeeded(context, pDebuggerPatchSkip); - } - else - { - SendSetThreadContextNeeded(context); - } + SendSetThreadContextNeeded(context, &debuggerSteppingInfo); } #endif return retVal; @@ -16699,7 +16698,7 @@ void Debugger::StartCanaryThread() #ifndef DACCESS_COMPILE #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT -void Debugger::SendSetThreadContextNeeded(CONTEXT *context, DebuggerPatchSkip *patchSkip) +void Debugger::SendSetThreadContextNeeded(CONTEXT *context, DebuggerSteppingInfo *pDebuggerSteppingInfo) { STATIC_CONTRACT_NOTHROW; STATIC_CONTRACT_GC_NOTRIGGER; @@ -16750,39 +16749,21 @@ void Debugger::SendSetThreadContextNeeded(CONTEXT *context, DebuggerPatchSkip *p // adjust context size if the context pointer is not aligned with the buffer we allocated contextSize -= (DWORD)((BYTE*)pContext-(BYTE*)pBuffer); - bool fIsInPlaceSingleStep = patchSkip != nullptr && patchSkip->IsInPlaceSingleStep(); - bool fSSCompleted = patchSkip != nullptr && patchSkip->IsSingleStepCompleted(); - CORDB_ADDRESS_TYPE *patchSkipAddr = patchSkip != nullptr && fIsInPlaceSingleStep ? patchSkip->GetAddress() : nullptr; - _ASSERTE(!fIsInPlaceSingleStep || fSSCompleted || pContext->Rip == patchSkipAddr); - - PRD_TYPE opcode = CORDbg_BREAK_INSTRUCTION; - if (fIsInPlaceSingleStep) - { - if (!fSSCompleted) - { - DebuggerControllerPatch *patch = DebuggerController::GetPatchTable()->GetPatch((CORDB_ADDRESS_TYPE *)pContext->Rip); - if (patch != NULL) - { - LOG((LF_CORDB, LL_INFO10000, "D::SSTCN Patch found at address 0x%p opcode=0x%x\n", pContext->Rip, patch->opcode)); - opcode = patch->opcode; - } - } - else - { - LOG((LF_CORDB, LL_INFO10000, "D::SSTCN Patch found at address 0x%p opcode=0x%x deleting patch\n", pContext->Rip, patch->opcode)); - TRACE_FREE(patchSkip); - patchSkip->Delete(); - } - } - // send the context to the right side LOG((LF_CORDB, LL_INFO10000, "D::SSTCN ContextFlags=0x%X contextSize=%d..\n", contextFlags, contextSize)); EX_TRY { + bool fIsInPlaceSingleStep = false; + PRD_TYPE opcode = CORDbg_BREAK_INSTRUCTION; + if (pDebuggerSteppingInfo) + { + fIsInPlaceSingleStep = pDebuggerSteppingInfo->IsInPlaceSingleStep(); + opcode = pDebuggerSteppingInfo->GetOpcode(); + } SetThreadContextNeededFlare((TADDR)pContext, contextSize, - patchSkipAddr, - (((fSSCompleted&0x1)<<9)|((fIsInPlaceSingleStep&0x1)<<8)|(opcode&0xFF))); + fIsInPlaceSingleStep, + opcode); } EX_CATCH { @@ -16801,7 +16782,7 @@ BOOL Debugger::IsOutOfProcessSetContextEnabled() return m_fOutOfProcessSetContextEnabled; } #else -void Debugger::SendSetThreadContextNeeded(CONTEXT* context) +void Debugger::SendSetThreadContextNeeded(CONTEXT* context, DebuggerSteppingInfo *pDebuggerSteppingInfo) { _ASSERTE(!"SendSetThreadContextNeeded is not enabled on this platform"); } diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index c76235aea22f43..2b252c7b566d12 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -1873,7 +1873,7 @@ extern "C" void __stdcall NotifyRightSideOfSyncCompleteFlare(void); extern "C" void __stdcall NotifySecondChanceReadyForDataFlare(void); #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) -extern "C" void __stdcall SetThreadContextNeededFlare(TADDR pContext, DWORD size, CORDB_ADDRESS_TYPE* patchSkipAddr, DWORD opcode); +extern "C" void __stdcall SetThreadContextNeededFlare(TADDR pContext, DWORD size, bool fIsInPlaceSingleStep, PRD_TYPE opcode); #else #error Platform not supported #endif @@ -1888,6 +1888,7 @@ extern "C" void __stdcall SetThreadContextNeededFlare(TADDR pContext, DWORD size struct ShouldAttachDebuggerParams; struct EnsureDebuggerAttachedParams; struct SendMDANotificationParams; +class DebuggerSteppingInfo; // class Debugger: This class implements DebugInterface to provide // the hooks to the Runtime directly. @@ -3059,7 +3060,7 @@ class Debugger : public DebugInterface BOOL m_fOutOfProcessSetContextEnabled; public: // Used by Debugger::FirstChanceNativeException to update the context from out of process - void SendSetThreadContextNeeded(CONTEXT *context, DebuggerPatchSkip *patchSkip = nullptr); + void SendSetThreadContextNeeded(CONTEXT *context, DebuggerSteppingInfo *pDebuggerSteppingInfo = NULL); BOOL IsOutOfProcessSetContextEnabled(); }; diff --git a/src/coreclr/vm/dbginterface.h b/src/coreclr/vm/dbginterface.h index 17680ff249153d..a50c33c1af108c 100644 --- a/src/coreclr/vm/dbginterface.h +++ b/src/coreclr/vm/dbginterface.h @@ -19,6 +19,8 @@ typedef DPTR(struct ICorDebugInfo::NativeVarInfo) PTR_NativeVarInfo; typedef void (*FAVORCALLBACK)(void *); +class DebuggerSteppingInfo; + // // The purpose of this object is to serve as an entry point to the // debugger, which used to reside in a separate DLL. @@ -193,7 +195,7 @@ class DebugInterface // Used by EditAndContinueModule::FixContextAndResume - virtual void SendSetThreadContextNeeded(CONTEXT *context, DebuggerPatchSkip *patchSkip = nullptr) = 0; + virtual void SendSetThreadContextNeeded(CONTEXT *context, DebuggerSteppingInfo *pDebuggerSteppingInfo = nullptr) = 0; virtual BOOL IsOutOfProcessSetContextEnabled() = 0; #endif // FEATURE_METADATA_UPDATER From e8b4128c86d4f87e512837e27b7b7df20bdbe2df Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Fri, 4 Oct 2024 23:48:36 -0400 Subject: [PATCH 10/26] Track InplaceSteppingThreads at the process level on the RS, ApplyRemotePatch/RemoveRemotePatch for patch writing --- src/coreclr/debug/di/process.cpp | 103 +++++++++++++++---------------- src/coreclr/debug/di/rspriv.h | 79 ++++++++++++++++++++++-- 2 files changed, 125 insertions(+), 57 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index f357d05038cb6b..7927dcffe0474e 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -981,11 +981,6 @@ CordbProcess::CordbProcess(ULONG64 clrInstanceId, m_fAssertOnTargetInconsistency(false), m_runtimeOffsetsInitialized(false), m_writableMetadataUpdateMode(LegacyCompatPolicy) -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - , - m_fExpectingSingleStep(false), - m_patchSkipAddr(NULL) -#endif { _ASSERTE((m_id == 0) == (pShim == NULL)); @@ -1354,6 +1349,7 @@ void CordbProcess::Neuter() // Take the process lock. RSLockHolder lockHolder(GetProcessLock()); + m_inplaceSteppingThreads.DeleteAll(); NeuterChildren(); @@ -7293,6 +7289,7 @@ HRESULT CordbProcess::WriteMemory(CORDB_ADDRESS address, DWORD size, } EX_CATCH_HRESULT(hr); SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); + IfFailThrow(hr); } // Since we may have @@ -11159,7 +11156,7 @@ void CordbProcess::FilterClrNotification( #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN\n")); #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) // Before we can read the left side context information, we must: @@ -11182,21 +11179,21 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); if (pThread == NULL) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Failed to find thread\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Failed to find thread\n")); ThrowHR(E_UNEXPECTED); } IDacDbiInterface* pDAC = GetDAC(); if (pDAC == NULL) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - DAC not initialized\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- DAC not initialized\n")); ThrowHR(E_UNEXPECTED); } HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); if (hOutOfProcThread == NULL) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Failed to get thread handle\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Failed to get thread handle\n")); ThrowHR(E_UNEXPECTED); } @@ -11205,7 +11202,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); if (!fSuccess) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from DuplicateHandle\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from DuplicateHandle\n")); ThrowHR(HRESULT_FROM_GetLastError()); } @@ -11213,7 +11210,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) DWORD previousSuspendCount = ::SuspendThread(hThread); if (previousSuspendCount == (DWORD)-1) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from SuspendThread\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from SuspendThread\n")); ThrowHR(HRESULT_FROM_GetLastError()); } @@ -11226,7 +11223,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) BOOL success = ::GetThreadContext(hThread, (CONTEXT*)(&context)); if (!success) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from GetThreadContext\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from GetThreadContext\n")); ThrowHR(HRESULT_FROM_GetLastError()); } @@ -11235,13 +11232,13 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) DWORD contextSize = (DWORD)context.Rdx; bool fIsInPlaceSingleStep = (bool)context.R8; - PRD_TYPE opcode = (PRD_TYPE)context.R9&0xFF; + PRD_TYPE opcode = (PRD_TYPE)context.R9; if (contextSize == 0 || contextSize > sizeof(CONTEXT) + 25000) { - _ASSERTE(!"Corrupted HandleSetThreadContextNeeded message received"); + _ASSERTE(!"CDP::HSTCN Corrupted message received"); - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Corrupted HandleSetThreadContextNeeded message received\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Corrupted message received\n")); ThrowHR(E_UNEXPECTED); } @@ -11253,7 +11250,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { _ASSERTE(!"ReadVirtual failed"); - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - ReadVirtual (error: 0x%X).\n", hr)); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- ReadVirtual (error: 0x%X).\n", hr)); ThrowHR(CORDBG_E_READVIRTUAL_FAILURE); } @@ -11262,7 +11259,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { _ASSERTE(!"ReadVirtual context size mismatch"); - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - ReadVirtual context size mismatch\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- ReadVirtual context size mismatch\n")); ThrowHR(ERROR_PARTIAL_COPY); } @@ -11279,12 +11276,12 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { _ASSERTE(!"InitializeContext unexpectedly succeeded or didn't return ERROR_INSUFFICIENT_BUFFER"); - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - InitializeContext unexpectedly succeeded or didn't return ERROR_INSUFFICIENT_BUFFER\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- InitializeContext unexpectedly succeeded or didn't return ERROR_INSUFFICIENT_BUFFER\n")); ThrowHR(E_UNEXPECTED); } - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - InitializeContext ContextSize %d\n", contextSize)); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- InitializeContext ContextSize %d\n", contextSize)); PVOID pBuffer = _alloca(contextSize); PCONTEXT pFrameContext = NULL; @@ -11294,7 +11291,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) HRESULT hr = HRESULT_FROM_WIN32(GetLastError()); _ASSERTE(!"InitializeContext failed"); - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from InitializeContext (error: 0x%X [%d]).\n", hr, GetLastError())); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from InitializeContext (error: 0x%X [%d]).\n", hr, GetLastError())); ThrowHR(hr); } @@ -11302,18 +11299,18 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) _ASSERTE((BYTE*)pFrameContext == pBuffer); success = CopyContext(pFrameContext, contextFlags, pContext); - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - CopyContext=%s %d\n", success?"SUCCESS":"FAIL", GetLastError())); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- CopyContext=%s %d\n", success?"SUCCESS":"FAIL", GetLastError())); if (!success) { HRESULT hr = HRESULT_FROM_WIN32(GetLastError()); _ASSERTE(!"CopyContext failed"); - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from CopyContext (error: 0x%X [%d]).\n", hr, GetLastError())); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from CopyContext (error: 0x%X [%d]).\n", hr, GetLastError())); ThrowHR(hr); } - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Set Thread Context - ID = 0x%X, SS enabled = %d\n", dwThreadId, /*(uint64_t)hThread,*/ (pContext->EFlags & 0x100) != 0)); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Set Thread Context - ID = 0x%X, SS enabled = %d\n", dwThreadId, /*(uint64_t)hThread,*/ (pContext->EFlags & 0x100) != 0)); DWORD lastError = 0; @@ -11324,25 +11321,25 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) lastError = ::GetLastError(); } - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Set Thread Context Completed: Success=%d GetLastError=%d hr=0x%X\n", success, lastError, HRESULT_FROM_WIN32(lastError))); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Set Thread Context Completed: Success=%d GetLastError=%d hr=0x%X\n", success, lastError, HRESULT_FROM_WIN32(lastError))); _ASSERTE(success); // Now that we have completed the SetThreadContext, resume the thread DWORD suspendCount = ::ResumeThread(hThread); if (suspendCount == (DWORD)-1) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from ResumeThread\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from ResumeThread\n")); ThrowHR(HRESULT_FROM_GetLastError()); } if (suspendCount != previousSuspendCount + 1) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from ResumeThread\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from ResumeThread\n")); ThrowHR(E_UNEXPECTED); } if (!success) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from SetThreadContext\n")); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from SetThreadContext\n")); ThrowHR(HRESULT_FROM_WIN32(lastError)); } @@ -11350,19 +11347,18 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)pFrameContext->Rip; - this->m_patchSkipAddr = patchSkipAddr; - this->m_fExpectingSingleStep = true; + pThread->SetPatchSkipAddr(patchSkipAddr); + + pThread->InternalSuspendOtherThreads(&m_userThreads); + + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- address=0x%p opcode=0x%x\n", patchSkipAddr, opcode)); + HRESULT hr = RemoveRemotePatch(this, (void*)patchSkipAddr, opcode); + IfFailThrow(hr); - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - address=0x%p opcode=0x%x\n", patchSkipAddr, opcode)); - EX_TRY { - TargetBuffer tb((void*)patchSkipAddr, sizeof(BYTE)); - SafeWriteBuffer(tb, (const BYTE*)&opcode); // throws + RSLockHolder lockHolder(GetProcessLock()); + m_inplaceSteppingThreads.Add(pThread); } - EX_CATCH_HRESULT(hr); - SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); - - pThread->InternalSuspendOtherThreads(&m_userThreads); } #else #error Platform not supported @@ -11624,29 +11620,32 @@ HRESULT CordbProcess::Filter( HandleSetThreadContextNeeded(dwThreadId); *pContinueStatus = DBG_CONTINUE; } - else if (dwFirstChance && pRecord->ExceptionCode == STATUS_SINGLE_STEP && m_fExpectingSingleStep) + else if (dwFirstChance && pRecord->ExceptionCode == STATUS_SINGLE_STEP) { - // we are expecting a single step exception CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); - if (pThread != NULL) + bool fIsInPlaceSingleStep = false; { - EX_TRY + RSLockHolder lockHolder(GetProcessLock()); + fIsInPlaceSingleStep = m_inplaceSteppingThreads.Contains(pThread); + } + if (pThread != NULL && fIsInPlaceSingleStep) + { + // we are expecting a single step exception + HRESULT hr = ApplyRemotePatch(this, pThread->GetPatchSkipAddr()); + IfFailThrow(hr); + + pThread->ClearPatchSkipAddr(); + { - BYTE opcode = CORDbg_BREAK_INSTRUCTION; - TargetBuffer tb((void*)m_patchSkipAddr, sizeof(BYTE)); - SafeWriteBuffer(tb, (const BYTE*)&opcode); // throws + RSLockHolder lockHolder(GetProcessLock()); + m_inplaceSteppingThreads.Remove(pThread); } - EX_CATCH_HRESULT(hr); - SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); - - m_fExpectingSingleStep = false; - m_patchSkipAddr = NULL; pThread->InternalResumeOtherThreads(); - } - // let the normal debugger stepper logic execute for this single step - *pContinueStatus = DBG_EXCEPTION_NOT_HANDLED; + // let the normal left side debugger stepper logic execute for this single step + *pContinueStatus = DBG_EXCEPTION_NOT_HANDLED; + } } #endif } diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index 48a38353eb372f..fd1cfdd86d6cee 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -4126,10 +4126,73 @@ class CordbProcess : COM_METHOD GetObjectInternal(CORDB_ADDRESS addr, CordbAppDomain* pAppDomainOverride, ICorDebugObjectValue **pObject); -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - bool m_fExpectingSingleStep; - CORDB_ADDRESS_TYPE * m_patchSkipAddr; -#endif + struct InplaceSteppingThreads + { + CordbThread *pThread = NULL; + InplaceSteppingThreads *pNext = NULL; + + void DeleteAll() + { + pThread = NULL; + while (pNext) + { + InplaceSteppingThreads *pNextNext = pNext->pNext; + delete pNext; + pNext = pNextNext; + } + } + + void Add(CordbThread *pThread) + { + + if (this->pThread != NULL) + { + InplaceSteppingThreads *pNew = new InplaceSteppingThreads(); + pNew->pThread = this->pThread; + pNew->pNext = this->pNext; + this->pNext = pNew; + } + + this->pThread = pThread; + } + + void Remove(CordbThread *pThread) + { + InplaceSteppingThreads *pThis = this; + while (pThis) + { + if (pThis->pThread == pThread) + { + InplaceSteppingThreads *pNext = pThis->pNext; + pThis->pThread = pNext ? pNext->pThread : NULL; + pThis->pNext = pNext ? pNext->pNext : NULL; + if (pNext) + { + delete pNext; + } + return; + } + pThis = pThis->pNext; + } + } + + bool Contains(CordbThread *pThread) + { + InplaceSteppingThreads *pThis = this; + while (pThis) + { + if (pThis->pThread == pThread) + { + return true; + } + pThis = pThis->pNext; + } + return false; + } + + bool IsEmptry() { return pThread == NULL; } + } m_inplaceSteppingThreads; + }; // Some IMDArocess APIs are supported as interop-only. @@ -6381,10 +6444,16 @@ class CordbThread : public CordbBase, public ICorDebugThread, #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT private: CordbSafeHashTable m_SuspendedThreads; - DWORD m_dwInternalSuspendCount; + DWORD m_dwInternalSuspendCount; + CORDB_ADDRESS_TYPE * m_patchSkipAddr; + public: HRESULT InternalSuspendOtherThreads(CordbSafeHashTable *pThreads); HRESULT InternalResumeOtherThreads(); + bool IsExpectingSingleStep() { return m_patchSkipAddr != NULL; } + void ClearPatchSkipAddr() { m_patchSkipAddr = NULL; } + void SetPatchSkipAddr(CORDB_ADDRESS_TYPE *patchSkipAddr) { m_patchSkipAddr = patchSkipAddr; } + void* GetPatchSkipAddr() { return (void*)m_patchSkipAddr; } #endif }; From 83a3043eda0d41de1438fd48fd333f593107a717 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Sat, 5 Oct 2024 12:28:19 -0400 Subject: [PATCH 11/26] Block detach if m_inplaceSteppingThreads is not empty --- src/coreclr/debug/di/process.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 7927dcffe0474e..57052207f69d2e 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -1349,7 +1349,9 @@ void CordbProcess::Neuter() // Take the process lock. RSLockHolder lockHolder(GetProcessLock()); +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT m_inplaceSteppingThreads.DeleteAll(); +#endif NeuterChildren(); @@ -15154,7 +15156,11 @@ HRESULT CordbProcess::IsReadyForDetach() // // If there are any outstanding steppers then fail the detach. // - if (m_steppers.IsInitialized() && (m_steppers.GetCount() > 0)) + if (m_steppers.IsInitialized() && (m_steppers.GetCount() > 0) +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + || !m_inplaceSteppingThreads.IsEmptry() +#endif + ) { return CORDBG_E_DETACH_FAILED_OUTSTANDING_STEPPERS; } From 961d07ca94a926dbb3fc97836aa6f5e381b6b97e Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Sat, 5 Oct 2024 12:32:31 -0400 Subject: [PATCH 12/26] Refactor InplaceSteppingThreads impl --- src/coreclr/debug/di/process.cpp | 60 ++++++++++++++++++++++ src/coreclr/debug/di/rspriv.h | 85 +++++++------------------------- 2 files changed, 79 insertions(+), 66 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 57052207f69d2e..a9a0364f2574ea 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -36,6 +36,66 @@ struct RSDebuggingInfo; extern RSDebuggingInfo * g_pRSDebuggingInfo; +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT +void InplaceSteppingThreads::DeleteAll() +{ + pThread = NULL; + while (pNext) + { + InplaceSteppingThreads *pNextNext = pNext->pNext; + delete pNext; + pNext = pNextNext; + } +} + +void InplaceSteppingThreads::Add(CordbThread *pThread) +{ + if (this->pThread != NULL) + { + InplaceSteppingThreads *pNew = new InplaceSteppingThreads(); + pNew->pThread = this->pThread; + pNew->pNext = this->pNext; + this->pNext = pNew; + } + + this->pThread = pThread; +} + +void InplaceSteppingThreads::Remove(CordbThread *pThread) +{ + InplaceSteppingThreads *pThis = this; + while (pThis) + { + if (pThis->pThread == pThread) + { + InplaceSteppingThreads *pNext = pThis->pNext; + pThis->pThread = pNext ? pNext->pThread : NULL; + pThis->pNext = pNext ? pNext->pNext : NULL; + if (pNext) + { + delete pNext; + } + return; + } + pThis = pThis->pNext; + } +} + +bool InplaceSteppingThreads::Contains(CordbThread *pThread) +{ + InplaceSteppingThreads *pThis = this; + while (pThis) + { + if (pThis->pThread == pThread) + { + return true; + } + pThis = pThis->pNext; + } + return false; +} +#endif // OUT_OF_PROCESS_SETTHREADCONTEXT + //--------------------------------------------------------------------------------------- // // OpenVirtualProcessImpl method called by the shim to get an ICorDebugProcess4 instance diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index fd1cfdd86d6cee..ee027049eeec0a 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -2921,6 +2921,22 @@ struct DbgAssertAppDomainDeletedData VMPTR_AppDomain m_vmAppDomainDeleted; }; + +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT +class InplaceSteppingThreads +{ + CordbThread *pThread = NULL; + InplaceSteppingThreads *pNext = NULL; + +public: + void DeleteAll(); + void Add(CordbThread *pThread); + void Remove(CordbThread *pThread); + bool Contains(CordbThread *pThread); + bool IsEmptry() { return pThread == NULL; } +}; +#endif // OUT_OF_PROCESS_SETTHREADCONTEXT + class CordbProcess : public CordbBase, public ICorDebugProcess, @@ -4126,72 +4142,9 @@ class CordbProcess : COM_METHOD GetObjectInternal(CORDB_ADDRESS addr, CordbAppDomain* pAppDomainOverride, ICorDebugObjectValue **pObject); - struct InplaceSteppingThreads - { - CordbThread *pThread = NULL; - InplaceSteppingThreads *pNext = NULL; - - void DeleteAll() - { - pThread = NULL; - while (pNext) - { - InplaceSteppingThreads *pNextNext = pNext->pNext; - delete pNext; - pNext = pNextNext; - } - } - - void Add(CordbThread *pThread) - { - - if (this->pThread != NULL) - { - InplaceSteppingThreads *pNew = new InplaceSteppingThreads(); - pNew->pThread = this->pThread; - pNew->pNext = this->pNext; - this->pNext = pNew; - } - - this->pThread = pThread; - } - - void Remove(CordbThread *pThread) - { - InplaceSteppingThreads *pThis = this; - while (pThis) - { - if (pThis->pThread == pThread) - { - InplaceSteppingThreads *pNext = pThis->pNext; - pThis->pThread = pNext ? pNext->pThread : NULL; - pThis->pNext = pNext ? pNext->pNext : NULL; - if (pNext) - { - delete pNext; - } - return; - } - pThis = pThis->pNext; - } - } - - bool Contains(CordbThread *pThread) - { - InplaceSteppingThreads *pThis = this; - while (pThis) - { - if (pThis->pThread == pThread) - { - return true; - } - pThis = pThis->pNext; - } - return false; - } - - bool IsEmptry() { return pThread == NULL; } - } m_inplaceSteppingThreads; +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + InplaceSteppingThreads m_inplaceSteppingThreads; +#endif // OUT_OF_PROCESS_SETTHREADCONTEXT }; From 7942742d847f8027d30c14f77cb12896a56f5586 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Sun, 6 Oct 2024 19:34:06 -0400 Subject: [PATCH 13/26] Track thread create and exit for thread suspension --- src/coreclr/debug/di/process.cpp | 282 ++++++++++++++++----------- src/coreclr/debug/di/rspriv.h | 60 +++--- src/coreclr/debug/di/rsthread.cpp | 125 ------------ src/coreclr/debug/di/shimprocess.cpp | 8 + 4 files changed, 208 insertions(+), 267 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index a9a0364f2574ea..a6a93cdd0fc9e0 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -36,66 +36,6 @@ struct RSDebuggingInfo; extern RSDebuggingInfo * g_pRSDebuggingInfo; -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT -void InplaceSteppingThreads::DeleteAll() -{ - pThread = NULL; - while (pNext) - { - InplaceSteppingThreads *pNextNext = pNext->pNext; - delete pNext; - pNext = pNextNext; - } -} - -void InplaceSteppingThreads::Add(CordbThread *pThread) -{ - if (this->pThread != NULL) - { - InplaceSteppingThreads *pNew = new InplaceSteppingThreads(); - pNew->pThread = this->pThread; - pNew->pNext = this->pNext; - this->pNext = pNew; - } - - this->pThread = pThread; -} - -void InplaceSteppingThreads::Remove(CordbThread *pThread) -{ - InplaceSteppingThreads *pThis = this; - while (pThis) - { - if (pThis->pThread == pThread) - { - InplaceSteppingThreads *pNext = pThis->pNext; - pThis->pThread = pNext ? pNext->pThread : NULL; - pThis->pNext = pNext ? pNext->pNext : NULL; - if (pNext) - { - delete pNext; - } - return; - } - pThis = pThis->pNext; - } -} - -bool InplaceSteppingThreads::Contains(CordbThread *pThread) -{ - InplaceSteppingThreads *pThis = this; - while (pThis) - { - if (pThis->pThread == pThread) - { - return true; - } - pThis = pThis->pNext; - } - return false; -} -#endif // OUT_OF_PROCESS_SETTHREADCONTEXT - //--------------------------------------------------------------------------------------- // // OpenVirtualProcessImpl method called by the shim to get an ICorDebugProcess4 instance @@ -352,6 +292,85 @@ static inline DWORD CordbGetWaitTimeout() } } +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT +HANDLE UnmanagedThreadTracker::GetThreadHandle(CordbProcess * pProcess) +{ + HANDLE hThread = m_hThread; + if (hThread == INVALID_HANDLE_VALUE) + { + // lookup the CordbThread by thread ID, so that we can access the left-side thread handle + CordbThread * pThread = pProcess->TryLookupOrCreateThreadByVolatileOSId(m_dwThreadId); + + IDacDbiInterface* pDAC = pProcess->GetDAC(); + if (pDAC == NULL) + { + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - DAC not initialized\n")); + ThrowHR(E_UNEXPECTED); + } + + HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); + if (hOutOfProcThread == NULL) + { + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Failed to get thread handle\n")); + ThrowHR(E_UNEXPECTED); + } + + // Duplicate the thread handle to the current process + BOOL fSuccess = ::DuplicateHandle(pProcess->UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); + if (!fSuccess) + { + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from DuplicateHandle\n")); + ThrowHR(HRESULT_FROM_GetLastError()); + } + + m_hThread = hThread; + } + + return hThread; +} + +void UnmanagedThreadTracker::Suspend() +{ + _ASSERTE(m_hThread != INVALID_HANDLE_VALUE); + if (m_hThread != INVALID_HANDLE_VALUE) + { + m_dwSuspendCount++; + ::SuspendThread(m_hThread); + } +} + +void UnmanagedThreadTracker::Resume() +{ + _ASSERTE(m_dwSuspendCount > 0); + if (m_dwSuspendCount == 0) + return; + m_dwSuspendCount--; + _ASSERTE(m_hThread != INVALID_HANDLE_VALUE); + if (m_hThread != INVALID_HANDLE_VALUE) + { + ::ResumeThread(m_hThread); + } +} + +void UnmanagedThreadTracker::Close() +{ + if (m_hThread != INVALID_HANDLE_VALUE) + { + for (DWORD i = 0; i < m_dwSuspendCount; i++) + { + ::ResumeThread(m_hThread); + } + } + m_dwSuspendCount = 0; + if (m_hThread == INVALID_HANDLE_VALUE) + { + return; + } + CloseHandle(m_hThread); + m_hThread = INVALID_HANDLE_VALUE; +} +#endif // OUT_OF_PROCESS_SETTHREADCONTEXT + //---------------------------------------------------------------------------- // Implementation of IDacDbiInterface::IMetaDataLookup. // lookup Internal Metadata Importer keyed by PEAssembly @@ -1410,7 +1429,12 @@ void CordbProcess::Neuter() RSLockHolder lockHolder(GetProcessLock()); #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - m_inplaceSteppingThreads.DeleteAll(); + CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); + for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) + { + cur->second.Close(); + } + m_unmanagedThreadHashTable.clear(); #endif NeuterChildren(); @@ -7351,7 +7375,6 @@ HRESULT CordbProcess::WriteMemory(CORDB_ADDRESS address, DWORD size, } EX_CATCH_HRESULT(hr); SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); - IfFailThrow(hr); } // Since we may have @@ -11232,40 +11255,21 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // However, there are situations where OpenThread can fail with an Access Denied error. // From https://github.com/dotnet/runtime/issues/107263, the control-c handler in // Windows causes the process to have higher privileges. - // We are now using the following approach to access the thread handle, which is the same - // approach used by CordbThread::RefreshHandle: - // 1. Get the thread handle from the DAC - // 2. Duplicate the handle to the current process - - // lookup the CordbThread by thread ID, so that we can access the left-side thread handle - CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); - if (pThread == NULL) - { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Failed to find thread\n")); - ThrowHR(E_UNEXPECTED); - } + // We are now caching the thread handle in the unmanaged thread hash table when the thread is created. - IDacDbiInterface* pDAC = GetDAC(); - if (pDAC == NULL) - { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- DAC not initialized\n")); - ThrowHR(E_UNEXPECTED); - } + CUnmanagedThreadHashTableImpl::iterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); - HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); - if (hOutOfProcThread == NULL) + if (curThread == m_unmanagedThreadHashTable.end() || curThread->second.GetThreadId() != dwThreadId) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Failed to get thread handle\n")); - ThrowHR(E_UNEXPECTED); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Thread not found\n")); + ThrowHR(CORDBG_E_BAD_THREAD_STATE); } - // Duplicate the thread handle to the current process - HandleHolder hThread; - BOOL fSuccess = DuplicateHandle(UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); - if (!fSuccess) + HANDLE hThread = curThread->second.GetThreadHandle(this); + if (hThread == INVALID_HANDLE_VALUE) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from DuplicateHandle\n")); - ThrowHR(HRESULT_FROM_GetLastError()); + LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Thread handle not found\n")); + ThrowHR(CORDBG_E_BAD_THREAD_STATE); } // Suspend the thread and so that we can read the thread context. @@ -11409,17 +11413,21 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)pFrameContext->Rip; - pThread->SetPatchSkipAddr(patchSkipAddr); - - pThread->InternalSuspendOtherThreads(&m_userThreads); - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- address=0x%p opcode=0x%x\n", patchSkipAddr, opcode)); HRESULT hr = RemoveRemotePatch(this, (void*)patchSkipAddr, opcode); IfFailThrow(hr); + curThread->second.SetPatchSkipAddress(patchSkipAddr); + + // suspend all other threads + CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); + for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) { - RSLockHolder lockHolder(GetProcessLock()); - m_inplaceSteppingThreads.Add(pThread); + if (cur->second.GetThreadId() == dwThreadId) + { + continue; + } + cur->second.Suspend(); } } #else @@ -11684,27 +11692,29 @@ HRESULT CordbProcess::Filter( } else if (dwFirstChance && pRecord->ExceptionCode == STATUS_SINGLE_STEP) { - CordbThread * pThread = TryLookupOrCreateThreadByVolatileOSId(dwThreadId); - bool fIsInPlaceSingleStep = false; - { - RSLockHolder lockHolder(GetProcessLock()); - fIsInPlaceSingleStep = m_inplaceSteppingThreads.Contains(pThread); - } - if (pThread != NULL && fIsInPlaceSingleStep) + CUnmanagedThreadHashTableImpl::iterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); + + if (curThread != m_unmanagedThreadHashTable.end() && + curThread->second.GetThreadId() == dwThreadId && + curThread->second.IsInPlaceStepping()) { // we are expecting a single step exception - HRESULT hr = ApplyRemotePatch(this, pThread->GetPatchSkipAddr()); + HRESULT hr = ApplyRemotePatch(this, curThread->second.GetPatchSkipAddress()); IfFailThrow(hr); - pThread->ClearPatchSkipAddr(); + curThread->second.ClearPatchSkipAddress(); + // resume all other threads + CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); + for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) { - RSLockHolder lockHolder(GetProcessLock()); - m_inplaceSteppingThreads.Remove(pThread); + if (cur->second.GetThreadId() == dwThreadId) + { + continue; + } + cur->second.Resume(); } - pThread->InternalResumeOtherThreads(); - // let the normal left side debugger stepper logic execute for this single step *pContinueStatus = DBG_EXCEPTION_NOT_HANDLED; } @@ -15216,15 +15226,24 @@ HRESULT CordbProcess::IsReadyForDetach() // // If there are any outstanding steppers then fail the detach. // - if (m_steppers.IsInitialized() && (m_steppers.GetCount() > 0) -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - || !m_inplaceSteppingThreads.IsEmptry() -#endif - ) + if (m_steppers.IsInitialized() && (m_steppers.GetCount() > 0)) { return CORDBG_E_DETACH_FAILED_OUTSTANDING_STEPPERS; } +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + { + CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); + for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) + { + if (cur->second.IsInPlaceStepping() || cur->second.IsSuspended()) + { + return CORDBG_E_DETACH_FAILED_OUTSTANDING_STEPPERS; + } + } + } +#endif + // // If there are any outstanding breakpoints then fail the detach. // @@ -15541,3 +15560,38 @@ void CordbProcess::HandleControlCTrapResult(HRESULT result) // Send the reply to the LS. SendIPCEvent(&eventControlCResult, sizeof(eventControlCResult)); } + +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT +void CordbProcess::HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent) +{ + //PUBLIC_API_ENTRY_FOR_SHIM(this); + + switch (pEvent->dwDebugEventCode) + { + case CREATE_PROCESS_DEBUG_EVENT: + { + HANDLE hThread = INVALID_HANDLE_VALUE; + ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateProcessInfo.hThread, GetCurrentProcess(), &hThread, THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_SUSPEND_RESUME, false, 0); + m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); + } + break; + case CREATE_THREAD_DEBUG_EVENT: + { + HANDLE hThread = INVALID_HANDLE_VALUE; + ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateThread.hThread, GetCurrentProcess(), &hThread, THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_SUSPEND_RESUME, false, 0); + m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); + } + break; + case EXIT_THREAD_DEBUG_EVENT: + { + CUnmanagedThreadHashTableImpl::iterator it = m_unmanagedThreadHashTable.find(pEvent->dwThreadId); + if (it != m_unmanagedThreadHashTable.end()) + { + it->second.Close(); + m_unmanagedThreadHashTable.erase(it); + } + } + break; + } +} +#endif // OUT_OF_PROCESS_SETTHREADCONTEXT \ No newline at end of file diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index ee027049eeec0a..d25a79f6cc9089 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -50,6 +50,8 @@ struct MachineInfo; #include "eventchannel.h" +#include + #undef ASSERT #define CRASH(x) _ASSERTE(!(x)) #define ASSERT(x) _ASSERTE(x) @@ -2883,6 +2885,10 @@ class IProcessShimHooks #ifdef FEATURE_INTEROP_DEBUGGING virtual bool IsUnmanagedThreadHijacked(ICorDebugThread * pICorDebugThread) = 0; #endif + +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + virtual void HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent) = 0; +#endif }; @@ -2921,20 +2927,30 @@ struct DbgAssertAppDomainDeletedData VMPTR_AppDomain m_vmAppDomainDeleted; }; - #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT -class InplaceSteppingThreads -{ - CordbThread *pThread = NULL; - InplaceSteppingThreads *pNext = NULL; - -public: - void DeleteAll(); - void Add(CordbThread *pThread); - void Remove(CordbThread *pThread); - bool Contains(CordbThread *pThread); - bool IsEmptry() { return pThread == NULL; } +class UnmanagedThreadTracker +{ + DWORD m_dwThreadId = 0; + HANDLE m_hThread = INVALID_HANDLE_VALUE; + CORDB_ADDRESS_TYPE *m_pPatchSkipAddress = NULL; + DWORD m_dwSuspendCount = 0; + +public: + UnmanagedThreadTracker(DWORD wThreadId, HANDLE hThread) : m_dwThreadId(wThreadId), m_hThread(hThread) {} + DWORD GetThreadId() { return m_dwThreadId; } + HANDLE GetThreadHandle(CordbProcess * pProcess); + bool IsInPlaceStepping() { return m_pPatchSkipAddress != NULL; } + void SetPatchSkipAddress(CORDB_ADDRESS_TYPE *pPatchSkipAddress) { m_pPatchSkipAddress = pPatchSkipAddress; } + CORDB_ADDRESS_TYPE *GetPatchSkipAddress() { return m_pPatchSkipAddress; } + void ClearPatchSkipAddress() { m_pPatchSkipAddress = NULL; } + bool IsSuspended() { return m_dwSuspendCount > 0; } + DWORD SuspendCount() { return m_dwSuspendCount; } + void Suspend(); + void Resume(); + void Close(); }; + +typedef std::unordered_map CUnmanagedThreadHashTableImpl; #endif // OUT_OF_PROCESS_SETTHREADCONTEXT class CordbProcess : @@ -4143,7 +4159,10 @@ class CordbProcess : COM_METHOD GetObjectInternal(CORDB_ADDRESS addr, CordbAppDomain* pAppDomainOverride, ICorDebugObjectValue **pObject); #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - InplaceSteppingThreads m_inplaceSteppingThreads; + CUnmanagedThreadHashTableImpl m_unmanagedThreadHashTable; + +public: + void HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent); #endif // OUT_OF_PROCESS_SETTHREADCONTEXT }; @@ -6393,21 +6412,6 @@ class CordbThread : public CordbBase, public ICorDebugThread, // offload to the shim to support V2 scenarios. HANDLE m_hCachedThread; HANDLE m_hCachedOutOfProcThread; - -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT -private: - CordbSafeHashTable m_SuspendedThreads; - DWORD m_dwInternalSuspendCount; - CORDB_ADDRESS_TYPE * m_patchSkipAddr; - -public: - HRESULT InternalSuspendOtherThreads(CordbSafeHashTable *pThreads); - HRESULT InternalResumeOtherThreads(); - bool IsExpectingSingleStep() { return m_patchSkipAddr != NULL; } - void ClearPatchSkipAddr() { m_patchSkipAddr = NULL; } - void SetPatchSkipAddr(CORDB_ADDRESS_TYPE *patchSkipAddr) { m_patchSkipAddr = patchSkipAddr; } - void* GetPatchSkipAddr() { return (void*)m_patchSkipAddr; } -#endif }; /* ------------------------------------------------------------------------- * diff --git a/src/coreclr/debug/di/rsthread.cpp b/src/coreclr/debug/di/rsthread.cpp index fdb459c91d64d1..619b37c87ef8b8 100644 --- a/src/coreclr/debug/di/rsthread.cpp +++ b/src/coreclr/debug/di/rsthread.cpp @@ -85,11 +85,6 @@ CordbThread::CordbThread(CordbProcess * pProcess, VMPTR_Thread vmThread) : m_userState(kInvalidUserState), m_hCachedThread(INVALID_HANDLE_VALUE), m_hCachedOutOfProcThread(INVALID_HANDLE_VALUE) -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - , - m_SuspendedThreads(11), - m_dwInternalSuspendCount(0) -#endif { m_fHasUnhandledException = FALSE; m_pExceptionRecord = NULL; @@ -148,28 +143,6 @@ void CordbThread::Neuter() _ASSERTE(GetProcess()->ThreadHoldsProcessLock()); -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - InternalResumeOtherThreads(); // ensure that if we had suspended other threads, they are resumed. - if (m_dwInternalSuspendCount > 0) - { - HANDLE hOutOfProcThread = GetProcess()->GetDAC()->GetThreadHandle(m_vmThreadToken); - if (hOutOfProcThread != NULL) - { - HandleHolder hThread; - BOOL fSuccess = DuplicateHandle(GetProcess()->UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); - if (fSuccess) - { - while (m_dwInternalSuspendCount > 0) - { - m_dwInternalSuspendCount--; - ::ResumeThread(hThread); - } - } - } - m_dwInternalSuspendCount = 0; - } -#endif - delete m_pExceptionRecord; m_pExceptionRecord = NULL; @@ -2814,104 +2787,6 @@ HRESULT CordbThread::GetBlockingObjects(ICorDebugBlockingObjectEnum **ppBlocking return hr; } -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT -HRESULT CordbThread::InternalSuspendOtherThreads(CordbSafeHashTable *pThreads) -{ - RSLockHolder lockHolder(GetProcess()->GetProcessLock()); - IDacDbiInterface* pDAC = GetProcess()->GetDAC(); - if (pDAC == NULL) - { - return E_UNEXPECTED; - } - - InternalResumeOtherThreads(); // clear m_SuspendedThreads - - DWORD dwThreadId = pDAC->TryGetVolatileOSThreadID(m_vmThreadToken); - HASHFIND find; - for (CordbThread * pThread = pThreads->FindFirst(&find); - pThread != NULL; - pThread = pThreads->FindNext(&find)) - { - _ASSERTE(pThread != NULL); - - // Get the OS tid. This returns 0 if the thread is switched out. - DWORD dwThreadId2 = pDAC->TryGetVolatileOSThreadID(pThread->m_vmThreadToken); - if (dwThreadId2 == dwThreadId) - { - continue; - } - - HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); - if (hOutOfProcThread == NULL) - { - continue; - } - - HandleHolder hThread; - BOOL fSuccess = DuplicateHandle(GetProcess()->UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); - if (!fSuccess) - { - continue; - } - if (::SuspendThread(hThread) != (DWORD)-1) - { - pThread->m_dwInternalSuspendCount++; - CordbThread *tmpThread = m_SuspendedThreads.GetBase(VmPtrToCookie(pThread->m_vmThreadToken)); - _ASSERTE(tmpThread == NULL); - if (tmpThread == NULL) - { - m_SuspendedThreads.AddBaseOrThrow(pThread); - } - } - } - - return S_OK; -} - -HRESULT CordbThread::InternalResumeOtherThreads() -{ - RSLockHolder lockHolder(GetProcess()->GetProcessLock()); - - IDacDbiInterface* pDAC = GetProcess()->GetDAC(); - if (pDAC == NULL) - { - return E_UNEXPECTED; - } - - UINT32 count = m_SuspendedThreads.GetCount(); - UINT32 idx = 0; - HASHFIND find; - while (idx++ < count) - { - CordbThread * pThread = m_SuspendedThreads.FindFirst(&find); - _ASSERTE(pThread != NULL); - if (pThread == NULL) - { - break; - } - - if (pThread->m_dwInternalSuspendCount > 0) - { - HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); - if (hOutOfProcThread != NULL) - { - HandleHolder hThread; - BOOL fSuccess = DuplicateHandle(GetProcess()->UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); - if (fSuccess) - { - pThread->m_dwInternalSuspendCount--; - ::ResumeThread(hThread); - } - - } - } - m_SuspendedThreads.RemoveBase(VmPtrToCookie(pThread->m_vmThreadToken)); - } - - return S_OK; -} -#endif // OUT_OF_PROCESS_SETTHREADCONTEXT - #ifdef FEATURE_INTEROP_DEBUGGING /* ------------------------------------------------------------------------- * * Unmanaged Thread classes diff --git a/src/coreclr/debug/di/shimprocess.cpp b/src/coreclr/debug/di/shimprocess.cpp index c44be066370a5e..4b09d7993a77cc 100644 --- a/src/coreclr/debug/di/shimprocess.cpp +++ b/src/coreclr/debug/di/shimprocess.cpp @@ -789,6 +789,14 @@ HRESULT ShimProcess::HandleWin32DebugEvent(const DEBUG_EVENT * pEvent) } } } +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + else if (pEvent->dwDebugEventCode == CREATE_THREAD_DEBUG_EVENT || + pEvent->dwDebugEventCode == EXIT_THREAD_DEBUG_EVENT || + pEvent->dwDebugEventCode == CREATE_PROCESS_DEBUG_EVENT) + { + m_pProcess->HandleDebugEventForInPlaceStepping(pEvent); + } +#endif // Do standard event handling, including Handling loader-breakpoint, // and callback into CordbProcess for Attach if needed. From 657102bec83152ed0211eaab62d7941eedfcf598 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Sun, 6 Oct 2024 20:21:58 -0400 Subject: [PATCH 14/26] Change comments to minimize size of change --- src/coreclr/debug/di/process.cpp | 40 ++++++++++++++++---------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index a6a93cdd0fc9e0..9acc8afd0c23ee 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11241,7 +11241,7 @@ void CordbProcess::FilterClrNotification( #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded \n")); #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) // Before we can read the left side context information, we must: @@ -11261,14 +11261,14 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) if (curThread == m_unmanagedThreadHashTable.end() || curThread->second.GetThreadId() != dwThreadId) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Thread not found\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Thread not found\n")); ThrowHR(CORDBG_E_BAD_THREAD_STATE); } HANDLE hThread = curThread->second.GetThreadHandle(this); if (hThread == INVALID_HANDLE_VALUE) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Thread handle not found\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Thread handle not found\n")); ThrowHR(CORDBG_E_BAD_THREAD_STATE); } @@ -11276,7 +11276,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) DWORD previousSuspendCount = ::SuspendThread(hThread); if (previousSuspendCount == (DWORD)-1) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from SuspendThread\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from SuspendThread\n")); ThrowHR(HRESULT_FROM_GetLastError()); } @@ -11289,7 +11289,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) BOOL success = ::GetThreadContext(hThread, (CONTEXT*)(&context)); if (!success) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from GetThreadContext\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from GetThreadContext\n")); ThrowHR(HRESULT_FROM_GetLastError()); } @@ -11302,9 +11302,9 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) if (contextSize == 0 || contextSize > sizeof(CONTEXT) + 25000) { - _ASSERTE(!"CDP::HSTCN Corrupted message received"); + _ASSERTE(!"RS HandleSetThreadContextNeeded Corrupted message received"); - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Corrupted message received\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Corrupted message received\n")); ThrowHR(E_UNEXPECTED); } @@ -11316,7 +11316,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { _ASSERTE(!"ReadVirtual failed"); - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- ReadVirtual (error: 0x%X).\n", hr)); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - ReadVirtual (error: 0x%X).\n", hr)); ThrowHR(CORDBG_E_READVIRTUAL_FAILURE); } @@ -11325,7 +11325,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { _ASSERTE(!"ReadVirtual context size mismatch"); - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- ReadVirtual context size mismatch\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - ReadVirtual context size mismatch\n")); ThrowHR(ERROR_PARTIAL_COPY); } @@ -11342,12 +11342,12 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { _ASSERTE(!"InitializeContext unexpectedly succeeded or didn't return ERROR_INSUFFICIENT_BUFFER"); - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- InitializeContext unexpectedly succeeded or didn't return ERROR_INSUFFICIENT_BUFFER\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - InitializeContext unexpectedly succeeded or didn't return ERROR_INSUFFICIENT_BUFFER\n")); ThrowHR(E_UNEXPECTED); } - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- InitializeContext ContextSize %d\n", contextSize)); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - InitializeContext ContextSize %d\n", contextSize)); PVOID pBuffer = _alloca(contextSize); PCONTEXT pFrameContext = NULL; @@ -11357,7 +11357,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) HRESULT hr = HRESULT_FROM_WIN32(GetLastError()); _ASSERTE(!"InitializeContext failed"); - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from InitializeContext (error: 0x%X [%d]).\n", hr, GetLastError())); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from InitializeContext (error: 0x%X [%d]).\n", hr, GetLastError())); ThrowHR(hr); } @@ -11365,18 +11365,18 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) _ASSERTE((BYTE*)pFrameContext == pBuffer); success = CopyContext(pFrameContext, contextFlags, pContext); - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- CopyContext=%s %d\n", success?"SUCCESS":"FAIL", GetLastError())); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - CopyContext=%s %d\n", success?"SUCCESS":"FAIL", GetLastError())); if (!success) { HRESULT hr = HRESULT_FROM_WIN32(GetLastError()); _ASSERTE(!"CopyContext failed"); - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from CopyContext (error: 0x%X [%d]).\n", hr, GetLastError())); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from CopyContext (error: 0x%X [%d]).\n", hr, GetLastError())); ThrowHR(hr); } - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Set Thread Context - ID = 0x%X, SS enabled = %d\n", dwThreadId, /*(uint64_t)hThread,*/ (pContext->EFlags & 0x100) != 0)); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Set Thread Context - ID = 0x%X, SS enabled = %d\n", dwThreadId, /*(uint64_t)hThread,*/ (pContext->EFlags & 0x100) != 0)); DWORD lastError = 0; @@ -11387,25 +11387,25 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) lastError = ::GetLastError(); } - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Set Thread Context Completed: Success=%d GetLastError=%d hr=0x%X\n", success, lastError, HRESULT_FROM_WIN32(lastError))); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Set Thread Context Completed: Success=%d GetLastError=%d hr=0x%X\n", success, lastError, HRESULT_FROM_WIN32(lastError))); _ASSERTE(success); // Now that we have completed the SetThreadContext, resume the thread DWORD suspendCount = ::ResumeThread(hThread); if (suspendCount == (DWORD)-1) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from ResumeThread\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from ResumeThread\n")); ThrowHR(HRESULT_FROM_GetLastError()); } if (suspendCount != previousSuspendCount + 1) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from ResumeThread\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from ResumeThread\n")); ThrowHR(E_UNEXPECTED); } if (!success) { - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- Unexpected result from SetThreadContext\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from SetThreadContext\n")); ThrowHR(HRESULT_FROM_WIN32(lastError)); } @@ -11413,7 +11413,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)pFrameContext->Rip; - LOG((LF_CORDB, LL_INFO10000, "CDP::HSTCN- address=0x%p opcode=0x%x\n", patchSkipAddr, opcode)); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - address=0x%p opcode=0x%x\n", patchSkipAddr, opcode)); HRESULT hr = RemoveRemotePatch(this, (void*)patchSkipAddr, opcode); IfFailThrow(hr); From 826721d68a5a1ce97d309cb1486317c166c775d1 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Sun, 6 Oct 2024 20:23:47 -0400 Subject: [PATCH 15/26] Fix blank space diff --- src/coreclr/debug/di/process.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 9acc8afd0c23ee..ac88cb02c07404 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11241,7 +11241,7 @@ void CordbProcess::FilterClrNotification( #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded \n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded\n")); #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) // Before we can read the left side context information, we must: From 331d4fdfc94981c1fc43fb53b7898d31511827f1 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Sun, 6 Oct 2024 23:00:14 -0400 Subject: [PATCH 16/26] Keep track of single step thread suspension count at the process level --- src/coreclr/debug/di/process.cpp | 98 ++++++++++++-------------------- src/coreclr/debug/di/rspriv.h | 6 +- 2 files changed, 38 insertions(+), 66 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index ac88cb02c07404..3d229f4b99d6f6 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -293,42 +293,6 @@ static inline DWORD CordbGetWaitTimeout() } #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT -HANDLE UnmanagedThreadTracker::GetThreadHandle(CordbProcess * pProcess) -{ - HANDLE hThread = m_hThread; - if (hThread == INVALID_HANDLE_VALUE) - { - // lookup the CordbThread by thread ID, so that we can access the left-side thread handle - CordbThread * pThread = pProcess->TryLookupOrCreateThreadByVolatileOSId(m_dwThreadId); - - IDacDbiInterface* pDAC = pProcess->GetDAC(); - if (pDAC == NULL) - { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - DAC not initialized\n")); - ThrowHR(E_UNEXPECTED); - } - - HANDLE hOutOfProcThread = pDAC->GetThreadHandle(pThread->m_vmThreadToken); - if (hOutOfProcThread == NULL) - { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Failed to get thread handle\n")); - ThrowHR(E_UNEXPECTED); - } - - // Duplicate the thread handle to the current process - BOOL fSuccess = ::DuplicateHandle(pProcess->UnsafeGetProcessHandle(), hOutOfProcThread, ::GetCurrentProcess(), &hThread, 0, FALSE, DUPLICATE_SAME_ACCESS); - if (!fSuccess) - { - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Unexpected result from DuplicateHandle\n")); - ThrowHR(HRESULT_FROM_GetLastError()); - } - - m_hThread = hThread; - } - - return hThread; -} - void UnmanagedThreadTracker::Suspend() { _ASSERTE(m_hThread != INVALID_HANDLE_VALUE); @@ -354,20 +318,19 @@ void UnmanagedThreadTracker::Resume() void UnmanagedThreadTracker::Close() { - if (m_hThread != INVALID_HANDLE_VALUE) - { - for (DWORD i = 0; i < m_dwSuspendCount; i++) - { - ::ResumeThread(m_hThread); - } - } + HANDLE hThread = m_hThread; + DWORD dwSuspendCount = m_dwSuspendCount; + m_hThread = INVALID_HANDLE_VALUE; m_dwSuspendCount = 0; - if (m_hThread == INVALID_HANDLE_VALUE) + if (hThread == INVALID_HANDLE_VALUE) { return; } - CloseHandle(m_hThread); - m_hThread = INVALID_HANDLE_VALUE; + for (DWORD i = 0; i < dwSuspendCount; i++) + { + ::ResumeThread(hThread); + } + ::CloseHandle(hThread); } #endif // OUT_OF_PROCESS_SETTHREADCONTEXT @@ -1060,6 +1023,10 @@ CordbProcess::CordbProcess(ULONG64 clrInstanceId, m_fAssertOnTargetInconsistency(false), m_runtimeOffsetsInitialized(false), m_writableMetadataUpdateMode(LegacyCompatPolicy) +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + , + m_dwOutOfProcessStepping(0) +#endif { _ASSERTE((m_id == 0) == (pShim == NULL)); @@ -1435,6 +1402,7 @@ void CordbProcess::Neuter() cur->second.Close(); } m_unmanagedThreadHashTable.clear(); + m_dwOutOfProcessStepping = 0; #endif NeuterChildren(); @@ -11265,7 +11233,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) ThrowHR(CORDBG_E_BAD_THREAD_STATE); } - HANDLE hThread = curThread->second.GetThreadHandle(this); + HANDLE hThread = curThread->second.GetThreadHandle(); if (hThread == INVALID_HANDLE_VALUE) { LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Thread handle not found\n")); @@ -11302,9 +11270,9 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) if (contextSize == 0 || contextSize > sizeof(CONTEXT) + 25000) { - _ASSERTE(!"RS HandleSetThreadContextNeeded Corrupted message received"); + _ASSERTE(!"Corrupted HandleSetThreadContextNeeded message received"); - LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Corrupted message received\n")); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Corrupted HandleSetThreadContextNeeded message received\n")); ThrowHR(E_UNEXPECTED); } @@ -11420,6 +11388,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) curThread->second.SetPatchSkipAddress(patchSkipAddr); // suspend all other threads + m_dwOutOfProcessStepping++; CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) { @@ -11690,10 +11659,10 @@ HRESULT CordbProcess::Filter( HandleSetThreadContextNeeded(dwThreadId); *pContinueStatus = DBG_CONTINUE; } - else if (dwFirstChance && pRecord->ExceptionCode == STATUS_SINGLE_STEP) + else if (dwFirstChance && pRecord->ExceptionCode == STATUS_SINGLE_STEP && m_dwOutOfProcessStepping > 0) { CUnmanagedThreadHashTableImpl::iterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); - + _ASSERTE(curThread != m_unmanagedThreadHashTable.end()); if (curThread != m_unmanagedThreadHashTable.end() && curThread->second.GetThreadId() == dwThreadId && curThread->second.IsInPlaceStepping()) @@ -11704,6 +11673,8 @@ HRESULT CordbProcess::Filter( curThread->second.ClearPatchSkipAddress(); + m_dwOutOfProcessStepping--; + // resume all other threads CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) @@ -15232,15 +15203,9 @@ HRESULT CordbProcess::IsReadyForDetach() } #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + if (m_dwOutOfProcessStepping > 0) { - CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); - for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) - { - if (cur->second.IsInPlaceStepping() || cur->second.IsSuspended()) - { - return CORDBG_E_DETACH_FAILED_OUTSTANDING_STEPPERS; - } - } + return CORDBG_E_DETACH_FAILED_OUTSTANDING_STEPPERS; } #endif @@ -15569,17 +15534,26 @@ void CordbProcess::HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent switch (pEvent->dwDebugEventCode) { case CREATE_PROCESS_DEBUG_EVENT: + if (GetProcessId(pEvent->u.CreateProcessInfo.hProcess) == GetProcessId(UnsafeGetProcessHandle())) { HANDLE hThread = INVALID_HANDLE_VALUE; - ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateProcessInfo.hThread, GetCurrentProcess(), &hThread, THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_SUSPEND_RESUME, false, 0); + ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateProcessInfo.hThread, ::GetCurrentProcess(), &hThread, THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_SUSPEND_RESUME, false, 0); m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); } break; case CREATE_THREAD_DEBUG_EVENT: { HANDLE hThread = INVALID_HANDLE_VALUE; - ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateThread.hThread, GetCurrentProcess(), &hThread, THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_SUSPEND_RESUME, false, 0); - m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); + ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateThread.hThread, ::GetCurrentProcess(), &hThread, THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_SUSPEND_RESUME, false, 0); + std::pair newItem = m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); + _ASSERTE(newItem.second); + if (newItem.second && m_dwOutOfProcessStepping > 0) // if the insert was successful and we have out-of-process stepping enabled + { + for (DWORD i = 0; i < m_dwOutOfProcessStepping; i++) + { + newItem.first->second.Suspend(); + } + } } break; case EXIT_THREAD_DEBUG_EVENT: diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index d25a79f6cc9089..ddb5fdae050de4 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -2938,13 +2938,11 @@ class UnmanagedThreadTracker public: UnmanagedThreadTracker(DWORD wThreadId, HANDLE hThread) : m_dwThreadId(wThreadId), m_hThread(hThread) {} DWORD GetThreadId() { return m_dwThreadId; } - HANDLE GetThreadHandle(CordbProcess * pProcess); + HANDLE GetThreadHandle() { return m_hThread; } bool IsInPlaceStepping() { return m_pPatchSkipAddress != NULL; } void SetPatchSkipAddress(CORDB_ADDRESS_TYPE *pPatchSkipAddress) { m_pPatchSkipAddress = pPatchSkipAddress; } CORDB_ADDRESS_TYPE *GetPatchSkipAddress() { return m_pPatchSkipAddress; } void ClearPatchSkipAddress() { m_pPatchSkipAddress = NULL; } - bool IsSuspended() { return m_dwSuspendCount > 0; } - DWORD SuspendCount() { return m_dwSuspendCount; } void Suspend(); void Resume(); void Close(); @@ -4160,7 +4158,7 @@ class CordbProcess : #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT CUnmanagedThreadHashTableImpl m_unmanagedThreadHashTable; - + DWORD m_dwOutOfProcessStepping; public: void HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent); #endif // OUT_OF_PROCESS_SETTHREADCONTEXT From 141eb8cec2f9e685196636dc5c202e326dc4ca02 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Mon, 7 Oct 2024 22:52:05 -0400 Subject: [PATCH 17/26] Cleanup and correctly pass bool from LS to RS --- src/coreclr/debug/di/process.cpp | 47 +++++++++++++++++++++-------- src/coreclr/debug/di/rspriv.h | 1 + src/coreclr/debug/ee/controller.cpp | 4 +-- src/coreclr/debug/ee/controller.h | 6 ++-- src/coreclr/debug/ee/debugger.cpp | 18 ++++++----- 5 files changed, 50 insertions(+), 26 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 3d229f4b99d6f6..6ae14ea25f5ae5 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -1047,6 +1047,11 @@ CordbProcess::CordbProcess(ULONG64 clrInstanceId, // This is cleared in code:CordbProcess::Neuter m_pProcess.Assign(this); +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + m_unmanagedThreadHashTable.reserve(512); // reserve 512 buckets for threads + m_unmanagedThreadHashTable.max_load_factor(0.75f); // keep the load factor below 0.75 +#endif + #ifdef _DEBUG // On Debug builds, we'll ASSERT by default whenever the target appears to be corrupt or // otherwise inconsistent (both in DAC and DBI). But we also need the ability to @@ -1396,8 +1401,8 @@ void CordbProcess::Neuter() RSLockHolder lockHolder(GetProcessLock()); #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); - for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) + CUnmanagedThreadHashTableIterator end = m_unmanagedThreadHashTable.end(); + for (CUnmanagedThreadHashTableIterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) { cur->second.Close(); } @@ -11225,7 +11230,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // Windows causes the process to have higher privileges. // We are now caching the thread handle in the unmanaged thread hash table when the thread is created. - CUnmanagedThreadHashTableImpl::iterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); + CUnmanagedThreadHashTableIterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); if (curThread == m_unmanagedThreadHashTable.end() || curThread->second.GetThreadId() != dwThreadId) { @@ -11265,7 +11270,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) TADDR lsContextAddr = (TADDR)context.Rcx; DWORD contextSize = (DWORD)context.Rdx; - bool fIsInPlaceSingleStep = (bool)context.R8; + bool fIsInPlaceSingleStep = (bool)(context.R8&0x1); PRD_TYPE opcode = (PRD_TYPE)context.R9; if (contextSize == 0 || contextSize > sizeof(CONTEXT) + 25000) @@ -11377,10 +11382,13 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) ThrowHR(HRESULT_FROM_WIN32(lastError)); } + if (fIsInPlaceSingleStep) { CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)pFrameContext->Rip; + printf("RS HandleSetThreadContextNeeded - fIsInPlaceSingleStep=%d opcode=%x address=%p\n", fIsInPlaceSingleStep, (DWORD)opcode, (void*)patchSkipAddr); + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - address=0x%p opcode=0x%x\n", patchSkipAddr, opcode)); HRESULT hr = RemoveRemotePatch(this, (void*)patchSkipAddr, opcode); IfFailThrow(hr); @@ -11389,8 +11397,8 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // suspend all other threads m_dwOutOfProcessStepping++; - CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); - for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) + CUnmanagedThreadHashTableIterator end = m_unmanagedThreadHashTable.end(); + for (CUnmanagedThreadHashTableIterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) { if (cur->second.GetThreadId() == dwThreadId) { @@ -11661,7 +11669,8 @@ HRESULT CordbProcess::Filter( } else if (dwFirstChance && pRecord->ExceptionCode == STATUS_SINGLE_STEP && m_dwOutOfProcessStepping > 0) { - CUnmanagedThreadHashTableImpl::iterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); + printf("RS Filter - Single Step at 0x%p\n", (void*)pRecord->ExceptionAddress); + CUnmanagedThreadHashTableIterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); _ASSERTE(curThread != m_unmanagedThreadHashTable.end()); if (curThread != m_unmanagedThreadHashTable.end() && curThread->second.GetThreadId() == dwThreadId && @@ -11671,13 +11680,15 @@ HRESULT CordbProcess::Filter( HRESULT hr = ApplyRemotePatch(this, curThread->second.GetPatchSkipAddress()); IfFailThrow(hr); + printf("RS Filter - Single Step at 0x%p - clear patch skip at 0x%p\n", (void*)pRecord->ExceptionAddress, curThread->second.GetPatchSkipAddress()); + curThread->second.ClearPatchSkipAddress(); m_dwOutOfProcessStepping--; // resume all other threads - CUnmanagedThreadHashTableImpl::iterator end = m_unmanagedThreadHashTable.end(); - for (CUnmanagedThreadHashTableImpl::iterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) + CUnmanagedThreadHashTableIterator end = m_unmanagedThreadHashTable.end(); + for (CUnmanagedThreadHashTableIterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) { if (cur->second.GetThreadId() == dwThreadId) { @@ -15531,21 +15542,31 @@ void CordbProcess::HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent { //PUBLIC_API_ENTRY_FOR_SHIM(this); + const DWORD dwDesiredAccess = THREAD_GET_CONTEXT | THREAD_QUERY_INFORMATION | THREAD_SET_CONTEXT | THREAD_SET_INFORMATION | THREAD_SUSPEND_RESUME | THREAD_TERMINATE; + switch (pEvent->dwDebugEventCode) { case CREATE_PROCESS_DEBUG_EVENT: if (GetProcessId(pEvent->u.CreateProcessInfo.hProcess) == GetProcessId(UnsafeGetProcessHandle())) { HANDLE hThread = INVALID_HANDLE_VALUE; - ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateProcessInfo.hThread, ::GetCurrentProcess(), &hThread, THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_SUSPEND_RESUME, false, 0); + ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateProcessInfo.hThread, ::GetCurrentProcess(), &hThread, dwDesiredAccess, false, 0); m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); } break; case CREATE_THREAD_DEBUG_EVENT: { HANDLE hThread = INVALID_HANDLE_VALUE; - ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateThread.hThread, ::GetCurrentProcess(), &hThread, THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_SUSPEND_RESUME, false, 0); - std::pair newItem = m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); + ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateThread.hThread, ::GetCurrentProcess(), &hThread, dwDesiredAccess, false, 0); + std::pair newItem = m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); + _ASSERTE(newItem.second); + if (!newItem.second) + { + // If the insert failed, we need to close the stagnant item and replace it with the new one + newItem.first->second.Close(); + m_unmanagedThreadHashTable.erase(newItem.first); + newItem = m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); + } _ASSERTE(newItem.second); if (newItem.second && m_dwOutOfProcessStepping > 0) // if the insert was successful and we have out-of-process stepping enabled { @@ -15558,7 +15579,7 @@ void CordbProcess::HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent break; case EXIT_THREAD_DEBUG_EVENT: { - CUnmanagedThreadHashTableImpl::iterator it = m_unmanagedThreadHashTable.find(pEvent->dwThreadId); + CUnmanagedThreadHashTableIterator it = m_unmanagedThreadHashTable.find(pEvent->dwThreadId); if (it != m_unmanagedThreadHashTable.end()) { it->second.Close(); diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index ddb5fdae050de4..a1d058321b0102 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -2949,6 +2949,7 @@ class UnmanagedThreadTracker }; typedef std::unordered_map CUnmanagedThreadHashTableImpl; +typedef CUnmanagedThreadHashTableImpl::iterator CUnmanagedThreadHashTableIterator; // iterator for the hash table #endif // OUT_OF_PROCESS_SETTHREADCONTEXT class CordbProcess : diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 329976050e3d32..0d4b8381d06528 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -4470,12 +4470,12 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, NativeWalker::DecodeInstructionForPatchSkip(patchBypassRX, &(m_instrAttrib)); -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT if (g_pDebugInterface->IsOutOfProcessSetContextEnabled() && m_instrAttrib.m_fIsCall) { m_fInPlaceSS = true; } -#endif // !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) +#endif #if defined(TARGET_AMD64) diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 301b87205afc95..fe4949b86b8c50 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -1517,9 +1517,9 @@ class DebuggerPatchSkip : public DebuggerController CORDB_ADDRESS_TYPE *m_address; int m_iOrigDisp; // the original displacement of a relative call or jump InstructionAttribute m_instrAttrib; // info about the instruction being skipped over -#if !defined(FEATURE_EMULATE_SINGLESTEP) && defined(OUT_OF_PROCESS_SETTHREADCONTEXT) +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT bool m_fInPlaceSS; // is this an in-place single-step instruction? -#endif +#endif // OUT_OF_PROCESS_SETTHREADCONTEXT #ifndef FEATURE_EMULATE_SINGLESTEP // this is shared among all the skippers and the controller. see the comments // right before the definition of SharedPatchBypassBuffer for lifetime info. @@ -1537,7 +1537,7 @@ class DebuggerPatchSkip : public DebuggerController { // only in-place single steps over call intructions are supported at this time #ifndef FEATURE_EMULATE_SINGLESTEP - return m_instrAttrib.m_fIsCall && m_fInPlaceSS; + return m_instrAttrib.m_fIsCall && m_fInPlaceSS; #else #error single step emulation not supported with out of process set thread context #endif diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 889a01b8426260..013a1618218b27 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -16749,17 +16749,19 @@ void Debugger::SendSetThreadContextNeeded(CONTEXT *context, DebuggerSteppingInfo // adjust context size if the context pointer is not aligned with the buffer we allocated contextSize -= (DWORD)((BYTE*)pContext-(BYTE*)pBuffer); + bool fIsInPlaceSingleStep = false; + PRD_TYPE opcode = CORDbg_BREAK_INSTRUCTION; + if (pDebuggerSteppingInfo) + { + fIsInPlaceSingleStep = pDebuggerSteppingInfo->IsInPlaceSingleStep(); + opcode = pDebuggerSteppingInfo->GetOpcode(); + } + // send the context to the right side - LOG((LF_CORDB, LL_INFO10000, "D::SSTCN ContextFlags=0x%X contextSize=%d..\n", contextFlags, contextSize)); + LOG((LF_CORDB, LL_INFO10000, "D::SSTCN ContextFlags=0x%X contextSize=%d fIsInPlaceSingleStep=%d opcode=%x\n", contextFlags, contextSize, fIsInPlaceSingleStep, opcode)); EX_TRY { - bool fIsInPlaceSingleStep = false; - PRD_TYPE opcode = CORDbg_BREAK_INSTRUCTION; - if (pDebuggerSteppingInfo) - { - fIsInPlaceSingleStep = pDebuggerSteppingInfo->IsInPlaceSingleStep(); - opcode = pDebuggerSteppingInfo->GetOpcode(); - } + //printf("D::SSTCN fIsInPlaceSingleStep=%d opcode=%x address=%p\n", fIsInPlaceSingleStep, (DWORD)opcode, (void*)pContext->Rip); SetThreadContextNeededFlare((TADDR)pContext, contextSize, fIsInPlaceSingleStep, From dc0ae8c2e44ad5a330851ca07c97c75e576a31ab Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Mon, 7 Oct 2024 22:52:36 -0400 Subject: [PATCH 18/26] Workaround crash caused by SSP restore on fake context --- src/coreclr/vm/threadsuspend.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 1506516f12447e..dd9b7c2e818e76 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1000,9 +1000,9 @@ BOOL Thread::ReadyForAsyncException() } else { - CONTEXT ctx; - SetIP(&ctx, 0); - SetSP(&ctx, 0); + CONTEXT ctx = { 0 }; + // SetIP(&ctx, 0); + // SetSP(&ctx, 0); FillRegDisplay(&rd, &ctx); } } From 983616f2a24a63bea81cc755db6309b40f9d808b Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Tue, 8 Oct 2024 00:22:20 -0400 Subject: [PATCH 19/26] Change page protections when clearing or writing breakpoints --- src/coreclr/debug/di/process.cpp | 128 ++++++++++++++++++++++++------- src/coreclr/debug/di/rspriv.h | 1 + 2 files changed, 101 insertions(+), 28 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 6ae14ea25f5ae5..0f4b14cbd39f46 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11382,17 +11382,47 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) ThrowHR(HRESULT_FROM_WIN32(lastError)); } - if (fIsInPlaceSingleStep) { CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)pFrameContext->Rip; printf("RS HandleSetThreadContextNeeded - fIsInPlaceSingleStep=%d opcode=%x address=%p\n", fIsInPlaceSingleStep, (DWORD)opcode, (void*)patchSkipAddr); + + HANDLE hProcess = UnsafeGetProcessHandle(); + LPVOID baseAddress = (LPVOID)(patchSkipAddr); + DWORD oldProt; + + if (!VirtualProtectEx(hProcess, + baseAddress, + CORDbg_BREAK_INSTRUCTION_SIZE, + PAGE_EXECUTE_READWRITE, &oldProt)) + { + // we may be seeing unwriteable directly mapped executable memory. + // let's try copy-on-write instead, + if (!VirtualProtectEx(hProcess, + baseAddress, + CORDbg_BREAK_INSTRUCTION_SIZE, + PAGE_EXECUTE_WRITECOPY, &oldProt)) + { + _ASSERTE(!"VirtualProtect of code page failed"); + ThrowHR(HRESULT_FROM_GetLastError()); + } + } + LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - address=0x%p opcode=0x%x\n", patchSkipAddr, opcode)); HRESULT hr = RemoveRemotePatch(this, (void*)patchSkipAddr, opcode); IfFailThrow(hr); + if (!VirtualProtectEx(hProcess, + baseAddress, + CORDbg_BREAK_INSTRUCTION_SIZE, + oldProt, &oldProt)) + { + _ASSERTE(!"VirtualProtect of code page failed"); + ThrowHR(HRESULT_FROM_GetLastError()); + } + curThread->second.SetPatchSkipAddress(patchSkipAddr); // suspend all other threads @@ -11411,6 +11441,72 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) #error Platform not supported #endif } + +bool CordbProcess::HandleInPlaceSingleStep(DWORD dwThreadId, PVOID pExceptionAddress) +{ + printf("RS Filter - Single Step at 0x%p\n", pExceptionAddress); + CUnmanagedThreadHashTableIterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); + _ASSERTE(curThread != m_unmanagedThreadHashTable.end()); + if (curThread != m_unmanagedThreadHashTable.end() && + curThread->second.GetThreadId() == dwThreadId && + curThread->second.IsInPlaceStepping()) + { + // this is an in-place step, so place the breakpoint instruction back to the patch location + printf("RS Filter - Single Step at 0x%p - clear patch skip at 0x%p\n", pExceptionAddress, curThread->second.GetPatchSkipAddress()); + + HANDLE hProcess = UnsafeGetProcessHandle(); + LPVOID baseAddress = (LPVOID)( curThread->second.GetPatchSkipAddress()); + DWORD oldProt; + + if (!VirtualProtectEx(hProcess, + baseAddress, + CORDbg_BREAK_INSTRUCTION_SIZE, + PAGE_EXECUTE_READWRITE, &oldProt)) + { + // we may be seeing unwriteable directly mapped executable memory. + // let's try copy-on-write instead, + if (!VirtualProtectEx(hProcess, + baseAddress, + CORDbg_BREAK_INSTRUCTION_SIZE, + PAGE_EXECUTE_WRITECOPY, &oldProt)) + { + _ASSERTE(!"VirtualProtect of code page failed"); + ThrowHR(HRESULT_FROM_GetLastError()); + } + } + + HRESULT hr = ApplyRemotePatch(this, curThread->second.GetPatchSkipAddress()); + IfFailThrow(hr); + + if (!VirtualProtectEx(hProcess, + baseAddress, + CORDbg_BREAK_INSTRUCTION_SIZE, + oldProt, &oldProt)) + { + _ASSERTE(!"VirtualProtect of code page failed"); + ThrowHR(HRESULT_FROM_GetLastError()); + } + + curThread->second.ClearPatchSkipAddress(); + + m_dwOutOfProcessStepping--; + + // resume all other threads + CUnmanagedThreadHashTableIterator end = m_unmanagedThreadHashTable.end(); + for (CUnmanagedThreadHashTableIterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) + { + if (cur->second.GetThreadId() == dwThreadId) + { + continue; + } + cur->second.Resume(); + } + + return true; + } + + return false; +} #endif // OUT_OF_PROCESS_SETTHREADCONTEXT // @@ -11669,34 +11765,10 @@ HRESULT CordbProcess::Filter( } else if (dwFirstChance && pRecord->ExceptionCode == STATUS_SINGLE_STEP && m_dwOutOfProcessStepping > 0) { - printf("RS Filter - Single Step at 0x%p\n", (void*)pRecord->ExceptionAddress); - CUnmanagedThreadHashTableIterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); - _ASSERTE(curThread != m_unmanagedThreadHashTable.end()); - if (curThread != m_unmanagedThreadHashTable.end() && - curThread->second.GetThreadId() == dwThreadId && - curThread->second.IsInPlaceStepping()) - { - // we are expecting a single step exception - HRESULT hr = ApplyRemotePatch(this, curThread->second.GetPatchSkipAddress()); - IfFailThrow(hr); - - printf("RS Filter - Single Step at 0x%p - clear patch skip at 0x%p\n", (void*)pRecord->ExceptionAddress, curThread->second.GetPatchSkipAddress()); - - curThread->second.ClearPatchSkipAddress(); - - m_dwOutOfProcessStepping--; - - // resume all other threads - CUnmanagedThreadHashTableIterator end = m_unmanagedThreadHashTable.end(); - for (CUnmanagedThreadHashTableIterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) - { - if (cur->second.GetThreadId() == dwThreadId) - { - continue; - } - cur->second.Resume(); - } + // this may be an in-place step, and if so place the breakpoint instruction back to the patch location and resume the threads + if (HandleInPlaceSingleStep(dwThreadId, pRecord->ExceptionAddress)) + { // let the normal left side debugger stepper logic execute for this single step *pContinueStatus = DBG_EXCEPTION_NOT_HANDLED; } diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index a1d058321b0102..93ab7c1868fddb 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -3317,6 +3317,7 @@ class CordbProcess : #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT void HandleSetThreadContextNeeded(DWORD dwThreadId); + bool HandleInPlaceSingleStep(DWORD dwThreadId, PVOID pExceptionAddress); #endif // From 79a1b716793fd685fdf57e6f60542fe3162b83de Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Tue, 8 Oct 2024 14:25:20 -0400 Subject: [PATCH 20/26] Remove debug output and perform some cleanup --- src/coreclr/debug/di/process.cpp | 7 +------ src/coreclr/debug/ee/debugger.cpp | 15 +++------------ 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 0f4b14cbd39f46..901b1e3864b6a0 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -11386,9 +11386,6 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) { CORDB_ADDRESS_TYPE *patchSkipAddr = (CORDB_ADDRESS_TYPE*)pFrameContext->Rip; - printf("RS HandleSetThreadContextNeeded - fIsInPlaceSingleStep=%d opcode=%x address=%p\n", fIsInPlaceSingleStep, (DWORD)opcode, (void*)patchSkipAddr); - - HANDLE hProcess = UnsafeGetProcessHandle(); LPVOID baseAddress = (LPVOID)(patchSkipAddr); DWORD oldProt; @@ -11444,7 +11441,6 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) bool CordbProcess::HandleInPlaceSingleStep(DWORD dwThreadId, PVOID pExceptionAddress) { - printf("RS Filter - Single Step at 0x%p\n", pExceptionAddress); CUnmanagedThreadHashTableIterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); _ASSERTE(curThread != m_unmanagedThreadHashTable.end()); if (curThread != m_unmanagedThreadHashTable.end() && @@ -11452,7 +11448,6 @@ bool CordbProcess::HandleInPlaceSingleStep(DWORD dwThreadId, PVOID pExceptionAdd curThread->second.IsInPlaceStepping()) { // this is an in-place step, so place the breakpoint instruction back to the patch location - printf("RS Filter - Single Step at 0x%p - clear patch skip at 0x%p\n", pExceptionAddress, curThread->second.GetPatchSkipAddress()); HANDLE hProcess = UnsafeGetProcessHandle(); LPVOID baseAddress = (LPVOID)( curThread->second.GetPatchSkipAddress()); @@ -15612,7 +15607,7 @@ void CordbProcess::HandleControlCTrapResult(HRESULT result) #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT void CordbProcess::HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent) { - //PUBLIC_API_ENTRY_FOR_SHIM(this); + PUBLIC_API_ENTRY_FOR_SHIM(this); const DWORD dwDesiredAccess = THREAD_GET_CONTEXT | THREAD_QUERY_INFORMATION | THREAD_SET_CONTEXT | THREAD_SET_INFORMATION | THREAD_SUSPEND_RESUME | THREAD_TERMINATE; diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 013a1618218b27..856960a78e623d 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -16749,23 +16749,14 @@ void Debugger::SendSetThreadContextNeeded(CONTEXT *context, DebuggerSteppingInfo // adjust context size if the context pointer is not aligned with the buffer we allocated contextSize -= (DWORD)((BYTE*)pContext-(BYTE*)pBuffer); - bool fIsInPlaceSingleStep = false; - PRD_TYPE opcode = CORDbg_BREAK_INSTRUCTION; - if (pDebuggerSteppingInfo) - { - fIsInPlaceSingleStep = pDebuggerSteppingInfo->IsInPlaceSingleStep(); - opcode = pDebuggerSteppingInfo->GetOpcode(); - } + bool fIsInPlaceSingleStep = pDebuggerSteppingInfo != NULL && pDebuggerSteppingInfo->IsInPlaceSingleStep(); + PRD_TYPE opcode = pDebuggerSteppingInfo != NULL ? pDebuggerSteppingInfo->GetOpcode() : CORDbg_BREAK_INSTRUCTION; // send the context to the right side LOG((LF_CORDB, LL_INFO10000, "D::SSTCN ContextFlags=0x%X contextSize=%d fIsInPlaceSingleStep=%d opcode=%x\n", contextFlags, contextSize, fIsInPlaceSingleStep, opcode)); EX_TRY { - //printf("D::SSTCN fIsInPlaceSingleStep=%d opcode=%x address=%p\n", fIsInPlaceSingleStep, (DWORD)opcode, (void*)pContext->Rip); - SetThreadContextNeededFlare((TADDR)pContext, - contextSize, - fIsInPlaceSingleStep, - opcode); + SetThreadContextNeededFlare((TADDR)pContext, contextSize, fIsInPlaceSingleStep, opcode); } EX_CATCH { From 21693336699e800a64422be48dfa992b781a6dd6 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Tue, 8 Oct 2024 23:48:47 -0400 Subject: [PATCH 21/26] Track unmanaged threads with SHash instead of unordered_map --- src/coreclr/debug/di/process.cpp | 95 ++++++++++++++++---------------- src/coreclr/debug/di/rspriv.h | 27 ++++++--- 2 files changed, 67 insertions(+), 55 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 901b1e3864b6a0..7bc244dc0f049f 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -1047,11 +1047,6 @@ CordbProcess::CordbProcess(ULONG64 clrInstanceId, // This is cleared in code:CordbProcess::Neuter m_pProcess.Assign(this); -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - m_unmanagedThreadHashTable.reserve(512); // reserve 512 buckets for threads - m_unmanagedThreadHashTable.max_load_factor(0.75f); // keep the load factor below 0.75 -#endif - #ifdef _DEBUG // On Debug builds, we'll ASSERT by default whenever the target appears to be corrupt or // otherwise inconsistent (both in DAC and DBI). But we also need the ability to @@ -1401,12 +1396,19 @@ void CordbProcess::Neuter() RSLockHolder lockHolder(GetProcessLock()); #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - CUnmanagedThreadHashTableIterator end = m_unmanagedThreadHashTable.end(); - for (CUnmanagedThreadHashTableIterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) + CUnmanagedThreadHashTableIterator beginIter = m_unmanagedThreadHashTable.End(); + CUnmanagedThreadHashTableIterator endIter = m_unmanagedThreadHashTable.End(); + for (CUnmanagedThreadHashTableIterator it = beginIter; it != endIter; ++it) { - cur->second.Close(); + UnmanagedThreadTracker * pUnmanagedThread = *it; + _ASSERTE(pUnmanagedThread != NULL); + if (pUnmanagedThread != NULL) + { + pUnmanagedThread->Close(); + delete pUnmanagedThread; + } } - m_unmanagedThreadHashTable.clear(); + m_unmanagedThreadHashTable.RemoveAll(); m_dwOutOfProcessStepping = 0; #endif @@ -11230,15 +11232,15 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // Windows causes the process to have higher privileges. // We are now caching the thread handle in the unmanaged thread hash table when the thread is created. - CUnmanagedThreadHashTableIterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); + UnmanagedThreadTracker * curThread = m_unmanagedThreadHashTable.Lookup(dwThreadId); - if (curThread == m_unmanagedThreadHashTable.end() || curThread->second.GetThreadId() != dwThreadId) + if (curThread == NULL || curThread->GetThreadId() != dwThreadId) { LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Thread not found\n")); ThrowHR(CORDBG_E_BAD_THREAD_STATE); } - HANDLE hThread = curThread->second.GetThreadHandle(); + HANDLE hThread = curThread->GetThreadHandle(); if (hThread == INVALID_HANDLE_VALUE) { LOG((LF_CORDB, LL_INFO10000, "RS HandleSetThreadContextNeeded - Thread handle not found\n")); @@ -11420,18 +11422,21 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) ThrowHR(HRESULT_FROM_GetLastError()); } - curThread->second.SetPatchSkipAddress(patchSkipAddr); + curThread->SetPatchSkipAddress(patchSkipAddr); // suspend all other threads m_dwOutOfProcessStepping++; - CUnmanagedThreadHashTableIterator end = m_unmanagedThreadHashTable.end(); - for (CUnmanagedThreadHashTableIterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) + CUnmanagedThreadHashTableIterator beginIter = m_unmanagedThreadHashTable.End(); + CUnmanagedThreadHashTableIterator endIter = m_unmanagedThreadHashTable.End(); + for (CUnmanagedThreadHashTableIterator curIter = beginIter; curIter != endIter; ++curIter) { - if (cur->second.GetThreadId() == dwThreadId) + UnmanagedThreadTracker * pUnmanagedThread = *curIter; + _ASSERTE(pUnmanagedThread != NULL); + if (pUnmanagedThread == NULL || pUnmanagedThread->GetThreadId() == dwThreadId) { continue; } - cur->second.Suspend(); + pUnmanagedThread->Suspend(); } } #else @@ -11441,16 +11446,16 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) bool CordbProcess::HandleInPlaceSingleStep(DWORD dwThreadId, PVOID pExceptionAddress) { - CUnmanagedThreadHashTableIterator curThread = m_unmanagedThreadHashTable.find(dwThreadId); - _ASSERTE(curThread != m_unmanagedThreadHashTable.end()); - if (curThread != m_unmanagedThreadHashTable.end() && - curThread->second.GetThreadId() == dwThreadId && - curThread->second.IsInPlaceStepping()) + UnmanagedThreadTracker * curThread = m_unmanagedThreadHashTable.Lookup(dwThreadId); + _ASSERTE(curThread != NULL); + if (curThread != NULL && + curThread->GetThreadId() == dwThreadId && + curThread->IsInPlaceStepping()) { // this is an in-place step, so place the breakpoint instruction back to the patch location HANDLE hProcess = UnsafeGetProcessHandle(); - LPVOID baseAddress = (LPVOID)( curThread->second.GetPatchSkipAddress()); + LPVOID baseAddress = (LPVOID)(curThread->GetPatchSkipAddress()); DWORD oldProt; if (!VirtualProtectEx(hProcess, @@ -11470,7 +11475,7 @@ bool CordbProcess::HandleInPlaceSingleStep(DWORD dwThreadId, PVOID pExceptionAdd } } - HRESULT hr = ApplyRemotePatch(this, curThread->second.GetPatchSkipAddress()); + HRESULT hr = ApplyRemotePatch(this, curThread->GetPatchSkipAddress()); IfFailThrow(hr); if (!VirtualProtectEx(hProcess, @@ -11482,19 +11487,22 @@ bool CordbProcess::HandleInPlaceSingleStep(DWORD dwThreadId, PVOID pExceptionAdd ThrowHR(HRESULT_FROM_GetLastError()); } - curThread->second.ClearPatchSkipAddress(); + curThread->ClearPatchSkipAddress(); m_dwOutOfProcessStepping--; // resume all other threads - CUnmanagedThreadHashTableIterator end = m_unmanagedThreadHashTable.end(); - for (CUnmanagedThreadHashTableIterator cur = m_unmanagedThreadHashTable.begin(); cur != end; ++cur ) + CUnmanagedThreadHashTableIterator beginIter = m_unmanagedThreadHashTable.End(); + CUnmanagedThreadHashTableIterator endIter = m_unmanagedThreadHashTable.End(); + for (CUnmanagedThreadHashTableIterator curIter = beginIter; curIter != endIter; ++curIter) { - if (cur->second.GetThreadId() == dwThreadId) + UnmanagedThreadTracker * pUnmanagedThread = *curIter; + _ASSERTE(pUnmanagedThread != NULL); + if (pUnmanagedThread == NULL || pUnmanagedThread->GetThreadId() == dwThreadId) { continue; } - cur->second.Resume(); + pUnmanagedThread->Resume(); } return true; @@ -15609,7 +15617,7 @@ void CordbProcess::HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent { PUBLIC_API_ENTRY_FOR_SHIM(this); - const DWORD dwDesiredAccess = THREAD_GET_CONTEXT | THREAD_QUERY_INFORMATION | THREAD_SET_CONTEXT | THREAD_SET_INFORMATION | THREAD_SUSPEND_RESUME | THREAD_TERMINATE; + const DWORD dwDesiredAccess = THREAD_GET_CONTEXT | THREAD_SET_CONTEXT | THREAD_SUSPEND_RESUME; switch (pEvent->dwDebugEventCode) { @@ -15618,39 +15626,32 @@ void CordbProcess::HandleDebugEventForInPlaceStepping(const DEBUG_EVENT * pEvent { HANDLE hThread = INVALID_HANDLE_VALUE; ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateProcessInfo.hThread, ::GetCurrentProcess(), &hThread, dwDesiredAccess, false, 0); - m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); + m_unmanagedThreadHashTable.AddNoThrow(new UnmanagedThreadTracker(pEvent->dwThreadId, hThread)); } break; case CREATE_THREAD_DEBUG_EVENT: { HANDLE hThread = INVALID_HANDLE_VALUE; ::DuplicateHandle(::GetCurrentProcess(), pEvent->u.CreateThread.hThread, ::GetCurrentProcess(), &hThread, dwDesiredAccess, false, 0); - std::pair newItem = m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); - _ASSERTE(newItem.second); - if (!newItem.second) - { - // If the insert failed, we need to close the stagnant item and replace it with the new one - newItem.first->second.Close(); - m_unmanagedThreadHashTable.erase(newItem.first); - newItem = m_unmanagedThreadHashTable.insert(std::make_pair(pEvent->dwThreadId, UnmanagedThreadTracker(pEvent->dwThreadId, hThread))); - } - _ASSERTE(newItem.second); - if (newItem.second && m_dwOutOfProcessStepping > 0) // if the insert was successful and we have out-of-process stepping enabled + UnmanagedThreadTracker * newItem = new UnmanagedThreadTracker(pEvent->dwThreadId, hThread); + m_unmanagedThreadHashTable.AddNoThrow(newItem); + if (newItem && m_dwOutOfProcessStepping > 0) // if the insert was successful and we have out-of-process stepping enabled { for (DWORD i = 0; i < m_dwOutOfProcessStepping; i++) { - newItem.first->second.Suspend(); + newItem->Suspend(); } } } break; case EXIT_THREAD_DEBUG_EVENT: { - CUnmanagedThreadHashTableIterator it = m_unmanagedThreadHashTable.find(pEvent->dwThreadId); - if (it != m_unmanagedThreadHashTable.end()) + UnmanagedThreadTracker * pUnmanagedThread = m_unmanagedThreadHashTable.Lookup(pEvent->dwThreadId); + if (pUnmanagedThread != NULL) { - it->second.Close(); - m_unmanagedThreadHashTable.erase(it); + pUnmanagedThread->Close(); + m_unmanagedThreadHashTable.Remove(pEvent->dwThreadId); + delete pUnmanagedThread; } } break; diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index 93ab7c1868fddb..7e2b49b3170107 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -50,7 +50,7 @@ struct MachineInfo; #include "eventchannel.h" -#include +#include #undef ASSERT #define CRASH(x) _ASSERTE(!(x)) @@ -2930,26 +2930,37 @@ struct DbgAssertAppDomainDeletedData #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT class UnmanagedThreadTracker { - DWORD m_dwThreadId = 0; + DWORD m_dwThreadId = (DWORD)-1; HANDLE m_hThread = INVALID_HANDLE_VALUE; CORDB_ADDRESS_TYPE *m_pPatchSkipAddress = NULL; DWORD m_dwSuspendCount = 0; public: UnmanagedThreadTracker(DWORD wThreadId, HANDLE hThread) : m_dwThreadId(wThreadId), m_hThread(hThread) {} - DWORD GetThreadId() { return m_dwThreadId; } - HANDLE GetThreadHandle() { return m_hThread; } - bool IsInPlaceStepping() { return m_pPatchSkipAddress != NULL; } + + DWORD GetThreadId() const { return m_dwThreadId; } + HANDLE GetThreadHandle() const { return m_hThread; } + bool IsInPlaceStepping() const { return m_pPatchSkipAddress != NULL; } void SetPatchSkipAddress(CORDB_ADDRESS_TYPE *pPatchSkipAddress) { m_pPatchSkipAddress = pPatchSkipAddress; } - CORDB_ADDRESS_TYPE *GetPatchSkipAddress() { return m_pPatchSkipAddress; } + CORDB_ADDRESS_TYPE *GetPatchSkipAddress() const { return m_pPatchSkipAddress; } void ClearPatchSkipAddress() { m_pPatchSkipAddress = NULL; } void Suspend(); void Resume(); void Close(); }; -typedef std::unordered_map CUnmanagedThreadHashTableImpl; -typedef CUnmanagedThreadHashTableImpl::iterator CUnmanagedThreadHashTableIterator; // iterator for the hash table +class EMPTY_BASES_DECL CUnmanagedThreadSHashTraits : public DefaultSHashTraits +{ + public: + typedef DWORD key_t; + + static key_t GetKey(const element_t &e) { return e->GetThreadId(); } + static BOOL Equals(const key_t &e, const key_t &f) { return e == f; } + static count_t Hash(const key_t &e) { return (count_t)(e ^ (e >> 16) * 0x45D9F43); } +}; + +typedef SHash CUnmanagedThreadHashTableImpl; +typedef SHash::Iterator CUnmanagedThreadHashTableIterator; #endif // OUT_OF_PROCESS_SETTHREADCONTEXT class CordbProcess : From 190dfab35e02b1274ab3fd9f58a138068d57c29d Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Fri, 11 Oct 2024 12:08:38 -0400 Subject: [PATCH 22/26] Revert "Workaround crash caused by SSP restore on fake context" This reverts commit 874c465495edef0c85f55bdda9a82850c68a9af6. --- src/coreclr/vm/threadsuspend.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index dd9b7c2e818e76..1506516f12447e 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1000,9 +1000,9 @@ BOOL Thread::ReadyForAsyncException() } else { - CONTEXT ctx = { 0 }; - // SetIP(&ctx, 0); - // SetSP(&ctx, 0); + CONTEXT ctx; + SetIP(&ctx, 0); + SetSP(&ctx, 0); FillRegDisplay(&rd, &ctx); } } From 2feac803da1f8179c0711cb7d30000a197ddff90 Mon Sep 17 00:00:00 2001 From: "Tom McDonald (from Dev Box)" Date: Fri, 11 Oct 2024 15:34:48 -0400 Subject: [PATCH 23/26] Fix func-eval abort with minimal code change --- src/coreclr/vm/threadsuspend.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 1506516f12447e..71f59672eba1c7 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1001,6 +1001,7 @@ BOOL Thread::ReadyForAsyncException() else { CONTEXT ctx; + ctx.ContextFlags = CONTEXT_CONTROL; SetIP(&ctx, 0); SetSP(&ctx, 0); FillRegDisplay(&rd, &ctx); From cc1bdc8edfe4b6babba63636a8c411f648fbac54 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 14 Oct 2024 19:47:37 -0400 Subject: [PATCH 24/26] Apply suggestions from noahfalk Co-authored-by: Noah Falk --- src/coreclr/debug/ee/controller.h | 6 +++++- src/coreclr/debug/ee/debugger.cpp | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index fe4949b86b8c50..4e852edfbb854d 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -1537,7 +1537,11 @@ class DebuggerPatchSkip : public DebuggerController { // only in-place single steps over call intructions are supported at this time #ifndef FEATURE_EMULATE_SINGLESTEP - return m_instrAttrib.m_fIsCall && m_fInPlaceSS; +#ifndef FEATURE_EMULATE_SINGLESTEP + // only in-place single steps over call intructions are supported at this time + _ASSERTE(m_instrAttrib.m_fIsCall); + return m_fInPlaceSS; +#else #else #error single step emulation not supported with out of process set thread context #endif diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 856960a78e623d..3c3883cb6590ab 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -5488,6 +5488,8 @@ bool Debugger::FirstChanceNativeException(EXCEPTION_RECORD *exception, #if defined(OUT_OF_PROCESS_SETTHREADCONTEXT) && !defined(DACCESS_COMPILE) if (retVal && fIsVEH) { + // This does not return. Out-of-proc debugger will update the thread context + // within this call. SendSetThreadContextNeeded(context, &debuggerSteppingInfo); } #endif From 42c90ebe1b84880fe417e26bbb629018b757ee79 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 14 Oct 2024 20:35:39 -0400 Subject: [PATCH 25/26] Always define IsInPlaceSingleStep and return false if not implemented --- src/coreclr/debug/ee/controller.cpp | 18 ++---------------- src/coreclr/debug/ee/controller.h | 13 +++++++------ 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 0d4b8381d06528..2d01f1904436e9 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -4482,11 +4482,7 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, // The code below handles RIP-relative addressing on AMD64. The original implementation made the assumption that // we are only using RIP-relative addressing to access read-only data (see VSW 246145 for more information). This // has since been expanded to handle RIP-relative writes as well. - if (m_instrAttrib.m_dwOffsetToDisp != 0 -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - && !IsInPlaceSingleStep() -#endif - ) + if (m_instrAttrib.m_dwOffsetToDisp != 0 && !IsInPlaceSingleStep()) { _ASSERTE(m_instrAttrib.m_cbInstr != 0); @@ -4592,9 +4588,7 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, patchBypassRX = NativeWalker::SetupOrSimulateInstructionForPatchSkip(context, m_pSharedPatchBypassBuffer, (const BYTE *)patch->address, patch->opcode); #endif //TARGET_ARM64 -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT if (!IsInPlaceSingleStep()) -#endif { //set eip to point to buffer... SetIP(context, (PCODE)patchBypassRX); @@ -4822,9 +4816,7 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont { // Fixup return address on stack #if defined(TARGET_X86) || defined(TARGET_AMD64) -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT if (!IsInPlaceSingleStep()) -#endif { // Fixup return address on stack SIZE_T *sp = (SIZE_T *) GetSP(context); @@ -4855,9 +4847,7 @@ TP_RESULT DebuggerPatchSkip::TriggerExceptionHook(Thread *thread, CONTEXT * cont ((size_t)GetIP(context) > (size_t)patchBypass && (size_t)GetIP(context) <= (size_t)(patchBypass + MAX_INSTRUCTION_LENGTH + 1))) { -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT if (!IsInPlaceSingleStep()) -#endif { LOG((LF_CORDB, LL_INFO10000, "Bypass instruction redirected because still in skip area.\n" "\tm_fIsCall = %s, patchBypass = %p, m_address = %p\n", @@ -4930,11 +4920,7 @@ bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip) #if defined(TARGET_AMD64) // Dev11 91932: for RIP-relative writes we need to copy the value that was written in our buffer to the actual address _ASSERTE(m_pSharedPatchBypassBuffer); - if (m_pSharedPatchBypassBuffer->RipTargetFixup -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - && !IsInPlaceSingleStep() -#endif - ) + if (m_pSharedPatchBypassBuffer->RipTargetFixup && !IsInPlaceSingleStep()) { _ASSERTE(m_pSharedPatchBypassBuffer->RipTargetFixupSize); diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 4e852edfbb854d..30521e90a27a2d 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -1532,22 +1532,23 @@ class DebuggerPatchSkip : public DebuggerController BYTE* patchBypass = m_pSharedPatchBypassBuffer->PatchBypass; return (CORDB_ADDRESS_TYPE *)patchBypass; } -#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT + +#endif // !FEATURE_EMULATE_SINGLESTEP + BOOL IsInPlaceSingleStep() { - // only in-place single steps over call intructions are supported at this time -#ifndef FEATURE_EMULATE_SINGLESTEP +#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT #ifndef FEATURE_EMULATE_SINGLESTEP // only in-place single steps over call intructions are supported at this time _ASSERTE(m_instrAttrib.m_fIsCall); return m_fInPlaceSS; #else +#error only non-emulated single-steps with OUT_OF_PROCESS_SETTHREADCONTEXT enabled are supported +#endif #else -#error single step emulation not supported with out of process set thread context + return false; #endif } -#endif -#endif // !FEATURE_EMULATE_SINGLESTEP }; /* ------------------------------------------------------------------------- * From efafaa75b4265595df5d7dd86a54fe8589baad7f Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 14 Oct 2024 20:40:04 -0400 Subject: [PATCH 26/26] CR feedback from mikelle-rogers - correctly set begin iterator for suspending/resuming/closing threads --- src/coreclr/debug/di/process.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 7bc244dc0f049f..f12b6166019a04 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -1396,7 +1396,7 @@ void CordbProcess::Neuter() RSLockHolder lockHolder(GetProcessLock()); #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT - CUnmanagedThreadHashTableIterator beginIter = m_unmanagedThreadHashTable.End(); + CUnmanagedThreadHashTableIterator beginIter = m_unmanagedThreadHashTable.Begin(); CUnmanagedThreadHashTableIterator endIter = m_unmanagedThreadHashTable.End(); for (CUnmanagedThreadHashTableIterator it = beginIter; it != endIter; ++it) { @@ -11426,7 +11426,7 @@ void CordbProcess::HandleSetThreadContextNeeded(DWORD dwThreadId) // suspend all other threads m_dwOutOfProcessStepping++; - CUnmanagedThreadHashTableIterator beginIter = m_unmanagedThreadHashTable.End(); + CUnmanagedThreadHashTableIterator beginIter = m_unmanagedThreadHashTable.Begin(); CUnmanagedThreadHashTableIterator endIter = m_unmanagedThreadHashTable.End(); for (CUnmanagedThreadHashTableIterator curIter = beginIter; curIter != endIter; ++curIter) { @@ -11492,7 +11492,7 @@ bool CordbProcess::HandleInPlaceSingleStep(DWORD dwThreadId, PVOID pExceptionAdd m_dwOutOfProcessStepping--; // resume all other threads - CUnmanagedThreadHashTableIterator beginIter = m_unmanagedThreadHashTable.End(); + CUnmanagedThreadHashTableIterator beginIter = m_unmanagedThreadHashTable.Begin(); CUnmanagedThreadHashTableIterator endIter = m_unmanagedThreadHashTable.End(); for (CUnmanagedThreadHashTableIterator curIter = beginIter; curIter != endIter; ++curIter) {