Skip to content

Remove extra x86 unwind from DBI#130011

Open
rcj1 wants to merge 3 commits into
dotnet:mainfrom
rcj1:frameinfo
Open

Remove extra x86 unwind from DBI#130011
rcj1 wants to merge 3 commits into
dotnet:mainfrom
rcj1:frameinfo

Conversation

@rcj1

@rcj1 rcj1 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

In the DBI, we unwind the stack walk one frame ahead when we need to generate a frame pointer on x86. This is because the frame pointer is derived from the caller context. I replaced this with inline physical unwind, and derived the frame pointer from the formula here:

pRD->PCTAddr = pRD->pCurrentContext->Esp - pCodeInfo->GetCodeManager()->GetStackParameterSize(pCodeInfo) - sizeof(DWORD);

We had unwound one ahead when we are on a runtime-unwindable or managed frame. In either case, we would unhijack the runtime-unwindable stub, or do one physical unwind, plus however many unwinds it took to get us to the next managed frame. Here, we stop after the first step to calculate the frame pointer. This is a behavioral change, if the caller is non-managed, but it should produce if anything more accurate / strictly ordered frame boundaries, especially in use cases such as GetStackRange.

The goal of this PR is to hide as much of the platform-specific details behind DacDbi as possible - we will no longer have to do a custom adjustment in DBI just for X86 targets. In addition, we consolidate the retrieval of general stack frame info into the DacDbi API GetStackWalkCurrentFrameInfo.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Copilot AI review requested due to automatic review settings June 30, 2026 01:15
@rcj1 rcj1 requested a review from jkotas June 30, 2026 01:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the DBI-side “unwind one frame ahead” mechanism used to derive an x86 frame pointer, and instead computes the x86 frame pointer inside DAC stackwalking logic (alongside related cleanup of the legacy COM/DDI surface and RS caching state).

Changes:

  • Removes the GetFramePointer method from the DacDbi COM/DDI surface and the managed legacy cDAC fallback layer.
  • Simplifies CordbStackWalk by deleting the “one frame ahead” caching/bookkeeping and relying on GetStackWalkCurrentFrameInfo to provide the needed frame pointer.
  • Adds x86-specific frame pointer computation in dacdbiimplstackwalk.cpp as part of GetFramePointerWorker / InitFrameData.
Show a summary per file
File Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs Removes managed DDI declaration for GetFramePointer.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Removes managed implementation/fallback for GetFramePointer.
src/coreclr/inc/dacdbi.idl Removes GetFramePointer from the COM interface and updates related comments.
src/coreclr/debug/inc/dacdbiinterface.h Removes GetFramePointer from the native interface contract docs.
src/coreclr/debug/di/rsstackwalk.cpp Deletes one-frame-ahead RS caching/unwind behavior and related error caching.
src/coreclr/debug/di/rspriv.h Removes RS bookkeeping fields tied to one-frame-ahead unwinding.
src/coreclr/debug/daccess/dacdbiimplstackwalk.cpp Introduces x86 frame pointer computation and uses it when filling Debugger_STRData.fp.
src/coreclr/debug/daccess/dacdbiimpl.h Removes GetFramePointer declaration from DAC DBI implementation header.

Copilot's findings

  • Files reviewed: 8/8 changed files
  • Comments generated: 4

Comment thread src/coreclr/debug/daccess/dacdbiimplstackwalk.cpp Outdated
Comment thread src/coreclr/debug/daccess/dacdbiimplstackwalk.cpp
Comment thread src/coreclr/debug/di/rsstackwalk.cpp Outdated
Comment thread src/coreclr/debug/di/rsstackwalk.cpp Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants