Use cdac for metadata serialization#131027
Conversation
rcj1
commented
Jul 19, 2026
rcj1
commented
- Remove the native path for reading metadata outside of DacDbi and introduce DacDbi APIs to size and fill a local metadata buffer that is generated by walking read-write metadata structures.
- Encapsulate this algorithm in the cDAC.
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR switches debugger metadata acquisition for read-write metadata from the legacy “remote MD structure deserialization” path to a DacDbi + cDAC-based serialization approach, removing the old custom data source infrastructure and associated layout/version plumbing.
Changes:
- Add DacDbi APIs to compute the size of, and fill, a serialized contiguous ECMA-335 metadata blob for a module’s writable (MDInternalRW) metadata.
- Move/encapsulate the read-write metadata reconstruction into the cDAC
EcmaMetadatacontract (with caching) and update the debugger to consume the serialized blob viaOpenScopeOnMemory. - Remove metadata custom data source / layout verification code paths and drop obsolete debugger “defines/version” fields and tests.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs | Updates unit tests and mock layouts after removing Debugger defines/version fields. |
| src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiDebuggerDumpTests.cs | Removes dump tests for DacDbi APIs that were deleted (defines/version). |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/StressTestApi/CdacStressApi.cs | Uses shared CorDbgHResults.ERROR_INSUFFICIENT_BUFFER constant. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs | Removes old methods; adds GetReadWriteMetadataSize / FillReadWriteMetadata. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs | Implements new read-write metadata sizing/filling via cDAC contract. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs | Switches insufficient-buffer HRESULT to shared constant. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Debugger.cs | Removes Debugger data descriptor fields for defines/version. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/EcmaMetadata_1.cs | Adds read-write metadata blob construction + caching; GetMetadata uses it for RW modules. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs | Updates DebuggerData projection to only include initialization state. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/CorDbHResults.cs | Adds ERROR_INSUFFICIENT_BUFFER constant. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IEcmaMetadata.cs | Adds GetReadWriteMetadata to the public contract interface. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs | Shrinks DebuggerData contract shape (drops defines/version). |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Removes Debugger field descriptors for defines/version. |
| src/coreclr/md/runtime/stgpool.cpp | Updates comment to reference new DacDbiInterfaceImpl::ReadStoragePool dependency. |
| src/coreclr/md/inc/VerifyLayouts.inc | Deletes layout-verification macro data (no longer needed). |
| src/coreclr/md/inc/verifylayouts.h | Deletes metadata layout verification infrastructure. |
| src/coreclr/md/inc/stgpooli.h | Removes VerifyLayoutsMD friend declarations. |
| src/coreclr/md/inc/stgpool.h | Removes VerifyLayoutsMD friend declarations. |
| src/coreclr/md/inc/recordpool.h | Removes VerifyLayoutsMD friend declarations. |
| src/coreclr/md/inc/metamodelrw.h | Removes custom data source hooks; adds DacDbiInterfaceImpl friend where needed. |
| src/coreclr/md/inc/metamodel.h | Removes VerifyLayoutsMD friend; adds DacDbiInterfaceImpl friend. |
| src/coreclr/md/inc/mdinternalrw.h | Removes VerifyLayoutsMD friend. |
| src/coreclr/md/inc/liteweightstgdb.h | Removes custom data source path and layout verifier friendship; adds DacDbiInterfaceImpl friend. |
| src/coreclr/md/enc/metamodelrw.cpp | Deletes FEATURE_METADATA_CUSTOM_DATA_SOURCE implementation (InitOnCustomDataSource). |
| src/coreclr/md/enc/liteweightstgdbrw.cpp | Deletes custom-data-source open path and related guards. |
| src/coreclr/md/datasource/targettypes.h | Deletes legacy target-type deserialization definitions. |
| src/coreclr/md/datasource/targettypes.cpp | Deletes legacy target-type deserialization implementation. |
| src/coreclr/md/datasource/stdafx.h | Deletes datasource PCH header. |
| src/coreclr/md/datasource/remotemdinternalrwsource.h | Deletes RemoteMDInternalRWSource (custom data source). |
| src/coreclr/md/datasource/remotemdinternalrwsource.cpp | Deletes RemoteMDInternalRWSource implementation. |
| src/coreclr/md/datasource/datatargetreader.h | Deletes DataTargetReader (remote read helper). |
| src/coreclr/md/datasource/datatargetreader.cpp | Deletes DataTargetReader implementation. |
| src/coreclr/md/datasource/CMakeLists.txt | Removes datasource library build. |
| src/coreclr/md/datasource/api.cpp | Deletes datasource factory entrypoint. |
| src/coreclr/md/compiler/verifylayouts.cpp | Deletes compilation unit that enforced layout verification. |
| src/coreclr/md/compiler/regmeta.h | Removes OpenExistingMD(IMDCustomDataSource*) declaration. |
| src/coreclr/md/compiler/regmeta.cpp | Removes custom-data-source open path and version-string fallback. |
| src/coreclr/md/compiler/disp.h | Removes IMetaDataDispenserCustom interface and helpers. |
| src/coreclr/md/compiler/disp.cpp | Removes custom-data-source scope opening implementation and QI support. |
| src/coreclr/md/compiler/CMakeLists.txt | Stops building verifylayouts-only WKS variant; uses common sources. |
| src/coreclr/md/CMakeLists.txt | Removes metadata custom data source/debuggee defines and datasource subdir. |
| src/coreclr/inc/utilcode.h | Removes VerifyLayoutsMD friend from CChainedHash. |
| src/coreclr/inc/shash.h | Removes VerifyLayoutsMD friend from SHash. |
| src/coreclr/inc/metadata.h | Removes IMDCustomDataSource/IMetaDataDispenserCustom and related declarations. |
| src/coreclr/inc/dacdbi.idl | Removes defines/version methods from the IDL definition of IDacDbiInterface. |
| src/coreclr/dlls/mscordbi/CMakeLists.txt | Removes mddatasource_dbi from CoreDBI link libraries. |
| src/coreclr/debug/inc/dacdbiinterface.h | Removes defines/version APIs; adds new read-write metadata serialization APIs. |
| src/coreclr/debug/ee/debugger.h | Removes metadata layout defines/version fields. |
| src/coreclr/debug/ee/debugger.cpp | Removes initialization of defines/version fields and version table comment block. |
| src/coreclr/debug/di/module.cpp | Switches to new DacDbi read-write metadata APIs and OpenScopeOnMemory. |
| src/coreclr/debug/di/CMakeLists.txt | Removes custom data source compile defines. |
| src/coreclr/debug/daccess/dacdbiimpl.h | Removes old APIs; adds new metadata serialization APIs + helper declaration. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Implements serialization of MDInternalRW metadata into a contiguous ECMA-335 image and new DacDbi entrypoints. |
| src/coreclr/debug/daccess/CMakeLists.txt | Enables FEATURE_METADATA_INTERNAL_APIS for daccess to access internal metadata types. |
| docs/design/datacontracts/EcmaMetadata.md | Updates contract documentation to include GetReadWriteMetadata and revised RW behavior. |
| docs/design/datacontracts/Debugger.md | Updates contract documentation for reduced DebuggerData and removed descriptors. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:405
- The copy should size the destination span to the actual blob length, not
cbBuffer. UsingcbBufferforces an unchecked(int)cast and can misbehave for large buffers; the contract already validatescbBufferis at least the blob size.
- Files reviewed: 56/56 changed files
- Comments generated: 4
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:419
- This
#if DEBUGvalidation block won’t compile:new byte[cbBuffer]uses auintlength, but array lengths must beint. Also, the span length casts are unchecked. Usechecked((int)cbBuffer)consistently here.
- Files reviewed: 56/56 changed files
- Comments generated: 4
| byte[] blob = _target.Contracts.EcmaMetadata.GetReadWriteMetadata(handle); | ||
| if (cbBuffer < (uint)blob.Length) | ||
| throw Marshal.GetExceptionForHR(CorDbgHResults.ERROR_INSUFFICIENT_BUFFER)!; | ||
|
|
||
| blob.AsSpan().CopyTo(new Span<byte>(pBuffer, (int)cbBuffer)); |
| ULONG32 cbSize = 0; | ||
| IfFailThrow(GetProcess()->GetDAC()->GetReadWriteMetadataSize(m_vmModule, &cbSize)); | ||
| CoTaskMemHolder<BYTE> pBuffer{ (BYTE*)CoTaskMemAlloc(cbSize) }; | ||
|
|
||
| IfFailThrow(GetProcess()->GetDAC()->FillReadWriteMetadata(m_vmModule, pBuffer, cbSize)); |
| void EnsureCapacity(ULONG32 needed) | ||
| { | ||
| if (needed <= m_capacity) | ||
| { | ||
| return; | ||
| } | ||
| ULONG32 newCapacity = (m_capacity == 0) ? 256 : m_capacity; | ||
| while (newCapacity < needed) | ||
| { | ||
| newCapacity *= 2; | ||
| } | ||
| BYTE * pNew = new BYTE[newCapacity]; | ||
| if (m_count != 0) | ||
| { | ||
| memcpy(pNew, m_pData, m_count); | ||
| } | ||
| delete[] m_pData; | ||
| m_pData = pNew; | ||
| m_capacity = newCapacity; | ||
| } |
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "cc878ae8c7c9eeeaa4704da04e246ab688c063f2",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "cb84c8521ce6a4cbde63cb1a700fcb850be791ae",
"last_reviewed_commit": "cc878ae8c7c9eeeaa4704da04e246ab688c063f2",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "cb84c8521ce6a4cbde63cb1a700fcb850be791ae",
"last_recorded_worker_run_id": "29689531871",
"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": "cc878ae8c7c9eeeaa4704da04e246ab688c063f2",
"review_id": 4730879754
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. Out-of-process debuggers previously reconstructed a target's writable (MDInternalRW) metadata by reading raw MD structures via a IMDCustomDataSource/RemoteMDInternalRWSource path, which required the layout-version + defines-bitfield handshake (GetMDStructuresVersion/GetDefinesBitField) and the parallel VerifyLayouts machinery to keep DBI and CLR structure layouts in lockstep. This PR replaces that fragile, layout-coupled path with a DacDbi API that serializes a self-describing ECMA-335 image, deleting ~3000 lines of hard-to-maintain code.
Approach: Sound and consistent. GetReadWriteMetadataSize/FillReadWriteMetadata are added to the (internal, unstable) DacDbi interface; the consumer in CordbModule::RefreshMetaData now allocates a buffer and calls OpenScopeOnMemory(... ofTakeOwnership), removing the custom-data-source dependency entirely. The serialization is implemented both in native DAC (SerializeReadWriteMetadata) and cDAC (EcmaMetadata_1.BuildReadWriteMetadataBlob), and the cDAC DacDbiImpl cross-validates its output byte-for-byte against the legacy DAC under #if DEBUG, giving strong parity confidence. The VerifyLayoutsMD friend declarations, VerifyLayouts.inc/.h, the md/datasource directory, and the now-unused Debugger::m_defines/m_mdDataStructureVersion fields (and their datadescriptor.inc entries + Debugger contract fields) are all removed cleanly; I grepped for dangling references to every removed symbol and found none.
Summary: ✅ LGTM. This is a clean, well-scoped refactor with correct doc updates (EcmaMetadata.md, Debugger.md) matching the contract/abstraction changes as required by the cDAC instructions, appropriate test removal for the deleted GetDefinesBitField/GetMDStructuresVersion APIs, and no behavioral regressions I could find. Per the cDAC guidance for .NET 11 preview branches, the DacDbi COM surface changes are internal/unstable and not treated as breaking. One non-blocking performance suggestion is left inline (the native path serializes the full image twice per refresh, once for size and once for fill, whereas cDAC caches per generation). I could not build or run tests in this review environment, so native compilation and the cDAC cross-validation assertions should be confirmed by CI.
Detailed Findings
✅ Correctness — removed-symbol cleanup is complete
Verified no remaining references to GetDefinesBitField, GetMDStructuresVersion, VerifyLayoutsMD, FEATURE_METADATA_CUSTOM_DATA_SOURCE, FEATURE_METADATA_DEBUGGEE_DATA_SOURCE, CreateRemoteMDInternalRWSource, m_defines, or m_mdDataStructureVersion outside the deletions. RegMeta::GetVersionString correctly drops the now-impossible m_pvMd == NULL fallback branch.
✅ Serialization parity — native vs. cDAC
The native MetadataBlobBuilder/ReadStoragePool logic mirrors the managed BlobBuilder path field-for-field (signature, version string, stream headers, #JTD marker for all-4-byte columns, heap alignment, valid/sorted table masks, row counts). The #if DEBUG cross-validation in DacDbiImpl.FillReadWriteMetadata/GetReadWriteMetadataSize asserts byte-for-byte equality with the legacy DAC, which is the right safety net for this dual implementation.
✅ Bounds/robustness in native serialization
ReadStoragePool guards segment count (MaxPoolSegments) and total size (MaxPoolBytes) and throws CLDB_E_FILE_CORRUPT on overflow; SerializeReadWriteMetadata validates tableCount <= MaxTableCount and clamps the metadata version length. These are appropriate hardening for reading potentially-corrupt target memory.
✅ Documentation & tests
EcmaMetadata.md and Debugger.md are updated to match the new GetReadWriteMetadata API and the trimmed DebuggerData record, satisfying the cDAC doc-sync requirement. Test removals in DebuggerTests.cs and DacDbiDebuggerDumpTests.cs correctly track the deleted fields/APIs; the ERROR_INSUFFICIENT_BUFFER constant is consolidated into CorDbgHResults and reused, removing local duplicates.
💡 Performance (inline) — native path serializes twice per refresh
See inline comment on dacdbiimpl.cpp: CordbModule::RefreshMetaData calls GetReadWriteMetadataSize then FillReadWriteMetadata, each of which fully rebuilds the ECMA-335 image (walking all storage pools). The cDAC side caches per module/generation; the native side could do the same. Non-blocking.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 215.6 AIC · ⌖ 11.6 AIC · ⊞ 10K
| Module * pModule = vmModule.GetDacPtr(); | ||
| BYTE * pRawBlob = NULL; | ||
| ULONG32 blobSize = 0; | ||
| SerializeReadWriteMetadata(pModule, &pRawBlob, &blobSize); |
There was a problem hiding this comment.
💡 Suggestion: The native path fully re-serializes the metadata twice per module refresh — once in GetReadWriteMetadataSize (line 4298) and again here in FillReadWriteMetadata — because SerializeReadWriteMetadata rebuilds the entire ECMA-335 image from scratch each call, and the consumer (CordbModule::RefreshMetaData) calls both back-to-back. The cDAC managed implementation caches the built blob per module/generation (_readWriteMetadataBlob), so it only builds once. Consider caching the serialized blob (or its size) on the native side as well to avoid walking all storage pools twice. Not blocking, since this happens once per metadata refresh.
