From cbfafe4c8e8a46fcd70fb194cff0d4d3470dc857 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Thu, 10 Jul 2025 13:48:50 -0400 Subject: [PATCH 1/6] Avoid populating shared patch bypass buffer after initial creation --- src/coreclr/debug/ee/controller.cpp | 68 +++++++++++++++++------------ src/coreclr/debug/ee/controller.h | 12 ++++- 2 files changed, 52 insertions(+), 28 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index a7bed1a1001bfd..5e8c16429f3f5d 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -70,7 +70,7 @@ bool DebuggerControllerPatch::IsSafeForStackTrace() #ifndef FEATURE_EMULATE_SINGLESTEP // returns a pointer to the shared buffer. each call will AddRef() the object // before returning it so callers only need to Release() when they're finished with it. -SharedPatchBypassBuffer* DebuggerControllerPatch::GetOrCreateSharedPatchBypassBuffer() +SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer() { CONTRACTL { @@ -4518,39 +4518,53 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, // Create the shared instruction block. this will also create the shared RIP-relative buffer // - m_pSharedPatchBypassBuffer = patch->GetOrCreateSharedPatchBypassBuffer(); + m_pSharedPatchBypassBuffer = patch->GetSharedPatchBypassBuffer(); + SharedPatchBypassBuffer *pSharedPatchBypassBufferRW = NULL; + BYTE* patchBypassRW = NULL; + +#if defined(HOST_OSX) && defined(HOST_ARM64) + ExecutableWriterHolder sharedPatchBypassBufferWriterHolder; +#endif + + if (m_pSharedPatchBypassBuffer == NULL) + { + // If we don't have a shared patch buffer, create one + m_pSharedPatchBypassBuffer = patch->CreateSharedPatchBypassBuffer(); #if defined(HOST_OSX) && defined(HOST_ARM64) - ExecutableWriterHolder sharedPatchBypassBufferWriterHolder((SharedPatchBypassBuffer*)m_pSharedPatchBypassBuffer, sizeof(SharedPatchBypassBuffer)); - SharedPatchBypassBuffer *pSharedPatchBypassBufferRW = sharedPatchBypassBufferWriterHolder.GetRW(); + SharedPatchBypassBufferWriterHolder.AssignExecutableWriterHolder((SharedPatchBypassBuffer*)m_pSharedPatchBypassBuffer, sizeof(SharedPatchBypassBuffer)); + pSharedPatchBypassBufferRW = sharedPatchBypassBufferWriterHolder.GetRW(); #else // HOST_OSX && HOST_ARM64 - SharedPatchBypassBuffer *pSharedPatchBypassBufferRW = m_pSharedPatchBypassBuffer; + pSharedPatchBypassBufferRW = m_pSharedPatchBypassBuffer; #endif // HOST_OSX && HOST_ARM64 + // we do not write to the shared patch buffer if we are not creating it + patchBypassRW = pSharedPatchBypassBufferRW->PatchBypass; + } + BYTE* patchBypassRX = m_pSharedPatchBypassBuffer->PatchBypass; - BYTE* patchBypassRW = pSharedPatchBypassBufferRW->PatchBypass; LOG((LF_CORDB, LL_INFO10000, "DPS::DPS: Patch skip for opcode 0x%.4x at address %p buffer allocated at 0x%.8x\n", patch->opcode, patch->address, m_pSharedPatchBypassBuffer)); - // Copy the instruction block over to the patch skip - // WARNING: there used to be an issue here because CopyInstructionBlock copied the breakpoint from the - // jitted code stream into the patch buffer. Further below CORDbgSetInstruction would correct the - // first instruction. This buffer is shared by all threads so if another thread executed the buffer - // between this thread's execution of CopyInstructionBlock and CORDbgSetInstruction the wrong - // code would be executed. The bug has been fixed by changing CopyInstructionBlock to only copy - // the code bytes after the breakpoint. - // You might be tempted to stop copying the code at all, however that wouldn't work well with rejit. - // If we skip a breakpoint that is sitting at the beginning of a method, then the profiler rejits that - // method causing a jump-stamp to be placed, then we skip the breakpoint again, we need to make sure - // the 2nd skip executes the new jump-stamp code and not the original method prologue code. Copying - // the code every time ensures that we have the most up-to-date version of the code in the buffer. - _ASSERTE( patch->IsBound() ); - CopyInstructionBlock(patchBypassRW, (const BYTE *)patch->address); + if (patchBypassRW != NULL) + { + // Copy the instruction block over to the patch skip + // WARNING: there used to be an issue here because CopyInstructionBlock copied the breakpoint from the + // jitted code stream into the patch buffer. Further below CORDbgSetInstruction would correct the + // first instruction. This buffer is shared by all threads so if another thread executed the buffer + // between this thread's execution of CopyInstructionBlock and CORDbgSetInstruction the wrong + // code would be executed. The bug has been fixed by changing CopyInstructionBlock to only copy + // the code bytes after the breakpoint. + _ASSERTE( patch->IsBound() ); + CopyInstructionBlock(patchBypassRW, (const BYTE *)patch->address); - // Technically, we could create a patch skipper for an inactive patch, but we rely on the opcode being - // set here. - _ASSERTE( patch->IsActivated() ); - CORDbgSetInstruction((CORDB_ADDRESS_TYPE *)patchBypassRW, patch->opcode); + // Technically, we could create a patch skipper for an inactive patch, but we rely on the opcode being + // set here. + _ASSERTE( patch->IsActivated() ); + CORDbgSetInstruction((CORDB_ADDRESS_TYPE *)patchBypassRW, patch->opcode); + + LOG((LF_CORDB, LL_EVERYTHING, "SetInstruction was called\n")); + } + - LOG((LF_CORDB, LL_EVERYTHING, "SetInstruction was called\n")); // // Look at instruction to get some attributes // @@ -4565,12 +4579,12 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, #endif #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 && !IsInPlaceSingleStep()) + if (patchBypassRW != NULL && m_instrAttrib.m_dwOffsetToDisp != 0 && !IsInPlaceSingleStep()) { + _ASSERTE(pSharedPatchBypassBufferRW != NULL); _ASSERTE(m_instrAttrib.m_cbInstr != 0); // diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index b02cd2802d91ad..02e8fb0b118866 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -552,7 +552,17 @@ struct DebuggerControllerPatch #ifndef FEATURE_EMULATE_SINGLESTEP // gets a pointer to the shared buffer - SharedPatchBypassBuffer* GetOrCreateSharedPatchBypassBuffer(); + SharedPatchBypassBuffer* CreateSharedPatchBypassBuffer(); + SharedPatchBypassBuffer* GetSharedPatchBypassBuffer() + { + SharedPatchBypassBuffer *pRet = m_pSharedPatchBypassBuffer; + if (pRet != NULL) + { + // AddRef the buffer so that it doesn't go away while we're using it. + pRet->AddRef(); + } + return pRet; + } // entry point for general initialization when the controller is being created void Initialize() From 0c12fc68b3a8d08954f6a1f7490c49a3eb63ea74 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Thu, 10 Jul 2025 18:50:21 -0400 Subject: [PATCH 2/6] Assert m_pSharedPatchBypassBuffer is NULL in CreateSharedPatchBypassBuffer --- src/coreclr/debug/ee/controller.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 5e8c16429f3f5d..7eb7c7ed52dcb5 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -79,6 +79,7 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer( } CONTRACTL_END; + _ASSERTE(m_pSharedPatchBypassBuffer == NULL); if (m_pSharedPatchBypassBuffer == NULL) { void *pSharedPatchBypassBufferRX = g_pDebugger->GetInteropSafeExecutableHeap()->Alloc(sizeof(SharedPatchBypassBuffer)); From fbcde48b9e08435100a1df4824def39fff7f71a1 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 14 Jul 2025 12:55:23 -0400 Subject: [PATCH 3/6] Centralize shared bypass patch buffer creation --- src/coreclr/debug/ee/controller.cpp | 348 ++++++++++++++-------------- src/coreclr/debug/ee/controller.h | 8 +- 2 files changed, 175 insertions(+), 181 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 7eb7c7ed52dcb5..e5a223e8ea689d 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -67,10 +67,80 @@ bool DebuggerControllerPatch::IsSafeForStackTrace() } +#ifndef DACCESS_COMPILE #ifndef FEATURE_EMULATE_SINGLESTEP -// returns a pointer to the shared buffer. each call will AddRef() the object -// before returning it so callers only need to Release() when they're finished with it. -SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer() + +// +// We have to have a whole separate function for this because you +// can't use __try in a function that requires object unwinding... +// + +LONG FilterAccessViolation2(LPEXCEPTION_POINTERS ep, PVOID pv) +{ + LIMITED_METHOD_CONTRACT; + + return (ep->ExceptionRecord->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) + ? EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH; +} + +// This helper is required because the AVInRuntimeImplOkayHolder can not +// be directly placed inside the scope of a PAL_TRY +void _CopyInstructionBlockHelper(BYTE* to, const BYTE* from) +{ + AVInRuntimeImplOkayHolder AVOkay; + + // This function only copies the portion of the instruction that follows the + // breakpoint opcode, not the breakpoint itself + to += CORDbg_BREAK_INSTRUCTION_SIZE; + from += CORDbg_BREAK_INSTRUCTION_SIZE; + + // If an AV occurs because we walked off a valid page then we need + // to be certain that all bytes on the previous page were copied. + // We are certain that we copied enough bytes to contain the instruction + // because it must have fit within the valid page. + for (int i = 0; i < MAX_INSTRUCTION_LENGTH - CORDbg_BREAK_INSTRUCTION_SIZE; i++) + { + *to++ = *from++; + } + +} + +// WARNING: this function skips copying the first CORDbg_BREAK_INSTRUCTION_SIZE bytes by design +// See the comment at the callsite in DebuggerPatchSkip::DebuggerPatchSkip for more details on +// this +void DebuggerControllerPatch::CopyInstructionBlock(BYTE *to, const BYTE* from) +{ + // We wrap the memcpy in an exception handler to handle the + // extremely rare case where we're copying an instruction off the + // end of a method that is also at the end of a page, and the next + // page is unmapped. + struct Param + { + BYTE *to; + const BYTE* from; + } param; + param.to = to; + param.from = from; + PAL_TRY(Param *, pParam, ¶m) + { + _CopyInstructionBlockHelper(pParam->to, pParam->from); + } + PAL_EXCEPT_FILTER(FilterAccessViolation2) + { + // The whole point is that if we copy up the AV, then + // that's enough to execute, otherwise we would not have been + // able to execute the code anyway. So we just ignore the + // exception. + LOG((LF_CORDB, LL_INFO10000, + "DCP::CIP: AV copying instruction block ignored.\n")); + } + PAL_ENDTRY +} + + +// Creates a new shared patch bypass buffer +// Before returning it so callers only need to Release() when they're finished with it. +SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer(DebuggerControllerPatch *patch, InstructionAttribute * pInstrAttrib) { CONTRACTL { @@ -80,27 +150,107 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer( CONTRACTL_END; _ASSERTE(m_pSharedPatchBypassBuffer == NULL); - if (m_pSharedPatchBypassBuffer == NULL) + if (m_pSharedPatchBypassBuffer != NULL) { - void *pSharedPatchBypassBufferRX = g_pDebugger->GetInteropSafeExecutableHeap()->Alloc(sizeof(SharedPatchBypassBuffer)); + _ASSERTE(!"DCP::CSPBB: Shared patch buffer already exists!!"); + m_pSharedPatchBypassBuffer->AddRef(); + return m_pSharedPatchBypassBuffer; + } + + // create the shared patch bypass buffer + // we do not write to the shared patch buffer if we are not creating it + + SharedPatchBypassBuffer *pSharedPatchBypassBufferRX = (SharedPatchBypassBuffer*)g_pDebugger->GetInteropSafeExecutableHeap()->Alloc(sizeof(SharedPatchBypassBuffer)); #if defined(HOST_OSX) && defined(HOST_ARM64) - ExecutableWriterHolder sharedPatchBypassBufferWriterHolder((SharedPatchBypassBuffer*)pSharedPatchBypassBufferRX, sizeof(SharedPatchBypassBuffer)); - void *pSharedPatchBypassBufferRW = sharedPatchBypassBufferWriterHolder.GetRW(); + ExecutableWriterHolder sharedPatchBypassBufferWriterHolder((SharedPatchBypassBuffer*)pSharedPatchBypassBufferRX, sizeof(SharedPatchBypassBuffer)); + SharedPatchBypassBuffer *pSharedPatchBypassBufferRW = sharedPatchBypassBufferWriterHolder.GetRW(); #else // HOST_OSX && HOST_ARM64 - void *pSharedPatchBypassBufferRW = pSharedPatchBypassBufferRX; + SharedPatchBypassBuffer *pSharedPatchBypassBufferRW = pSharedPatchBypassBufferRX; #endif // HOST_OSX && HOST_ARM64 - new (pSharedPatchBypassBufferRW) SharedPatchBypassBuffer(); - m_pSharedPatchBypassBuffer = (SharedPatchBypassBuffer*)pSharedPatchBypassBufferRX; - - _ASSERTE(m_pSharedPatchBypassBuffer); - TRACE_ALLOC(m_pSharedPatchBypassBuffer); - } + new (pSharedPatchBypassBufferRW) SharedPatchBypassBuffer(); + m_pSharedPatchBypassBuffer = (SharedPatchBypassBuffer*)pSharedPatchBypassBufferRX; + _ASSERTE(m_pSharedPatchBypassBuffer); + TRACE_ALLOC(m_pSharedPatchBypassBuffer); m_pSharedPatchBypassBuffer->AddRef(); + BYTE* patchBypassRW = pSharedPatchBypassBufferRW->PatchBypass; + BYTE* patchBypassRX = m_pSharedPatchBypassBuffer->PatchBypass; + + LOG((LF_CORDB, LL_INFO10000, "DCP::CSPBB: Patch skip for opcode 0x%.4x at address %p buffer allocated at 0x%.8x\n", patch->opcode, patch->address, m_pSharedPatchBypassBuffer)); + + // CopyInstructionBlock copies all the code bytes except the breakpoint byte(s). + _ASSERTE( patch->IsBound() ); + _ASSERTE(DebuggerController::HasLock()); + CopyInstructionBlock(patchBypassRW, (const BYTE *)patch->address); + + // Technically, we could create a patch skipper for an inactive patch, but we rely on the opcode being + // set here. + _ASSERTE( patch->IsActivated() ); + CORDbgSetInstruction((CORDB_ADDRESS_TYPE *)patchBypassRW, patch->opcode); + + LOG((LF_CORDB, LL_EVERYTHING, "DCP::CSPBB: SetInstruction was called\n")); + + // + // Look at instruction to get some attributes + // + NativeWalker::DecodeInstructionForPatchSkip(patchBypassRX, pInstrAttrib); + +#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 (pInstrAttrib->m_dwOffsetToDisp != 0) + { + _ASSERTE(pSharedPatchBypassBufferRW != NULL); + _ASSERTE(pInstrAttrib->m_cbInstr != 0); + + // + // Populate the RIP-relative buffer with the current value if needed + // + + BYTE* bufferBypassRW = pSharedPatchBypassBufferRW->BypassBuffer; + + // Overwrite the *signed* displacement. + int dwOldDisp = *(int*)(&patchBypassRX[pInstrAttrib->m_dwOffsetToDisp]); + int dwNewDisp = offsetof(SharedPatchBypassBuffer, BypassBuffer) - + (offsetof(SharedPatchBypassBuffer, PatchBypass) + pInstrAttrib->m_cbInstr); + *(int*)(&patchBypassRW[pInstrAttrib->m_dwOffsetToDisp]) = dwNewDisp; + + // This could be an LEA, which we'll just have to change into a MOV and copy the original address. + if (((patchBypassRX[0] == 0x4C) || (patchBypassRX[0] == 0x48)) && (patchBypassRX[1] == 0x8d)) + { + patchBypassRW[1] = 0x8b; // MOV reg, mem + _ASSERTE((int)sizeof(void*) <= SharedPatchBypassBuffer::cbBufferBypass); + *(void**)bufferBypassRW = (void*)(patch->address + pInstrAttrib->m_cbInstr + dwOldDisp); + } + else + { + _ASSERTE(pInstrAttrib->m_cOperandSize <= SharedPatchBypassBuffer::cbBufferBypass); + // Copy the data into our buffer. + memcpy(bufferBypassRW, patch->address + pInstrAttrib->m_cbInstr + dwOldDisp, pInstrAttrib->m_cOperandSize); + + if (pInstrAttrib->m_fIsWrite) + { + // save the actual destination address and size so when we TriggerSingleStep() we can update the value + pSharedPatchBypassBufferRW->RipTargetFixup = (UINT_PTR)(patch->address + pInstrAttrib->m_cbInstr + dwOldDisp); + pSharedPatchBypassBufferRW->RipTargetFixupSize = pInstrAttrib->m_cOperandSize; + } + } + } + + #endif // TARGET_AMD64 + + // Since we just created a new buffer of code, but the CPU caches code and may + // not be aware of our changes. This should force the CPU to dump any cached + // instructions it has in this region and load the new ones from memory + FlushInstructionCache(GetCurrentProcess(), patchBypassRW + CORDbg_BREAK_INSTRUCTION_SIZE, + MAX_INSTRUCTION_LENGTH - CORDbg_BREAK_INSTRUCTION_SIZE); + return m_pSharedPatchBypassBuffer; } #endif // !FEATURE_EMULATE_SINGLESTEP +#endif // !DACCESS_COMPILE // @todo - remove all this splicing trash // This Sort/Splice stuff just reorders the patches within a particular chain such @@ -4520,57 +4670,16 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, // m_pSharedPatchBypassBuffer = patch->GetSharedPatchBypassBuffer(); - SharedPatchBypassBuffer *pSharedPatchBypassBufferRW = NULL; - BYTE* patchBypassRW = NULL; - -#if defined(HOST_OSX) && defined(HOST_ARM64) - ExecutableWriterHolder sharedPatchBypassBufferWriterHolder; -#endif if (m_pSharedPatchBypassBuffer == NULL) { // If we don't have a shared patch buffer, create one - m_pSharedPatchBypassBuffer = patch->CreateSharedPatchBypassBuffer(); -#if defined(HOST_OSX) && defined(HOST_ARM64) - SharedPatchBypassBufferWriterHolder.AssignExecutableWriterHolder((SharedPatchBypassBuffer*)m_pSharedPatchBypassBuffer, sizeof(SharedPatchBypassBuffer)); - pSharedPatchBypassBufferRW = sharedPatchBypassBufferWriterHolder.GetRW(); -#else // HOST_OSX && HOST_ARM64 - pSharedPatchBypassBufferRW = m_pSharedPatchBypassBuffer; -#endif // HOST_OSX && HOST_ARM64 - - // we do not write to the shared patch buffer if we are not creating it - patchBypassRW = pSharedPatchBypassBufferRW->PatchBypass; + m_pSharedPatchBypassBuffer = patch->CreateSharedPatchBypassBuffer(patch, &m_instrAttrib); } - - BYTE* patchBypassRX = m_pSharedPatchBypassBuffer->PatchBypass; - LOG((LF_CORDB, LL_INFO10000, "DPS::DPS: Patch skip for opcode 0x%.4x at address %p buffer allocated at 0x%.8x\n", patch->opcode, patch->address, m_pSharedPatchBypassBuffer)); - - if (patchBypassRW != NULL) + else { - // Copy the instruction block over to the patch skip - // WARNING: there used to be an issue here because CopyInstructionBlock copied the breakpoint from the - // jitted code stream into the patch buffer. Further below CORDbgSetInstruction would correct the - // first instruction. This buffer is shared by all threads so if another thread executed the buffer - // between this thread's execution of CopyInstructionBlock and CORDbgSetInstruction the wrong - // code would be executed. The bug has been fixed by changing CopyInstructionBlock to only copy - // the code bytes after the breakpoint. - _ASSERTE( patch->IsBound() ); - CopyInstructionBlock(patchBypassRW, (const BYTE *)patch->address); - - // Technically, we could create a patch skipper for an inactive patch, but we rely on the opcode being - // set here. - _ASSERTE( patch->IsActivated() ); - CORDbgSetInstruction((CORDB_ADDRESS_TYPE *)patchBypassRW, patch->opcode); - - LOG((LF_CORDB, LL_EVERYTHING, "SetInstruction was called\n")); + NativeWalker::DecodeInstructionForPatchSkip(m_pSharedPatchBypassBuffer->PatchBypass, &m_instrAttrib); } - - - // - // Look at instruction to get some attributes - // - - NativeWalker::DecodeInstructionForPatchSkip(patchBypassRX, &(m_instrAttrib)); #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT if (g_pDebugInterface->IsOutOfProcessSetContextEnabled() && m_instrAttrib.m_fIsCall) @@ -4579,51 +4688,6 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, } #endif -#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 (patchBypassRW != NULL && m_instrAttrib.m_dwOffsetToDisp != 0 && !IsInPlaceSingleStep()) - { - _ASSERTE(pSharedPatchBypassBufferRW != NULL); - _ASSERTE(m_instrAttrib.m_cbInstr != 0); - - // - // Populate the RIP-relative buffer with the current value if needed - // - - BYTE* bufferBypassRW = pSharedPatchBypassBufferRW->BypassBuffer; - - // Overwrite the *signed* displacement. - int dwOldDisp = *(int*)(&patchBypassRX[m_instrAttrib.m_dwOffsetToDisp]); - int dwNewDisp = offsetof(SharedPatchBypassBuffer, BypassBuffer) - - (offsetof(SharedPatchBypassBuffer, PatchBypass) + m_instrAttrib.m_cbInstr); - *(int*)(&patchBypassRW[m_instrAttrib.m_dwOffsetToDisp]) = dwNewDisp; - - // This could be an LEA, which we'll just have to change into a MOV and copy the original address. - if (((patchBypassRX[0] == 0x4C) || (patchBypassRX[0] == 0x48)) && (patchBypassRX[1] == 0x8d)) - { - patchBypassRW[1] = 0x8b; // MOV reg, mem - _ASSERTE((int)sizeof(void*) <= SharedPatchBypassBuffer::cbBufferBypass); - *(void**)bufferBypassRW = (void*)(patch->address + m_instrAttrib.m_cbInstr + dwOldDisp); - } - else - { - _ASSERTE(m_instrAttrib.m_cOperandSize <= SharedPatchBypassBuffer::cbBufferBypass); - // Copy the data into our buffer. - memcpy(bufferBypassRW, patch->address + m_instrAttrib.m_cbInstr + dwOldDisp, m_instrAttrib.m_cOperandSize); - - 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); - pSharedPatchBypassBufferRW->RipTargetFixupSize = m_instrAttrib.m_cOperandSize; - } - } - } - -#endif // TARGET_AMD64 - #endif // !FEATURE_EMULATE_SINGLESTEP // Signals our thread that the debugger will be manipulating the context @@ -4687,7 +4751,9 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, #else // FEATURE_EMULATE_SINGLESTEP #ifdef TARGET_ARM64 - patchBypassRX = NativeWalker::SetupOrSimulateInstructionForPatchSkip(context, m_pSharedPatchBypassBuffer, (const BYTE *)patch->address, patch->opcode); + BYTE* patchBypassRX = NativeWalker::SetupOrSimulateInstructionForPatchSkip(context, m_pSharedPatchBypassBuffer, (const BYTE *)patch->address, patch->opcode); +#else + BYTE* patchBypassRX = m_pSharedPatchBypassBuffer->PatchBypass; #endif //TARGET_ARM64 if (!IsInPlaceSingleStep()) @@ -4757,80 +4823,6 @@ void DebuggerPatchSkip::DebuggerDetachClean() #endif // !FEATURE_EMULATE_SINGLESTEP } - -// -// We have to have a whole separate function for this because you -// can't use __try in a function that requires object unwinding... -// - -LONG FilterAccessViolation2(LPEXCEPTION_POINTERS ep, PVOID pv) -{ - LIMITED_METHOD_CONTRACT; - - return (ep->ExceptionRecord->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) - ? EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH; -} - -// This helper is required because the AVInRuntimeImplOkayHolder can not -// be directly placed inside the scope of a PAL_TRY -void _CopyInstructionBlockHelper(BYTE* to, const BYTE* from) -{ - AVInRuntimeImplOkayHolder AVOkay; - - // This function only copies the portion of the instruction that follows the - // breakpoint opcode, not the breakpoint itself - to += CORDbg_BREAK_INSTRUCTION_SIZE; - from += CORDbg_BREAK_INSTRUCTION_SIZE; - - // If an AV occurs because we walked off a valid page then we need - // to be certain that all bytes on the previous page were copied. - // We are certain that we copied enough bytes to contain the instruction - // because it must have fit within the valid page. - for (int i = 0; i < MAX_INSTRUCTION_LENGTH - CORDbg_BREAK_INSTRUCTION_SIZE; i++) - { - *to++ = *from++; - } - -} - -// WARNING: this function skips copying the first CORDbg_BREAK_INSTRUCTION_SIZE bytes by design -// See the comment at the callsite in DebuggerPatchSkip::DebuggerPatchSkip for more details on -// this -void DebuggerPatchSkip::CopyInstructionBlock(BYTE *to, const BYTE* from) -{ - // We wrap the memcpy in an exception handler to handle the - // extremely rare case where we're copying an instruction off the - // end of a method that is also at the end of a page, and the next - // page is unmapped. - struct Param - { - BYTE *to; - const BYTE* from; - } param; - param.to = to; - param.from = from; - PAL_TRY(Param *, pParam, ¶m) - { - _CopyInstructionBlockHelper(pParam->to, pParam->from); - } - PAL_EXCEPT_FILTER(FilterAccessViolation2) - { - // The whole point is that if we copy up the AV, then - // that's enough to execute, otherwise we would not have been - // able to execute the code anyway. So we just ignore the - // exception. - LOG((LF_CORDB, LL_INFO10000, - "DPS::DPS: AV copying instruction block ignored.\n")); - } - PAL_ENDTRY - - // We just created a new buffer of code, but the CPU caches code and may - // not be aware of our changes. This should force the CPU to dump any cached - // instructions it has in this region and load the new ones from memory - FlushInstructionCache(GetCurrentProcess(), to + CORDbg_BREAK_INSTRUCTION_SIZE, - MAX_INSTRUCTION_LENGTH - CORDbg_BREAK_INSTRUCTION_SIZE); -} - TP_RESULT DebuggerPatchSkip::TriggerPatch(DebuggerControllerPatch *patch, Thread *thread, TRIGGER_WHY tyWhy) diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 02e8fb0b118866..d119cf681c6d5b 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -550,9 +550,10 @@ struct DebuggerControllerPatch // Is this patch at a position at which it's safe to take a stack? bool IsSafeForStackTrace(); +#ifndef DACCESS_COMPILE #ifndef FEATURE_EMULATE_SINGLESTEP // gets a pointer to the shared buffer - SharedPatchBypassBuffer* CreateSharedPatchBypassBuffer(); + SharedPatchBypassBuffer* CreateSharedPatchBypassBuffer(DebuggerControllerPatch *patch, InstructionAttribute *pInstrAttrib); SharedPatchBypassBuffer* GetSharedPatchBypassBuffer() { SharedPatchBypassBuffer *pRet = m_pSharedPatchBypassBuffer; @@ -564,6 +565,8 @@ struct DebuggerControllerPatch return pRet; } + void CopyInstructionBlock(BYTE *to, const BYTE* from); + // entry point for general initialization when the controller is being created void Initialize() { @@ -577,6 +580,7 @@ struct DebuggerControllerPatch m_pSharedPatchBypassBuffer->Release(); } #endif // !FEATURE_EMULATE_SINGLESTEP +#endif // !DACCESS_COMPILE void LogInstance() { @@ -1525,8 +1529,6 @@ class DebuggerPatchSkip : public DebuggerController virtual DEBUGGER_CONTROLLER_TYPE GetDCType(void) { return DEBUGGER_CONTROLLER_PATCH_SKIP; } - void CopyInstructionBlock(BYTE *to, const BYTE* from); - void DecodeInstruction(CORDB_ADDRESS_TYPE *code); void DebuggerDetachClean(); From 79829af315183a9449e91bbdf3aa918b456d69e2 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 14 Jul 2025 14:21:41 -0400 Subject: [PATCH 4/6] Move debugger lock to DebuggerPatchSkip ctor --- src/coreclr/debug/ee/controller.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index e5a223e8ea689d..ef8b6d4f396550 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -181,7 +181,6 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer( // CopyInstructionBlock copies all the code bytes except the breakpoint byte(s). _ASSERTE( patch->IsBound() ); - _ASSERTE(DebuggerController::HasLock()); CopyInstructionBlock(patchBypassRW, (const BYTE *)patch->address); // Technically, we could create a patch skipper for an inactive patch, but we rely on the opcode being @@ -4669,6 +4668,7 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, // Create the shared instruction block. this will also create the shared RIP-relative buffer // + _ASSERTE(DebuggerController::HasLock()); m_pSharedPatchBypassBuffer = patch->GetSharedPatchBypassBuffer(); if (m_pSharedPatchBypassBuffer == NULL) From 4fcb9cb0d78022f239a86d611f80fd67384efa12 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 14 Jul 2025 20:03:01 -0400 Subject: [PATCH 5/6] Update comment in controller.cpp Co-authored-by: Noah Falk --- src/coreclr/debug/ee/controller.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index ef8b6d4f396550..070f50785dd3df 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -139,7 +139,7 @@ void DebuggerControllerPatch::CopyInstructionBlock(BYTE *to, const BYTE* from) // Creates a new shared patch bypass buffer -// Before returning it so callers only need to Release() when they're finished with it. +// AddRef() before returning it so callers need to Release() when they're finished with it. SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer(DebuggerControllerPatch *patch, InstructionAttribute * pInstrAttrib) { CONTRACTL From 165499a644f0c4ca98ba11d9d24393ab92c202d1 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 14 Jul 2025 21:10:58 -0400 Subject: [PATCH 6/6] Refactor shared patch buffer handling in DebuggerControllerPatch. Encapsulate instruction attribute into SharedBypasBuffer --- src/coreclr/debug/daccess/stdafx.h | 1 + src/coreclr/debug/ee/controller.cpp | 69 +++++++++++------------------ src/coreclr/debug/ee/controller.h | 16 +++---- src/coreclr/debug/ee/walker.h | 4 ++ 4 files changed, 37 insertions(+), 53 deletions(-) diff --git a/src/coreclr/debug/daccess/stdafx.h b/src/coreclr/debug/daccess/stdafx.h index bb7b7b2365de59..bff6a4f660379b 100644 --- a/src/coreclr/debug/daccess/stdafx.h +++ b/src/coreclr/debug/daccess/stdafx.h @@ -42,6 +42,7 @@ #include #include #include +#include #include #include #include diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 070f50785dd3df..f7dfde2ae67f83 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -140,7 +140,7 @@ void DebuggerControllerPatch::CopyInstructionBlock(BYTE *to, const BYTE* from) // Creates a new shared patch bypass buffer // AddRef() before returning it so callers need to Release() when they're finished with it. -SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer(DebuggerControllerPatch *patch, InstructionAttribute * pInstrAttrib) +SharedPatchBypassBuffer* DebuggerControllerPatch::GetOrCreateSharedPatchBypassBuffer() { CONTRACTL { @@ -149,16 +149,16 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer( } CONTRACTL_END; - _ASSERTE(m_pSharedPatchBypassBuffer == NULL); if (m_pSharedPatchBypassBuffer != NULL) { - _ASSERTE(!"DCP::CSPBB: Shared patch buffer already exists!!"); m_pSharedPatchBypassBuffer->AddRef(); return m_pSharedPatchBypassBuffer; } - // create the shared patch bypass buffer - // we do not write to the shared patch buffer if we are not creating it + // NOTE: in order to correctly single-step RIP-relative writes on multiple threads we need to set up + // a shared buffer with the instruction and a buffer for the RIP-relative value so that all threads + // are working on the same copy. as the single-steps complete the modified data in the buffer is + // copied back to the real address to ensure proper execution of the program. SharedPatchBypassBuffer *pSharedPatchBypassBufferRX = (SharedPatchBypassBuffer*)g_pDebugger->GetInteropSafeExecutableHeap()->Alloc(sizeof(SharedPatchBypassBuffer)); #if defined(HOST_OSX) && defined(HOST_ARM64) @@ -177,32 +177,33 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer( BYTE* patchBypassRW = pSharedPatchBypassBufferRW->PatchBypass; BYTE* patchBypassRX = m_pSharedPatchBypassBuffer->PatchBypass; - LOG((LF_CORDB, LL_INFO10000, "DCP::CSPBB: Patch skip for opcode 0x%.4x at address %p buffer allocated at 0x%.8x\n", patch->opcode, patch->address, m_pSharedPatchBypassBuffer)); + LOG((LF_CORDB, LL_INFO10000, "DCP::CSPBB: Patch skip for opcode 0x%.4x at address %p buffer allocated at 0x%.8x\n", this->opcode, this->address, m_pSharedPatchBypassBuffer)); // CopyInstructionBlock copies all the code bytes except the breakpoint byte(s). - _ASSERTE( patch->IsBound() ); - CopyInstructionBlock(patchBypassRW, (const BYTE *)patch->address); + _ASSERTE( this->IsBound() ); + CopyInstructionBlock(patchBypassRW, (const BYTE *)this->address); // Technically, we could create a patch skipper for an inactive patch, but we rely on the opcode being // set here. - _ASSERTE( patch->IsActivated() ); - CORDbgSetInstruction((CORDB_ADDRESS_TYPE *)patchBypassRW, patch->opcode); + _ASSERTE( this->IsActivated() ); + CORDbgSetInstruction((CORDB_ADDRESS_TYPE *)patchBypassRW, this->opcode); LOG((LF_CORDB, LL_EVERYTHING, "DCP::CSPBB: SetInstruction was called\n")); // // Look at instruction to get some attributes // - NativeWalker::DecodeInstructionForPatchSkip(patchBypassRX, pInstrAttrib); + InstructionAttribute instrAttrib = { 0 }; + NativeWalker::DecodeInstructionForPatchSkip(patchBypassRX, &instrAttrib); #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 (pInstrAttrib->m_dwOffsetToDisp != 0) + if (instrAttrib.m_dwOffsetToDisp != 0) { _ASSERTE(pSharedPatchBypassBufferRW != NULL); - _ASSERTE(pInstrAttrib->m_cbInstr != 0); + _ASSERTE(instrAttrib.m_cbInstr != 0); // // Populate the RIP-relative buffer with the current value if needed @@ -211,34 +212,36 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::CreateSharedPatchBypassBuffer( BYTE* bufferBypassRW = pSharedPatchBypassBufferRW->BypassBuffer; // Overwrite the *signed* displacement. - int dwOldDisp = *(int*)(&patchBypassRX[pInstrAttrib->m_dwOffsetToDisp]); + int dwOldDisp = *(int*)(&patchBypassRX[instrAttrib.m_dwOffsetToDisp]); int dwNewDisp = offsetof(SharedPatchBypassBuffer, BypassBuffer) - - (offsetof(SharedPatchBypassBuffer, PatchBypass) + pInstrAttrib->m_cbInstr); - *(int*)(&patchBypassRW[pInstrAttrib->m_dwOffsetToDisp]) = dwNewDisp; + (offsetof(SharedPatchBypassBuffer, PatchBypass) + instrAttrib.m_cbInstr); + *(int*)(&patchBypassRW[instrAttrib.m_dwOffsetToDisp]) = dwNewDisp; // This could be an LEA, which we'll just have to change into a MOV and copy the original address. if (((patchBypassRX[0] == 0x4C) || (patchBypassRX[0] == 0x48)) && (patchBypassRX[1] == 0x8d)) { patchBypassRW[1] = 0x8b; // MOV reg, mem _ASSERTE((int)sizeof(void*) <= SharedPatchBypassBuffer::cbBufferBypass); - *(void**)bufferBypassRW = (void*)(patch->address + pInstrAttrib->m_cbInstr + dwOldDisp); + *(void**)bufferBypassRW = (void*)(this->address + instrAttrib.m_cbInstr + dwOldDisp); } else { - _ASSERTE(pInstrAttrib->m_cOperandSize <= SharedPatchBypassBuffer::cbBufferBypass); + _ASSERTE(instrAttrib.m_cOperandSize <= SharedPatchBypassBuffer::cbBufferBypass); // Copy the data into our buffer. - memcpy(bufferBypassRW, patch->address + pInstrAttrib->m_cbInstr + dwOldDisp, pInstrAttrib->m_cOperandSize); + memcpy(bufferBypassRW, this->address + instrAttrib.m_cbInstr + dwOldDisp, instrAttrib.m_cOperandSize); - if (pInstrAttrib->m_fIsWrite) + if (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 + pInstrAttrib->m_cbInstr + dwOldDisp); - pSharedPatchBypassBufferRW->RipTargetFixupSize = pInstrAttrib->m_cOperandSize; + pSharedPatchBypassBufferRW->RipTargetFixup = (UINT_PTR)(this->address + instrAttrib.m_cbInstr + dwOldDisp); + pSharedPatchBypassBufferRW->RipTargetFixupSize = instrAttrib.m_cOperandSize; } } } #endif // TARGET_AMD64 + + m_pSharedPatchBypassBuffer->SetInstructionAttrib(instrAttrib); // Since we just created a new buffer of code, but the CPU caches code and may // not be aware of our changes. This should force the CPU to dump any cached @@ -4659,27 +4662,9 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread, // the single-step emulation itself. #ifndef FEATURE_EMULATE_SINGLESTEP - // NOTE: in order to correctly single-step RIP-relative writes on multiple threads we need to set up - // a shared buffer with the instruction and a buffer for the RIP-relative value so that all threads - // are working on the same copy. as the single-steps complete the modified data in the buffer is - // copied back to the real address to ensure proper execution of the program. - - // - // Create the shared instruction block. this will also create the shared RIP-relative buffer - // - _ASSERTE(DebuggerController::HasLock()); - m_pSharedPatchBypassBuffer = patch->GetSharedPatchBypassBuffer(); - - if (m_pSharedPatchBypassBuffer == NULL) - { - // If we don't have a shared patch buffer, create one - m_pSharedPatchBypassBuffer = patch->CreateSharedPatchBypassBuffer(patch, &m_instrAttrib); - } - else - { - NativeWalker::DecodeInstructionForPatchSkip(m_pSharedPatchBypassBuffer->PatchBypass, &m_instrAttrib); - } + m_pSharedPatchBypassBuffer = patch->GetOrCreateSharedPatchBypassBuffer(); + m_instrAttrib = m_pSharedPatchBypassBuffer->GetInstructionAttrib(); #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT if (g_pDebugInterface->IsOutOfProcessSetContextEnabled() && m_instrAttrib.m_fIsCall) diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index d119cf681c6d5b..32138edc36aba7 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -312,7 +312,11 @@ class SharedPatchBypassBuffer UINT_PTR RipTargetFixup; #endif + const InstructionAttribute& GetInstructionAttrib() { return m_instrAttrib; } + void SetInstructionAttrib(const InstructionAttribute& instrAttrib) { m_instrAttrib = instrAttrib; } + private: + InstructionAttribute m_instrAttrib; // info about the instruction being skipped over const static DWORD SentinelValue = 0xffffffff; LONG m_refCount; }; @@ -553,17 +557,7 @@ struct DebuggerControllerPatch #ifndef DACCESS_COMPILE #ifndef FEATURE_EMULATE_SINGLESTEP // gets a pointer to the shared buffer - SharedPatchBypassBuffer* CreateSharedPatchBypassBuffer(DebuggerControllerPatch *patch, InstructionAttribute *pInstrAttrib); - SharedPatchBypassBuffer* GetSharedPatchBypassBuffer() - { - SharedPatchBypassBuffer *pRet = m_pSharedPatchBypassBuffer; - if (pRet != NULL) - { - // AddRef the buffer so that it doesn't go away while we're using it. - pRet->AddRef(); - } - return pRet; - } + SharedPatchBypassBuffer* GetOrCreateSharedPatchBypassBuffer(); void CopyInstructionBlock(BYTE *to, const BYTE* from); diff --git a/src/coreclr/debug/ee/walker.h b/src/coreclr/debug/ee/walker.h index 63bd4cfa2fd273..4dcae6d75ed273 100644 --- a/src/coreclr/debug/ee/walker.h +++ b/src/coreclr/debug/ee/walker.h @@ -62,6 +62,8 @@ struct InstructionAttribute } }; +#ifndef DACCESS_COMPILE + /* ------------------------------------------------------------------------- * * Classes * ------------------------------------------------------------------------- */ @@ -280,4 +282,6 @@ class NativeWalker : public Walker }; #endif +#endif // DACCESS_COMPILE + #endif // WALKER_H_