[clr-interp] Fix debugger MethodEnter counter and INTOP_DEBUG_METHOD_ENTER assert#127753
[clr-interp] Fix debugger MethodEnter counter and INTOP_DEBUG_METHOD_ENTER assert#127753kotlarmilos wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @BrzVlad, @janvorli, @kg |
…R assert Two small interpreter-debugger callback correctness fixes. In DispatchMethodEnter, g_cTotalMethodEnter is supposed to count every controller with MethodEnter enabled regardless of which thread will actually fire the trigger. The increment was inside the per-thread filter, so unbound and other-thread controllers were not counted, and the post-loop _ASSERTE(g_cTotalMethodEnter == count) fired spuriously. Move ++count out of the filter. The trigger itself remains gated on thread affinity. In the interpreter dispatch loop, INTOP_DEBUG_METHOD_ENTER asserts that the seq-point offset patched into the bytecode by OnMethodEnter is INTOP_DEBUG_SEQ_POINT. Once the debugger sets a user breakpoint at that sequence point the opcode is overwritten with INTOP_BREAKPOINT, which is also valid here, so the assert is relaxed to accept either. This fixes the following interpreter debugger test failures: - JMC.jmcChange - JMC.jmcDelegates - JMC.jmcFunceval - Async.AsyncStepInto Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3a75885 to
f0e5112
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes two debug-time assertions in the CoreCLR interpreter + debugger integration that were causing interpreter debugger test failures when MethodEnter controllers are thread-affinitized and when the debugger installs a breakpoint on the method-entry sequence point.
Changes:
- Fix
DispatchMethodEnter’s debug-only controller counting so it matchesg_cTotalMethodEnterregardless of per-thread filtering. - Update the interpreter
INTOP_DEBUG_METHOD_ENTERassertion to allow the entry seq-point opcode to be eitherINTOP_DEBUG_SEQ_POINTorINTOP_BREAKPOINT(when patched by the debugger).
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/interpexec.cpp | Allow INTOP_DEBUG_METHOD_ENTER’s target opcode to be a debugger-patched breakpoint as well as the original debug seq-point. |
| src/coreclr/debug/ee/controller.cpp | Count all MethodEnter-enabled controllers (even if not firing on the current thread) so the g_cTotalMethodEnter consistency assert holds. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
| { | ||
| if (p->m_fEnableMethodEnter) | ||
| { | ||
| ++count; |
There was a problem hiding this comment.
Isn't this change impacting non-interpreter targets as well. Wasn't this failing before ?
There was a problem hiding this comment.
Yes, the change applies to all targets. In the JIT path, the MethodEnter callback is called from JIT_DbgIsJustMyCode inside the stepped code, so the calling thread is almost always the same as the thread the stepper is bound to, and count matches g_cTotalMethodEnter by coincidence. The interpreter calls OnMethodEnter from INTOP_DEBUG_METHOD_ENTER on every method entry whenever any stepper has MethodEnter enabled.
Description
In
DispatchMethodEnter,g_cTotalMethodEnteris supposed to count every controller withMethodEnterenabled regardless of which thread will actually fire the trigger. The increment was inside the per-thread filter, so unbound controllers were not counted, and the post-loop_ASSERTE(g_cTotalMethodEnter == count)fired. Move++countout of the filter.In the interpreter dispatch loop,
INTOP_DEBUG_METHOD_ENTERasserts that the seq-point offset patched into the bytecode byOnMethodEnterisINTOP_DEBUG_SEQ_POINT. Once the debugger sets a user breakpoint at that sequence point the opcode is overwritten withINTOP_BREAKPOINT, which is also valid here, so the assert is updated to support such cases.This fixes the following interpreter debugger test failures:
JMC.jmcChangeJMC.jmcDelegatesJMC.jmcFuncevalAsync.AsyncStepInto