Uh oh!
There was an error while loading. Please reload this page.
Fix interpreter SetIP breakpoint handling - #131784
Conversation
Preserve destination breakpoint bypasses across SetIP and emit debugger mappings for catch and filter entries with an exception on the evaluation stack. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 285cb880-7c44-45a2-a8bb-61eb764db71c
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Tagging subscribers to this area: @JulieLeeMSFT, @BrzVlad, @janvorli |
There was a problem hiding this comment.
Pull request overview
Updates CoreCLR interpreter debugging behavior to correctly honor breakpoint bypass state across SetIP (so the destination’s saved opcode executes exactly once), and improves debugger mapping emission for catch/filter entries where the IL stack is intentionally non-empty (exception object present).
Changes:
- Preserve a destination breakpoint bypass across
SetIPcontext mutation insideInterpBreakpoint, restoring it only when the new IP matches the saved bypass address. - Consume a matching breakpoint bypass before notifying the debugger (pre-callback) in the
INTOP_BREAKPOINTdispatch path to avoid redundant callbacks and ensure single-step execution of the saved opcode. - Emit IL->native mapping entries for filter/catch funclet entry IL offsets even when the IL stack isn’t empty, tagging them with
ICorDebugInfo::SOURCE_TYPE_INVALIDinstead of omitting the mapping.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/vm/interpexec.cpp | Adjusts interpreter breakpoint/bypass flow to correctly preserve and consume bypasses around debugger SetIP and callbacks. |
| src/coreclr/interpreter/compiler.cpp | Emits IL->native mappings for filter/catch entrypoints with non-empty IL stack using SOURCE_TYPE_INVALID rather than skipping the entry. |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
| // created for the destination before resuming at the new context. | ||
| if ((GetIP(&ctx) != (PCODE)ip) || (GetSP(&ctx) != (DWORD64)pFrame)) | ||
| { | ||
| if (GetIP(&ctx) == (PCODE)savedBypassAddress) |
There was a problem hiding this comment.
In which scenario does this happen ? Say the debugger wants to continue execution at a different ip. Then it would just set the ip to the new location and it just starts executing from there. What would be the point of setting a bypass. If the target instruction is a breakpoint, sounds like it should just execute it normally.
Maybe the debugger still wants to execute the original instruction that was trapped, before dispatching to the new location ? In that case, isn't savedBypassAddress pointing to the original breakpoint location ? Or is savedBypassAddress always pointing to the new target with the opcode from the other trapping instruction, suggesting that the debugger has enough information to always set the bypass to the right values. In which case, why are we doing this equality check, should we just set the bypass unconditionally, unifying with the code below ?
There was a problem hiding this comment.
In which scenario does this happen ?
This is for the case when SetIP moves execution to a place that already has a breakpoint. After debugger resumes we would hit the breakpoint again.
If the target instruction is a breakpoint, sounds like it should just execute it normally.
I'm not sure we can because we need to exit through ThrowResumeAfterCatchException to move to the new IP, thus skipping the regular breakpoint bypass path.
Maybe the debugger still wants to execute the original instruction that was trapped, before dispatching to the new location ?
I'm not sure that's what debugger want, I would expect that if execution is changed via SetIP, we just move to the new location.
should we just set the bypass unconditionally, unifying with the code below ?
We only need the bypass in case we go to breakpoint with SetIP other case will execute normally.
There was a problem hiding this comment.
I'm not sure that's what debugger want, I would expect that if execution is changed via SetIP, we just move to the new location.
Tom's reply from below seems to suggest this is actually the case, meaning that the debugger needs to first execute the original instruction before resuming to another location. If that is the case, I would have expected unconditionally storing like this:
pThreadContext->m_bypassAddress = GetIP(&ctx);
pThreadContext->m_bypassOpcode = savedBypassOpcode;
Also, isn't savedBypassAddress always equal to the original ip ? Why would it be different ? If they are the same, then doing comparison with one above and then with the other below just adds difficulty in following the code logic.
There was a problem hiding this comment.
Tom's reply from below seems to suggest this is actually the case, meaning that the debugger needs to first execute the original instruction before resuming to another location.
When the debugger stops at an instruction because it hit a breakpoint, the instruction at the current IP has not executed yet.
If SetIP changes the IP, we do not execute the original instruction first because execution moves to the destination.
Also, isn't savedBypassAddress always equal to the original ip ?
It is the original IP on normal continue. However, if SetIP targets another breakpoint, it is the destination IP. If the destination is not patched, if the destination is not patched, it will be NULL.
Why would it be different ?
savedBypassAddress will point to the new destination breakpoint because, once we have continued, we have already looked up the patch at the new IP.
There was a problem hiding this comment.
I believe things are starting to make sense now. Some conclusions that need validation
- the key expected flow logic is that, if we resume to an opcode that has a breakpoint, we don't trigger the breakpoint again, we just execute the opcode at that address and implicitly continue execution. This is what was counter intuitive to me.
- within
FirstChanceNativeException, the debugger sets both the IP, as well as the bypass info on the interp thread context for the destination IP. This destination IP can be either the ip of the breaking instruction or some other IP. - given
ProcessAnyPendingEvalsexecutes other managed code, the original bypass state gets lost so we have to save it.ProcessAnyPendingEvalsshould be able to change the ip again. Does it also set the bypass for the new destination ? From what I search through the code I suspect it might not - what happens if
ProcessAnyPendingEvalswill ask to resume to an ip that is different from originalsavedBypassAddress. It will not add the bypass if that location contains a breakpoint. Does this mean we don't support ip changes done from the eval execution ?
| InterpBreakpoint(ip, pFrame, stack, pInterpreterFrame); | ||
| int32_t bypassOpcode = 0; | ||
| if (pThreadContext->HasBypass(ip, &bypassOpcode)) |
There was a problem hiding this comment.
It is not clear to me why we want to execute the bypass byte code. Do we do that for JITted code too? I would expect that in native code, if I set a breakpoint at say a division instruction, we would not execute that instruction until we resume after breakpoint. Why would the interpreter be different?
There was a problem hiding this comment.
This bypass dispatch was intended for a case where SetIP redirects to a location with another breakpoint. In that case, when we resume execution via ThrowResumeAfterCatchException, we would hit the breakpoint at SetIP location, bypassing it here we can dispatch to the underlying opcode instead of hitting another breakpoint.
I thought this would be safe, as I don't expect we would have more than one bypass set at given ip and so if there is a bypass already set when we hit INTOP_BREAKPOINT it must have come from SetIP.
There was a problem hiding this comment.
Do we do that for JITted code too?
Yes we do have the same patch-skip concept for JITted code, implemented at the native instruction level.
I would expect that in native code, if I set a breakpoint at say a division instruction, we would not execute that instruction until we resume after breakpoint. Why would the interpreter be different?
Correct. We do not execute the patched instruction when stopping at the breakpoint. The bypass is installed when the debugger resumes execution and executes the saved original instruction once. With SetIP, if the destination contains a breakpoint patch, the interpreter needs the same behavior; otherwise, it immediately reports that destination breakpoint instead of executing the requested instruction.
Summary
Validation
Stepping.SetIPTestandStepping.SetIPTestwithUnvaildPos(2/2).Note
This pull request description was generated by GitHub Copilot.