Handle debugger patches in interpreter continuation checks - #134500
matouskozak wants to merge 1 commit into
Conversation
Validate continuation resume and suspend opcodes through the existing debugger patch lookup. Read a full interpreter opcode under the controller lock when a patch has already been removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a0bb491-72e1-47b5-9b03-0e3794b410a5
|
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.
Copilot review overview
🔵 Needs a closer look
Interpreter opcodes may be truncated on ARM and WASM before unpatched lookup.
Review effort: Lite
Findings: None
What changed in this PR
This pull request updates interpreter continuation checks to account for debugger breakpoint patches and full-width opcode reads.
Changes:
- Resolves patched opcodes before continuation assertions.
- Reads interpreter opcode slots under the controller lock.
| File | Summary |
|---|---|
src/coreclr/vm/interpexec.cpp |
Uses unpatched opcodes for continuation assertions. |
src/coreclr/debug/ee/controller.cpp |
Handles interpreter-specific opcode reads. |
| // And before it should be an INTOP_HANDLE_CONTINUATION_SUSPEND opcode | ||
| _ASSERTE(*ip == INTOP_HANDLE_CONTINUATION_RESUME); | ||
| _ASSERTE(*(ip-3) == INTOP_HANDLE_CONTINUATION_SUSPEND); | ||
| _ASSERTE(GetUnpatchedInterpreterOpcode(ip) == INTOP_HANDLE_CONTINUATION_RESUME); |
There was a problem hiding this comment.
Seems a bit weird to have a breakpoint in place of the continuation resume opcode. This opcode is basically restoring the state of the async method so, if you break here, you probably have invalid state of locals. I believe this could use a closer look.
There was a problem hiding this comment.
At the moment, the InterpreterStepHelper for INTOP_HANDLE_CONTINUATION_SUSPEND opcode, places the breakpoint on the next one which is the RESUME. These are just debugger steps and do not determine whether we do a full breakpoint stop.
We could possibly improve the stepper by adding a dedicated handling for when we encounter INTOP_HANDLE_CONTINUATION_SUSPEND and inspect the interpreter frame and based on continuation object determine where to put the next break.
Summary
A stepper breakpoint can replace
INTOP_HANDLE_CONTINUATION_RESUMEwithINTOP_BREAKPOINT.CHECK_FOR_CONTINUATIONchecks the raw opcode before normal breakpoint dispatch, so Checked builds can assert at a valid resume address.Note
This change and PR description were prepared with GitHub Copilot.