[clr-ios] Advance past owning InterpreterFrame at StackFrameIterator::Init call sites#127560
Open
kotlarmilos wants to merge 5 commits intodotnet:mainfrom
Open
[clr-ios] Advance past owning InterpreterFrame at StackFrameIterator::Init call sites#127560kotlarmilos wants to merge 5 commits intodotnet:mainfrom
StackFrameIterator::Init call sites#127560kotlarmilos wants to merge 5 commits intodotnet:mainfrom
Conversation
…Init call sites Follow-up to dotnet#126953. StackFrameIterator::Init asserts that the seed CONTEXT does not reference interpreted code, but that assert cannot hold on debugger paths whose CONTEXT was set up by InterpreterFrame::SetContextToInterpMethodContextFrame. Per the design discussed in dotnet#126953, callers must advance pStartFrame past the owning InterpreterFrame before constructing the iterator from an interpreted-IP CONTEXT. A new helper InterpreterFrame::TryGetOwningFrameFromContext returns the owning InterpreterFrame from the first-arg register slot when the IP lies in interpreted code, otherwise NULL. The three Init call sites that supply a CONTEXT (Thread::StackWalkFramesEx, ClrDataStackWalk::Init, DacDbiInterfaceImpl::IsThreadAtGCSafePlace) use the helper to compute pStartFrame. ResetRegDisp's existing inline interpreter handling also routes through the helper. The assert in Init now checks the actual caller-side invariant. When the IP is interpreted, m_crawl.pFrame must not be the owning InterpreterFrame. The next frame may legitimately be a different InterpreterFrame (for example, interp -> JIT -> interp). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @BrzVlad, @janvorli, @kg |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR interpreter/debugger stack-walking so that StackFrameIterator::Init can be seeded from a CONTEXT whose IP is in interpreted code, by requiring (and enforcing via assertions) that callers advance the start Frame past the owning InterpreterFrame. This prevents checked-build aborts when stackwalking interpreter-suspended threads while preserving the “tripwire” behavior for future call sites.
Changes:
- Introduces
InterpreterFrame::TryGetOwningFrameFromContext(T_CONTEXT*)to recover the owningInterpreterFramefrom the first-arg register when the IP is interpreted. - Updates
StackFrameIterator::Init/ResetRegDispand key debugger/DAC call sites to skip the owningInterpreterFramewhen starting from an interpreted-IPCONTEXT. - Adjusts the existing
Initassertion to validate the caller-side invariant (“start frame must not be the owningInterpreterFrame”) instead of asserting interpreted code can never occur.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/stackwalk.cpp | Advances pStartFrame past the owning InterpreterFrame for interpreted-IP contexts; updates iterator assertions and reuses the new helper in ResetRegDisp. |
| src/coreclr/vm/frames.h | Declares InterpreterFrame::TryGetOwningFrameFromContext and documents the intended calling protocol for stackwalk seeding. |
| src/coreclr/vm/frames.cpp | Implements TryGetOwningFrameFromContext by identifying interpreted IPs and extracting the owning frame from the first-arg register slot. |
| src/coreclr/debug/daccess/stack.cpp | Updates DAC stackwalk initialization to pass a start frame that skips the owning InterpreterFrame for interpreted-IP contexts. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Updates the DAC DBI “GC safe place” probe to seed stackwalks past the owning InterpreterFrame when needed. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 3
kotlarmilos
changed the title
kotlarmilos
changed the title
StackFrameIterator::Init call sites
noahfalk
reviewed
Apr 29, 2026
Address PR feedback from @noahfalk and @janvorli: rather than have every caller advance pStartFrame past the owning InterpreterFrame, do it inside StackFrameIterator::Init when the caller passes pFrame==NULL. - Init: when the CONTEXT references interpreted code, extract the owning InterpreterFrame from the first-arg register slot directly (no EECodeInfo lookup; we already have m_crawl.codeInfo populated by ProcessIp). If pFrame was NULL, auto-advance m_crawl.pFrame to the next Frame past the owner. If the caller passed an explicit pFrame, validate it is strictly past the owner (asserted with > since the Frame chain on this stack is laid out so a Frame closer to the leaf has a lower address than its caller's owning Frame). - ResetRegDisp: same inline fast-path; do not re-prove IsInterpretedCode via a helper that redoes EECodeInfo.Init. - Revert callsite advance in DacDbiInterfaceImpl::IsThreadAtGCSafePlace (now passes NULL), ClrDataStackWalk::Init (already passed NULL), and Thread::StackWalkFramesEx (no longer pre-advances). - Delete InterpreterFrame::TryGetOwningFrameFromContext helper and its declaration in frames.h; no remaining callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kotlarmilos
force-pushed
the
interp-stackwalkiter-init-callerfix
branch
from
bf6c98b to
936dd54
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #126953
Description
When a thread is suspended inside the interpreter,
InterpreterFrame::SetContextToInterpMethodContextFramerewrites its CONTEXT so that the IP points at the executing interpreted bytecode and the first-arg register holds the owningInterpreterFrame*. Debugger code paths that walk such a thread forwards this CONTEXT toStackFrameIterator::Init.Currently,
Initfails with_ASSERTE(!m_crawl.codeInfo.IsInterpretedCode()). The assert checks a property of the IP, but the IP points at interpreter code in the cases above, so the assert fires on every such walk.Approach
StackFrameIterator::Initresolves the start frame itself when the supplied CONTEXT references interpreted code, so debugger DAC callers no longer need to recompute it.cc @noahfalk @janvorli