Add cDAC tests and implementation for GetGenerationTable and GetFinalizationFillPointers by noahfalk · Pull Request #124674 · dotnet/runtime

noahfalk · 2026-02-20T20:43:22Z

Implement the four ISOSDacInterface8 methods in cDAC SOSDacImpl:

GetGenerationTable / GetGenerationTableSvr
GetFinalizationFillPointers / GetFinalizationFillPointersSvr

Add test infrastructure:

TestPlaceholderTarget.Builder: fluent builder that owns MockMemorySpace, accumulates types/globals, and wires contracts via TestContractRegistry.
GCHeapBuilder + extension methods (AddGCHeapWks/AddGCHeapSvr): configure GC mock data via Action and build directly into the target.
TestContractRegistry: Dictionary<Type, Lazy> replacement for Mock in the builder path.

Add tests:

3 contract-level tests in GCTests.cs (x4 arch = 12)
6 SOSDacImpl-level tests in SOSDacInterface8Tests.cs (x4 arch = 24)

Add documentation:

README.md files for cdac/, Legacy/, and tests/ directories
Copilot instruction to search for READMEs along the path hierarchy

Fixes #124692
Fixes #124693
Fixes #124694
Fixes #124695

…izationFillPointers Implement the four ISOSDacInterface8 methods in cDAC SOSDacImpl: - GetGenerationTable / GetGenerationTableSvr - GetFinalizationFillPointers / GetFinalizationFillPointersSvr Add test infrastructure: - TestPlaceholderTarget.Builder: fluent builder that owns MockMemorySpace, accumulates types/globals, and wires contracts via TestContractRegistry. - GCHeapBuilder + extension methods (AddGCHeapWks/AddGCHeapSvr): configure GC mock data via Action<GCHeapBuilder> and build directly into the target. - TestContractRegistry: Dictionary<Type, Lazy<IContract>> replacement for Mock<ContractRegistry> in the builder path. Add tests: - 3 contract-level tests in GCTests.cs (x4 arch = 12) - 6 SOSDacImpl-level tests in SOSDacInterface8Tests.cs (x4 arch = 24) Add documentation: - README.md files for cdac/, Legacy/, and tests/ directories - Copilot instruction to search for READMEs along the path hierarchy Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

dotnet-policy-service · 2026-02-20T20:44:34Z

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 implements four ISOSDacInterface8 methods in the cDAC SOSDacImpl layer for querying GC generation tables and finalization fill pointers, along with comprehensive test infrastructure and documentation.

Changes:

Implements GetGenerationTable, GetGenerationTableSvr, GetFinalizationFillPointers, and GetFinalizationFillPointersSvr in SOSDacImpl.cs by delegating to the existing IGC contract
Introduces TestPlaceholderTarget.Builder pattern with fluent API and TestContractRegistry to replace Mock<ContractRegistry> for more maintainable test setup
Adds GCHeapBuilder with extension methods (AddGCHeapWks/AddGCHeapSvr) to configure GC mock data declaratively
Provides 36 test cases total (3 contract-level + 6 SOSDacImpl-level × 4 architectures each) with proper sign-extension handling
Documents the cDAC architecture, test patterns, and SOSDacImpl implementation conventions in new README files

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/TestPlaceholderTarget.cs	Removes Moq dependency; adds fluent `Builder` class and `TestContractRegistry` for programmatic test target construction
src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.GC.cs	New file: GCHeapBuilder config class and extension methods to set up workstation/server GC heap mock data
src/native/managed/cdac/tests/GCTests.cs	New file: 3 contract-level tests verifying IGC.GetHeapData() correctly reads generation table and fill pointers
src/native/managed/cdac/tests/SOSDacInterface8Tests.cs	New file: 6 SOSDacImpl-level tests validating buffer sizing protocol, HResult codes, and pointer sign-extension
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs	Implements 4 ISOSDacInterface8 methods with proper validation, error handling, and debug cross-validation
src/native/managed/cdac/tests/README.md	New documentation: test architecture, builder patterns, sign-extension gotchas, and mock descriptor guidelines
src/native/managed/cdac/README.md	New documentation: cDAC architecture overview, project structure, and integration testing workflow
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/README.md	New documentation: SOSDacImpl implementation patterns, HResult conventions, and buffer sizing protocol
.github/copilot-instructions.md	Adds instruction to search for README files in directory hierarchy before making changes

Copilot · 2026-02-20T20:48:00Z

src/native/managed/cdac/tests/SOSDacInterface8Tests.cs

+        {
+            Assert.Equal(SignExtend(fillPointers[i], arch), (ulong)ptrs[i]);
+        }
+    }


Consider adding test cases for when the buffer size is insufficient (e.g., cFillPointers=3 when 7 are needed). The implementation should return S_FALSE and set *pNeeded to the required count, but this scenario is not explicitly tested. This would improve test coverage of the buffer sizing protocol.

Suggested change

}

}

[Theory]

[ClassData(typeof(MockTarget.StdArch))]

public void GetFinalizationFillPointersSvr_InsufficientBuffer_ReturnsSFalseAndNeededCount(MockTarget.Architecture arch)

{

var generations = new GCHeapBuilder.GenerationInput[]

{

new() { StartSegment = 0x1A00_0000, AllocationStart = 0x1A00_1000, AllocContextPointer = 0x1A00_2000, AllocContextLimit = 0x1A00_3000 },

new() { StartSegment = 0x1B00_0000, AllocationStart = 0x1B00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },

new() { StartSegment = 0x1C00_0000, AllocationStart = 0x1C00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },

new() { StartSegment = 0x1D00_0000, AllocationStart = 0x1D00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },

};

ulong[] fillPointers = [0x1111, 0x2222, 0x3333, 0x4444, 0x5555, 0x6666, 0x7777];

ISOSDacInterface8 dac8 = new SOSDacImpl(

new TestPlaceholderTarget.Builder(arch)

.AddGCHeapSvr(gc => gc

.SetGenerations(generations)

.SetFillPointers(fillPointers), out var heapAddr)

.Build(),

legacyObj: null);

uint needed;

ClrDataAddress* smallBuffer = stackalloc ClrDataAddress[3];

int hr = dac8.GetFinalizationFillPointersSvr((ClrDataAddress)heapAddr, 3, smallBuffer, &needed);

Assert.Equal(S_FALSE, hr);

Assert.Equal(7u, needed);

}

Copilot · 2026-02-20T20:48:01Z

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs

+#if DEBUG
+        if (_legacyImpl8 is not null)
+        {
+            uint pNeededLocal;
+            int hrLocal = _legacyImpl8.GetGenerationTable(cGenerations, pGenerationData, &pNeededLocal);
+            Debug.Assert(hrLocal == hr, $"cDAC: {hr:x}, DAC: {hrLocal:x}");
+        }


The debug cross-validation should also verify that the *pNeeded value matches between cDAC and legacy DAC, not just the HResult. Add an assertion like: if (pNeeded != null) Debug.Assert(*pNeeded == pNeededLocal); This follows the pattern used elsewhere in the file (e.g., line 933) and helps catch data mismatches during testing.

Copilot · 2026-02-20T20:48:01Z

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs

+#if DEBUG
+        if (_legacyImpl8 is not null)
+        {
+            uint pNeededLocal;
+            int hrLocal = _legacyImpl8.GetFinalizationFillPointers(cFillPointers, pFinalizationFillPointers, &pNeededLocal);
+            Debug.Assert(hrLocal == hr, $"cDAC: {hr:x}, DAC: {hrLocal:x}");
+        }


The debug cross-validation should also verify that the *pNeeded value matches between cDAC and legacy DAC, not just the HResult. Add an assertion like: if (pNeeded != null) Debug.Assert(*pNeeded == pNeededLocal); This follows the pattern used elsewhere in the file (e.g., line 933) and helps catch data mismatches during testing.

Copilot · 2026-02-20T20:48:01Z

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs

+#if DEBUG
+        if (_legacyImpl8 is not null)
+        {
+            uint pNeededLocal;
+            int hrLocal = _legacyImpl8.GetGenerationTableSvr(heapAddr, cGenerations, pGenerationData, &pNeededLocal);
+            Debug.Assert(hrLocal == hr, $"cDAC: {hr:x}, DAC: {hrLocal:x}");
+        }


The debug cross-validation should also verify that the *pNeeded value matches between cDAC and legacy DAC, not just the HResult. Add an assertion like: if (pNeeded != null) Debug.Assert(*pNeeded == pNeededLocal); This follows the pattern used elsewhere in the file (e.g., line 933) and helps catch data mismatches during testing.

Copilot · 2026-02-20T20:48:01Z

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs

+        {
+            uint pNeededLocal;
+            int hrLocal = _legacyImpl8.GetFinalizationFillPointersSvr(heapAddr, cFillPointers, pFinalizationFillPointers, &pNeededLocal);
+            Debug.Assert(hrLocal == hr, $"cDAC: {hr:x}, DAC: {hrLocal:x}");


The debug cross-validation should also verify that the *pNeeded value matches between cDAC and legacy DAC, not just the HResult. Add an assertion like: if (pNeeded != null) Debug.Assert(*pNeeded == pNeededLocal); This follows the pattern used elsewhere in the file (e.g., line 933) and helps catch data mismatches during testing.

Suggested change

Debug.Assert(hrLocal == hr, $"cDAC: {hr:x}, DAC: {hrLocal:x}");

Debug.Assert(hrLocal == hr, $"cDAC: {hr:x}, DAC: {hrLocal:x}");

if (pNeeded is not null)

{

Debug.Assert(*pNeeded == pNeededLocal);

}

Copilot · 2026-02-20T20:48:02Z

src/native/managed/cdac/tests/SOSDacInterface8Tests.cs

+    public void GetGenerationTable_ReturnsCorrectData(MockTarget.Architecture arch)
+    {
+        var generations = new GCHeapBuilder.GenerationInput[]
+        {
+            new() { StartSegment = 0x1A00_0000, AllocationStart = 0x1A00_1000, AllocContextPointer = 0x1A00_2000, AllocContextLimit = 0x1A00_3000 },
+            new() { StartSegment = 0x1B00_0000, AllocationStart = 0x1B00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },
+            new() { StartSegment = 0x1C00_0000, AllocationStart = 0x1C00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },
+            new() { StartSegment = 0x1D00_0000, AllocationStart = 0x1D00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },
+        };
+
+        ulong[] fillPointers = [0x1000, 0x2000, 0x3000, 0x4000, 0x5000, 0x6000, 0x7000];
+
+        ISOSDacInterface8 dac8 = new SOSDacImpl(
+            new TestPlaceholderTarget.Builder(arch)
+                .AddGCHeapWks(gc => gc
+                    .SetGenerations(generations)
+                    .SetFillPointers(fillPointers))
+                .Build(),
+            legacyObj: null);
+
+        // First call with cGenerations=0 to query needed count
+        uint needed;
+        int hr = dac8.GetGenerationTable(0, null, &needed);
+        Assert.Equal(S_FALSE, hr);
+        Assert.Equal(4u, needed);
+
+        // Second call with sufficient buffer
+        DacpGenerationData* genData = stackalloc DacpGenerationData[4];
+        hr = dac8.GetGenerationTable(4, genData, &needed);
+        Assert.Equal(S_OK, hr);
+
+        for (int i = 0; i < generations.Length; i++)
+        {
+            ulong expectedStartSeg = SignExtend(generations[i].StartSegment, arch);
+            ulong expectedAllocStart = SignExtend(generations[i].AllocationStart, arch);
+            ulong expectedAllocCtxPtr = SignExtend(generations[i].AllocContextPointer, arch);
+            ulong expectedAllocCtxLim = SignExtend(generations[i].AllocContextLimit, arch);
+            Assert.Equal(expectedStartSeg, (ulong)genData[i].start_segment);
+            Assert.Equal(expectedAllocStart, (ulong)genData[i].allocation_start);
+            Assert.Equal(expectedAllocCtxPtr, (ulong)genData[i].allocContextPtr);
+            Assert.Equal(expectedAllocCtxLim, (ulong)genData[i].allocContextLimit);
+        }
+    }


Consider adding test cases for when the buffer size is insufficient (e.g., cGenerations=2 when 4 are needed). The implementation should return S_FALSE and set *pNeeded to the required count, but this scenario is not explicitly tested. This would improve test coverage of the buffer sizing protocol.

Copilot · 2026-02-20T20:48:02Z

src/native/managed/cdac/tests/SOSDacInterface8Tests.cs

+    public void GetFinalizationFillPointers_ReturnsCorrectData(MockTarget.Architecture arch)
+    {
+        var generations = new GCHeapBuilder.GenerationInput[]
+        {
+            new() { StartSegment = 0xAA00_0000, AllocationStart = 0xAA00_1000, AllocContextPointer = 0xAA00_2000, AllocContextLimit = 0xAA00_3000 },
+            new() { StartSegment = 0xBB00_0000, AllocationStart = 0xBB00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },
+            new() { StartSegment = 0xCC00_0000, AllocationStart = 0xCC00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },
+            new() { StartSegment = 0xDD00_0000, AllocationStart = 0xDD00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },
+        };
+
+        ulong[] fillPointers = [0x1111, 0x2222, 0x3333, 0x4444, 0x5555, 0x6666, 0x7777];
+
+        ISOSDacInterface8 dac8 = new SOSDacImpl(
+            new TestPlaceholderTarget.Builder(arch)
+                .AddGCHeapWks(gc => gc
+                    .SetGenerations(generations)
+                    .SetFillPointers(fillPointers))
+                .Build(),
+            legacyObj: null);
+
+        // First call with cFillPointers=0 to query needed count
+        uint needed;
+        int hr = dac8.GetFinalizationFillPointers(0, null, &needed);
+        Assert.Equal(S_FALSE, hr);
+        Assert.Equal(7u, needed);
+
+        // Second call with sufficient buffer
+        ClrDataAddress* ptrs = stackalloc ClrDataAddress[7];
+        hr = dac8.GetFinalizationFillPointers(7, ptrs, &needed);
+        Assert.Equal(S_OK, hr);
+
+        for (int i = 0; i < fillPointers.Length; i++)
+        {
+            Assert.Equal(SignExtend(fillPointers[i], arch), (ulong)ptrs[i]);
+        }
+    }


Consider adding test cases for when the buffer size is insufficient (e.g., cFillPointers=3 when 7 are needed). The implementation should return S_FALSE and set *pNeeded to the required count, but this scenario is not explicitly tested. This would improve test coverage of the buffer sizing protocol.

Copilot · 2026-02-20T20:48:02Z

src/native/managed/cdac/tests/SOSDacInterface8Tests.cs

+    public void GetGenerationTableSvr_ReturnsCorrectData(MockTarget.Architecture arch)
+    {
+        var generations = new GCHeapBuilder.GenerationInput[]
+        {
+            new() { StartSegment = 0x1A00_0000, AllocationStart = 0x1A00_1000, AllocContextPointer = 0x1A00_2000, AllocContextLimit = 0x1A00_3000 },
+            new() { StartSegment = 0x1B00_0000, AllocationStart = 0x1B00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },
+            new() { StartSegment = 0x1C00_0000, AllocationStart = 0x1C00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },
+            new() { StartSegment = 0x1D00_0000, AllocationStart = 0x1D00_1000, AllocContextPointer = 0, AllocContextLimit = 0 },
+        };
+
+        ulong[] fillPointers = [0x1000, 0x2000, 0x3000, 0x4000, 0x5000, 0x6000, 0x7000];
+
+        ISOSDacInterface8 dac8 = new SOSDacImpl(
+            new TestPlaceholderTarget.Builder(arch)
+                .AddGCHeapSvr(gc => gc
+                    .SetGenerations(generations)
+                    .SetFillPointers(fillPointers), out var heapAddr)
+                .Build(),
+            legacyObj: null);
+
+        uint needed;
+        int hr = dac8.GetGenerationTableSvr((ClrDataAddress)heapAddr, 0, null, &needed);
+        Assert.Equal(S_FALSE, hr);
+        Assert.Equal(4u, needed);
+
+        DacpGenerationData* genData = stackalloc DacpGenerationData[4];
+        hr = dac8.GetGenerationTableSvr((ClrDataAddress)heapAddr, 4, genData, &needed);
+        Assert.Equal(S_OK, hr);
+
+        for (int i = 0; i < generations.Length; i++)
+        {
+            Assert.Equal(SignExtend(generations[i].StartSegment, arch), (ulong)genData[i].start_segment);
+            Assert.Equal(SignExtend(generations[i].AllocationStart, arch), (ulong)genData[i].allocation_start);
+            Assert.Equal(SignExtend(generations[i].AllocContextPointer, arch), (ulong)genData[i].allocContextPtr);
+            Assert.Equal(SignExtend(generations[i].AllocContextLimit, arch), (ulong)genData[i].allocContextLimit);
+        }
+    }


Consider adding test cases for when the buffer size is insufficient (e.g., cGenerations=2 when 4 are needed). The implementation should return S_FALSE and set *pNeeded to the required count, but this scenario is not explicitly tested. This would improve test coverage of the buffer sizing protocol.

noahfalk · 2026-02-20T20:49:05Z

.github/copilot-instructions.md

 Before completing, use the `code-review` skill to review your code changes. Any issues flagged as errors or warnings should be addressed before completing.

+Before making changes to a directory, search for `README.md` files in that directory and its parent directories up to the repository root. Read any you find — they contain conventions, patterns, and architectural context relevant to your work.
+


@stephentoub - Since earlier you mentioned context window sizes were a concern, any thoughts on using something like this as a mechanism for progressive discovery? I wanted to add more component-specific guidance (see READMEs elsewhere in this PR as an example) but I didn't want to bloat the context window for prompts working in unrelated parts of the code.

noahfalk · 2026-02-20T20:53:47Z

src/native/managed/cdac/tests/README.md

+`0xFFFFFFFF_AA000000` on 32-bit. Either use values below `0x80000000` or account
+for sign extension in assertions.
+
+## Creating a test target


@max-charlamb @rcj1 - I adjusted the pattern for creating the test target a bit to something that looked a little more streamlined and readable (IMO). I'm happy to get copilot to refactor the other unit tests to match but wanted to confirm everyone liked the style first.

noahfalk · 2026-02-20T20:56:39Z

src/native/managed/cdac/README.md

+[Loader](/docs/design/datacontracts/Loader.md) ·
+[RuntimeTypeSystem](/docs/design/datacontracts/RuntimeTypeSystem.md)
+
+## Integration testing with SOS


@max-charlamb @rcj1 - I didn't notice any instructions for doing the SOS cDAC testing. I may have missed them but below is what Copilot infered from the code and its still a TODO for me to confirm if this is right.

Copilot AI review requested due to automatic review settings February 20, 2026 20:43

github-actions bot added the area-Diagnostics-coreclr label Feb 20, 2026

dotnet-policy-service bot assigned noahfalk Feb 20, 2026

Copilot started reviewing on behalf of noahfalk February 20, 2026 20:44 View session

Copilot AI reviewed Feb 20, 2026

View reviewed changes

noahfalk commented Feb 20, 2026

View reviewed changes

		Before completing, use the `code-review` skill to review your code changes. Any issues flagged as errors or warnings should be addressed before completing.

		Before making changes to a directory, search for `README.md` files in that directory and its parent directories up to the repository root. Read any you find — they contain conventions, patterns, and architectural context relevant to your work.

Comments

Conversation

noahfalk commented Feb 20, 2026 • edited by steveisok Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Feb 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

noahfalk Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

noahfalk Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

noahfalk Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

noahfalk commented Feb 20, 2026 •

edited by steveisok

Loading