[cDAC] Fix x86 stackwalking bug

[rcj1]

Found during internal diagnostic testing.

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates the cDAC x86 GCInfo decoder to compute the outermost base frame pointer using the full stack frame size (including callee-saved register spills), aligning the managed implementation with the native gc_unwind_x86.inl behavior and fixing stackwalking in EBP-framed methods.

Changes:

• Adds a computed stack-size value that includes callee-saved register spill space.
• Uses the computed stack size when calculating the outermost base FP for x86 EBP frames.

Show a summary per file

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/X86/GCInfo.cs	Computes total stack size (raw + saved regs) and uses it in GetOutermostBaseFP to correct x86 stackwalking.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Copilot]

Copilot's findings

Suppressed comments (2)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/X86/GCInfo.cs:53

• Adding public uint StackSize { get; set; } introduces new public surface area on X86GCInfo even though it’s only consumed internally (the only usage is in the new dump test). Consider keeping this as a private/internal implementation detail (e.g., a private readonly field computed in the ctor and used by GetOutermostBaseFP) and have the test validate taAmbientESP directly without relying on a new public API.

    public uint RawStackSize { get; set; }
    public uint StackSize { get; set; }

src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiStackWalkDumpTests.cs:200

• This test’s main verification is the computed ambient SP (taAmbientESP). The extra decoder.StackSize assertion forces the production code to expose a new property solely for test purposes. You can keep the regression coverage by computing the expected stack size locally and removing the decoder.StackSize dependency.

                    uint expectedStackSize = decoder.RawStackSize
                        + uint.PopCount((uint)decoder.SavedRegsMask) * (uint)Target.PointerSize;
                    Assert.True(expectedStackSize > decoder.RawStackSize);
                    Assert.Equal(expectedStackSize, decoder.StackSize);

• Files reviewed: 2/2 changed files
• Comments generated: 0 new

[Copilot]

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

Suppressed comments (1)

src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiStackWalkDumpTests.cs:147

• This test explicitly loads the StackWalk heap dump, but the test class still overrides DumpType to "full" (so the other tests in this file that call InitializeDumpTest(config) will try to load full/.../StackWalk.dmp). The StackWalk debuggee is configured to generate only heap dumps (DumpTests/Debuggees/StackWalk/StackWalk.csproj has <DumpTypes>Heap</DumpTypes>), so those DumpType="full" tests appear to be skipped (no dump expected) rather than executed.

Consider aligning the class-level DumpType with the debuggee’s dump type (heap), and only using dumpType: "full" on individual tests that truly require full dumps, so the existing DacDbi stack-walk tests actually run.

    public unsafe void GetStackWalkCurrentFrameInfo_X86HandlerFrame_IncludesSavedRegistersInAmbientSP(TestConfiguration config)
    {
        InitializeDumpTest(config, DebuggeeName, dumpType: "heap");

• Files reviewed: 2/2 changed files
• Comments generated: 0 new

[rcj1]

/ba-g helix jobs