[cDAC] Implement GetMetadata for cDAC#131169
Open
barosiak wants to merge 2 commits into
Open
Conversation
|
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. |
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements DacDbiImpl.GetMetadata in the cDAC legacy Dbi layer by retrieving a target metadata span via the IEcmaMetadata contract (read-only metadata for normal modules; saved read-write metadata for reflection-emit/dynamic modules). The PR also extends the EcmaMetadata contract surface to expose the saved dynamic-metadata address, updates the contract design doc accordingly, and adds unit tests covering key GetMetadata scenarios.
Changes:
- Implement
DacDbiImpl.GetMetadatausingLoader+EcmaMetadatacontracts, including a new HRESULT for missing metadata. - Expose
IEcmaMetadata.GetReadWriteSavedMetadataAddress(implementation + abstractions) and document it. - Add unit tests for non-dynamic, dynamic, empty-metadata, and null-buffer paths.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/DacDbiImplTests.cs | Adds unit tests for DacDbiImpl.GetMetadata covering dynamic/non-dynamic, missing metadata, and null buffer. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs | Implements GetMetadata by selecting the correct metadata span based on module flags and mapping failures to HRESULTs. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/EcmaMetadata_1.cs | Exposes the saved read-write metadata span accessor needed for reflection-emit modules. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/CorDbHResults.cs | Adds CORDBG_E_MISSING_METADATA HRESULT constant used by GetMetadata. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IEcmaMetadata.cs | Extends the EcmaMetadata contract interface with GetReadWriteSavedMetadataAddress. |
| docs/design/datacontracts/EcmaMetadata.md | Documents the new EcmaMetadata contract API surface. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 2
rcj1
approved these changes
Jul 21, 2026
Contributor
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:359
- GetMetadata dereferences pTargetBuffer before validating it (and also accepts vmModule==0), unlike neighboring methods such as GetSymbolsBuffer/GetModuleData which validate pointer arguments up front. Relying on a null dereference to produce an HRESULT is brittle and can become an uncatchable AccessViolation depending on codegen / runtime settings; it also makes the contract less clear for callers. Add explicit argument validation and only write through pTargetBuffer after the null check.
*pTargetBuffer = default;
Contracts.ILoader loader = _target.Contracts.Loader;
Contracts.ModuleHandle handle = loader.GetModuleHandleFromModulePtr(new TargetPointer(vmModule));
Contracts.ModuleFlags flags = loader.GetFlags(handle);
Contracts.IEcmaMetadata ecmaMetadata = _target.Contracts.EcmaMetadata;
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
rcj1
approved these changes
Jul 21, 2026
hoyosjs
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements
DacDbiImpl.GetMetadatafor the cDAC.Changes
DacDbiImpl.cs- ImplementGetMetadata.CorDbHResults.cs- Add theCORDBG_E_MISSING_METADATAconstant.EcmaMetadata_1.cs,IEcmaMetadata.cs- ExposeGetReadWriteSavedMetadataAddresson the contract.EcmaMetadata.md- DocumentGetReadWriteSavedMetadataAddress.DacDbiImplTests.cs- Add tests for the non-dynamic, dynamic, empty-metadata, and null-buffer cases.