Add ISOSDacInterface18 for GCInfo inspection APIs

[max-charlamb]

Note

This content was generated with AI assistance.

ISOSDacInterface18 for GCInfo inspection APIs

Add ISOSDacInterface18 with 4 COM methods exposing GC info data through the cDAC for the SOS !GCInfo command:

• GetGCInfoHeader -- header fields (version, code size, prolog, stack base register, GS cookie, PSP sym, generics context)
• GetGCInfoInterruptibleRanges -- code regions where GC can interrupt execution
• GetGCInfoSafePoints -- specific offsets with point-in-time GC slot liveness
• GetGCInfoSlotLifetimes -- unified slot lifetime ranges (register + stack combined via SOSGCSlotLifetime with IsRegister discriminator)

IGCInfo contract changes

• Add GetHeader, GetSafePoints, GetSlotLifetimes to IGCInfo/IGCInfoDecoder
• Consolidate GetStackBaseRegister and GetSizeOfStackParameterArea into GCInfoHeader (accessed via GetHeader()). These fields require the same decode depth (DecodePoints.ReversePInvoke) as the rest of the header, so there's no performance benefit to keeping them separate.
• Keep GetCodeLength as a standalone method -- it only decodes to DecodePoints.CodeLength, which is significantly cheaper than the full header decode. Used on the hot path by EEJitManager/InterpreterJitManager/ReadyToRunJitManager for method size lookups.
• Keep GetCalleePoppedArgumentsSize as a standalone method -- it returns 0 via a default interface method on non-x86 platforms without any decoding. Only x86 reads the ArgCount header field.
• Eagerly decode safe points in DecodeSafePoints, stored denormalized
• Single-pass chunk decoder for interruptible slot lifetimes (O(transitions), not O(codeLength))
• Safe-point-only methods emit [safePointOffset, safePointOffset+1) per live slot per safe point
• Implement x86 GCInfo support (GetHeader/GetSafePoints/GetSlotLifetimes)
• Add GenericsContextKind.None as default enum value
• Fix GSCookieValidRange -- only set when GSCookie is present
• Native DAC returns E_NOINTERFACE for ISOSDacInterface18 (cDAC-only interface)

Bug fix: ARM thumb bit in relative offset computation

EEJitManager.GetMethodInfo and InterpreterJitManager.GetMethodInfo computed relativeOffset as jittedCodeAddress.Value - codeStart.Value. On ARM (Thumb-2), TargetCodePointer.Value includes the thumb bit (bit 0 = 1), but codeStart (a TargetPointer from the nibble map) does not. This produced a relative offset that was off by 1, causing FindSafePoint and EnumerateLiveSlots to look up the wrong offset.

Fixed by stripping the thumb bit via CodePointerUtils.AddressFromCodePointer before computing the difference. ReadyToRunJitManager was already doing this correctly.

Companion PR

• WIP: Update SOS for unified ISOSDacInterface18 GCInfo API diagnostics#5898 -- SOS !GCInfo command updated to use ISOSDacInterface18 with legacy-compatible timeline output format

[Copilot]

Pull request overview

This PR introduces a new SOS-DAC COM interface (ISOSDacInterface18) intended to expose GCInfo inspection APIs (header, safe points, interruptible ranges, and slot lifetimes), alongside expanding the managed cDAC IGCInfo contract and decoder surface to project the same data.

Changes:

• Add ISOSDacInterface18 (IDL + prebuilt headers + C# GeneratedComInterface) and wire it up through DAC QueryInterface.
• Implement GCInfo inspection entrypoints in the native DAC (ClrDataAccess) using GcInfoDecoder / GcInfoDumper, and extend the managed GCInfo decoder/contracts with header/safe point/lifetime APIs.
• Extend GCInfo platform traits to include PSP symbol encoding base across architectures; add placeholder (E_NOTIMPL) stubs in the managed legacy SOS DAC implementation.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs	Adds ISOSDacInterface18 exposure but currently returns E_NOTIMPL for all new methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ISOSDacInterface.cs	Defines GCInfo structs and ISOSDacInterface18 managed COM declarations.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/X86/GCInfo.cs	Implements newly added IGCInfoDecoder members on x86 by throwing NotSupportedException.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/RISCV64GCInfoTraits.cs	Adds PSP symbol stack-slot encoding base.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/LoongArch64GCInfoTraits.cs	Adds PSP symbol stack-slot encoding base.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/InterpreterGCInfoTraits.cs	Adds PSP symbol stack-slot encoding base.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/IGCInfoTraits.cs	Extends traits interface with PSP_SYM_STACK_SLOT_ENCBASE.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/ARMGCInfoTraits.cs	Adds PSP symbol stack-slot encoding base.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/ARM64GCInfoTraits.cs	Adds PSP symbol stack-slot encoding base.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/AMD64GCInfoTraits.cs	Adds PSP symbol stack-slot encoding base.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/IGCInfoDecoder.cs	Expands decoder interface to include header/safe points/lifetimes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/GCInfoDecoder.cs	Adds decoding/projection for header/safe points/lifetimes (includes a correctness issue in safe-point lookup and a potentially expensive lifetime algorithm).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/GCInfo_1.cs	Plumbs new IGCInfo contract methods to the decoder implementation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IGCInfo.cs	Adds public contract types (header/lifetime records) and new IGCInfo methods.
src/coreclr/pal/prebuilt/inc/sospriv.h	Updates prebuilt SOS-DAC header with new interface and structs.
src/coreclr/pal/prebuilt/idl/sospriv_i.cpp	Adds GUID definitions for new SOS-DAC interfaces/enums.
src/coreclr/inc/sospriv.idl	Adds IDL definitions for GCInfo structs and ISOSDacInterface18.
src/coreclr/debug/daccess/request.cpp	Implements ClrDataAccess::GetGCInfo* methods and supporting helpers.
src/coreclr/debug/daccess/dacimpl.h	Extends ClrDataAccess inheritance and declares ISOSDacInterface17/18 methods.
src/coreclr/debug/daccess/daccess.cpp	Extends QueryInterface to support ISOSDacInterface17/18.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

[noahfalk]

data contract markdown changes need to go with this?

[max-charlamb]

Yes -- added in b7b98ad. docs/design/datacontracts/GCInfo.md now documents the new GetHeader/GetSafePoints/GetSlotLifetimes APIs and the GCInfoHeader/GCSlotLifetime/SpecialSlot/GenericsContextKind types.

Note

This reply was generated by GitHub Copilot.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "fb7566b303727fe9d21dcd2dacf7fb869c4d6068",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "8a5301832157c4740b8c85ee25086762b29af531",
  "last_reviewed_commit": "fb7566b303727fe9d21dcd2dacf7fb869c4d6068",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "8a5301832157c4740b8c85ee25086762b29af531",
  "last_recorded_worker_run_id": "29684012971",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "fb7566b303727fe9d21dcd2dacf7fb869c4d6068",
      "review_id": 4730639589
    }
  ]
}

[github-actions]

Holistic Review

Motivation: The motivation is sound. Exposing GC info header, interruptible ranges, safe points, and slot lifetimes through the cDAC lets the SOS !GCInfo command work against cDAC-based data access (companion dotnet/diagnostics#5898), and consolidating GetStackBaseRegister/GetSizeOfStackParameterArea into GetHeader() is a reasonable cleanup since they share the same decode depth. The ARM Thumb-bit fix addresses a real off-by-one bug.

Approach: The contract-layer design is clean: a unified GCSlotLifetime with an IsRegister discriminator, eager safe-point decoding, and a single-pass chunk decoder for interruptible lifetimes. However, the PR appears to stop short of wiring the new COM surface into the cDAC's SOS implementation, so the headline capability may not actually be reachable (see Detailed Findings).

Summary: ⚠️ Needs Human Review. The GCInfo contract changes, x86 support, and the Thumb-bit fix look correct and are well-documented. But ISOSDacInterface18 is declared without any implementation in SOSDacImpl, and the new GetSafePoints/GetSlotLifetimes contract methods have no consumers in this PR — a human should confirm whether the COM wiring is intentionally deferred to a follow-up or is a missing piece. There are also no unit tests for the substantial new decode logic.

---

Detailed Findings

⚠️ Completeness — ISOSDacInterface18 is declared but not implemented

The PR title and description state the cDAC implements ISOSDacInterface18 ("Native DAC returns E_NOINTERFACE... cDAC-only interface"), but I could not find an implementation:

• Legacy/ISOSDacInterface.cs adds the [GeneratedComInterface] ISOSDacInterface18 definition plus the SOSGCInfoHeader / SOSGCSlotLifetime / SOSCodeRange structs.
• SOSDacImpl (SOSDacImpl.cs) still only derives from ISOSDacInterface...ISOSDacInterface17 (line ~36). It is not extended to ISOSDacInterface18, and no GetGCInfoHeader / GetGCInfoInterruptibleRanges / GetGCInfoSafePoints / GetGCInfoSlotLifetimes method bodies exist anywhere in the managed cDAC tree.
• Consequently the new IGCInfo.GetSafePoints / IGCInfo.GetSlotLifetimes contract methods have no consumers in this PR.

Since [GeneratedComClass] only exposes interfaces listed in the class's base list, a QueryInterface for GUID 3dccf95b-... against the cDAC would return E_NOINTERFACE, so the companion SOS !GCInfo change would not be able to reach these APIs through the cDAC. Please confirm whether the SOSDacImpl implementation is intentionally deferred to a follow-up PR; if so, the PR description (which reads as though the interface is fully implemented here) should be adjusted, and the added IDL/prebuilt COM surface without a backing implementation should be called out as a staging step.

⚠️ Test coverage — no unit tests for the new decode paths

This PR adds a large amount of non-trivial decode logic: the single-pass interruptible chunk decoder, the safe-point-only live-state RLE/direct-bitmap decoder, DenormInterruptibleOffset, and a full x86 GetSlotLifetimes transition walk. None of these have dedicated unit tests. The only test change removes two [SkipOnArch("x86")] attributes in StackReferenceDumpTests.cs, which exercises the pre-existing x86 live-slot enumeration path via dump tests rather than the new lifetime/header/safepoint APIs. Given the density of bit-level offset arithmetic (e.g. the +7 & ~7u alignment, numBitsPerOffset/numBitsPerPointer handling, RLE run/skip toggling), targeted unit tests over known-good GC info blobs would substantially de-risk this. A human should weigh whether dump-test coverage alone is sufficient.

✅ ARM Thumb-bit fix — correct and consistent

EEJitManager.GetMethodInfo and InterpreterJitManager.GetMethodInfo now strip the Thumb bit via CodePointerUtils.AddressFromCodePointer(jittedCodeAddress, Target) before computing relativeOffset, matching what ReadyToRunJitManager already did. This correctly fixes the off-by-one on ARM (Thumb-2) that would misdirect FindSafePoint/EnumerateLiveSlots. Good catch, and applied consistently across both managers.

✅ Documentation — GCInfo.md kept in sync

The contract doc is updated to match IGCInfo.cs: GetHeader replaces the two removed methods, GetSafePoints/GetSlotLifetimes are added, the stale "not implemented for x86" notes are removed, and the new GCInfoHeader/GCSlotLifetime/SpecialSlot/GenericsContextKind records are documented field-by-field. This satisfies the cDAC doc-sync requirement.

💡 Minor — duplicated register-mapping helper on x86

X86/GCInfo.cs adds RegMaskToRegNum while the file already has RegMaskToRegisterNumber. Consider consolidating to a single helper to avoid the two mappings drifting.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 246 AIC · ⌖ 15.4 AIC · ⊞ 10K