[clr-interp] Skip INTOP_CALL_FINALLY in DebuggerStepper::ShouldContinueStep

[kotlarmilos]

Description

INTOP_CALL_FINALLY is internal interpreter EH machinery (it dispatches a finally clause) and has no source-language counterpart, so the user should never stop on it during stepping. Without this skip, step-over across a try/finally block under the interpreter incorrectly stops at the synthesized CALL_FINALLY instruction.

The skip lives in DebuggerStepper::ShouldContinueStep so the existing "transparent" mechanism that already handles other internal stops handles this case too.

[Copilot]

Pull request overview

Fixes interpreter-specific debugging behaviors around JMC stepping, stack walking, and func-eval/breakpoint handling so that interpreter behavior matches JIT expectations and avoids redundant debugger callbacks.

Changes:

• Add interpreter-aware stepping logic to skip internal INTOP_CALL_FINALLY boundaries during JMC stepping.
• Filter Environment.CallEntryPoint from debugger stack walks (in-proc and DAC) to match JIT visibility behavior.
• Refine interpreter breakpoint + func-eval handling (pre-callback bypass checks, bypass save/restore around func-eval, and allow SetIP to change interpreter IP).

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/DebuggerEval.cs	Rename contract flag to EvalUsesHijack for interpreting func-eval behavior in stack walking.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameHandling/BaseFrameHandler.cs	Use EvalUsesHijack to decide whether to update context from func-eval frame.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameHandling/X86FrameHandler.cs	Same as BaseFrameHandler, with x86-specific context copying.
src/coreclr/vm/interpexec.cpp	Interpreter breakpoint handler updates (pre/post bypass checks, pending interpreter func-eval execution, SetIP support).
src/coreclr/vm/dbginterface.h	Add ExecutePendingInterpreterFuncEval hook for interpreter breakpoint handler.
src/coreclr/vm/datadescriptor/datadescriptor.inc	CDAC descriptor rename: EvalDuringException → EvalUsesHijack.
src/coreclr/debug/ee/interpreterwalker.h	Add InterpreterWalker::IsCallFinally() helper.
src/coreclr/debug/ee/interpreterwalker.cpp	Implement IsCallFinally().
src/coreclr/debug/ee/funceval.cpp	Replace m_evalDuringException checks with m_evalUsesHijack.
src/coreclr/debug/ee/frameinfo.cpp	Filter Environment.CallEntryPoint in debugger stack walk for interpreter mode.
src/coreclr/debug/ee/debugger.h	Rename state flag and skip bp-segment executability check when not using hijack.
src/coreclr/debug/ee/debugger.cpp	Interpreter-aware func-eval setup (skip executable heap allocation for interpreter) and expose ExecutePendingInterpreterFuncEval.
src/coreclr/debug/ee/controller.cpp	Stepper logic to skip INTOP_CALL_FINALLY boundaries when stepping interpreted code.
src/coreclr/debug/daccess/dacdbiimplstackwalk.cpp	DAC stack walk filtering for Environment.CallEntryPoint under interpreter.

Comments suppressed due to low confidence (1)

src/coreclr/debug/ee/debugger.cpp:14436

• In the interpreter func-eval path, bpInfoSegmentRX is NULL, but DebuggerEval::Init() is called before m_evalUsesHijack is flipped to false. Because the ctor currently initializes m_evalUsesHijack to !fInException (true for non-exception interpreter evals), Init() will dereference m_bpInfoSegment and can crash. Set m_evalUsesHijack correctly before calling Init (e.g., initialize it in the ctor from both fInException and whether bpInfoSegmentRX is non-null, or delay Init() until after the interpreter-specific flag update), and consider adding an assert/guard in Init to ensure m_bpInfoSegment is non-null when m_evalUsesHijack is true.

    // Create a DebuggerEval to hold info about this eval while its in progress. Constructor copies the thread's
    // CONTEXT.
    DebuggerEval *pDE = new (interopsafe, nothrow) DebuggerEval(filterContext, pEvalInfo, fInException, bpInfoSegmentRX);

    if (pDE == NULL)
    {
        return E_OUTOFMEMORY;
    }
    else if (!pDE->Init())
    {
        // We fail to change the m_breakpointInstruction field to PAGE_EXECUTE_READWRITE permission.
        return E_FAIL;
    }

[dotnet-policy-service]

Tagging subscribers to 'os-ios': @vitek-karas, @kotlarmilos, @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

[steveisok]

@kotlarmilos would it make sense to drop the #ifdef FEATURE_INTERPRETER guards around
the two CallEntryPoint filter hunks (in frameinfo.cpp::DebuggerWalkStackProc
and dacdbiimplstackwalk.cpp::UnwindStackWalkFrame) so the filter applies on
JIT targets too?

The comment "to match JIT behavior" suggests the JIT is expected to hide
CallEntryPoint — but in practice [UnmanagedCallersOnly] disables inlining
(UCO methods must have a real, callable entry point), so the frame is visible
to ICorDebug on JIT targets as well. We're seeing it in
dotnet/dotnet-diagnostictests on linux-x64, win-x64, win-x86, and osx-arm64
JIT legs in build
2955987,
adding one frame below Program.Main.

[kotlarmilos]

would it make sense to drop the #ifdef FEATURE_INTERPRETER guards around
the two CallEntryPoint filter hunks (in frameinfo.cpp::DebuggerWalkStackProc
and dacdbiimplstackwalk.cpp::UnwindStackWalkFrame) so the filter applies on
JIT targets too?

Yes, that makes sense. Let me resolve the conflicts and invoke the tests

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/coreclr/debug/ee/debugger.cpp:14299

• In the interpreter func-eval path bpInfoSegmentRX is intentionally NULL, but DebuggerEval currently sets m_evalUsesHijack = !fInException in the constructor. That means for interpreter non-exception evals (fInException == false) m_evalUsesHijack remains true when pDE->Init() runs, and Init() will dereference m_bpInfoSegment (NULL) in the executability assert/check. Set m_evalUsesHijack correctly before calling Init() (e.g., pass a "uses hijack" flag into the constructor or defer Init() until after the interpreter branch sets the flag).

    // Create a DebuggerEval to hold info about this eval while its in progress. Constructor copies the thread's
    // CONTEXT.
    DebuggerEval *pDE = new (interopsafe, nothrow) DebuggerEval(filterContext, pEvalInfo, fInException, bpInfoSegmentRX);

    if (pDE == NULL)
    {
        return E_OUTOFMEMORY;
    }
    else if (!pDE->Init())
    {
        // We fail to change the m_breakpointInstruction field to PAGE_EXECUTE_READWRITE permission.
        return E_FAIL;
    }

[Copilot]

Copilot's findings

• Files reviewed: 17/17 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 21/21 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[BrzVlad]

If stepping can only happen at places where we generate debug seq points, wouldn't it make more sense to simply not emit these opcodes in the first place. Is there a reason why we would want to generate them just so that we always ignore them afterwards ?

[kotlarmilos]

The skip isn't about ignoring INTOP_DEBUG_SEQ_POINTs, it's about the EH dispatch opcodes. INTOP_CALL_FINALLY and INTOP_LEAVE_CATCH live in compiler-generated EH dispatch blocks with ilOffset = -1 and don't get their own IL to native map entries, but the stepper still lands on them because the entry for the leave maps to the start of that EH dispatch block.

The op == INTOP_DEBUG_SEQ_POINT is defensive check here, if it can't happen we should drop it.

[janvorli]

JIT has matching concept of a call that calls finally. I would like to understand why it doesn't require extra treatment. Isn't the problem actually the fact that we generate the seq point on the INTOP_CALL_FINALLY while we should not?

[kotlarmilos]

The stepper handles call to finally in JIT because it is recognized as a call to funclet via IsAddrWithinMethodIncludingFunclet or fCallingIntoFunclet in BBJ_CALLFINALLY. The interpreter doesn't use funclets, so that path doesn't apply.

My understanding on the seq points is that we don't actually emit them on INTOP_CALL_FINALLY. The IL to native map only gets entries from instructions whose ilOffset is > 0, and the EH dispatch block has offset -1. The IL leave itself is compiled to an INTOP_BR in the user's source BB, and the INTOP_BR jumps into the EH dispatch block.

[janvorli]

The interpreter doesn't use funclets, so that path doesn't apply.

Interepreter actually does use funclets, but they are interpreted. If you look at the generated code though, the layout of the code matches what JIT would generate. It seems that IsAddrWithinMethodIncludingFunclet would work for the interpreted code too.

My understanding on the seq points is that we don't actually emit them on INTOP_CALL_FINALLY

So I am not sure then why the debugger stops on the instruction. But I am not an expert on how the single stepping behaves at this level.

[kotlarmilos]

Interepreter actually does use funclets, but they are interpreted.

You are right, they are funclets at the bytecode level. Looking at the walker, I noticed that INTOP_CALL_FINALLY and INTOP_LEAVE_CATCH are classified as WALK_BRANCH:

    else if (m_opcode == INTOP_CALL_FINALLY || m_opcode == INTOP_LEAVE_CATCH)
    {
        // Exception handling branches
        m_type = WALK_BRANCH;
        m_nextIP = GetBranchTarget();
        LOG((LF_CORDB, LL_INFO10000, "InterpreterWalker::Decode: WALK_BRANCH (EH) to %p\n", m_nextIP));
    }

Should it be WALK_CALL since it is a direct call to a funclet?

[janvorli]

It seems it would make sense to mark the INTOP_CALL_FINALLY as WALK_CALL. The INTOP_LEAVE_CATCH though it just a regular branch, so it seems this one should stay.

[noahfalk]

It seems that IsAddrWithinMethodIncludingFunclet would work for the interpreted code too.

I don't see it getting called right now? For interpretter stepping I expect the call chain looks like:

InterpreterWalker::Init
InterpreterStepHelper::SetupStep
DebuggerStepper::TrapInterpreterCodeStep
DebuggerStepper::TrapStep

IsAddrWithinMethodIncludingFunclet gets called in DebuggerStepper::TrapStep on the non-interpreter part of the code path whereas the interpreter case veers off into TrapInterpreterCodeStep.

Of course even if IsAddrWithinMethodIncludingFunclet isn't being called now we could always change the code to start invoking it within SetupStep. Doing so seems like it adds unneeded complexity though. Ultimately our goal is that step over operations should trace into the code for finally clauses but shouldn't trace into normal method calls. For jitted code we use the same assembly call opcode for both so extra logic is necessary to discriminate which call instructions represent funclet/finally and which represent the normal method call. For the interpreter it doesn't sound like we have that ambiguity. INTOP_FINALLY is already distinct from INTOP_CALL so we can report it as WALK_BRANCH up-front rather than reporting it as WALK_CALL, then classifying the called IP as a funclet, then treating WALK_CALL on a funclet as if we had specified WALK_BRANCH.

[kotlarmilos]

The defensive guard this PR adds in ShouldContinueStep doesn't seem to fire in real use case. We can introduce it later if we start hitting scenarios where the stepper stops on EH dispatch without source mapping

[Copilot]

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 2

[kotlarmilos]

Closing PR per #126954 (comment)