[interp] Fix doubled interpreter frames in DAC/DBI stack walks#126953
[interp] Fix doubled interpreter frames in DAC/DBI stack walks#126953kotlarmilos wants to merge 5 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes two interpreter-specific debugger stack-walk issues in CoreCLR that caused duplicated frames and incorrect source line mapping when unwinding interpreter call frames.
Changes:
- Adjust
StackFrameIteratorto advance the explicit frame chain pastInterpreterFrameafter theDummyCallerIPtransition to native code, preventing the debugger path from re-entering the interpreter chain and duplicating frames. - Adjust interpreter unwinding to set the caller IP to
(pFrame->ip - 1)so return-address mapping lands within the call instruction range (aligning with JIT-style return-address adjustment).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/vm/stackwalk.cpp | Skips past InterpreterFrame in the explicit frame chain after transitioning out of the last interpreted frame, avoiding duplicate interpreter frame reporting in debugger walks. |
| src/coreclr/vm/eetwain.cpp | Adjusts the unwound interpreter caller IP by -1 to correct non-leaf frame line mapping in stack traces/debugger views. |
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Fix two issues in the StackFrameIterator for FEATURE_INTERPRETER: 1. Double-frame bug (stackwalk.cpp): After the DummyCallerIP transition from the last interpreter frame to native code, advance m_crawl.pFrame past the InterpreterFrame. Without this, the DAC/DBI debugger path (which starts with context already pointing into the interpreter chain) would encounter the InterpreterFrame again and re-enter the chain, reporting each interpreter frame twice. The GC path is unaffected because it already advances pFrame when entering the chain via the explicit frame handler. 2. Line number off-by-one (eetwain.cpp): In VirtualUnwindInterpreterCallFrame, subtract 1 from pFrame->ip when unwinding to a parent frame. The interpreter stores ip after advancing past the call instruction (ip += N), so the raw IP points to the start of the next instruction. Subtracting 1 places the IP within the call instruction, ensuring the debug info maps to the correct IL offset and source line for non-leaf frames. This is analogous to how JIT return addresses are adjusted for native code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8113c17 to
123c90a
Compare
The stack walker should return the real IP. IP adjustment for call sites belongs on the consumer side (EH already does it; debugger-side handling for the interpreter line-number off-by-one is tracked separately). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // frame, because the stack frame iterator assumes that when it is walking interpreted frames, it has | ||
| // already processed the interpreter frame. Without this skip, the stack walk would end up walking the | ||
| // interpreted frames twice. | ||
| m_crawl.GotoNextFrame(); |
There was a problem hiding this comment.
The normal invariant enforced by NextRaw() is that m_crawl moves past the InterpreterFrame before calling ProcessCurrentFrame() with a CONTEXT pointing at interpreted code. Also in general it is the caller's job to advance the Frame, not ProcessCurrentFrame() (the comments on this method say this is called after we've stopped). I'd suggest:
- In StackFrameIterator::Init() assert that the CONTEXT doesn't point at interpreted code.
- In StackFrameIterator::ResetRegDisp - there is already a bunch of logic that tries to start the Frame pointer at the correct position in the Frame chain. Currently that logic uses stack pointer comparisons but it probably needs to be updated to handle CONTEXTs that reference interpreter code. I'm guessing we'd need to map the interpreter code to its logically associated InterpreterFrame, then advance the Frame pointer to the next Frame in the chain.
- Remove this code here because now ResetRegDisp() will have initialized the state correctly.
There was a problem hiding this comment.
Done - Init asserts the CONTEXT isn't interpreted, ResetRegDisp reads the owning InterpreterFrame* from the CONTEXT's first-arg register and advances m_crawl.pFrame past it before ProcessCurrentFrame runs.
Move the doubled-interpreter-frames fix from ProcessCurrentFrame to ResetRegDisp so the StackFrameIterator starts from a correct state when DAC/DBI seeds it with a CONTEXT pointing into interpreted code. - Init: assert the CONTEXT does not reference interpreted code; that path is for OS-thread-top contexts and only DAC/DBI seeds interpreter contexts (via ResetRegDisp). - ResetRegDisp: when CONTEXT references interpreted code, locate the owning InterpreterFrame whose InterpMethodContextFrame chain contains the SP, and advance m_crawl.pFrame past it. - ProcessCurrentFrame: drop the post-hoc workaround that skipped the re-encountered InterpreterFrame; no longer needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When the StackFrameIterator is seeded from a CONTEXT pointing into interpreted code, InterpreterFrame::SetContextToInterpMethodContextFrame already writes the owning InterpreterFrame pointer into the first-arg register. Read it from the CONTEXT instead of searching the explicit Frame chain for an InterpMethodContextFrame whose address matches SP. Drops the chain search and removes the implicit assumption that the seeded context always belongs to the topmost InterpreterFrame, which also generalizes correctly to mid-stack seeds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
When DAC/DBI seeds StackFrameIterator from a CONTEXT pointing into interpreted code, m_crawl.pFrame is initialized to the thread's top explicit Frame, which is the InterpreterFrame that owns the executing InterpMethodContextFrame chain.
ResetRegDisp reads the owning InterpreterFrame* from the CONTEXT's first-arg register and advances m_crawl.pFrame past it before ProcessCurrentFrame runs.
Tests
Fixes the following interpreter debugger test failures:
StackWalking.NestedExceptionStackWalking.ChildParentTest