[cdac] Implement IGCInfo for x86; consolidate decoder under GCInfo contract#129456
Open
max-charlamb wants to merge 4 commits into
Open
[cdac] Implement IGCInfo for x86; consolidate decoder under GCInfo contract#129456max-charlamb wants to merge 4 commits into
max-charlamb wants to merge 4 commits into
Conversation
…ntract Fixes the cDAC `GetCodeHeaderData` failure that surfaced as `Unable to get codeHeader information` when SOS ran `!clru` against an IL stub MethodDesc on Windows x86, .NET 11, with cDAC enabled (the new default behavior introduced by dotnet/diagnostics#5874). x86 uses a fundamentally different GC info encoding from every other architecture: the legacy bit-packed `InfoHdr` byte-stream format from `src/coreclr/vm/gc_unwind_x86.inl` and `src/coreclr/inc/gcdecoder.cpp` (`USE_GC_INFO_DECODER` is defined for every target except x86, see `eetwain.h:34`). The GCInfo contract had no x86 implementation, so it fell through to `default(GCInfo)` and threw `NotImplementedException` from the interface's default `DecodePlatformSpecificGCInfo`. Any SOS path that needed method size on x86 (`!clru`, `GetCodeHeaderData`, `GetMethodRegionInfo`) failed. The cDAC already had a substantial x86 `InfoHdr` decoder under `Contracts/StackWalk/Context/X86/GCInfoDecoding/`, used by the x86 stack walker. This change relocates that decoder under the `GCInfo` contract so there is a single canonical x86 GC info implementation shared between SOS callers and the stack walker. Changes: * Move `Contracts/StackWalk/Context/X86/GCInfoDecoding/*` -> `Contracts/GCInfo/X86/*` (6 files, tracked as renames). Rename namespace `StackWalkHelpers.X86` -> `GCInfoHelpers.X86`. * Rename the moved class `GCInfo` -> `X86GCInfo` to avoid collision with the empty `Contracts.GCInfo` IGCInfo fallback struct. * Make `relativeOffset` ctor arg optional. Implement `IGCInfoDecoder` directly on `X86GCInfo`: `GetCodeLength` / `GetStackBaseRegister` / `GetSizeOfStackParameterArea` are wired up. `GetInterruptibleRanges` and `EnumerateLiveSlots` throw `NotSupportedException` (future work, needed for `!gcroot` / `!clrstack -l` etc.). * Add `GCInfoX86_1` IGCInfo for x86; register it in `CoreCLRContracts.cs` for `RuntimeInfoArchitecture.X86`. * Update `ExecutionManagerCore.GetStackParameterSize` to delegate to `IGCInfo.GetSizeOfStackParameterArea` (one source of truth). * `X86Unwinder` continues to construct `X86GCInfo` directly because it needs offset-bound state (`IsInProlog`/`IsInEpilog`/`PushedArgSize`) not exposed through `IGCInfoDecoder`. Tests: * Add `VarargPInvoke_GetCodeHeaderDataForILStub_ReturnsMethodSize` regression test that asserts the IL stub path returns S_OK with non-zero MethodSize. Runs against the existing cdac-dump-helix `windows_x86` matrix on every PR. Docs: * Update `docs/design/datacontracts/GCInfo.md` to reflect partial x86 support and document `GetSizeOfStackParameterArea`. * Update `docs/design/datacontracts/StackWalk.md` x86 section to point at the consolidated decoder location. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an x86 implementation of the cDAC IGCInfo contract by reusing the existing managed x86 InfoHdr decoder (relocated under the GCInfo contract) so SOS callers can query method size / stack parameter area on Windows x86, and includes a dump-based regression test plus documentation updates.
Changes:
- Register a new x86
IGCInfoimplementation (GCInfoX86_1) and wire it up forRuntimeInfoArchitecture.X86. - Consolidate x86 GCInfo decoding under the GCInfo contract and update the x86 unwinder and ExecutionManager to use the shared decoder / contract APIs.
- Add a dump-based regression test for
ISOSDacInterface.GetCodeHeaderDataon an IL stub and update design docs.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/DumpTests/StackWalkDumpTests.cs | Adds regression coverage validating GetCodeHeaderData returns a non-zero method size for an IL stub. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/CoreCLRContracts.cs | Registers GCInfoX86_1 for x86 targets. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/X86/X86Unwinder.cs | Updates x86 unwinder to use the relocated/renamed X86GCInfo type. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/X86/InfoHdr.cs | Moves x86 GCInfo decoder support type under GCInfoHelpers.X86 namespace. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/X86/GCTransition.cs | Moves x86 GCInfo decoder support type under GCInfoHelpers.X86 namespace. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/X86/GCInfoTargetExtensions.cs | Moves x86 GCInfo decoder support extensions under GCInfoHelpers.X86 namespace. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/X86/GCInfo.cs | Renames GCInfo → X86GCInfo, makes relativeOffset optional, and implements IGCInfoDecoder queries needed by SOS/ExecutionManager. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/X86/GCArgTable.cs | Moves x86 GCInfo decoder support type under GCInfoHelpers.X86 namespace. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/X86/CallPattern.cs | Moves x86 GCInfo decoder support type under GCInfoHelpers.X86 namespace. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/GCInfoX86_1.cs | Introduces the x86 IGCInfo implementation backed by X86GCInfo. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs | Delegates x86 stack parameter size computation to the IGCInfo contract for a single source of truth. |
| docs/design/datacontracts/StackWalk.md | Documents the consolidated location and sharing model for the x86 GCInfo decoder. |
| docs/design/datacontracts/GCInfo.md | Updates contract API docs (but the intro still incorrectly claims x86 is unsupported). |
Open
3 tasks
Tweaks the second intro paragraph in GCInfo.md to say x86 is "partially supported" rather than "not currently supported", reflecting the IGCInfo implementation added in the first commit. Addresses dotnet#129456 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address PR feedback: the IGCInfo contract was conflating two distinct concepts under one API name. GcInfoDecoder::GetSizeOfStackParameterArea returns the outgoing-argument scratch area (_fixedStackParameterScratchArea, platform-specific, used by the GC scanner). EECodeManager::GetStackParameterSize returns the x86 __stdcall callee-popped argument size (used by the debugger to adjust SP after return). Keep IGCInfo.GetSizeOfStackParameterArea as the GcInfoDecoder scratch-area accessor (consumed by GcScanner). On x86 it now throws NotSupportedException since x86 has no separate scratch-area concept. Add IGCInfo.GetCalleePoppedArgumentsSize mirroring EECodeManager::GetStackParameterSize: 0 by default, x86 returns varargs ? 0 : argCount * ptrSize. ExecutionManagerCore.GetStackParameterSize now routes through this new method. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Note
This PR was authored with assistance from GitHub Copilot.
Summary
Fixes a cDAC
GetCodeHeaderDatafailure that surfaced asUnable to get codeHeader informationwhen SOS ran!clruagainst an IL stub MethodDesc on Windows x86 with cDAC enabled (the new default behavior on .NET 11 introduced by dotnet/diagnostics#5874).The CI failure that motivated this is
SOSMethodTests.VarargPInvokeInteropMDon x86 in dotnet/diagnostics:!IP2MDreturned the IL stub MethodDesc correctly, but the immediate follow-up!clru <MD>printed onlyUnable to get codeHeader information. x64 / arm64 / .NET 8/9/10 were unaffected.Root cause
x86 uses a fundamentally different GC info encoding from every other architecture: the legacy bit-packed
InfoHdrbyte-stream format fromsrc/coreclr/vm/gc_unwind_x86.inlandsrc/coreclr/inc/gcdecoder.cpp(USE_GC_INFO_DECODERis defined for every target except x86, seeeetwain.h:34).The cDAC
GCInfocontract registered IGCInfo implementations for X64/Arm64/Arm/LoongArch64/RiscV64 but not X86 -- so on x86 it fell through todefault(GCInfo)and threwNotImplementedExceptionfrom the interface's defaultDecodePlatformSpecificGCInfo. Any SOS path that needed method size on x86 (!clru,GetCodeHeaderData,GetMethodRegionInfo) failed.Approach
The cDAC already had a substantial x86
InfoHdrdecoder underContracts/StackWalk/Context/X86/GCInfoDecoding/, used by the x86 stack walker. Rather than write a parallel decoder, this PR relocates that existing decoder under theGCInfocontract so there is one canonical x86 GC info implementation shared between SOS callers and the stack walker -- mirroring how the other architectures' decoders are structured.Changes
Contracts/StackWalk/Context/X86/GCInfoDecoding/*→Contracts/GCInfo/X86/*(6 files, tracked as renames). Rename namespaceStackWalkHelpers.X86→GCInfoHelpers.X86.GCInfo→X86GCInfoto avoid collision with the emptyContracts.GCInfoIGCInfo fallback struct.relativeOffsetctor arg optional. ImplementIGCInfoDecoderdirectly onX86GCInfo:GetCodeLength/GetStackBaseRegister/GetSizeOfStackParameterAreaare wired up.GetInterruptibleRangesandEnumerateLiveSlotsthrowNotSupportedException(future work, needed for!gcroot/!clrstack -letc.).GCInfoX86_1IGCInfo for x86; register it inCoreCLRContracts.csforRuntimeInfoArchitecture.X86.ExecutionManagerCore.GetStackParameterSizeto delegate toIGCInfo.GetSizeOfStackParameterArea(one source of truth).X86Unwindercontinues to constructX86GCInfodirectly because it needs offset-bound state (IsInProlog/IsInEpilog/PushedArgSize) not exposed throughIGCInfoDecoder.Tests
VarargPInvoke_GetCodeHeaderDataForILStub_ReturnsMethodSizeregression test incdac/tests/DumpTests/StackWalkDumpTests.cs-- asserts the IL stub path returns S_OK with non-zeroMethodSize. Runs against the existing cdac-dump-helixwindows_x86matrix on every PR (no pipeline changes needed).SOSMethodTests.VarargPInvokeInteropMDx86 .NET 11prev6 cDAC: 4/4 pass after this fix; failed before.Docs
docs/design/datacontracts/GCInfo.md-- intro now reflects partial x86 support;GetSizeOfStackParameterAreaAPI documented; per-method status notes for x86.docs/design/datacontracts/StackWalk.md-- x86 section points at the consolidated decoder location and explains how it's shared.Out of scope (future work)
GetInterruptibleRangesandEnumerateLiveSlotsfor x86. The underlying transition data is decoded but the adapter to the cDACIGCInfoDecodershape is not wired up yet. This is what's needed to unblock!gcroot,!clrstack -l,!peon x86 cDAC. The two pre-existing[SkipOnArch("x86", "GCInfo decoder does not support x86")]markers inStackReferenceDumpTests.csshould become removable once that lands.