Implement DacDbi cDAC APIs CreateRefWalk, DeleteRefWalk, WalkRefs

[rcj1]

Implement these APIs. Debug validation only on handles because of existing delta w/parity on GC stack refs.

[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

This PR adds cDAC-managed implementations for the DacDbi reference-walking APIs (CreateRefWalk/DeleteRefWalk/WalkRefs), updates the corresponding native DacDbi interface signatures to remove the walkFQ parameter, and adds dump-based validation tests (cross-checking results against GC handles and stack-walk reference enumeration).

Changes:

• Implement managed ref-walk enumeration plumbing in Microsoft.Diagnostics.DataContractReader.Legacy (including a new RefWalk helper).
• Update DacDbi interop shapes (managed COM projection + native IDL/headers + DI usage) to match the revised reference-walk API signature.
• Extend stack-walk reference data to expose IsInteriorPointer, and add dump-based tests for handle-only and stack reference walks.

Show a summary per file

File	Description
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiRefWalkDumpTests.cs	Adds dump-based integration tests for ref-walk APIs vs GC handle table and stack-walk references.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs	Introduces DacGcReference + CorGCReferenceType and updates COM method signatures for ref walking.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/Helpers/RefWalk.cs	Adds managed iterator implementing the ref-walk behavior over handles and stack references.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements CreateRefWalk/DeleteRefWalk/WalkRefs in the cDAC DacDbi implementation (with DEBUG parity hooks).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_1.cs	Plumbs IsInteriorPointer into emitted StackReferenceData.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/GC/StackRefData.cs	Adds IsInteriorPointer tracking to internal stack ref capture data.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/GC/GcScanContext.cs	Sets IsInteriorPointer based on GC_CALL_INTERIOR flags while recording stack refs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IStackWalk.cs	Adds a new public IsInteriorPointer property to StackReferenceData.
src/coreclr/inc/dacdbi.idl	Updates the native DacDbi interface signature for CreateRefWalk (removes walkFQ).
src/coreclr/debug/inc/dacdbiinterface.h	Updates the native interface declaration + comments to match the new signature.
src/coreclr/debug/di/rspriv.h	Renames stored state from mEnumStacksFQ to mEnumStacks after walkFQ removal.
src/coreclr/debug/di/process.cpp	Updates DAC CreateRefWalk call site to match the new signature.
src/coreclr/debug/daccess/dacdbiimpl.h	Updates DacDbiInterfaceImpl and DacRefWalker signatures (removes walkFQ).
src/coreclr/debug/daccess/dacdbiimpl.cpp	Implements the updated CreateRefWalk signature and updates DacRefWalker ctor accordingly.

Copilot's findings

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IStackWalk.cs:44

• StackReferenceData is a public type and this adds a new public property. The PR description doesn't link an api-approved issue / proposal for the new public surface, which is normally required for additions to public APIs. If this is intended to be internal-only plumbing between cDAC assemblies, consider keeping it non-public (e.g., make the property internal and add InternalsVisibleTo for the legacy consumer) or link the approved API review issue.

public class StackReferenceData
{
    public bool HasRegisterInformation { get; init; }
    public bool IsInteriorPointer { get; init; }
    public int Register { get; init; }
    public int Offset { get; init; }
    public TargetPointer Address { get; init; }
    public TargetPointer Object { get; init; }

• Files reviewed: 14/14 changed files
• Comments generated: 5

[Copilot]

Copilot's findings

• Files reviewed: 14/14 changed files
• Comments generated: 5

[Copilot]

Copilot's findings

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

[github-actions]

🤖 Copilot Code Review — PR #129345

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: This PR implements three cDAC DacDbi APIs (CreateRefWalk, DeleteRefWalk, WalkRefs) that were previously stubbed as E_NOTIMPL, removes the dead walkFQ parameter, and adds IsInteriorPointer plumbing needed for correct stack reference reporting. The motivation is well-justified — mWalkFQ was stored but never read, and the FQ iteration loop in DacRefWalker::Next() was dead code (mFQStart/mFQEnd/mFQCurr were always PTR_NULL). The sole caller passed the same value for both walkStacks and walkFQ.

Approach: The implementation follows established cDAC patterns cleanly. RefWalk uses the IEnum<T> + GCHandle lifecycle pattern (consistent with HeapWalk). The generator-based Walk() is natural and readable. The DEBUG cross-validation is correctly scoped to handle-prefix parity only (sensibly skipping stack references where ordering may differ between cDAC and legacy).

Summary: ✅ LGTM. The code is correct, follows established patterns, and the earlier review feedback (compile errors with pointer/array mismatch, test mask bug) has been addressed in subsequent commits. Minor robustness suggestions below are non-blocking and consistent with existing native behavior.

---

Detailed Findings

✅ walkFQ Removal — Safe and complete

Verified that:

• mWalkFQ was stored but never read by Init(), Next(), or any other method.
• Init() never initializes mFQStart/mFQEnd/mFQCurr (always PTR_NULL), so the FQ loop in Next() was dead code.
• The sole caller (CordbRefEnum::Next) passed mEnumStacksFQ for both walkStacks and walkFQ.
• All 6 files referencing walkFQ have been updated consistently (dacdbiimpl.cpp/h, process.cpp, rspriv.h, dacdbiinterface.h, dacdbi.idl).

---

✅ DacGcReference Struct Layout — Correct

The managed struct at FieldOffset values {0, 8, 8, 16, 24} matches the native DacGcReference in dacdbistructures.h:

• vmDomain (8 bytes) + pObject/objHnd union (8 bytes) + dwType (uint, 4 bytes + 4 padding) + i64ExtraData (8 bytes) = 32 bytes total.

The pObject/objHnd overlay at offset 8 is intentional and correct — the native struct uses a union at that position.

---

✅ Interior Pointer Handling — Correctly wired end-to-end

The new IsInteriorPointer property flows from GcScanFlags.GC_CALL_INTERIOR through GcScanContext.GCEnumCallback/GCReportCallback → StackRefData → StackReferenceData → RefWalk.WalkStacks(). The logic if (IsInteriorPointer || Address == Null) → pObject | 1 correctly mirrors native DacStackReferenceWalker behavior: interior pointers, frame-reported references, and enregistered vars are reported as direct object pointers (low bit set), while regular stack slots are reported by handle address.

---

✅ Handle Walking Logic — Correct

GetRequestedHandleTypes() correctly maps CorGCReferenceType flags to HandleType values, filters against supported types, and TryMapHandle correctly reverses the mapping including RefCounted → strong/weak split based on refcount and Dependent → secondary pointer as extra data.

---

✅ Resource Lifecycle — Correct

CreateRefWalk allocates a GCHandle via IEnum<T>.GetHandle(). DeleteRefWalk disposes the enumerator and frees the GCHandle. The test helper uses try/finally to ensure DeleteRefWalk runs. This matches the HeapWalk pattern exactly.

---

✅ Tests — Well-structured and correct

• WalkRefs_StrongHandles_MatchHandleTable validates exact handle-set equality against IGC.GetHandles.
• WalkRefs_StacksOnly_MatchStackReferenceWalk correctly uses (CorGCReferenceType)0 as the handle mask to walk stacks only, and validates count parity (filtering CDAC_DEFERRED_FRAME markers).
• Both use [ConditionalTheory] + [MemberData] following existing DumpTest conventions.

---

✅ API Surface Concerns — Not applicable per cDAC instructions

Per .github/instructions/cdac.instructions.md (.NET 11 dev branch):

• StackReferenceData.IsInteriorPointer is on an IContract implementation — "New APIs on implementations of IContract... do NOT need to go through API review."
• DacGcReference and CorGCReferenceType are internal DacDbi types — "The DacDbi COM interface is internal and unstable. MUST NOT flag changes to DacDbi as breaking changes."

---

💡 Minor — Defensive *pHandle initialization in CreateRefWalk

If new RefWalk(...) or GetHandle() throws, *pHandle is left uninitialized (the caller observes whatever was there before). While the COM convention says out params are only meaningful on S_OK, and the native implementation has the same pattern, adding *pHandle = 0; immediately after the null check would prevent misuse by callers that don't check HRESULT. Non-blocking — consistent with existing native behavior.

---

💡 Minor — Per-handle allocation in TryMapHandle (follow-up)

_gc.GetHandleTypes([handle.Type])[0] allocates a single-element array per handle for type resolution. If HandleData.Type values correspond directly to HandleType values, a direct cast (HandleType)handle.Type would avoid the per-handle allocation. Not performance-critical for diagnostics code — suitable for a follow-up.

Generated by Code Review for issue #129345 · ● 10.1M · ◷

[max-charlamb]

We need to thread through resolveInteriorPointers. If I recall correctly, ISOSDacInterface does not want to resolve interior pointers, but ICorDebug does.

Currently StackWalk_1.WalkStackReferences hardcodes GcScanContext scanContext = new(_target, resolveInteriorPointers: false);

[max-charlamb]

Also if it wasn't clear, the StackWalk_1 doesn't support interior refs at all currently, but I still think it makes sense to use the APIs as if it were.

[rcj1]

I thought the idea was to implement this

[max-charlamb]

It is, but we haven't done it yet.

[rcj1]

Yes, I thought you wanted it in this PR. Do you not?

[max-charlamb]

Threading through resolveInteriorPointers, yes. But actually dealing with them is more complex:

        if (flags.HasFlag(GcScanFlags.GC_CALL_INTERIOR) && ResolveInteriorPointers)
        {
            // TODO(stackref): handle interior pointers
            // https://github.com/dotnet/runtime/issues/125728
            throw new NotImplementedException();
        }

[rcj1]

I understand that, and I would be willing to implement it.

[noahfalk]

This looks good to me. I'm fine if you want to add that interior pointer functionality in this PR or in a separate one.

Also fyi I filed an issue tracking the pre-existing missing finalizer queue enumeration functionality that this PR made more apparent: #129559. I don't think it is a high priority to fix given nobody noticed it was missing for at least a decade.

EDIT: Correction - it was noticed six years ago: #39333.

[noahfalk]

I see this test for strong handles and the one below for stack only but it didn't look like any other kind of handle has coverage yet.