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
|
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
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
The approach looks fine to me - I'm mostly just curious about testing. We need to ensure that the new behavior is compatible with the old one and that cDAC going forward has test coverage around this.
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (4)
src/coreclr/debug/di/module.cpp:507
CoTaskMemAlloc(cbSize)can return null on OOM. If that happens, the subsequentFillReadWriteMetadatacall will fail withE_INVALIDARG(null buffer) rather than surfacing an OOM, andOpenScopeOnMemorycould also dereference a null pointer depending on the path. Please add an explicit allocation failure check and throwE_OUTOFMEMORY(or equivalent) before calling into DAC/metadata dispenser.
ULONG32 cbSize = 0;
IfFailThrow(GetProcess()->GetDAC()->GetReadWriteMetadataSize(m_vmModule, &cbSize));
CoTaskMemHolder<BYTE> pBuffer{ (BYTE*)CoTaskMemAlloc(cbSize) };
IfFailThrow(GetProcess()->GetDAC()->FillReadWriteMetadata(m_vmModule, pBuffer, cbSize));
IMetaDataDispenserEx * pDisp = GetProcess()->GetDispenser();
_ASSERTE(pDisp != NULL);
IfFailThrow(pDisp->OpenScopeOnMemory(pBuffer, cbSize, ofTakeOwnership, IID_IMetaDataImport, (IUnknown**)&m_pIMImport));
pBuffer.Detach(); // ownership transferred to the IMetaDataImport
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:407
new Span<byte>(pBuffer, (int)cbBuffer)can overflow ifcbBufferis >int.MaxValue, and it also overstates the destination length (we only need to copyblob.Lengthbytes). Using the blob length avoids the cast and keeps the copy bounded to the actual serialized size.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiDebuggerDumpTests.cs:58- This test suite removed coverage for the old defines/version APIs, but the PR also introduces new DacDbi APIs (
GetReadWriteMetadataSize/FillReadWriteMetadata) that are now used by the debugger (CordbModule::RefreshMetaData). Please add dump-based integration tests that (1) call the new APIs, (2) validateFillReadWriteMetadatareturns a blob of the expected size, and (3) optionally validateOpenScopeOnMemorysucceeds on the returned image (or at least that the blob begins with the ECMA metadata signatureBSJB).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IEcmaMetadata.cs:15 - This adds a new
publiccontract method (IEcmaMetadata.GetReadWriteMetadata) which expands public surface area. The PR description doesn’t link anapi-approvedissue/proposal, so it’s unclear whether this new public API has gone through the required approval process. If this API is intended to be internal-only, consider making itinternal(or otherwise non-public) until approved; otherwise please link the approved issue and ensure the final approved API shape matches exactly.
- Files reviewed: 56/56 changed files
- Comments generated: 1
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (4)
src/coreclr/debug/di/module.cpp:503
- The CoTaskMemAlloc result isn't checked for NULL before being passed to FillReadWriteMetadata / OpenScopeOnMemory. This can turn an OOM into a null-dereference (or an E_INVALIDARG from DAC) and is inconsistent with CopyRemoteMetaData(), which throws OOM on allocation failure.
ULONG32 cbSize = 0;
IfFailThrow(GetProcess()->GetDAC()->GetReadWriteMetadataSize(m_vmModule, &cbSize));
CoTaskMemHolder<BYTE> pBuffer{ (BYTE*)CoTaskMemAlloc(cbSize) };
IfFailThrow(GetProcess()->GetDAC()->FillReadWriteMetadata(m_vmModule, pBuffer, cbSize));
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:447
- This copy uses cbBuffer for the Span length and casts it to int. Since you already validated cbBuffer against blob.Length, you can avoid the potentially-overflowing cast and the unnecessarily-large destination Span by copying exactly blob.Length bytes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:465 - This Span length casts cbBuffer (uint) to int without a checked conversion. Even if today callers pass the exact size, this is still an unchecked narrowing conversion in unsafe code; use checked((int)cbBuffer) for robustness.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IEcmaMetadata.cs:16 - This PR adds new public API surface (IEcmaMetadata.GetReadWriteMetadata). Per dotnet/runtime process, new public APIs require an approved proposal (an issue labeled api-approved with an approved API shape comment) or the API should be made internal/experimental-only pending review. No approved issue is referenced in the PR metadata provided here.
- Files reviewed: 56/56 changed files
- Comments generated: 3
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (4)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:456
cbBufferis auint, sonew byte[cbBuffer]does not compile (array lengths requireint). This will break DEBUG builds.
Cast with checked((int)cbBuffer) (and consider guarding against values > int.MaxValue if that can happen).
src/coreclr/debug/di/module.cpp:503
- After
CoTaskMemAlloc(cbSize), the code doesn't check for allocation failure. If allocation returns NULL, the subsequent DAC call will fail with a misleading HRESULT.
Match CopyRemoteMetaData's pattern and throw OOM when the allocation fails.
ULONG32 cbSize = 0;
IfFailThrow(GetProcess()->GetDAC()->GetReadWriteMetadataSize(m_vmModule, &cbSize));
CoTaskMemHolder<BYTE> pBuffer{ (BYTE*)CoTaskMemAlloc(cbSize) };
IfFailThrow(GetProcess()->GetDAC()->FillReadWriteMetadata(m_vmModule, pBuffer, cbSize));
src/coreclr/debug/daccess/dacdbiimpl.cpp:4318
FillReadWriteMetadatatreats a nullpBufferasE_INVALIDARG, but this is a pointer-out parameter and should reportE_POINTER(consistent withGetReadWriteMetadataSize, typical COM conventions, and the managed wrappers throwingArgumentNullException→E_POINTER).
if (pBuffer == NULL)
{
ThrowHR(E_INVALIDARG);
}
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiDebuggerDumpTests.cs:53
- The PR introduces new DacDbi APIs (
GetReadWriteMetadataSize/FillReadWriteMetadata), but this dump integration test suite no longer exercises any DacDbi metadata path. Adding a basic success + sanity test here would help catch regressions (e.g., non-zero size, Fill succeeds, and the bytes can be opened by a metadata dispenser).
Without coverage, failures in the new serialization path may only show up in product debugging scenarios.
- Files reviewed: 56/56 changed files
- Comments generated: 0 new
Our tests expect to be able to toggle cDAC support on and off. Although most tests run with the managed host enabled, some of them explicitly disable it. Right now if a test disables managed SOS hosting and attempts to run: !runtimes --usecdac false the native host doesn't recognize the option and ignores it. The test then runs using default native host policy which will load cDAC. This means tests that claim to be validating the legacy DAC path are actually validating the SOS bundled cDAC instead. This was blocking Rachel's PR dotnet/runtime#131027 because she had both runtime and cDAC changes included but the test was only using the runtime portion of the change without the matching cDAC portion.
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (4)
src/coreclr/debug/di/module.cpp:503
- CoTaskMemAlloc(cbSize) can return null; the code then passes a null buffer into FillReadWriteMetadata/OpenScopeOnMemory, which can turn an OOM into a misleading failure or crash. Add an explicit null check and throw OOM (same pattern is already used in CopyRemoteMetaData).
ULONG32 cbSize = 0;
IfFailThrow(GetProcess()->GetDAC()->GetReadWriteMetadataSize(m_vmModule, &cbSize));
CoTaskMemHolder<BYTE> pBuffer{ (BYTE*)CoTaskMemAlloc(cbSize) };
IfFailThrow(GetProcess()->GetDAC()->FillReadWriteMetadata(m_vmModule, pBuffer, cbSize));
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:447
- The destination Span length is created from (int)cbBuffer, which can overflow for large buffers and is larger than necessary. Since the API already validated cbBuffer >= blob.Length, the Span length should use blob.Length to avoid the cast and to scope the copy to the actual serialized metadata size.
src/coreclr/debug/daccess/dacdbiimpl.cpp:4316 - GetReadWriteMetadataSize returns E_POINTER for a null out-parameter, but FillReadWriteMetadata returns E_INVALIDARG for a null output buffer. For consistency (and to match the managed Legacy shim behavior), a null output buffer should return E_POINTER.
EX_TRY
{
if (pBuffer == NULL)
{
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IEcmaMetadata.cs:16
- This adds new public API surface (IEcmaMetadata.GetReadWriteMetadata) and the PR also changes existing public contract surface (DebuggerData shape) without any linked
api-approvedissue/proposal in the PR description. dotnet/runtime requires approved API review for new public APIs; please link the approved issue or make the new surface internal/pending-review.
- Files reviewed: 56/56 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (7)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:449
- Copying the metadata blob into a destination span sized to cbBuffer forces an (int) cast and can throw for large cbBuffer values, and it also writes an undefined tail when cbBuffer > blob.Length. Since the API only promises to fill blob.Length bytes, copy into a span sized to blob.Length instead.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:468 - DEBUG cross-validation compares the entire cbBuffer even though FillReadWriteMetadata is only required to write the first N bytes of the blob. If a caller passes a larger cbBuffer, the tail of pBuffer may contain unrelated data and cause a false mismatch. Compare only the expected blob size.
src/coreclr/debug/di/module.cpp:503 - CoTaskMemAlloc(cbSize) can return NULL on OOM; the current code would then pass a null buffer to FillReadWriteMetadata and lose the real failure reason (and may return E_INVALIDARG instead of E_OUTOFMEMORY). Check the allocation result and throw/return E_OUTOFMEMORY explicitly.
ULONG32 cbSize = 0;
IfFailThrow(GetProcess()->GetDAC()->GetReadWriteMetadataSize(m_vmModule, &cbSize));
CoTaskMemHolder<BYTE> pBuffer{ (BYTE*)CoTaskMemAlloc(cbSize) };
if (pBuffer == NULL)
{
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IEcmaMetadata.cs:17
- IEcmaMetadata is public, and this PR adds a new public member (GetReadWriteMetadata). New public APIs require an approved API proposal (api-approved issue + approved API shape). The PR description doesn’t reference any such issue, so this can’t be validated and is a merge blocker unless the API is made non-public or an approved issue is linked.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs:9 - DebuggerData is public and this change removes previously exposed members (DefinesBitField, MDStructuresVersion). That’s a breaking public API change and should go through the same API-approval process (or be made non-public) before it can merge.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:433 - New DacDbi APIs GetReadWriteMetadataSize/FillReadWriteMetadata are introduced here but there are no tests exercising them (no usages found under src/native/managed/cdac/tests). Adding at least one dump-based integration test that calls both APIs and validates the buffer can be opened by a MetadataReader would help prevent regressions.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:438 - FillReadWriteMetadata throws ArgumentNullException for a null pBuffer, which maps to E_POINTER. The native DacDbiInterfaceImpl::FillReadWriteMetadata returns E_INVALIDARG for a null buffer, so the DEBUG cross-validation (ValidateHResult) can assert due to mismatched HRESULTs. Use ArgumentException here to return E_INVALIDARG and match the legacy/native behavior.
- Files reviewed: 56/56 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (4)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:464
- The DEBUG cross-check allocates
new byte[cbBuffer], butcbBufferis auint, which is not a valid array length type in C#. This won’t compile.
src/coreclr/debug/daccess/dacdbiimpl.cpp:3919 SerializeReadWriteMetadatabounds-checkstableCountagainstMaxTableCount = 64, but then indexespMiniMd->m_Schema.m_cRecs[i]and related metadata arrays which are sized toTBL_COUNT(ECMA-335 table count). Ifm_TblCountwere ever >TBL_COUNT, this would read past the end of those arrays in DAC code.
const ULONG64 MaxPoolBytes = 100000000;
const ULONG32 MaxPoolSegments = 1000;
const ULONG32 MaxTableCount = 64;
src/coreclr/debug/di/module.cpp:511
- This PR introduces a new dump-debugging metadata path that depends on the new DAC APIs
GetReadWriteMetadataSize/FillReadWriteMetadata, but there are no tests covering the new API pair (success path + insufficient-buffer behavior) or validating that the returned blob can be opened as metadata. Adding a dump test would help catch regressions in the serialization logic.
_ASSERTE(m_pIMImport == NULL);
ULONG32 cbSize = 0;
IfFailThrow(GetProcess()->GetDAC()->GetReadWriteMetadataSize(m_vmModule, &cbSize));
CoTaskMemHolder<BYTE> pBuffer{ (BYTE*)CoTaskMemAlloc(cbSize) };
if (pBuffer == NULL)
{
ThrowOutOfMemory();
}
IfFailThrow(GetProcess()->GetDAC()->FillReadWriteMetadata(m_vmModule, pBuffer, cbSize));
IMetaDataDispenserEx * pDisp = GetProcess()->GetDispenser();
_ASSERTE(pDisp != NULL);
IfFailThrow(pDisp->OpenScopeOnMemory(pBuffer, cbSize, ofTakeOwnership, IID_IMetaDataImport, (IUnknown**)&m_pIMImport));
pBuffer.Detach(); // ownership transferred to the IMetaDataImport
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IEcmaMetadata.cs:17
- This adds a new
publiccontract method (GetReadWriteMetadata). Per the repo’s API approval gate for new public surface, the PR needs to link anapi-approvedissue (with the approved API shape) or the API should be made non-public until it’s been through that process.
- Files reviewed: 56/56 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/coreclr/debug/daccess/dacdbiimpl.cpp:4133
ReadStoragePoolexpects aStgPool/StgPoolSeg-style object at the provided address, butCMiniMdRW::m_Tablesis an array ofMetaData::TableRW, which contains itsRecordPoolstorage as a member. The current code passes the address of theTableRWobject itself, which relies onTableRW'sm_RecordStoragebeing located at offset 0. That layout dependency is brittle and could silently break ifTableRWever grows a leading field.
Prefer passing the address of the TableRW::m_RecordStorage member explicitly so the reader is anchored to the correct subobject regardless of layout changes.
NewArrayHolder<PoolData> tables = new PoolData[tableCount == 0 ? 1 : tableCount];
TADDR tablesBase = PTR_HOST_MEMBER_TADDR(CMiniMdRW, pMiniMd, m_Tables);
ULONG32 tableStride = (ULONG32)sizeof(MetaData::TableRW);
for (ULONG32 i = 0; i < tableCount; i++)
{
ReadStoragePool(tablesBase + i * tableStride, tables[i]);
}
src/coreclr/debug/di/module.cpp:511
- This path calls
GetReadWriteMetadataSizeand thenFillReadWriteMetadataback-to-back. In the current DAC implementation, both methods reconstruct the serialized metadata blob viaSerializeReadWriteMetadata, soRefreshMetaDataends up walking the target metadata and allocating/copying twice per refresh.
Consider an API/implementation shape that allows retrieving the bytes in a single pass (for example, a TargetBuffer-style API where DAC allocates once, or a Fill... API that can return the required size without rebuilding).
_ASSERTE(m_pIMImport == NULL);
ULONG32 cbSize = 0;
IfFailThrow(GetProcess()->GetDAC()->GetReadWriteMetadataSize(m_vmModule, &cbSize));
CoTaskMemHolder<BYTE> pBuffer{ (BYTE*)CoTaskMemAlloc(cbSize) };
if (pBuffer == NULL)
{
ThrowOutOfMemory();
}
IfFailThrow(GetProcess()->GetDAC()->FillReadWriteMetadata(m_vmModule, pBuffer, cbSize));
IMetaDataDispenserEx * pDisp = GetProcess()->GetDispenser();
_ASSERTE(pDisp != NULL);
IfFailThrow(pDisp->OpenScopeOnMemory(pBuffer, cbSize, ofTakeOwnership, IID_IMetaDataImport, (IUnknown**)&m_pIMImport));
pBuffer.Detach(); // ownership transferred to the IMetaDataImport
- Files reviewed: 57/57 changed files
- Comments generated: 0 new
