Skip to content

Implement DacDbi cDAC APIs CreateRefWalk, DeleteRefWalk, WalkRefs#129345

Open
rcj1 wants to merge 4 commits into
mainfrom
copilot/implement-cdac-apis
Open

Implement DacDbi cDAC APIs CreateRefWalk, DeleteRefWalk, WalkRefs#129345
rcj1 wants to merge 4 commits into
mainfrom
copilot/implement-cdac-apis

Conversation

@rcj1

@rcj1 rcj1 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

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

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 12, 2026 19:23
@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 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 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

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copilot AI review requested due to automatic review settings June 13, 2026 00:06

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.

Copilot's findings

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

/// <summary>
/// Drives <see cref="DacDbiImpl.WalkRefs"/> to completion and returns every reference reported.
/// </summary>
private static unsafe List<DacGcReference> WalkAllRefs(DacDbiImpl dbi, bool walkStacks, CorGCReferenceType handleWalkMask, uint batchSize = 32)
Comment on lines +3536 to +3548
int hr = HResults.S_OK;
RefWalk? walk = null;
try
{
if (pHandle is null)
throw new NullReferenceException(nameof(pHandle));
walk = new RefWalk(_target, walkStacks != Interop.BOOL.FALSE, handleWalkMask);
*pHandle = (nuint)((IEnum<DacGcReference>)walk).GetHandle();
}
catch (System.Exception ex)
{
hr = ex.HResult;
}
Comment on lines +3594 to +3613
// Should be called repeatedly until it returns S_FALSE.
public int WalkRefs(nuint handle, uint count, [In, MarshalUsing(CountElementName = "count"), Out] DacGcReference[] refs, uint* pFetched)
{
RefWalk walk;
try
{
if (pFetched is null)
throw new NullReferenceException(nameof(pFetched));
if (handle == 0)
throw new ArgumentException("Handle is invalid.", nameof(handle));
GCHandle gcHandle = GCHandle.FromIntPtr((nint)handle);
if (gcHandle.Target is not RefWalk rw)
throw new ArgumentException("Handle does not reference a valid RefWalk instance.", nameof(handle));
walk = rw;
*pFetched = 0;
}
catch (System.Exception ex)
{
return ex.HResult;
}
Comment on lines 37 to 41
public class StackReferenceData
{
public bool HasRegisterInformation { get; init; }
public bool IsInteriorPointer { get; init; }
public int Register { get; init; }
@github-actions

This comment has been minimized.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 13, 2026 00:19

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.

Copilot's findings

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

Comment on lines +3540 to +3543
if (pHandle is null)
throw new NullReferenceException(nameof(pHandle));
walk = new RefWalk(_target, walkStacks != Interop.BOOL.FALSE, handleWalkMask);
*pHandle = (nuint)((IEnum<DacGcReference>)walk).GetHandle();
Comment on lines +3600 to +3608
if (pFetched is null)
throw new NullReferenceException(nameof(pFetched));
if (handle == 0)
throw new ArgumentException("Handle is invalid.", nameof(handle));
GCHandle gcHandle = GCHandle.FromIntPtr((nint)handle);
if (gcHandle.Target is not RefWalk rw)
throw new ArgumentException("Handle does not reference a valid RefWalk instance.", nameof(handle));
walk = rw;
*pFetched = 0;
Comment on lines +364 to +375
[Flags]
public enum CorGCReferenceType : uint
{
CorHandleStrong = 1 << 0,
CorHandleStrongPinning = 1 << 1,
CorHandleWeakShort = 1 << 2,
CorHandleWeakLong = 1 << 3,
CorHandleWeakRefCount = 1 << 4,
CorHandleStrongRefCount = 1 << 5,
CorHandleStrongDependent = 1 << 6,
CorReferenceStack = 0x80000001,
}
Comment on lines +133 to +141
[StructLayout(LayoutKind.Explicit)]
public struct DacGcReference
{
[FieldOffset(0)] public ulong vmDomain;
[FieldOffset(8)] public ulong pObject;
[FieldOffset(8)] public ulong objHnd;
[FieldOffset(16)] public CorGCReferenceType dwType;
[FieldOffset(24)] public ulong i64ExtraData;
}
@github-actions

Copy link
Copy Markdown
Contributor

🤖 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/GCReportCallbackStackRefDataStackReferenceDataRefWalk.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 ·

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.

3 participants