Skip to content

cDAC: Add GetGCDescSeries contract API and continuation pretty-printing#127419

Merged
rcj1 merged 9 commits intomainfrom
copilot/implement-continuation-pretty-printing
Apr 30, 2026
Merged

cDAC: Add GetGCDescSeries contract API and continuation pretty-printing#127419
rcj1 merged 9 commits intomainfrom
copilot/implement-continuation-pretty-printing

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 25, 2026

Summary

Implements continuation pretty-printing in the cDAC, mirroring AsyncContinuationsManager::PrintContinuationName from asynccontinuations.h, and adds a new GetGCDescSeries API to the RuntimeTypeSystem contract.

Fixes the WebApp3 test failure seen in recent runtime-diagnostics legs such as this: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1395576

The GetGCDescSeries API accepts an optional numComponents parameter (defaulting to 0 for non-array types) and handles both regular (positive NumSeries) and value-class repeating (negative NumSeries) GCDesc layouts. For value-class GCDesc, the outer loop iterates numComponents times — callers must pass the actual element count to enumerate pointer runs across array elements.

Note

This PR was created with the assistance of GitHub Copilot (AI-generated content).

…ation pretty-printing

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6e988c50-2e06-4957-952d-9b624dd862ca

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Comment thread docs/design/datacontracts/RuntimeTypeSystem.md
Copilot AI review requested due to automatic review settings April 25, 2026 18:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 extends the cDAC RuntimeTypeSystem contract surface to expose GCDesc “series” (GC pointer runs) and uses that information to pretty-print dynamically-created continuation types in a way that mirrors the runtime’s AsyncContinuationsManager::PrintContinuationName.

Changes:

  • Add IRuntimeTypeSystem.GetGCDescSeries and implement it in RuntimeTypeSystem_1 to enumerate GC pointer runs from the GCDesc preceding a MethodTable.
  • Add continuation synthetic-name formatting to TypeNameBuilder using GetGCDescSeries.
  • Add a new ContinuationObject data descriptor/type entry plus mocks, tests, and contract documentation updates.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.RuntimeTypeSystem.cs Adds mock helpers to allocate MethodTables with GCDesc (regular + repeating/value-class layouts) and exposes ContinuationObjectSize.
src/native/managed/cdac/tests/MethodTableTests.cs Adds unit tests covering GetGCDescSeries behaviors across MethodTable/non-MethodTable, no-GC, regular series, and repeating series cases.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/TypeNameBuilder.cs Uses IsContinuation + GetGCDescSeries to generate the Continuation_<dataSize>[_<gcOffset>_<gcCount>]* synthetic name instead of (dynamicClass).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs Implements GetGCDescSeries by reading GCDesc memory preceding the MethodTable and normalizing sizes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/DataType.cs Adds DataType.ContinuationObject for retrieving continuation header sizing info from the contract descriptor.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs Adds the GetGCDescSeries(TypeHandle, uint objectSize) contract API.
src/coreclr/vm/datadescriptor/datadescriptor.inc Adds a ContinuationObject type descriptor entry so the reader can query its size.
docs/design/datacontracts/RuntimeTypeSystem.md Updates contract documentation to include the new API and describe GCDesc series layouts.

Comment thread src/native/managed/cdac/tests/MethodTableTests.cs
@rcj1 rcj1 requested review from max-charlamb and noahfalk April 25, 2026 19:41
@rcj1 rcj1 marked this pull request as ready for review April 25, 2026 19:42
@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.

@hoyosjs
Copy link
Copy Markdown
Member

hoyosjs commented Apr 27, 2026

Might be missing https://github.com/hoyosjs/runtime/blob/9ef63b95ace0796ca363bc53687ddbb670f1119b/src/coreclr/gc/gcdesc.h#L227-L231. NVM - looks like this is not stored inline with CGCDescSeries array

@hoyosjs
Copy link
Copy Markdown
Member

hoyosjs commented Apr 27, 2026

dotnet/diagnostics#5815

Copilot AI review requested due to automatic review settings April 28, 2026 23:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment thread docs/design/datacontracts/RuntimeTypeSystem.md Outdated
Comment thread docs/design/datacontracts/RuntimeTypeSystem.md Outdated
rcj1 and others added 2 commits April 29, 2026 11:33
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 29, 2026 18:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread src/native/managed/cdac/tests/MethodTableTests.cs
Comment thread src/native/managed/cdac/tests/MethodTableTests.cs
Comment thread docs/design/datacontracts/RuntimeTypeSystem.md Outdated
@github-actions

This comment has been minimized.

Copilot AI added a commit that referenced this pull request Apr 30, 2026
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e9402571-fdfe-4171-b9e1-eec00046cb3e

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@rcj1
Copy link
Copy Markdown
Contributor

rcj1 commented Apr 30, 2026

@copilot please update tests to fix #127419 (comment)

Copilot AI requested review from Copilot and removed request for Copilot April 30, 2026 02:38
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 30, 2026

@copilot please update tests to fix #127419 (comment)

Fixed in eddce51. The two failing tests now pass numComponents=1 to GetGCDescSeries (since value-class GCDesc requires at least one component to yield output). Both tests were also updated to use the correct array layout: startOffset relative to the object pointer (not including ObjHeaderSize), baseSize covering only the array header, and explicit componentSize/array flags matching the element structure.

Copilot finished work on behalf of rcj1 April 30, 2026 02:39
Comment thread docs/design/datacontracts/RuntimeTypeSystem.md Outdated
Copilot AI review requested due to automatic review settings April 30, 2026 14:00
@rcj1
Copy link
Copy Markdown
Contributor

rcj1 commented Apr 30, 2026

/ba-g comment only change

@rcj1 rcj1 merged commit 4ff20e6 into main Apr 30, 2026
22 of 99 checks passed
@rcj1 rcj1 deleted the copilot/implement-continuation-pretty-printing branch April 30, 2026 14:09
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #127419

Note

This review was generated by Copilot (Claude Opus 4.6) with corroborating analysis from GPT-5.3-Codex and Claude Haiku 4.5.

Holistic Assessment

Motivation: The PR adds GCDesc parsing and continuation type pretty-printing to the cdac (diagnostics DataContractReader). This enables the managed debugger to read GC pointer runs from memory and produce human-readable names for dynamically-created continuation method tables, matching the native PrintContinuationName implementation. The motivation is clear and justified.

Approach: The implementation correctly mirrors the native CGCDesc memory layout (both regular positive-numSeries and value-class negative-numSeries formats). The AppendContinuationName logic faithfully reproduces the native AsyncContinuationsManager::PrintContinuationName behavior. Tests are comprehensive.

Summary: ⚠️ Needs Human Review. The code is functionally correct — GCDesc parsing matches native layout, arithmetic is correct (uint truncation handles wrapping properly), and the continuation name matches native output. However, two robustness concerns around defensive coding and a test coverage gap warrant human attention.


Detailed Findings

✅ Correctness — GCDesc parsing logic matches native layout

The regular series path correctly reads from mtAddress - (3 + 2*i) * ptrSize (size then offset), and the value-class path correctly reads val_serie_item fields using half-pointer-width typed reads for endianness safety. Sign extension of numSeries is properly handled on both 32-bit ((long)(int)) and 64-bit ((long)) targets. The rawSeriesSize + objectSize arithmetic correctly exploits unsigned wrapping at pointer width, and the (uint) cast on the result truncates to 32 bits, giving correct results for both 32-bit and 64-bit targets.

✅ Correctness — AppendContinuationName matches native behavior

The managed implementation correctly uses sizeof(ContinuationObject) (== OFFSETOF__CORINFO_Continuation__data = 3×ptr + 8) as the continuation data offset. The formula seriesSize / pointerSize produces the correct GC pointer count because GetGCDescSeries already returns rawSeriesSize + baseSize (since objectSize == baseSize when numComponents == 0), matching the native (GetSeriesSize() + GetBaseSize()) / TARGET_POINTER_SIZE.

⚠️ Robustness — objectSize - pointerSize underflow risk in value-class path

File: RuntimeTypeSystem_1.cs, line 632

if (currentOffset > objectSize - pointerSize)
    yield break;

If malformed target data has objectSize < pointerSize (conceivable in a corrupted dump), this unsigned subtraction would underflow to a very large value, disabling the bounds check entirely. While objectSize = baseSize + numComponents * componentSize should always be ≥ 2×pointerSize in practice (baseSize includes ObjHeader + MT*), adding an early guard like if (objectSize < pointerSize) yield break; before the loop would make this robust against corrupted data. (Advisory — not merge-blocking, since this is a diagnostic tool reading existing well-formed targets in normal operation.)

⚠️ Robustness — Unbounded allocation from untrusted absNumSeries

File: RuntimeTypeSystem_1.cs, line 606

var seriesItems = new (uint Nptrs, uint Skip)[absNumSeries];

In a corrupted dump, numSeries could be a very large negative value (e.g., -2147483648), causing absNumSeries to be enormous and triggering an OOM. A sanity cap (e.g., if (absNumSeries > 1000) yield break;) would make this robust for diagnostic scenarios. (Advisory — similar patterns exist elsewhere in the codebase without caps, but flagging for awareness.)

💡 Test coverage — No integration test for AppendContinuationName

The PR has excellent unit tests for GetGCDescSeries covering regular series, value-class series, and array scenarios on both architectures. However, there's no end-to-end test that exercises AppendContinuationName through TypeNameBuilder to verify the produced name string matches the expected format (Continuation_<dataSize>[_<gcOffset>_<gcCount>]*). A test asserting the final string output would catch any future regression in the name-building logic. (Follow-up suggestion, not blocking.)

✅ Tests — GCDesc test coverage is thorough

Tests cover: non-MethodTable handles (empty), no GC pointers (empty), single regular series, multiple regular series ordering, single value-class series, multiple value-class items, regular series with array numComponents, and value-class repeating with array numComponents. Both 32-bit and 64-bit architectures are tested via [ClassData(typeof(MockTarget.StdArch))].

Generated by Code Review for issue #127419 ·

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.

7 participants