Skip to content

[WIP][clr-interp] Support for managed debugger breakpoints #123251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
matouskozak wants to merge 10 commits into
base: main
Choose a base branch
from
+981 −69
Draft

[WIP][clr-interp] Support for managed debugger breakpoints #123251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6b4cde7
refactor ExecutionControl
matouskozak Dec 16, 2025
086893a
enable breakpoints in the interpreter
matouskozak Dec 18, 2025
c97afe7
support for regular breakpoints with interpreter
matouskozak Jan 16, 2026
dce1adc
insert IL offset 0 pointing to IR 0
matouskozak Jan 16, 2026
37b12bb
disable interpreter breakpoint support for BROWSER
matouskozak Jan 19, 2026
e1e1a1d
Basic single stepping support for interpreter
matouskozak Jan 30, 2026
68d70d4
disable CheckRegDisplaySP for interpreter TODO
matouskozak Jan 30, 2026
784a37b
call OnMethodEnter to notify debugger
matouskozak Jan 30, 2026
9da30bb
modify cmakelist
matouskozak Jan 30, 2026
8006149
keep interpreter NOPs for debug builds
matouskozak Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/coreclr/debug/ee/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ set(CORDBEE_HEADERS_DAC

list(APPEND CORDBEE_SOURCES_WKS ${ARCH_SOURCES_DIR}/walker.cpp)

if(FEATURE_INTERPRETER)
list(APPEND CORDBEE_SOURCES_WKS interpreterwalker.cpp)
list(APPEND CORDBEE_HEADERS_WKS interpreterwalker.h)
endif(FEATURE_INTERPRETER)

if (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_I386)
list(APPEND CORDBEE_SOURCES_WKS ${ARCH_SOURCES_DIR}/debuggerregdisplayhelper.cpp)
endif ()
Expand Down
304 changes: 287 additions & 17 deletions src/coreclr/debug/ee/controller.cpp
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/coreclr/debug/ee/controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include "frameinfo.h"
#include "executioncontrol.h"
#include "interpreterwalker.h"

/* ------------------------------------------------------------------------- *
* Forward declarations
Expand Down
11 changes: 11 additions & 0 deletions src/coreclr/debug/ee/debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5600,6 +5600,17 @@ void Debugger::OnMethodEnter(void * pIP)
FramePointer fp = LEAF_MOST_FRAME;
DebuggerController::DispatchMethodEnter(pIP, fp);
}
/******************************************************************************
* IsMethodEnterEnabled
* Returns true if any stepper/controller has requested method-enter callbacks.
* Used by the interpreter to gate OnMethodEnter calls.
******************************************************************************/
bool Debugger::IsMethodEnterEnabled()
{
LIMITED_METHOD_CONTRACT;
return DebuggerController::GetTotalMethodEnter() > 0;
}

/******************************************************************************
* GetJMCFlagAddr
* Provide an address of the flag that the JMC probes use to decide whether
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/debug/ee/debugger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2683,6 +2683,7 @@ class Debugger : public DebugInterface
#ifndef DACCESS_COMPILE
virtual void OnMethodEnter(void * pIP);
virtual DWORD* GetJMCFlagAddr(Module * pModule);
virtual bool IsMethodEnterEnabled();
#endif

// GetJMCFlagAddr provides a unique flag for each module. UpdateModuleJMCFlag
Expand Down
113 changes: 104 additions & 9 deletions src/coreclr/debug/ee/executioncontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,40 @@

#ifdef FEATURE_INTERPRETER
#include "../../interpreter/intops.h"

// TODO: Remove, this is just for pretty printing opcodes in logging.
// Generate local copy of interpreter opcode length table for use in debugger.
// This avoids linker dependency on the interpreter module.
static const uint8_t s_interpOpLen[] = {
#define OPDEF(a,b,c,d,e,f) c,
#include "../../interpreter/inc/intops.def"
#undef OPDEF
};

// Generate local opcode name table (same pattern as intops.cpp)
struct InterpOpNameChars
{
#define OPDEF(a,b,c,d,e,f) char a[sizeof(b)];
#include "../../interpreter/inc/intops.def"
#undef OPDEF
};

static const InterpOpNameChars s_interpOpNameChars = {
#define OPDEF(a,b,c,d,e,f) b,
#include "../../interpreter/inc/intops.def"
#undef OPDEF
};

static const uint32_t s_interpOpNameOffsets[] = {
#define OPDEF(a,b,c,d,e,f) offsetof(InterpOpNameChars, a),
#include "../../interpreter/inc/intops.def"
#undef OPDEF
};

static const char* GetInterpOpName(int op)
{
return ((const char*)&s_interpOpNameChars) + s_interpOpNameOffsets[op];
}
#endif

#if !defined(DACCESS_COMPILE)
Expand All @@ -23,10 +57,9 @@
// InterpreterExecutionControl - Interpreter bytecode breakpoints
//=============================================================================

InterpreterExecutionControl InterpreterExecutionControl::s_instance;

InterpreterExecutionControl* InterpreterExecutionControl::GetInstance()
{
static InterpreterExecutionControl s_instance;
return &s_instance;
}

Expand All @@ -39,11 +72,31 @@ bool InterpreterExecutionControl::ApplyPatch(DebuggerControllerPatch* patch)
LOG((LF_CORDB, LL_INFO10000, "InterpreterEC::ApplyPatch %p at bytecode addr %p\n",
patch, patch->address));

patch->opcode = *(int32_t*)patch->address;
*(uint32_t*)patch->address = INTOP_BREAKPOINT;

LOG((LF_CORDB, LL_EVERYTHING, "InterpreterEC::ApplyPatch Breakpoint inserted at %p, saved opcode %x\n",
patch->address, patch->opcode));
// Check if there is already a breakpoint patch at this address
uint32_t currentOpcode = *(uint32_t*)patch->address;
if (currentOpcode == INTOP_BREAKPOINT || currentOpcode == INTOP_SINGLESTEP)
Copy link
Member

@noahfalk noahfalk Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems suspicious. the debugger could do something like this:
EnableSingleStepping()
ApplyPatch(nextAddress)
DisableSingleStepping()

The debugger would expect the breakpoint is still set at the end of that sequence but this code appears to avoid setting a breakpoint when single stepping is also enabled.

My guess is you either want one special opcode with state tracking on the side to describe what happens when you hit it, or you want three opcodes representing BREAKPOINT, SINGLESTEP, BREAKPOINT_AND_SINGLESTEP.

Also there is a comment lower down with broader questions about the single stepping abstraction. If we decide to go with the debugger having an interpretter single step emulator then I'd expect INTOP_SINGLESTEP goes away.

{
LOG((LF_CORDB, LL_INFO1000, "InterpreterEC::ApplyPatch Patch already applied at %p\n",
patch->address));
return false;
}

patch->opcode = currentOpcode; // Save original opcode

// Check if this is a single-step patch by looking at the controller's thread's interpreter SS flag.
Thread* pThread = patch->controller->GetThread();
if (pThread != NULL && pThread->IsInterpreterSingleStepEnabled())
{
*(uint32_t*)patch->address = INTOP_SINGLESTEP;
LOG((LF_CORDB, LL_INFO10000, "InterpreterEC::ApplyPatch SingleStep inserted at %p, saved opcode 0x%x (%s)\n",
patch->address, patch->opcode, GetInterpOpName(patch->opcode)));
}
else
{
*(uint32_t*)patch->address = INTOP_BREAKPOINT;
LOG((LF_CORDB, LL_INFO10000, "InterpreterEC::ApplyPatch Breakpoint inserted at %p, saved opcode 0x%x (%s)\n",
patch->address, patch->opcode, GetInterpOpName(patch->opcode)));
}
Comment on lines +75 to +99
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bytecode patch operation at lines 76-99 has a potential race condition. The sequence of reading the current opcode (line 76), checking it (line 77), saving it (line 84), and writing the patch (line 90 or 96) is not atomic. If two threads try to apply patches at the same address simultaneously, or if one thread is executing the code while another is patching it, the results could be unpredictable. Consider using InterlockedCompareExchange or a similar atomic operation to ensure the patch is applied atomically. Additionally, ensure proper memory barriers are in place so that the patch is visible to all threads before execution continues.

Suggested change
// Check if there is already a breakpoint patch at this address
uint32_t currentOpcode = *(uint32_t*)patch->address;
if (currentOpcode == INTOP_BREAKPOINT || currentOpcode == INTOP_SINGLESTEP)
{
LOG((LF_CORDB, LL_INFO1000, "InterpreterEC::ApplyPatch Patch already applied at %p\n",
patch->address));
return false;
}
patch->opcode = currentOpcode; // Save original opcode
// Check if this is a single-step patch by looking at the controller's thread's interpreter SS flag.
Thread* pThread = patch->controller->GetThread();
if (pThread != NULL && pThread->IsInterpreterSingleStepEnabled())
{
*(uint32_t*)patch->address = INTOP_SINGLESTEP;
LOG((LF_CORDB, LL_INFO10000, "InterpreterEC::ApplyPatch SingleStep inserted at %p, saved opcode 0x%x (%s)\n",
patch->address, patch->opcode, GetInterpOpName(patch->opcode)));
}
else
{
*(uint32_t*)patch->address = INTOP_BREAKPOINT;
LOG((LF_CORDB, LL_INFO10000, "InterpreterEC::ApplyPatch Breakpoint inserted at %p, saved opcode 0x%x (%s)\n",
patch->address, patch->opcode, GetInterpOpName(patch->opcode)));
}
// Use an atomic compare-exchange loop to apply the patch safely.
volatile LONG* pOpcode = reinterpret_cast<volatile LONG*>(patch->address);
// Check if this is a single-step patch by looking at the controller's thread's interpreter SS flag.
Thread* pThread = patch->controller->GetThread();
const bool isSingleStep = (pThread != NULL) && pThread->IsInterpreterSingleStepEnabled();
while (true)
{
// Atomically read the current opcode with a full memory barrier.
uint32_t currentOpcode = static_cast<uint32_t>(InterlockedCompareExchange(pOpcode, 0, 0));
// If there is already a breakpoint or single-step at this address, do not apply another patch.
if (currentOpcode == INTOP_BREAKPOINT || currentOpcode == INTOP_SINGLESTEP)
{
LOG((LF_CORDB, LL_INFO1000, "InterpreterEC::ApplyPatch Patch already applied at %p\n",
patch->address));
return false;
}
LONG newOpcode = isSingleStep
? static_cast<LONG>(INTOP_SINGLESTEP)
: static_cast<LONG>(INTOP_BREAKPOINT);
// Attempt to atomically replace the current opcode with the new one.
uint32_t originalOpcode = static_cast<uint32_t>(InterlockedCompareExchange(
pOpcode,
newOpcode,
static_cast<LONG>(currentOpcode)));
// If the value we observed is still current, we successfully applied the patch.
if (originalOpcode == currentOpcode)
{
patch->opcode = currentOpcode; // Save original opcode
if (isSingleStep)
{
LOG((LF_CORDB, LL_INFO10000, "InterpreterEC::ApplyPatch SingleStep inserted at %p, saved opcode 0x%x (%s)\n",
patch->address, patch->opcode, GetInterpOpName(patch->opcode)));
}
else
{
LOG((LF_CORDB, LL_INFO10000, "InterpreterEC::ApplyPatch Breakpoint inserted at %p, saved opcode 0x%x (%s)\n",
patch->address, patch->opcode, GetInterpOpName(patch->opcode)));
}
break;
}
// Another thread modified the opcode concurrently; retry with the new value.
}

Copilot uses AI. Check for mistakes.

return true;
}
Expand All @@ -54,8 +107,8 @@ bool InterpreterExecutionControl::UnapplyPatch(DebuggerControllerPatch* patch)
_ASSERTE(patch->address != NULL);
_ASSERTE(patch->IsActivated());

LOG((LF_CORDB, LL_INFO1000, "InterpreterEC::UnapplyPatch %p at bytecode addr %p, replacing with original opcode 0x%x\n",
patch, patch->address, patch->opcode));
LOG((LF_CORDB, LL_INFO1000, "InterpreterEC::UnapplyPatch %p at bytecode addr %p, replacing with original opcode 0x%x (%s)\n",
patch, patch->address, patch->opcode, GetInterpOpName(patch->opcode)));

// Restore the original opcode
*(uint32_t*)patch->address = (uint32_t)patch->opcode; // Opcodes are stored in uint32_t slots
Expand All @@ -67,5 +120,47 @@ bool InterpreterExecutionControl::UnapplyPatch(DebuggerControllerPatch* patch)
return true;
}

BreakpointInfo InterpreterExecutionControl::GetBreakpointInfo(const void* address) const
Copy link
Member

@noahfalk noahfalk Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll come back to this since I need to run for the moment, but I'm suspicious about the abstraction here. I think of these ExecutionControl APIs as being a layer below the debugger, something the debugger depends on. I did not expect them to be using the debugger patch table.

{
_ASSERTE(address != NULL);

BreakpointInfo info = { INTOP_NOP, false };

DebuggerController::ControllerLockHolder lockController;

DebuggerPatchTable* patchTable = DebuggerController::GetPatchTable();
if (patchTable != NULL)
{
DebuggerControllerPatch* patch = patchTable->GetPatch((CORDB_ADDRESS_TYPE*)address);
if (patch != NULL && patch->IsActivated())
{
// Get the original opcode from the first activated patch
info.originalOpcode = (InterpOpcode)patch->opcode;

// Iterate through ALL patches at this address to check if ANY is a step-out patch.
// Multiple patches can exist at the same address (e.g., IDE breakpoint + step-out from Debugger.Break()).
// The step-out patch may not be the first in the list, so we must check all of them.
for (DebuggerControllerPatch* p = patch; p != NULL; p = patchTable->GetNextPatch(p))
{
if (p->GetKind() == PATCH_KIND_NATIVE_MANAGED)
{
info.isStepOut = true;
break;
}
}

LOG((LF_CORDB, LL_INFO10000, "InterpreterEC::GetBreakpointInfo at %p: opcode=0x%x (%s), isStepOut=%d\n",
address, info.originalOpcode, GetInterpOpName(info.originalOpcode), info.isStepOut));
return info;
}
}

// No patch at this address, read opcode from memory, not a step-out
info.originalOpcode = (InterpOpcode)(*(const uint32_t*)address);
info.isStepOut = false;
LOG((LF_CORDB, LL_INFO10000, "InterpreterEC::GetBreakpointInfo at %p: no patch, opcode=0x%x (%s)\n",
address, info.originalOpcode, GetInterpOpName(info.originalOpcode)));
return info;
}
#endif // FEATURE_INTERPRETER
#endif // !DACCESS_COMPILE
11 changes: 8 additions & 3 deletions src/coreclr/debug/ee/executioncontrol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ struct DebuggerControllerPatch;

#ifdef FEATURE_INTERPRETER

// Result of GetBreakpointInfo - combines opcode and step-out flag
struct BreakpointInfo
{
InterpOpcode originalOpcode;
bool isStepOut;
};
Comment on lines 17 to +24
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header file uses InterpOpcode type but doesn't include the header that defines it. While this works when included from files that already have the interpreter headers included (like interpexec.cpp), it violates header self-containment principles and could cause compilation errors if this header is included elsewhere. Add #include "../../interpreter/inc/intopsshared.h" after line 15 or before the BreakpointInfo struct definition to ensure the header is self-contained.

Copilot uses AI. Check for mistakes.

class IExecutionControl
{
Expand All @@ -26,8 +32,6 @@ class IExecutionControl
virtual bool UnapplyPatch(DebuggerControllerPatch* patch) = 0;
};

typedef DPTR(IExecutionControl) PTR_IExecutionControl;

// Interpreter execution control using bytecode patching
class InterpreterExecutionControl : public IExecutionControl
{
Expand All @@ -40,9 +44,10 @@ class InterpreterExecutionControl : public IExecutionControl
// Remove a breakpoint patch and restore original instruction
virtual bool UnapplyPatch(DebuggerControllerPatch* patch) override;

// Get breakpoint info (original opcode and step-out flag)
BreakpointInfo GetBreakpointInfo(const void* address) const;
private:
InterpreterExecutionControl() = default;
static InterpreterExecutionControl s_instance;
};

#endif // FEATURE_INTERPRETER
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/ee/functioninfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2060,7 +2060,7 @@ void DebuggerMethodInfo::CreateDJIsForMethodDesc(MethodDesc * pMethodDesc)
{
// Some versions may not be compiled yet - skip those for now
// if they compile later the JitCompiled callback will add a DJI to our cache at that time
PCODE codeAddr = itr->GetNativeCode();
PCODE codeAddr = GetInterpreterCodeFromInterpreterPrecodeIfPresent(itr->GetNativeCode());
LOG((LF_CORDB, LL_INFO10000, "DMI::CDJIFMD (%d) Native code for DJI - %p\n", ++count, codeAddr));
if (codeAddr)
{
Expand Down
Loading
Loading