[cDAC] Add IMetaDataImport COM wrapper over MetadataReader for no-fallback mode

[max-charlamb]

Note

This PR description was AI/Copilot-generated.

Summary

Implements a managed [GeneratedComClass] wrapper (MetaDataImportImpl) that adapts System.Reflection.Metadata.MetadataReader to the IMetaDataImport, IMetaDataImport2, and IMetaDataAssemblyImport COM interfaces. This enables SOS and ClrMD to query metadata in cDAC mode, with optional legacy DAC fallback for methods not yet implemented in the managed layer.

Motivation

In cDAC no-fallback mode, ClrDataModule.GetInterface() returned NotHandled for IMetaDataImport QIs when _legacyModulePointer == 0, meaning diagnostic tools couldn't access type/method/field metadata. The cDAC already has access to MetadataReader via the EcmaMetadata contract, so a thin COM wrapper bridges the gap.

When a legacy DAC is available, the wrapper uses it for #if DEBUG validation (asserting that cDAC and DAC produce identical results) and as a fallback for the ~45 methods not yet implemented in managed code.

Changes

File	Description
IMetaDataImport.cs	Managed [GeneratedComInterface] definitions for IMetaDataImport (51 methods), IMetaDataImport2 (8 methods), IMetaDataAssemblyImport (14 methods), ASSEMBLYMETADATA struct, and internal CldbHResults constants
MetaDataImportImpl.cs	[GeneratedComClass] implementation — 28 full cDAC implementations via MetadataReader + ~45 legacy-delegated stubs. Uses explicit interface notation.
OutputBufferHelpers.cs	CopyStringToBuffer split into two overloads: void (for callers that don't check truncation) and out bool truncated (for MetaDataImportImpl)
ClrDataModule.cs	Wire up wrapper via ICustomQueryInterface — creates MetaDataImportImpl with both MetadataReader and optional legacy IMetaDataImport
MetaDataImportImplTests.cs	50 unit tests using synthetic metadata built with MetadataBuilder
MetaDataImportDumpTests.cs	3 dump-based integration tests verifying semantic parity against real metadata
MultiModule debuggee	Test debuggee with non-const fields, user strings, and methods for dump tests

Implemented methods (28 cDAC, ~45 legacy fallback)

Enum (cDAC):EnumInterfaceImpls, EnumFields, EnumGenericParams, CloseEnum, CountEnum, ResetEnum

Properties (cDAC):GetTypeDefProps, GetTypeRefProps, GetMethodProps, GetFieldProps, GetMemberProps, GetInterfaceImplProps, GetNestedClassProps, GetGenericParamProps, GetMemberRefProps, GetModuleRefProps, GetParamProps, GetClassLayout, GetUserString, GetParamForMethodIndex

Blob/token (cDAC):GetRVA, GetSigFromToken, GetTypeSpecFromToken, GetCustomAttributeByName, IsValidToken, FindTypeDefByName

Assembly (cDAC):GetAssemblyProps, GetAssemblyRefProps, GetAssemblyFromScope

Legacy fallback: All remaining methods delegate to _legacyImport/_legacyImport2/_legacyAssemblyImport or return E_NOTIMPL when no legacy is available.

Native parity behaviors

• <Module> parent mapping: GetMethodProps/GetFieldProps/GetMemberRefProps map TypeDef RID 1 to mdTypeDefNil (0x00000000) via MapGlobalParentToken, matching native RegMeta
• Constant defaults: GetFieldProps/GetParamProps return ELEMENT_TYPE_VOID when no constant is present
• User string heap: GetUserString uses raw #US heap byte parsing to exactly match native blob size validation (odd-length check, terminal byte stripping)
• Assembly flags: GetAssemblyProps ORs afPublicKey into flags when public key blob is non-empty
• Truncation: All string-returning methods return CLDB_S_TRUNCATION when buffer is too small
• Record not found: GetNestedClassProps/GetClassLayout/GetAssemblyProps return CLDB_E_RECORD_NOTFOUND for missing records

Design decisions

• Explicit interface implementation: All ~73 COM methods use explicit interface notation (int IMetaDataImport.Method(...)) to keep the public surface clean
• ICustomQueryInterface: QI for IMetaDataImport returns an IMetaDataImport2 vtable so callers always get the full interface
• HCORENUM pattern: Uses GCHandle.Alloc to box MetadataEnum objects; ConcurrentDictionary<nint, byte> tracks ownership for routing CloseEnum/CountEnum/ResetEnum between cDAC and legacy handles
• #if DEBUG validation: Every cDAC-implemented method (with 2 justified exceptions) cross-checks its output against the legacy DAC in debug builds
• CopyStringToBuffer overloads: void overload for callers that don't need truncation info; out bool truncated overload for MetaDataImportImpl callers that return CLDB_S_TRUNCATION

Testing

• 1845 unit tests pass (all cDAC tests including 50 MetaDataImportImpl tests)
• 214 dump-based tests pass locally (3 are MetaDataImport-specific)
• Tests cover: enum pagination, global/non-global parent mapping, constant handling, user string char counts, truncation, nested classes, generic params, assembly properties, invalid tokens, QI vtable correctness, and E_NOTIMPL fallback behavior

[Copilot]

Pull request overview

This PR adds a managed COM-compatible IMetaDataImport/IMetaDataImport2 implementation backed by System.Reflection.Metadata.MetadataReader, and wires it into cDAC’s ClrDataModule so diagnostic tools can query metadata when running in no-fallback mode (legacy DAC unavailable).

Changes:

• Introduces generated COM interface definitions for IMetaDataImport and IMetaDataImport2.
• Implements MetadataImportWrapper to adapt MetadataReader to the COM metadata import APIs (with a subset implemented and the rest stubbed).
• Updates ClrDataModule to provide IMetaDataImport in no-fallback mode and adds unit tests validating implemented behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/native/managed/cdac/tests/MetadataImportWrapperTests.cs	Adds unit tests that exercise the wrapper’s enumeration and property APIs over synthetic metadata.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetadataImportWrapper.cs	Implements the COM wrapper over MetadataReader including enum-handle handling and a set of metadata accessors.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IMetaDataImport.cs	Adds managed [GeneratedComInterface] declarations for IMetaDataImport/IMetaDataImport2 with vtable ordering.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Hooks IMetaDataImport QI to return the managed wrapper when legacy DAC is unavailable.

[jkoritzinsky]

You'll likely need to add support for IMetaDataAssemblyImport as well for assembly-level info

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

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

[github-actions]

🤖 Copilot Code Review — PR #127028

Note

This review was AI-generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, with Claude Sonnet 4.5 and GPT-5.3-Codex sub-agents).

Holistic Assessment

Motivation: Well-justified. A managed MetaDataImportImpl wrapping System.Reflection.Metadata.MetadataReader is essential for cDAC no-fallback mode where the legacy DAC is unavailable. SOS and ClrMD depend on IMetaDataImport for metadata queries, making this a required capability.

Approach: Sound. Implementing ~30 commonly-used methods with MetadataReader while delegating ~30 rarely-used ones to the legacy DAC is pragmatic. The ICustomQueryInterface vtable redirect to handle ClrMD's beyond-vtable IMetaDataImport2 slot access is clever and well-documented. The extensive #if DEBUG validation blocks comparing output against the legacy DAC are excellent for catching parity gaps during development.

Summary: ⚠️Needs Human Review. The code is well-structured, follows cDAC conventions, and handles COM interop correctly. There are a few thread-safety concerns with lazy initialization and a minor correctness gap in IsValidToken that warrant maintainer attention. None appear to be critical bugs in practice, but a human reviewer should evaluate whether they're acceptable for the diagnostics use case.

---

Detailed Findings

⚠️IsValidToken — RID 0 early rejection incorrectly covers user string tokens

MetaDataImportImpl.cs:819-838 — IsValidToken rejects RID 0 before checking the token type:

intrid=(int)(tk&0x00FFFFFF);inttokenType=(int)(tk>>24);if(rid==0)return0;// FALSE — but this runs before the UserString branch

For metadata table tokens, RID 0 is nil/invalid. However, for user string tokens (0x70), the "RID" is actually a heap offset, and offset 0 is a valid entry in the #US heap (the required null/empty byte per ECMA-335). The native IsValidToken would return TRUE for 0x70000000 when the heap is non-empty, while this implementation returns FALSE.

Fix: Move the rid == 0 check after the UserStringTokenType branch, or restrict it to table tokens only:

constintUserStringTokenType=0x70;if(tokenType==UserStringTokenType){intheapSize=_reader.GetHeapSize(HeapIndex.UserString);returnrid<heapSize?1:0;}if(rid==0)return0;// Only for table tokens

Advisory — unlikely to cause real-world issues since offset 0 is never referenced by IL, but it is a parity gap with native. Flagged by GPT-5.3-Codex, verified by primary reviewer.

⚠️ Lazy dictionary initialization without memory barriers (thread safety)

MetaDataImportImpl.cs:554,1336 — _interfaceImplToTypeDef and _paramToMethod use ??= for lazy initialization:

_interfaceImplToTypeDef??=BuildInterfaceImplLookup();

Since MetaDataImportImpl is a COM object callable from multiple threads (as acknowledged by the use of ConcurrentDictionary for _cdacEnumHandles), two threads could race on first access. The dictionaries are fully constructed before assignment and reference writes are atomic in .NET, so the practical risk is very low. However, the codebase convention states that ??= is not thread-safe and cross-thread field access should use Volatile or Interlocked.

Volatile.Write/Volatile.Read or LazyInitializer.EnsureInitialized would address this cleanly.

Advisory — not merge-blocking. The benign-race pattern here (two threads build independent dictionaries, one wins, both are equivalent) is safe in practice on x86/x64 and very likely safe on ARM given .NET's runtime guarantees. Flagged by all three models.

⚠️ GCHandle leak if CloseEnum is never called

MetaDataImportImpl.cs:66-73 — AllocEnum allocates GCHandle instances tracked in _cdacEnumHandles, but there's no finalizer or IDisposable to clean them up if the COM consumer forgets to call CloseEnum. While this matches the native IMetaDataImport contract (callers own enum lifetime), diagnostic tools may not always clean up on error paths.

Advisory — follows the same contract as native. A finalizer iterating _cdacEnumHandles.Keys and freeing outstanding handles would be a defensive improvement. Not merge-blocking since the native API has the same contract.

✅ COM ref counting — Correct

Both ICustomQueryInterface.GetInterface implementations (in MetaDataImportImpl.cs:47 and ClrDataModule.cs:111) correctly use ComInterfaceMarshaller<IMetaDataImport2>.ConvertToUnmanaged() which returns an already-AddRef'd pointer, consistent with COM QueryInterface semantics. The IMetaDataImport→IMetaDataImport2 vtable redirect for ClrMD compatibility is validated end-to-end by the test at line 964.

✅ GetUserString raw heap parsing — Correct

MetaDataImportImpl.cs:1188-1250 — Correctly reads from raw #US heap bytes rather than using MetadataReader.GetUserString(), avoiding potential discrepancies. The odd-size validation (blobSize % sizeof(char) == 0 → corrupt) correctly enforces the terminal byte requirement per ECMA-335 §II.24.2.4. pchString correctly returns character count without null terminator, matching native RegMeta semantics.

✅ Empty catch blocks in ClrDataModule.GetInterface — Acceptable

The two bare catch {} blocks (lines 85-87, 98-100) silently swallow exceptions when probing for metadata reader availability and legacy QI support. This is appropriate: both are optional capability probes. If both fail, the method returns CustomQueryInterfaceResult.NotHandled (line 103), which is the correct degradation path.

✅ DEBUG validation blocks — Excellent

The #if DEBUG blocks throughout MetaDataImportImpl comparing cDAC output against legacy DAC output (via Debug.ValidateHResult and per-field assertions) are a strong engineering practice for ensuring semantic parity. The ValidateBlobsEqual helper for byte-level blob comparison is thorough.

✅ Test coverage — Comprehensive

~1100 lines of unit tests covering: enum pagination, string truncation with CLDB_S_TRUNCATION, not-found error paths, GetMemberProps dispatch (method vs field table index), global method parent mapping (MapGlobalParentToken), constant value handling for both fields and params, vtable slot verification for the ClrMD compatibility fix, and enum lifecycle (CountEnum, ResetEnum, CloseEnum). The dump-based integration tests validate real-world metadata against actual process dumps.

💡 Static _testProvider field in tests

MetaDataImportImplTests.cs:161 — _testProvider is a shared static field used to prevent GC collection. Consider [ThreadStatic] for parallel test safety. Low practical risk since xUnit serializes tests within a class.

💡 _metaDataImportImpl lazy init race in ClrDataModule

ClrDataModule.cs:107 — _metaDataImportImpl ??= wrapper is a documented benign race. Both wrappers are functionally equivalent, the extra one is GC'd, and line 108 re-reads to ensure consistency. Acceptable pattern.

Generated by Code Review for issue #127028 · ◷

[noahfalk]

A few nits, but overall looks good to me!

[max-charlamb]

/ba-g previous test run passed. only nit style changes from comments

[max-charlamb]

Note

This PR description was AI/Copilot-generated.

Summary

Implements a managed [GeneratedComClass] wrapper (MetaDataImportImpl) that adapts System.Reflection.Metadata.MetadataReader to the IMetaDataImport, IMetaDataImport2, and IMetaDataAssemblyImport COM interfaces. This enables SOS and ClrMD to query metadata in cDAC mode, with optional legacy DAC fallback for methods not yet implemented in the managed layer.

Motivation

In cDAC no-fallback mode, ClrDataModule.GetInterface() returned NotHandled for IMetaDataImport QIs when _legacyModulePointer == 0, meaning diagnostic tools couldn't access type/method/field metadata. The cDAC already has access to MetadataReader via the EcmaMetadata contract, so a thin COM wrapper bridges the gap.

When a legacy DAC is available, the wrapper uses it for #if DEBUG validation (asserting that cDAC and DAC produce identical results) and as a fallback for the ~45 methods not yet implemented in managed code.

Changes

File	Description
IMetaDataImport.cs	Managed [GeneratedComInterface] definitions for IMetaDataImport (51 methods), IMetaDataImport2 (8 methods), IMetaDataAssemblyImport (14 methods), ASSEMBLYMETADATA struct, and internal CldbHResults constants
MetaDataImportImpl.cs	[GeneratedComClass] implementation — 28 full cDAC implementations via MetadataReader + ~45 legacy-delegated stubs. Uses explicit interface notation.
OutputBufferHelpers.cs	CopyStringToBuffer split into two overloads: void (for callers that don't check truncation) and out bool truncated (for MetaDataImportImpl)
ClrDataModule.cs	Wire up wrapper via ICustomQueryInterface — creates MetaDataImportImpl with both MetadataReader and optional legacy IMetaDataImport
MetaDataImportImplTests.cs	50 unit tests using synthetic metadata built with MetadataBuilder
MetaDataImportDumpTests.cs	3 dump-based integration tests verifying semantic parity against real metadata
MultiModule debuggee	Test debuggee with non-const fields, user strings, and methods for dump tests

Implemented methods (28 cDAC, ~45 legacy fallback)

Enum (cDAC):EnumInterfaceImpls, EnumFields, EnumGenericParams, CloseEnum, CountEnum, ResetEnum

Properties (cDAC):GetTypeDefProps, GetTypeRefProps, GetMethodProps, GetFieldProps, GetMemberProps, GetInterfaceImplProps, GetNestedClassProps, GetGenericParamProps, GetMemberRefProps, GetModuleRefProps, GetParamProps, GetClassLayout, GetUserString, GetParamForMethodIndex

Blob/token (cDAC):GetRVA, GetSigFromToken, GetTypeSpecFromToken, GetCustomAttributeByName, IsValidToken, FindTypeDefByName

Assembly (cDAC):GetAssemblyProps, GetAssemblyRefProps, GetAssemblyFromScope

Legacy fallback: All remaining methods delegate to _legacyImport/_legacyImport2/_legacyAssemblyImport or return E_NOTIMPL when no legacy is available.

Native parity behaviors

• <Module> parent mapping: GetMethodProps/GetFieldProps/GetMemberRefProps map TypeDef RID 1 to mdTypeDefNil (0x00000000) via MapGlobalParentToken, matching native RegMeta
• Constant defaults: GetFieldProps/GetParamProps return ELEMENT_TYPE_VOID when no constant is present
• User string heap: GetUserString uses raw #US heap byte parsing to exactly match native blob size validation (odd-length check, terminal byte stripping)
• Assembly flags: GetAssemblyProps ORs afPublicKey into flags when public key blob is non-empty
• Truncation: All string-returning methods return CLDB_S_TRUNCATION when buffer is too small
• Record not found: GetNestedClassProps/GetClassLayout/GetAssemblyProps return CLDB_E_RECORD_NOTFOUND for missing records

Design decisions

• Explicit interface implementation: All ~73 COM methods use explicit interface notation (int IMetaDataImport.Method(...)) to keep the public surface clean
• ICustomQueryInterface: QI for IMetaDataImport returns an IMetaDataImport2 vtable so callers always get the full interface
• HCORENUM pattern: Uses GCHandle.Alloc to box MetadataEnum objects; ConcurrentDictionary<nint, byte> tracks ownership for routing CloseEnum/CountEnum/ResetEnum between cDAC and legacy handles
• #if DEBUG validation: Every cDAC-implemented method (with 2 justified exceptions) cross-checks its output against the legacy DAC in debug builds
• CopyStringToBuffer overloads: void overload for callers that don't need truncation info; out bool truncated overload for MetaDataImportImpl callers that return CLDB_S_TRUNCATION

Testing

• 1845 unit tests pass (all cDAC tests including 50 MetaDataImportImpl tests)
• 214 dump-based tests pass locally (3 are MetaDataImport-specific)
• Tests cover: enum pagination, global/non-global parent mapping, constant handling, user string char counts, truncation, nested classes, generic params, assembly properties, invalid tokens, QI vtable correctness, and E_NOTIMPL fallback behavior

[Copilot]

Pull request overview

This PR adds a managed COM-compatible IMetaDataImport/IMetaDataImport2 implementation backed by System.Reflection.Metadata.MetadataReader, and wires it into cDAC’s ClrDataModule so diagnostic tools can query metadata when running in no-fallback mode (legacy DAC unavailable).

Changes:

• Introduces generated COM interface definitions for IMetaDataImport and IMetaDataImport2.
• Implements MetadataImportWrapper to adapt MetadataReader to the COM metadata import APIs (with a subset implemented and the rest stubbed).
• Updates ClrDataModule to provide IMetaDataImport in no-fallback mode and adds unit tests validating implemented behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/native/managed/cdac/tests/MetadataImportWrapperTests.cs	Adds unit tests that exercise the wrapper’s enumeration and property APIs over synthetic metadata.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetadataImportWrapper.cs	Implements the COM wrapper over MetadataReader including enum-handle handling and a set of metadata accessors.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IMetaDataImport.cs	Adds managed [GeneratedComInterface] declarations for IMetaDataImport/IMetaDataImport2 with vtable ordering.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Hooks IMetaDataImport QI to return the managed wrapper when legacy DAC is unavailable.

[jkoritzinsky]

You'll likely need to add support for IMetaDataAssemblyImport as well for assembly-level info

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

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

[github-actions]

🤖 Copilot Code Review — PR #127028

Note

This review was AI-generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, with Claude Sonnet 4.5 and GPT-5.3-Codex sub-agents).

Holistic Assessment

Motivation: Well-justified. A managed MetaDataImportImpl wrapping System.Reflection.Metadata.MetadataReader is essential for cDAC no-fallback mode where the legacy DAC is unavailable. SOS and ClrMD depend on IMetaDataImport for metadata queries, making this a required capability.

Approach: Sound. Implementing ~30 commonly-used methods with MetadataReader while delegating ~30 rarely-used ones to the legacy DAC is pragmatic. The ICustomQueryInterface vtable redirect to handle ClrMD's beyond-vtable IMetaDataImport2 slot access is clever and well-documented. The extensive #if DEBUG validation blocks comparing output against the legacy DAC are excellent for catching parity gaps during development.

Summary: ⚠️Needs Human Review. The code is well-structured, follows cDAC conventions, and handles COM interop correctly. There are a few thread-safety concerns with lazy initialization and a minor correctness gap in IsValidToken that warrant maintainer attention. None appear to be critical bugs in practice, but a human reviewer should evaluate whether they're acceptable for the diagnostics use case.

---

Detailed Findings

⚠️IsValidToken — RID 0 early rejection incorrectly covers user string tokens

MetaDataImportImpl.cs:819-838 — IsValidToken rejects RID 0 before checking the token type:

intrid=(int)(tk&0x00FFFFFF);inttokenType=(int)(tk>>24);if(rid==0)return0;// FALSE — but this runs before the UserString branch

For metadata table tokens, RID 0 is nil/invalid. However, for user string tokens (0x70), the "RID" is actually a heap offset, and offset 0 is a valid entry in the #US heap (the required null/empty byte per ECMA-335). The native IsValidToken would return TRUE for 0x70000000 when the heap is non-empty, while this implementation returns FALSE.

Fix: Move the rid == 0 check after the UserStringTokenType branch, or restrict it to table tokens only:

constintUserStringTokenType=0x70;if(tokenType==UserStringTokenType){intheapSize=_reader.GetHeapSize(HeapIndex.UserString);returnrid<heapSize?1:0;}if(rid==0)return0;// Only for table tokens

Advisory — unlikely to cause real-world issues since offset 0 is never referenced by IL, but it is a parity gap with native. Flagged by GPT-5.3-Codex, verified by primary reviewer.

⚠️ Lazy dictionary initialization without memory barriers (thread safety)

MetaDataImportImpl.cs:554,1336 — _interfaceImplToTypeDef and _paramToMethod use ??= for lazy initialization:

_interfaceImplToTypeDef??=BuildInterfaceImplLookup();

Since MetaDataImportImpl is a COM object callable from multiple threads (as acknowledged by the use of ConcurrentDictionary for _cdacEnumHandles), two threads could race on first access. The dictionaries are fully constructed before assignment and reference writes are atomic in .NET, so the practical risk is very low. However, the codebase convention states that ??= is not thread-safe and cross-thread field access should use Volatile or Interlocked.

Volatile.Write/Volatile.Read or LazyInitializer.EnsureInitialized would address this cleanly.

Advisory — not merge-blocking. The benign-race pattern here (two threads build independent dictionaries, one wins, both are equivalent) is safe in practice on x86/x64 and very likely safe on ARM given .NET's runtime guarantees. Flagged by all three models.

⚠️ GCHandle leak if CloseEnum is never called

MetaDataImportImpl.cs:66-73 — AllocEnum allocates GCHandle instances tracked in _cdacEnumHandles, but there's no finalizer or IDisposable to clean them up if the COM consumer forgets to call CloseEnum. While this matches the native IMetaDataImport contract (callers own enum lifetime), diagnostic tools may not always clean up on error paths.

Advisory — follows the same contract as native. A finalizer iterating _cdacEnumHandles.Keys and freeing outstanding handles would be a defensive improvement. Not merge-blocking since the native API has the same contract.

✅ COM ref counting — Correct

Both ICustomQueryInterface.GetInterface implementations (in MetaDataImportImpl.cs:47 and ClrDataModule.cs:111) correctly use ComInterfaceMarshaller<IMetaDataImport2>.ConvertToUnmanaged() which returns an already-AddRef'd pointer, consistent with COM QueryInterface semantics. The IMetaDataImport→IMetaDataImport2 vtable redirect for ClrMD compatibility is validated end-to-end by the test at line 964.

✅ GetUserString raw heap parsing — Correct

MetaDataImportImpl.cs:1188-1250 — Correctly reads from raw #US heap bytes rather than using MetadataReader.GetUserString(), avoiding potential discrepancies. The odd-size validation (blobSize % sizeof(char) == 0 → corrupt) correctly enforces the terminal byte requirement per ECMA-335 §II.24.2.4. pchString correctly returns character count without null terminator, matching native RegMeta semantics.

✅ Empty catch blocks in ClrDataModule.GetInterface — Acceptable

The two bare catch {} blocks (lines 85-87, 98-100) silently swallow exceptions when probing for metadata reader availability and legacy QI support. This is appropriate: both are optional capability probes. If both fail, the method returns CustomQueryInterfaceResult.NotHandled (line 103), which is the correct degradation path.

✅ DEBUG validation blocks — Excellent

The #if DEBUG blocks throughout MetaDataImportImpl comparing cDAC output against legacy DAC output (via Debug.ValidateHResult and per-field assertions) are a strong engineering practice for ensuring semantic parity. The ValidateBlobsEqual helper for byte-level blob comparison is thorough.

✅ Test coverage — Comprehensive

~1100 lines of unit tests covering: enum pagination, string truncation with CLDB_S_TRUNCATION, not-found error paths, GetMemberProps dispatch (method vs field table index), global method parent mapping (MapGlobalParentToken), constant value handling for both fields and params, vtable slot verification for the ClrMD compatibility fix, and enum lifecycle (CountEnum, ResetEnum, CloseEnum). The dump-based integration tests validate real-world metadata against actual process dumps.

💡 Static _testProvider field in tests

MetaDataImportImplTests.cs:161 — _testProvider is a shared static field used to prevent GC collection. Consider [ThreadStatic] for parallel test safety. Low practical risk since xUnit serializes tests within a class.

💡 _metaDataImportImpl lazy init race in ClrDataModule

ClrDataModule.cs:107 — _metaDataImportImpl ??= wrapper is a documented benign race. Both wrappers are functionally equivalent, the extra one is GC'd, and line 108 re-reads to ensure consistency. Acceptable pattern.

Generated by Code Review for issue #127028 · ◷

[noahfalk]

A few nits, but overall looks good to me!

[max-charlamb]

/ba-g previous test run passed. only nit style changes from comments

[max-charlamb]

Note

This PR description was AI/Copilot-generated.

Summary

Implements a managed [GeneratedComClass] wrapper (MetaDataImportImpl) that adapts System.Reflection.Metadata.MetadataReader to the IMetaDataImport, IMetaDataImport2, and IMetaDataAssemblyImport COM interfaces. This enables SOS and ClrMD to query metadata in cDAC mode, with optional legacy DAC fallback for methods not yet implemented in the managed layer.

Motivation

In cDAC no-fallback mode, ClrDataModule.GetInterface() returned NotHandled for IMetaDataImport QIs when _legacyModulePointer == 0, meaning diagnostic tools couldn't access type/method/field metadata. The cDAC already has access to MetadataReader via the EcmaMetadata contract, so a thin COM wrapper bridges the gap.

When a legacy DAC is available, the wrapper uses it for #if DEBUG validation (asserting that cDAC and DAC produce identical results) and as a fallback for the ~45 methods not yet implemented in managed code.

Changes

File	Description
IMetaDataImport.cs	Managed [GeneratedComInterface] definitions for IMetaDataImport (51 methods), IMetaDataImport2 (8 methods), IMetaDataAssemblyImport (14 methods), ASSEMBLYMETADATA struct, and internal CldbHResults constants
MetaDataImportImpl.cs	[GeneratedComClass] implementation — 28 full cDAC implementations via MetadataReader + ~45 legacy-delegated stubs. Uses explicit interface notation.
OutputBufferHelpers.cs	CopyStringToBuffer split into two overloads: void (for callers that don't check truncation) and out bool truncated (for MetaDataImportImpl)
ClrDataModule.cs	Wire up wrapper via ICustomQueryInterface — creates MetaDataImportImpl with both MetadataReader and optional legacy IMetaDataImport
MetaDataImportImplTests.cs	50 unit tests using synthetic metadata built with MetadataBuilder
MetaDataImportDumpTests.cs	3 dump-based integration tests verifying semantic parity against real metadata
MultiModule debuggee	Test debuggee with non-const fields, user strings, and methods for dump tests

Implemented methods (28 cDAC, ~45 legacy fallback)

Enum (cDAC):EnumInterfaceImpls, EnumFields, EnumGenericParams, CloseEnum, CountEnum, ResetEnum

Properties (cDAC):GetTypeDefProps, GetTypeRefProps, GetMethodProps, GetFieldProps, GetMemberProps, GetInterfaceImplProps, GetNestedClassProps, GetGenericParamProps, GetMemberRefProps, GetModuleRefProps, GetParamProps, GetClassLayout, GetUserString, GetParamForMethodIndex

Blob/token (cDAC):GetRVA, GetSigFromToken, GetTypeSpecFromToken, GetCustomAttributeByName, IsValidToken, FindTypeDefByName

Assembly (cDAC):GetAssemblyProps, GetAssemblyRefProps, GetAssemblyFromScope

Legacy fallback: All remaining methods delegate to _legacyImport/_legacyImport2/_legacyAssemblyImport or return E_NOTIMPL when no legacy is available.

Native parity behaviors

• <Module> parent mapping: GetMethodProps/GetFieldProps/GetMemberRefProps map TypeDef RID 1 to mdTypeDefNil (0x00000000) via MapGlobalParentToken, matching native RegMeta
• Constant defaults: GetFieldProps/GetParamProps return ELEMENT_TYPE_VOID when no constant is present
• User string heap: GetUserString uses raw #US heap byte parsing to exactly match native blob size validation (odd-length check, terminal byte stripping)
• Assembly flags: GetAssemblyProps ORs afPublicKey into flags when public key blob is non-empty
• Truncation: All string-returning methods return CLDB_S_TRUNCATION when buffer is too small
• Record not found: GetNestedClassProps/GetClassLayout/GetAssemblyProps return CLDB_E_RECORD_NOTFOUND for missing records

Design decisions

• Explicit interface implementation: All ~73 COM methods use explicit interface notation (int IMetaDataImport.Method(...)) to keep the public surface clean
• ICustomQueryInterface: QI for IMetaDataImport returns an IMetaDataImport2 vtable so callers always get the full interface
• HCORENUM pattern: Uses GCHandle.Alloc to box MetadataEnum objects; ConcurrentDictionary<nint, byte> tracks ownership for routing CloseEnum/CountEnum/ResetEnum between cDAC and legacy handles
• #if DEBUG validation: Every cDAC-implemented method (with 2 justified exceptions) cross-checks its output against the legacy DAC in debug builds
• CopyStringToBuffer overloads: void overload for callers that don't need truncation info; out bool truncated overload for MetaDataImportImpl callers that return CLDB_S_TRUNCATION

Testing

• 1845 unit tests pass (all cDAC tests including 50 MetaDataImportImpl tests)
• 214 dump-based tests pass locally (3 are MetaDataImport-specific)
• Tests cover: enum pagination, global/non-global parent mapping, constant handling, user string char counts, truncation, nested classes, generic params, assembly properties, invalid tokens, QI vtable correctness, and E_NOTIMPL fallback behavior

[Copilot]

Pull request overview

This PR adds a managed COM-compatible IMetaDataImport/IMetaDataImport2 implementation backed by System.Reflection.Metadata.MetadataReader, and wires it into cDAC’s ClrDataModule so diagnostic tools can query metadata when running in no-fallback mode (legacy DAC unavailable).

Changes:

• Introduces generated COM interface definitions for IMetaDataImport and IMetaDataImport2.
• Implements MetadataImportWrapper to adapt MetadataReader to the COM metadata import APIs (with a subset implemented and the rest stubbed).
• Updates ClrDataModule to provide IMetaDataImport in no-fallback mode and adds unit tests validating implemented behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/native/managed/cdac/tests/MetadataImportWrapperTests.cs	Adds unit tests that exercise the wrapper’s enumeration and property APIs over synthetic metadata.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetadataImportWrapper.cs	Implements the COM wrapper over MetadataReader including enum-handle handling and a set of metadata accessors.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IMetaDataImport.cs	Adds managed [GeneratedComInterface] declarations for IMetaDataImport/IMetaDataImport2 with vtable ordering.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Hooks IMetaDataImport QI to return the managed wrapper when legacy DAC is unavailable.

[jkoritzinsky]

You'll likely need to add support for IMetaDataAssemblyImport as well for assembly-level info

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

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

[github-actions]

🤖 Copilot Code Review — PR #127028

Note

This review was AI-generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, with Claude Sonnet 4.5 and GPT-5.3-Codex sub-agents).

Holistic Assessment

Motivation: Well-justified. A managed MetaDataImportImpl wrapping System.Reflection.Metadata.MetadataReader is essential for cDAC no-fallback mode where the legacy DAC is unavailable. SOS and ClrMD depend on IMetaDataImport for metadata queries, making this a required capability.

Approach: Sound. Implementing ~30 commonly-used methods with MetadataReader while delegating ~30 rarely-used ones to the legacy DAC is pragmatic. The ICustomQueryInterface vtable redirect to handle ClrMD's beyond-vtable IMetaDataImport2 slot access is clever and well-documented. The extensive #if DEBUG validation blocks comparing output against the legacy DAC are excellent for catching parity gaps during development.

Summary: ⚠️Needs Human Review. The code is well-structured, follows cDAC conventions, and handles COM interop correctly. There are a few thread-safety concerns with lazy initialization and a minor correctness gap in IsValidToken that warrant maintainer attention. None appear to be critical bugs in practice, but a human reviewer should evaluate whether they're acceptable for the diagnostics use case.

---

Detailed Findings

⚠️IsValidToken — RID 0 early rejection incorrectly covers user string tokens

MetaDataImportImpl.cs:819-838 — IsValidToken rejects RID 0 before checking the token type:

intrid=(int)(tk&0x00FFFFFF);inttokenType=(int)(tk>>24);if(rid==0)return0;// FALSE — but this runs before the UserString branch

For metadata table tokens, RID 0 is nil/invalid. However, for user string tokens (0x70), the "RID" is actually a heap offset, and offset 0 is a valid entry in the #US heap (the required null/empty byte per ECMA-335). The native IsValidToken would return TRUE for 0x70000000 when the heap is non-empty, while this implementation returns FALSE.

Fix: Move the rid == 0 check after the UserStringTokenType branch, or restrict it to table tokens only:

constintUserStringTokenType=0x70;if(tokenType==UserStringTokenType){intheapSize=_reader.GetHeapSize(HeapIndex.UserString);returnrid<heapSize?1:0;}if(rid==0)return0;// Only for table tokens

Advisory — unlikely to cause real-world issues since offset 0 is never referenced by IL, but it is a parity gap with native. Flagged by GPT-5.3-Codex, verified by primary reviewer.

⚠️ Lazy dictionary initialization without memory barriers (thread safety)

MetaDataImportImpl.cs:554,1336 — _interfaceImplToTypeDef and _paramToMethod use ??= for lazy initialization:

_interfaceImplToTypeDef??=BuildInterfaceImplLookup();

Since MetaDataImportImpl is a COM object callable from multiple threads (as acknowledged by the use of ConcurrentDictionary for _cdacEnumHandles), two threads could race on first access. The dictionaries are fully constructed before assignment and reference writes are atomic in .NET, so the practical risk is very low. However, the codebase convention states that ??= is not thread-safe and cross-thread field access should use Volatile or Interlocked.

Volatile.Write/Volatile.Read or LazyInitializer.EnsureInitialized would address this cleanly.

Advisory — not merge-blocking. The benign-race pattern here (two threads build independent dictionaries, one wins, both are equivalent) is safe in practice on x86/x64 and very likely safe on ARM given .NET's runtime guarantees. Flagged by all three models.

⚠️ GCHandle leak if CloseEnum is never called

MetaDataImportImpl.cs:66-73 — AllocEnum allocates GCHandle instances tracked in _cdacEnumHandles, but there's no finalizer or IDisposable to clean them up if the COM consumer forgets to call CloseEnum. While this matches the native IMetaDataImport contract (callers own enum lifetime), diagnostic tools may not always clean up on error paths.

Advisory — follows the same contract as native. A finalizer iterating _cdacEnumHandles.Keys and freeing outstanding handles would be a defensive improvement. Not merge-blocking since the native API has the same contract.

✅ COM ref counting — Correct

Both ICustomQueryInterface.GetInterface implementations (in MetaDataImportImpl.cs:47 and ClrDataModule.cs:111) correctly use ComInterfaceMarshaller<IMetaDataImport2>.ConvertToUnmanaged() which returns an already-AddRef'd pointer, consistent with COM QueryInterface semantics. The IMetaDataImport→IMetaDataImport2 vtable redirect for ClrMD compatibility is validated end-to-end by the test at line 964.

✅ GetUserString raw heap parsing — Correct

MetaDataImportImpl.cs:1188-1250 — Correctly reads from raw #US heap bytes rather than using MetadataReader.GetUserString(), avoiding potential discrepancies. The odd-size validation (blobSize % sizeof(char) == 0 → corrupt) correctly enforces the terminal byte requirement per ECMA-335 §II.24.2.4. pchString correctly returns character count without null terminator, matching native RegMeta semantics.

✅ Empty catch blocks in ClrDataModule.GetInterface — Acceptable

The two bare catch {} blocks (lines 85-87, 98-100) silently swallow exceptions when probing for metadata reader availability and legacy QI support. This is appropriate: both are optional capability probes. If both fail, the method returns CustomQueryInterfaceResult.NotHandled (line 103), which is the correct degradation path.

✅ DEBUG validation blocks — Excellent

The #if DEBUG blocks throughout MetaDataImportImpl comparing cDAC output against legacy DAC output (via Debug.ValidateHResult and per-field assertions) are a strong engineering practice for ensuring semantic parity. The ValidateBlobsEqual helper for byte-level blob comparison is thorough.

✅ Test coverage — Comprehensive

~1100 lines of unit tests covering: enum pagination, string truncation with CLDB_S_TRUNCATION, not-found error paths, GetMemberProps dispatch (method vs field table index), global method parent mapping (MapGlobalParentToken), constant value handling for both fields and params, vtable slot verification for the ClrMD compatibility fix, and enum lifecycle (CountEnum, ResetEnum, CloseEnum). The dump-based integration tests validate real-world metadata against actual process dumps.

💡 Static _testProvider field in tests

MetaDataImportImplTests.cs:161 — _testProvider is a shared static field used to prevent GC collection. Consider [ThreadStatic] for parallel test safety. Low practical risk since xUnit serializes tests within a class.

💡 _metaDataImportImpl lazy init race in ClrDataModule

ClrDataModule.cs:107 — _metaDataImportImpl ??= wrapper is a documented benign race. Both wrappers are functionally equivalent, the extra one is GC'd, and line 108 re-reads to ensure consistency. Acceptable pattern.

Generated by Code Review for issue #127028 · ◷

[noahfalk]

A few nits, but overall looks good to me!

[max-charlamb]

/ba-g previous test run passed. only nit style changes from comments

[max-charlamb]

Note

This PR description was AI/Copilot-generated.

Summary

Implements a managed [GeneratedComClass] wrapper (MetaDataImportImpl) that adapts System.Reflection.Metadata.MetadataReader to the IMetaDataImport, IMetaDataImport2, and IMetaDataAssemblyImport COM interfaces. This enables SOS and ClrMD to query metadata in cDAC mode, with optional legacy DAC fallback for methods not yet implemented in the managed layer.

Motivation

In cDAC no-fallback mode, ClrDataModule.GetInterface() returned NotHandled for IMetaDataImport QIs when _legacyModulePointer == 0, meaning diagnostic tools couldn't access type/method/field metadata. The cDAC already has access to MetadataReader via the EcmaMetadata contract, so a thin COM wrapper bridges the gap.

When a legacy DAC is available, the wrapper uses it for #if DEBUG validation (asserting that cDAC and DAC produce identical results) and as a fallback for the ~45 methods not yet implemented in managed code.

Changes

File	Description
IMetaDataImport.cs	Managed [GeneratedComInterface] definitions for IMetaDataImport (51 methods), IMetaDataImport2 (8 methods), IMetaDataAssemblyImport (14 methods), ASSEMBLYMETADATA struct, and internal CldbHResults constants
MetaDataImportImpl.cs	[GeneratedComClass] implementation — 28 full cDAC implementations via MetadataReader + ~45 legacy-delegated stubs. Uses explicit interface notation.
OutputBufferHelpers.cs	CopyStringToBuffer split into two overloads: void (for callers that don't check truncation) and out bool truncated (for MetaDataImportImpl)
ClrDataModule.cs	Wire up wrapper via ICustomQueryInterface — creates MetaDataImportImpl with both MetadataReader and optional legacy IMetaDataImport
MetaDataImportImplTests.cs	50 unit tests using synthetic metadata built with MetadataBuilder
MetaDataImportDumpTests.cs	3 dump-based integration tests verifying semantic parity against real metadata
MultiModule debuggee	Test debuggee with non-const fields, user strings, and methods for dump tests

Implemented methods (28 cDAC, ~45 legacy fallback)

Enum (cDAC):EnumInterfaceImpls, EnumFields, EnumGenericParams, CloseEnum, CountEnum, ResetEnum

Properties (cDAC):GetTypeDefProps, GetTypeRefProps, GetMethodProps, GetFieldProps, GetMemberProps, GetInterfaceImplProps, GetNestedClassProps, GetGenericParamProps, GetMemberRefProps, GetModuleRefProps, GetParamProps, GetClassLayout, GetUserString, GetParamForMethodIndex

Blob/token (cDAC):GetRVA, GetSigFromToken, GetTypeSpecFromToken, GetCustomAttributeByName, IsValidToken, FindTypeDefByName

Assembly (cDAC):GetAssemblyProps, GetAssemblyRefProps, GetAssemblyFromScope

Legacy fallback: All remaining methods delegate to _legacyImport/_legacyImport2/_legacyAssemblyImport or return E_NOTIMPL when no legacy is available.

Native parity behaviors

• <Module> parent mapping: GetMethodProps/GetFieldProps/GetMemberRefProps map TypeDef RID 1 to mdTypeDefNil (0x00000000) via MapGlobalParentToken, matching native RegMeta
• Constant defaults: GetFieldProps/GetParamProps return ELEMENT_TYPE_VOID when no constant is present
• User string heap: GetUserString uses raw #US heap byte parsing to exactly match native blob size validation (odd-length check, terminal byte stripping)
• Assembly flags: GetAssemblyProps ORs afPublicKey into flags when public key blob is non-empty
• Truncation: All string-returning methods return CLDB_S_TRUNCATION when buffer is too small
• Record not found: GetNestedClassProps/GetClassLayout/GetAssemblyProps return CLDB_E_RECORD_NOTFOUND for missing records

Design decisions

• Explicit interface implementation: All ~73 COM methods use explicit interface notation (int IMetaDataImport.Method(...)) to keep the public surface clean
• ICustomQueryInterface: QI for IMetaDataImport returns an IMetaDataImport2 vtable so callers always get the full interface
• HCORENUM pattern: Uses GCHandle.Alloc to box MetadataEnum objects; ConcurrentDictionary<nint, byte> tracks ownership for routing CloseEnum/CountEnum/ResetEnum between cDAC and legacy handles
• #if DEBUG validation: Every cDAC-implemented method (with 2 justified exceptions) cross-checks its output against the legacy DAC in debug builds
• CopyStringToBuffer overloads: void overload for callers that don't need truncation info; out bool truncated overload for MetaDataImportImpl callers that return CLDB_S_TRUNCATION

Testing

• 1845 unit tests pass (all cDAC tests including 50 MetaDataImportImpl tests)
• 214 dump-based tests pass locally (3 are MetaDataImport-specific)
• Tests cover: enum pagination, global/non-global parent mapping, constant handling, user string char counts, truncation, nested classes, generic params, assembly properties, invalid tokens, QI vtable correctness, and E_NOTIMPL fallback behavior

[Copilot]

Pull request overview

This PR adds a managed COM-compatible IMetaDataImport/IMetaDataImport2 implementation backed by System.Reflection.Metadata.MetadataReader, and wires it into cDAC’s ClrDataModule so diagnostic tools can query metadata when running in no-fallback mode (legacy DAC unavailable).

Changes:

• Introduces generated COM interface definitions for IMetaDataImport and IMetaDataImport2.
• Implements MetadataImportWrapper to adapt MetadataReader to the COM metadata import APIs (with a subset implemented and the rest stubbed).
• Updates ClrDataModule to provide IMetaDataImport in no-fallback mode and adds unit tests validating implemented behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/native/managed/cdac/tests/MetadataImportWrapperTests.cs	Adds unit tests that exercise the wrapper’s enumeration and property APIs over synthetic metadata.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetadataImportWrapper.cs	Implements the COM wrapper over MetadataReader including enum-handle handling and a set of metadata accessors.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IMetaDataImport.cs	Adds managed [GeneratedComInterface] declarations for IMetaDataImport/IMetaDataImport2 with vtable ordering.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Hooks IMetaDataImport QI to return the managed wrapper when legacy DAC is unavailable.

[jkoritzinsky]

You'll likely need to add support for IMetaDataAssemblyImport as well for assembly-level info

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

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

[github-actions]

🤖 Copilot Code Review — PR #127028

Note

This review was AI-generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, with Claude Sonnet 4.5 and GPT-5.3-Codex sub-agents).

Holistic Assessment

Motivation: Well-justified. A managed MetaDataImportImpl wrapping System.Reflection.Metadata.MetadataReader is essential for cDAC no-fallback mode where the legacy DAC is unavailable. SOS and ClrMD depend on IMetaDataImport for metadata queries, making this a required capability.

Approach: Sound. Implementing ~30 commonly-used methods with MetadataReader while delegating ~30 rarely-used ones to the legacy DAC is pragmatic. The ICustomQueryInterface vtable redirect to handle ClrMD's beyond-vtable IMetaDataImport2 slot access is clever and well-documented. The extensive #if DEBUG validation blocks comparing output against the legacy DAC are excellent for catching parity gaps during development.

Summary: ⚠️Needs Human Review. The code is well-structured, follows cDAC conventions, and handles COM interop correctly. There are a few thread-safety concerns with lazy initialization and a minor correctness gap in IsValidToken that warrant maintainer attention. None appear to be critical bugs in practice, but a human reviewer should evaluate whether they're acceptable for the diagnostics use case.

---

Detailed Findings

⚠️IsValidToken — RID 0 early rejection incorrectly covers user string tokens

MetaDataImportImpl.cs:819-838 — IsValidToken rejects RID 0 before checking the token type:

intrid=(int)(tk&0x00FFFFFF);inttokenType=(int)(tk>>24);if(rid==0)return0;// FALSE — but this runs before the UserString branch

For metadata table tokens, RID 0 is nil/invalid. However, for user string tokens (0x70), the "RID" is actually a heap offset, and offset 0 is a valid entry in the #US heap (the required null/empty byte per ECMA-335). The native IsValidToken would return TRUE for 0x70000000 when the heap is non-empty, while this implementation returns FALSE.

Fix: Move the rid == 0 check after the UserStringTokenType branch, or restrict it to table tokens only:

constintUserStringTokenType=0x70;if(tokenType==UserStringTokenType){intheapSize=_reader.GetHeapSize(HeapIndex.UserString);returnrid<heapSize?1:0;}if(rid==0)return0;// Only for table tokens

Advisory — unlikely to cause real-world issues since offset 0 is never referenced by IL, but it is a parity gap with native. Flagged by GPT-5.3-Codex, verified by primary reviewer.

⚠️ Lazy dictionary initialization without memory barriers (thread safety)

MetaDataImportImpl.cs:554,1336 — _interfaceImplToTypeDef and _paramToMethod use ??= for lazy initialization:

_interfaceImplToTypeDef??=BuildInterfaceImplLookup();

Since MetaDataImportImpl is a COM object callable from multiple threads (as acknowledged by the use of ConcurrentDictionary for _cdacEnumHandles), two threads could race on first access. The dictionaries are fully constructed before assignment and reference writes are atomic in .NET, so the practical risk is very low. However, the codebase convention states that ??= is not thread-safe and cross-thread field access should use Volatile or Interlocked.

Volatile.Write/Volatile.Read or LazyInitializer.EnsureInitialized would address this cleanly.

Advisory — not merge-blocking. The benign-race pattern here (two threads build independent dictionaries, one wins, both are equivalent) is safe in practice on x86/x64 and very likely safe on ARM given .NET's runtime guarantees. Flagged by all three models.

⚠️ GCHandle leak if CloseEnum is never called

MetaDataImportImpl.cs:66-73 — AllocEnum allocates GCHandle instances tracked in _cdacEnumHandles, but there's no finalizer or IDisposable to clean them up if the COM consumer forgets to call CloseEnum. While this matches the native IMetaDataImport contract (callers own enum lifetime), diagnostic tools may not always clean up on error paths.

Advisory — follows the same contract as native. A finalizer iterating _cdacEnumHandles.Keys and freeing outstanding handles would be a defensive improvement. Not merge-blocking since the native API has the same contract.

✅ COM ref counting — Correct

Both ICustomQueryInterface.GetInterface implementations (in MetaDataImportImpl.cs:47 and ClrDataModule.cs:111) correctly use ComInterfaceMarshaller<IMetaDataImport2>.ConvertToUnmanaged() which returns an already-AddRef'd pointer, consistent with COM QueryInterface semantics. The IMetaDataImport→IMetaDataImport2 vtable redirect for ClrMD compatibility is validated end-to-end by the test at line 964.

✅ GetUserString raw heap parsing — Correct

MetaDataImportImpl.cs:1188-1250 — Correctly reads from raw #US heap bytes rather than using MetadataReader.GetUserString(), avoiding potential discrepancies. The odd-size validation (blobSize % sizeof(char) == 0 → corrupt) correctly enforces the terminal byte requirement per ECMA-335 §II.24.2.4. pchString correctly returns character count without null terminator, matching native RegMeta semantics.

✅ Empty catch blocks in ClrDataModule.GetInterface — Acceptable

The two bare catch {} blocks (lines 85-87, 98-100) silently swallow exceptions when probing for metadata reader availability and legacy QI support. This is appropriate: both are optional capability probes. If both fail, the method returns CustomQueryInterfaceResult.NotHandled (line 103), which is the correct degradation path.

✅ DEBUG validation blocks — Excellent

The #if DEBUG blocks throughout MetaDataImportImpl comparing cDAC output against legacy DAC output (via Debug.ValidateHResult and per-field assertions) are a strong engineering practice for ensuring semantic parity. The ValidateBlobsEqual helper for byte-level blob comparison is thorough.

✅ Test coverage — Comprehensive

~1100 lines of unit tests covering: enum pagination, string truncation with CLDB_S_TRUNCATION, not-found error paths, GetMemberProps dispatch (method vs field table index), global method parent mapping (MapGlobalParentToken), constant value handling for both fields and params, vtable slot verification for the ClrMD compatibility fix, and enum lifecycle (CountEnum, ResetEnum, CloseEnum). The dump-based integration tests validate real-world metadata against actual process dumps.

💡 Static _testProvider field in tests

MetaDataImportImplTests.cs:161 — _testProvider is a shared static field used to prevent GC collection. Consider [ThreadStatic] for parallel test safety. Low practical risk since xUnit serializes tests within a class.

💡 _metaDataImportImpl lazy init race in ClrDataModule

ClrDataModule.cs:107 — _metaDataImportImpl ??= wrapper is a documented benign race. Both wrappers are functionally equivalent, the extra one is GC'd, and line 108 re-reads to ensure consistency. Acceptable pattern.

Generated by Code Review for issue #127028 · ◷

[noahfalk]

A few nits, but overall looks good to me!

[max-charlamb]

/ba-g previous test run passed. only nit style changes from comments

[max-charlamb]

Note

This PR description was AI/Copilot-generated.

Summary

Implements a managed [GeneratedComClass] wrapper (MetaDataImportImpl) that adapts System.Reflection.Metadata.MetadataReader to the IMetaDataImport, IMetaDataImport2, and IMetaDataAssemblyImport COM interfaces. This enables SOS and ClrMD to query metadata in cDAC mode, with optional legacy DAC fallback for methods not yet implemented in the managed layer.

Motivation

In cDAC no-fallback mode, ClrDataModule.GetInterface() returned NotHandled for IMetaDataImport QIs when _legacyModulePointer == 0, meaning diagnostic tools couldn't access type/method/field metadata. The cDAC already has access to MetadataReader via the EcmaMetadata contract, so a thin COM wrapper bridges the gap.

When a legacy DAC is available, the wrapper uses it for #if DEBUG validation (asserting that cDAC and DAC produce identical results) and as a fallback for the ~45 methods not yet implemented in managed code.

Changes

File	Description
IMetaDataImport.cs	Managed [GeneratedComInterface] definitions for IMetaDataImport (51 methods), IMetaDataImport2 (8 methods), IMetaDataAssemblyImport (14 methods), ASSEMBLYMETADATA struct, and internal CldbHResults constants
MetaDataImportImpl.cs	[GeneratedComClass] implementation — 28 full cDAC implementations via MetadataReader + ~45 legacy-delegated stubs. Uses explicit interface notation.
OutputBufferHelpers.cs	CopyStringToBuffer split into two overloads: void (for callers that don't check truncation) and out bool truncated (for MetaDataImportImpl)
ClrDataModule.cs	Wire up wrapper via ICustomQueryInterface — creates MetaDataImportImpl with both MetadataReader and optional legacy IMetaDataImport
MetaDataImportImplTests.cs	50 unit tests using synthetic metadata built with MetadataBuilder
MetaDataImportDumpTests.cs	3 dump-based integration tests verifying semantic parity against real metadata
MultiModule debuggee	Test debuggee with non-const fields, user strings, and methods for dump tests

Implemented methods (28 cDAC, ~45 legacy fallback)

Enum (cDAC):EnumInterfaceImpls, EnumFields, EnumGenericParams, CloseEnum, CountEnum, ResetEnum

Properties (cDAC):GetTypeDefProps, GetTypeRefProps, GetMethodProps, GetFieldProps, GetMemberProps, GetInterfaceImplProps, GetNestedClassProps, GetGenericParamProps, GetMemberRefProps, GetModuleRefProps, GetParamProps, GetClassLayout, GetUserString, GetParamForMethodIndex

Blob/token (cDAC):GetRVA, GetSigFromToken, GetTypeSpecFromToken, GetCustomAttributeByName, IsValidToken, FindTypeDefByName

Assembly (cDAC):GetAssemblyProps, GetAssemblyRefProps, GetAssemblyFromScope

Legacy fallback: All remaining methods delegate to _legacyImport/_legacyImport2/_legacyAssemblyImport or return E_NOTIMPL when no legacy is available.

Native parity behaviors

• <Module> parent mapping: GetMethodProps/GetFieldProps/GetMemberRefProps map TypeDef RID 1 to mdTypeDefNil (0x00000000) via MapGlobalParentToken, matching native RegMeta
• Constant defaults: GetFieldProps/GetParamProps return ELEMENT_TYPE_VOID when no constant is present
• User string heap: GetUserString uses raw #US heap byte parsing to exactly match native blob size validation (odd-length check, terminal byte stripping)
• Assembly flags: GetAssemblyProps ORs afPublicKey into flags when public key blob is non-empty
• Truncation: All string-returning methods return CLDB_S_TRUNCATION when buffer is too small
• Record not found: GetNestedClassProps/GetClassLayout/GetAssemblyProps return CLDB_E_RECORD_NOTFOUND for missing records

Design decisions

• Explicit interface implementation: All ~73 COM methods use explicit interface notation (int IMetaDataImport.Method(...)) to keep the public surface clean
• ICustomQueryInterface: QI for IMetaDataImport returns an IMetaDataImport2 vtable so callers always get the full interface
• HCORENUM pattern: Uses GCHandle.Alloc to box MetadataEnum objects; ConcurrentDictionary<nint, byte> tracks ownership for routing CloseEnum/CountEnum/ResetEnum between cDAC and legacy handles
• #if DEBUG validation: Every cDAC-implemented method (with 2 justified exceptions) cross-checks its output against the legacy DAC in debug builds
• CopyStringToBuffer overloads: void overload for callers that don't need truncation info; out bool truncated overload for MetaDataImportImpl callers that return CLDB_S_TRUNCATION

Testing

• 1845 unit tests pass (all cDAC tests including 50 MetaDataImportImpl tests)
• 214 dump-based tests pass locally (3 are MetaDataImport-specific)
• Tests cover: enum pagination, global/non-global parent mapping, constant handling, user string char counts, truncation, nested classes, generic params, assembly properties, invalid tokens, QI vtable correctness, and E_NOTIMPL fallback behavior

[Copilot]

Pull request overview

This PR adds a managed COM-compatible IMetaDataImport/IMetaDataImport2 implementation backed by System.Reflection.Metadata.MetadataReader, and wires it into cDAC’s ClrDataModule so diagnostic tools can query metadata when running in no-fallback mode (legacy DAC unavailable).

Changes:

• Introduces generated COM interface definitions for IMetaDataImport and IMetaDataImport2.
• Implements MetadataImportWrapper to adapt MetadataReader to the COM metadata import APIs (with a subset implemented and the rest stubbed).
• Updates ClrDataModule to provide IMetaDataImport in no-fallback mode and adds unit tests validating implemented behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/native/managed/cdac/tests/MetadataImportWrapperTests.cs	Adds unit tests that exercise the wrapper’s enumeration and property APIs over synthetic metadata.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetadataImportWrapper.cs	Implements the COM wrapper over MetadataReader including enum-handle handling and a set of metadata accessors.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IMetaDataImport.cs	Adds managed [GeneratedComInterface] declarations for IMetaDataImport/IMetaDataImport2 with vtable ordering.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Hooks IMetaDataImport QI to return the managed wrapper when legacy DAC is unavailable.

[jkoritzinsky]

You'll likely need to add support for IMetaDataAssemblyImport as well for assembly-level info

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

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

[github-actions]

🤖 Copilot Code Review — PR #127028

Note

This review was AI-generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, with Claude Sonnet 4.5 and GPT-5.3-Codex sub-agents).

Holistic Assessment

Motivation: Well-justified. A managed MetaDataImportImpl wrapping System.Reflection.Metadata.MetadataReader is essential for cDAC no-fallback mode where the legacy DAC is unavailable. SOS and ClrMD depend on IMetaDataImport for metadata queries, making this a required capability.

Approach: Sound. Implementing ~30 commonly-used methods with MetadataReader while delegating ~30 rarely-used ones to the legacy DAC is pragmatic. The ICustomQueryInterface vtable redirect to handle ClrMD's beyond-vtable IMetaDataImport2 slot access is clever and well-documented. The extensive #if DEBUG validation blocks comparing output against the legacy DAC are excellent for catching parity gaps during development.

Summary: ⚠️Needs Human Review. The code is well-structured, follows cDAC conventions, and handles COM interop correctly. There are a few thread-safety concerns with lazy initialization and a minor correctness gap in IsValidToken that warrant maintainer attention. None appear to be critical bugs in practice, but a human reviewer should evaluate whether they're acceptable for the diagnostics use case.

---

Detailed Findings

⚠️IsValidToken — RID 0 early rejection incorrectly covers user string tokens

MetaDataImportImpl.cs:819-838 — IsValidToken rejects RID 0 before checking the token type:

intrid=(int)(tk&0x00FFFFFF);inttokenType=(int)(tk>>24);if(rid==0)return0;// FALSE — but this runs before the UserString branch

For metadata table tokens, RID 0 is nil/invalid. However, for user string tokens (0x70), the "RID" is actually a heap offset, and offset 0 is a valid entry in the #US heap (the required null/empty byte per ECMA-335). The native IsValidToken would return TRUE for 0x70000000 when the heap is non-empty, while this implementation returns FALSE.

Fix: Move the rid == 0 check after the UserStringTokenType branch, or restrict it to table tokens only:

constintUserStringTokenType=0x70;if(tokenType==UserStringTokenType){intheapSize=_reader.GetHeapSize(HeapIndex.UserString);returnrid<heapSize?1:0;}if(rid==0)return0;// Only for table tokens

Advisory — unlikely to cause real-world issues since offset 0 is never referenced by IL, but it is a parity gap with native. Flagged by GPT-5.3-Codex, verified by primary reviewer.

⚠️ Lazy dictionary initialization without memory barriers (thread safety)

MetaDataImportImpl.cs:554,1336 — _interfaceImplToTypeDef and _paramToMethod use ??= for lazy initialization:

_interfaceImplToTypeDef??=BuildInterfaceImplLookup();

Since MetaDataImportImpl is a COM object callable from multiple threads (as acknowledged by the use of ConcurrentDictionary for _cdacEnumHandles), two threads could race on first access. The dictionaries are fully constructed before assignment and reference writes are atomic in .NET, so the practical risk is very low. However, the codebase convention states that ??= is not thread-safe and cross-thread field access should use Volatile or Interlocked.

Volatile.Write/Volatile.Read or LazyInitializer.EnsureInitialized would address this cleanly.

Advisory — not merge-blocking. The benign-race pattern here (two threads build independent dictionaries, one wins, both are equivalent) is safe in practice on x86/x64 and very likely safe on ARM given .NET's runtime guarantees. Flagged by all three models.

⚠️ GCHandle leak if CloseEnum is never called

MetaDataImportImpl.cs:66-73 — AllocEnum allocates GCHandle instances tracked in _cdacEnumHandles, but there's no finalizer or IDisposable to clean them up if the COM consumer forgets to call CloseEnum. While this matches the native IMetaDataImport contract (callers own enum lifetime), diagnostic tools may not always clean up on error paths.

Advisory — follows the same contract as native. A finalizer iterating _cdacEnumHandles.Keys and freeing outstanding handles would be a defensive improvement. Not merge-blocking since the native API has the same contract.

✅ COM ref counting — Correct

Both ICustomQueryInterface.GetInterface implementations (in MetaDataImportImpl.cs:47 and ClrDataModule.cs:111) correctly use ComInterfaceMarshaller<IMetaDataImport2>.ConvertToUnmanaged() which returns an already-AddRef'd pointer, consistent with COM QueryInterface semantics. The IMetaDataImport→IMetaDataImport2 vtable redirect for ClrMD compatibility is validated end-to-end by the test at line 964.

✅ GetUserString raw heap parsing — Correct

MetaDataImportImpl.cs:1188-1250 — Correctly reads from raw #US heap bytes rather than using MetadataReader.GetUserString(), avoiding potential discrepancies. The odd-size validation (blobSize % sizeof(char) == 0 → corrupt) correctly enforces the terminal byte requirement per ECMA-335 §II.24.2.4. pchString correctly returns character count without null terminator, matching native RegMeta semantics.

✅ Empty catch blocks in ClrDataModule.GetInterface — Acceptable

The two bare catch {} blocks (lines 85-87, 98-100) silently swallow exceptions when probing for metadata reader availability and legacy QI support. This is appropriate: both are optional capability probes. If both fail, the method returns CustomQueryInterfaceResult.NotHandled (line 103), which is the correct degradation path.

✅ DEBUG validation blocks — Excellent

The #if DEBUG blocks throughout MetaDataImportImpl comparing cDAC output against legacy DAC output (via Debug.ValidateHResult and per-field assertions) are a strong engineering practice for ensuring semantic parity. The ValidateBlobsEqual helper for byte-level blob comparison is thorough.

✅ Test coverage — Comprehensive

~1100 lines of unit tests covering: enum pagination, string truncation with CLDB_S_TRUNCATION, not-found error paths, GetMemberProps dispatch (method vs field table index), global method parent mapping (MapGlobalParentToken), constant value handling for both fields and params, vtable slot verification for the ClrMD compatibility fix, and enum lifecycle (CountEnum, ResetEnum, CloseEnum). The dump-based integration tests validate real-world metadata against actual process dumps.

💡 Static _testProvider field in tests

MetaDataImportImplTests.cs:161 — _testProvider is a shared static field used to prevent GC collection. Consider [ThreadStatic] for parallel test safety. Low practical risk since xUnit serializes tests within a class.

💡 _metaDataImportImpl lazy init race in ClrDataModule

ClrDataModule.cs:107 — _metaDataImportImpl ??= wrapper is a documented benign race. Both wrappers are functionally equivalent, the extra one is GC'd, and line 108 re-reads to ensure consistency. Acceptable pattern.

Generated by Code Review for issue #127028 · ◷

[noahfalk]

A few nits, but overall looks good to me!

[max-charlamb]

/ba-g previous test run passed. only nit style changes from comments

[max-charlamb]

Note

This PR description was AI/Copilot-generated.

Summary

Implements a managed [GeneratedComClass] wrapper (MetaDataImportImpl) that adapts System.Reflection.Metadata.MetadataReader to the IMetaDataImport, IMetaDataImport2, and IMetaDataAssemblyImport COM interfaces. This enables SOS and ClrMD to query metadata in cDAC mode, with optional legacy DAC fallback for methods not yet implemented in the managed layer.

Motivation

In cDAC no-fallback mode, ClrDataModule.GetInterface() returned NotHandled for IMetaDataImport QIs when _legacyModulePointer == 0, meaning diagnostic tools couldn't access type/method/field metadata. The cDAC already has access to MetadataReader via the EcmaMetadata contract, so a thin COM wrapper bridges the gap.

When a legacy DAC is available, the wrapper uses it for #if DEBUG validation (asserting that cDAC and DAC produce identical results) and as a fallback for the ~45 methods not yet implemented in managed code.

Changes

File	Description
IMetaDataImport.cs	Managed [GeneratedComInterface] definitions for IMetaDataImport (51 methods), IMetaDataImport2 (8 methods), IMetaDataAssemblyImport (14 methods), ASSEMBLYMETADATA struct, and internal CldbHResults constants
MetaDataImportImpl.cs	[GeneratedComClass] implementation — 28 full cDAC implementations via MetadataReader + ~45 legacy-delegated stubs. Uses explicit interface notation.
OutputBufferHelpers.cs	CopyStringToBuffer split into two overloads: void (for callers that don't check truncation) and out bool truncated (for MetaDataImportImpl)
ClrDataModule.cs	Wire up wrapper via ICustomQueryInterface — creates MetaDataImportImpl with both MetadataReader and optional legacy IMetaDataImport
MetaDataImportImplTests.cs	50 unit tests using synthetic metadata built with MetadataBuilder
MetaDataImportDumpTests.cs	3 dump-based integration tests verifying semantic parity against real metadata
MultiModule debuggee	Test debuggee with non-const fields, user strings, and methods for dump tests

Implemented methods (28 cDAC, ~45 legacy fallback)

Enum (cDAC):EnumInterfaceImpls, EnumFields, EnumGenericParams, CloseEnum, CountEnum, ResetEnum

Properties (cDAC):GetTypeDefProps, GetTypeRefProps, GetMethodProps, GetFieldProps, GetMemberProps, GetInterfaceImplProps, GetNestedClassProps, GetGenericParamProps, GetMemberRefProps, GetModuleRefProps, GetParamProps, GetClassLayout, GetUserString, GetParamForMethodIndex

Blob/token (cDAC):GetRVA, GetSigFromToken, GetTypeSpecFromToken, GetCustomAttributeByName, IsValidToken, FindTypeDefByName

Assembly (cDAC):GetAssemblyProps, GetAssemblyRefProps, GetAssemblyFromScope

Legacy fallback: All remaining methods delegate to _legacyImport/_legacyImport2/_legacyAssemblyImport or return E_NOTIMPL when no legacy is available.

Native parity behaviors

• <Module> parent mapping: GetMethodProps/GetFieldProps/GetMemberRefProps map TypeDef RID 1 to mdTypeDefNil (0x00000000) via MapGlobalParentToken, matching native RegMeta
• Constant defaults: GetFieldProps/GetParamProps return ELEMENT_TYPE_VOID when no constant is present
• User string heap: GetUserString uses raw #US heap byte parsing to exactly match native blob size validation (odd-length check, terminal byte stripping)
• Assembly flags: GetAssemblyProps ORs afPublicKey into flags when public key blob is non-empty
• Truncation: All string-returning methods return CLDB_S_TRUNCATION when buffer is too small
• Record not found: GetNestedClassProps/GetClassLayout/GetAssemblyProps return CLDB_E_RECORD_NOTFOUND for missing records

Design decisions

• Explicit interface implementation: All ~73 COM methods use explicit interface notation (int IMetaDataImport.Method(...)) to keep the public surface clean
• ICustomQueryInterface: QI for IMetaDataImport returns an IMetaDataImport2 vtable so callers always get the full interface
• HCORENUM pattern: Uses GCHandle.Alloc to box MetadataEnum objects; ConcurrentDictionary<nint, byte> tracks ownership for routing CloseEnum/CountEnum/ResetEnum between cDAC and legacy handles
• #if DEBUG validation: Every cDAC-implemented method (with 2 justified exceptions) cross-checks its output against the legacy DAC in debug builds
• CopyStringToBuffer overloads: void overload for callers that don't need truncation info; out bool truncated overload for MetaDataImportImpl callers that return CLDB_S_TRUNCATION

Testing

• 1845 unit tests pass (all cDAC tests including 50 MetaDataImportImpl tests)
• 214 dump-based tests pass locally (3 are MetaDataImport-specific)
• Tests cover: enum pagination, global/non-global parent mapping, constant handling, user string char counts, truncation, nested classes, generic params, assembly properties, invalid tokens, QI vtable correctness, and E_NOTIMPL fallback behavior

[Copilot]

Pull request overview

This PR adds a managed COM-compatible IMetaDataImport/IMetaDataImport2 implementation backed by System.Reflection.Metadata.MetadataReader, and wires it into cDAC’s ClrDataModule so diagnostic tools can query metadata when running in no-fallback mode (legacy DAC unavailable).

Changes:

• Introduces generated COM interface definitions for IMetaDataImport and IMetaDataImport2.
• Implements MetadataImportWrapper to adapt MetadataReader to the COM metadata import APIs (with a subset implemented and the rest stubbed).
• Updates ClrDataModule to provide IMetaDataImport in no-fallback mode and adds unit tests validating implemented behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/native/managed/cdac/tests/MetadataImportWrapperTests.cs	Adds unit tests that exercise the wrapper’s enumeration and property APIs over synthetic metadata.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetadataImportWrapper.cs	Implements the COM wrapper over MetadataReader including enum-handle handling and a set of metadata accessors.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IMetaDataImport.cs	Adds managed [GeneratedComInterface] declarations for IMetaDataImport/IMetaDataImport2 with vtable ordering.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Hooks IMetaDataImport QI to return the managed wrapper when legacy DAC is unavailable.

[jkoritzinsky]

You'll likely need to add support for IMetaDataAssemblyImport as well for assembly-level info

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

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

[github-actions]

🤖 Copilot Code Review — PR #127028

Note

This review was AI-generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, with Claude Sonnet 4.5 and GPT-5.3-Codex sub-agents).

Holistic Assessment

Motivation: Well-justified. A managed MetaDataImportImpl wrapping System.Reflection.Metadata.MetadataReader is essential for cDAC no-fallback mode where the legacy DAC is unavailable. SOS and ClrMD depend on IMetaDataImport for metadata queries, making this a required capability.

Approach: Sound. Implementing ~30 commonly-used methods with MetadataReader while delegating ~30 rarely-used ones to the legacy DAC is pragmatic. The ICustomQueryInterface vtable redirect to handle ClrMD's beyond-vtable IMetaDataImport2 slot access is clever and well-documented. The extensive #if DEBUG validation blocks comparing output against the legacy DAC are excellent for catching parity gaps during development.

Summary: ⚠️Needs Human Review. The code is well-structured, follows cDAC conventions, and handles COM interop correctly. There are a few thread-safety concerns with lazy initialization and a minor correctness gap in IsValidToken that warrant maintainer attention. None appear to be critical bugs in practice, but a human reviewer should evaluate whether they're acceptable for the diagnostics use case.

---

Detailed Findings

⚠️IsValidToken — RID 0 early rejection incorrectly covers user string tokens

MetaDataImportImpl.cs:819-838 — IsValidToken rejects RID 0 before checking the token type:

intrid=(int)(tk&0x00FFFFFF);inttokenType=(int)(tk>>24);if(rid==0)return0;// FALSE — but this runs before the UserString branch

For metadata table tokens, RID 0 is nil/invalid. However, for user string tokens (0x70), the "RID" is actually a heap offset, and offset 0 is a valid entry in the #US heap (the required null/empty byte per ECMA-335). The native IsValidToken would return TRUE for 0x70000000 when the heap is non-empty, while this implementation returns FALSE.

Fix: Move the rid == 0 check after the UserStringTokenType branch, or restrict it to table tokens only:

constintUserStringTokenType=0x70;if(tokenType==UserStringTokenType){intheapSize=_reader.GetHeapSize(HeapIndex.UserString);returnrid<heapSize?1:0;}if(rid==0)return0;// Only for table tokens

Advisory — unlikely to cause real-world issues since offset 0 is never referenced by IL, but it is a parity gap with native. Flagged by GPT-5.3-Codex, verified by primary reviewer.

⚠️ Lazy dictionary initialization without memory barriers (thread safety)

MetaDataImportImpl.cs:554,1336 — _interfaceImplToTypeDef and _paramToMethod use ??= for lazy initialization:

_interfaceImplToTypeDef??=BuildInterfaceImplLookup();

Since MetaDataImportImpl is a COM object callable from multiple threads (as acknowledged by the use of ConcurrentDictionary for _cdacEnumHandles), two threads could race on first access. The dictionaries are fully constructed before assignment and reference writes are atomic in .NET, so the practical risk is very low. However, the codebase convention states that ??= is not thread-safe and cross-thread field access should use Volatile or Interlocked.

Volatile.Write/Volatile.Read or LazyInitializer.EnsureInitialized would address this cleanly.

Advisory — not merge-blocking. The benign-race pattern here (two threads build independent dictionaries, one wins, both are equivalent) is safe in practice on x86/x64 and very likely safe on ARM given .NET's runtime guarantees. Flagged by all three models.

⚠️ GCHandle leak if CloseEnum is never called

MetaDataImportImpl.cs:66-73 — AllocEnum allocates GCHandle instances tracked in _cdacEnumHandles, but there's no finalizer or IDisposable to clean them up if the COM consumer forgets to call CloseEnum. While this matches the native IMetaDataImport contract (callers own enum lifetime), diagnostic tools may not always clean up on error paths.

Advisory — follows the same contract as native. A finalizer iterating _cdacEnumHandles.Keys and freeing outstanding handles would be a defensive improvement. Not merge-blocking since the native API has the same contract.

✅ COM ref counting — Correct

Both ICustomQueryInterface.GetInterface implementations (in MetaDataImportImpl.cs:47 and ClrDataModule.cs:111) correctly use ComInterfaceMarshaller<IMetaDataImport2>.ConvertToUnmanaged() which returns an already-AddRef'd pointer, consistent with COM QueryInterface semantics. The IMetaDataImport→IMetaDataImport2 vtable redirect for ClrMD compatibility is validated end-to-end by the test at line 964.

✅ GetUserString raw heap parsing — Correct

MetaDataImportImpl.cs:1188-1250 — Correctly reads from raw #US heap bytes rather than using MetadataReader.GetUserString(), avoiding potential discrepancies. The odd-size validation (blobSize % sizeof(char) == 0 → corrupt) correctly enforces the terminal byte requirement per ECMA-335 §II.24.2.4. pchString correctly returns character count without null terminator, matching native RegMeta semantics.

✅ Empty catch blocks in ClrDataModule.GetInterface — Acceptable

The two bare catch {} blocks (lines 85-87, 98-100) silently swallow exceptions when probing for metadata reader availability and legacy QI support. This is appropriate: both are optional capability probes. If both fail, the method returns CustomQueryInterfaceResult.NotHandled (line 103), which is the correct degradation path.

✅ DEBUG validation blocks — Excellent

The #if DEBUG blocks throughout MetaDataImportImpl comparing cDAC output against legacy DAC output (via Debug.ValidateHResult and per-field assertions) are a strong engineering practice for ensuring semantic parity. The ValidateBlobsEqual helper for byte-level blob comparison is thorough.

✅ Test coverage — Comprehensive

~1100 lines of unit tests covering: enum pagination, string truncation with CLDB_S_TRUNCATION, not-found error paths, GetMemberProps dispatch (method vs field table index), global method parent mapping (MapGlobalParentToken), constant value handling for both fields and params, vtable slot verification for the ClrMD compatibility fix, and enum lifecycle (CountEnum, ResetEnum, CloseEnum). The dump-based integration tests validate real-world metadata against actual process dumps.

💡 Static _testProvider field in tests

MetaDataImportImplTests.cs:161 — _testProvider is a shared static field used to prevent GC collection. Consider [ThreadStatic] for parallel test safety. Low practical risk since xUnit serializes tests within a class.

💡 _metaDataImportImpl lazy init race in ClrDataModule

ClrDataModule.cs:107 — _metaDataImportImpl ??= wrapper is a documented benign race. Both wrappers are functionally equivalent, the extra one is GC'd, and line 108 re-reads to ensure consistency. Acceptable pattern.

Generated by Code Review for issue #127028 · ◷

[noahfalk]

A few nits, but overall looks good to me!

[max-charlamb]

/ba-g previous test run passed. only nit style changes from comments

[max-charlamb]

Note

This PR description was AI/Copilot-generated.

Summary

Implements a managed [GeneratedComClass] wrapper (MetaDataImportImpl) that adapts System.Reflection.Metadata.MetadataReader to the IMetaDataImport, IMetaDataImport2, and IMetaDataAssemblyImport COM interfaces. This enables SOS and ClrMD to query metadata in cDAC mode, with optional legacy DAC fallback for methods not yet implemented in the managed layer.

Motivation

In cDAC no-fallback mode, ClrDataModule.GetInterface() returned NotHandled for IMetaDataImport QIs when _legacyModulePointer == 0, meaning diagnostic tools couldn't access type/method/field metadata. The cDAC already has access to MetadataReader via the EcmaMetadata contract, so a thin COM wrapper bridges the gap.

When a legacy DAC is available, the wrapper uses it for #if DEBUG validation (asserting that cDAC and DAC produce identical results) and as a fallback for the ~45 methods not yet implemented in managed code.

Changes

File	Description
IMetaDataImport.cs	Managed [GeneratedComInterface] definitions for IMetaDataImport (51 methods), IMetaDataImport2 (8 methods), IMetaDataAssemblyImport (14 methods), ASSEMBLYMETADATA struct, and internal CldbHResults constants
MetaDataImportImpl.cs	[GeneratedComClass] implementation — 28 full cDAC implementations via MetadataReader + ~45 legacy-delegated stubs. Uses explicit interface notation.
OutputBufferHelpers.cs	CopyStringToBuffer split into two overloads: void (for callers that don't check truncation) and out bool truncated (for MetaDataImportImpl)
ClrDataModule.cs	Wire up wrapper via ICustomQueryInterface — creates MetaDataImportImpl with both MetadataReader and optional legacy IMetaDataImport
MetaDataImportImplTests.cs	50 unit tests using synthetic metadata built with MetadataBuilder
MetaDataImportDumpTests.cs	3 dump-based integration tests verifying semantic parity against real metadata
MultiModule debuggee	Test debuggee with non-const fields, user strings, and methods for dump tests

Implemented methods (28 cDAC, ~45 legacy fallback)

Enum (cDAC):EnumInterfaceImpls, EnumFields, EnumGenericParams, CloseEnum, CountEnum, ResetEnum

Properties (cDAC):GetTypeDefProps, GetTypeRefProps, GetMethodProps, GetFieldProps, GetMemberProps, GetInterfaceImplProps, GetNestedClassProps, GetGenericParamProps, GetMemberRefProps, GetModuleRefProps, GetParamProps, GetClassLayout, GetUserString, GetParamForMethodIndex

Blob/token (cDAC):GetRVA, GetSigFromToken, GetTypeSpecFromToken, GetCustomAttributeByName, IsValidToken, FindTypeDefByName

Assembly (cDAC):GetAssemblyProps, GetAssemblyRefProps, GetAssemblyFromScope

Legacy fallback: All remaining methods delegate to _legacyImport/_legacyImport2/_legacyAssemblyImport or return E_NOTIMPL when no legacy is available.

Native parity behaviors

• <Module> parent mapping: GetMethodProps/GetFieldProps/GetMemberRefProps map TypeDef RID 1 to mdTypeDefNil (0x00000000) via MapGlobalParentToken, matching native RegMeta
• Constant defaults: GetFieldProps/GetParamProps return ELEMENT_TYPE_VOID when no constant is present
• User string heap: GetUserString uses raw #US heap byte parsing to exactly match native blob size validation (odd-length check, terminal byte stripping)
• Assembly flags: GetAssemblyProps ORs afPublicKey into flags when public key blob is non-empty
• Truncation: All string-returning methods return CLDB_S_TRUNCATION when buffer is too small
• Record not found: GetNestedClassProps/GetClassLayout/GetAssemblyProps return CLDB_E_RECORD_NOTFOUND for missing records

Design decisions

• Explicit interface implementation: All ~73 COM methods use explicit interface notation (int IMetaDataImport.Method(...)) to keep the public surface clean
• ICustomQueryInterface: QI for IMetaDataImport returns an IMetaDataImport2 vtable so callers always get the full interface
• HCORENUM pattern: Uses GCHandle.Alloc to box MetadataEnum objects; ConcurrentDictionary<nint, byte> tracks ownership for routing CloseEnum/CountEnum/ResetEnum between cDAC and legacy handles
• #if DEBUG validation: Every cDAC-implemented method (with 2 justified exceptions) cross-checks its output against the legacy DAC in debug builds
• CopyStringToBuffer overloads: void overload for callers that don't need truncation info; out bool truncated overload for MetaDataImportImpl callers that return CLDB_S_TRUNCATION

Testing

• 1845 unit tests pass (all cDAC tests including 50 MetaDataImportImpl tests)
• 214 dump-based tests pass locally (3 are MetaDataImport-specific)
• Tests cover: enum pagination, global/non-global parent mapping, constant handling, user string char counts, truncation, nested classes, generic params, assembly properties, invalid tokens, QI vtable correctness, and E_NOTIMPL fallback behavior

[Copilot]

Pull request overview

This PR adds a managed COM-compatible IMetaDataImport/IMetaDataImport2 implementation backed by System.Reflection.Metadata.MetadataReader, and wires it into cDAC’s ClrDataModule so diagnostic tools can query metadata when running in no-fallback mode (legacy DAC unavailable).

Changes:

• Introduces generated COM interface definitions for IMetaDataImport and IMetaDataImport2.
• Implements MetadataImportWrapper to adapt MetadataReader to the COM metadata import APIs (with a subset implemented and the rest stubbed).
• Updates ClrDataModule to provide IMetaDataImport in no-fallback mode and adds unit tests validating implemented behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/native/managed/cdac/tests/MetadataImportWrapperTests.cs	Adds unit tests that exercise the wrapper’s enumeration and property APIs over synthetic metadata.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetadataImportWrapper.cs	Implements the COM wrapper over MetadataReader including enum-handle handling and a set of metadata accessors.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IMetaDataImport.cs	Adds managed [GeneratedComInterface] declarations for IMetaDataImport/IMetaDataImport2 with vtable ordering.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Hooks IMetaDataImport QI to return the managed wrapper when legacy DAC is unavailable.

[jkoritzinsky]

You'll likely need to add support for IMetaDataAssemblyImport as well for assembly-level info

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

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

[github-actions]

🤖 Copilot Code Review — PR #127028

Note

This review was AI-generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, with Claude Sonnet 4.5 and GPT-5.3-Codex sub-agents).

Holistic Assessment

Motivation: Well-justified. A managed MetaDataImportImpl wrapping System.Reflection.Metadata.MetadataReader is essential for cDAC no-fallback mode where the legacy DAC is unavailable. SOS and ClrMD depend on IMetaDataImport for metadata queries, making this a required capability.

Approach: Sound. Implementing ~30 commonly-used methods with MetadataReader while delegating ~30 rarely-used ones to the legacy DAC is pragmatic. The ICustomQueryInterface vtable redirect to handle ClrMD's beyond-vtable IMetaDataImport2 slot access is clever and well-documented. The extensive #if DEBUG validation blocks comparing output against the legacy DAC are excellent for catching parity gaps during development.

Summary: ⚠️Needs Human Review. The code is well-structured, follows cDAC conventions, and handles COM interop correctly. There are a few thread-safety concerns with lazy initialization and a minor correctness gap in IsValidToken that warrant maintainer attention. None appear to be critical bugs in practice, but a human reviewer should evaluate whether they're acceptable for the diagnostics use case.

---

Detailed Findings

⚠️IsValidToken — RID 0 early rejection incorrectly covers user string tokens

MetaDataImportImpl.cs:819-838 — IsValidToken rejects RID 0 before checking the token type:

intrid=(int)(tk&0x00FFFFFF);inttokenType=(int)(tk>>24);if(rid==0)return0;// FALSE — but this runs before the UserString branch

For metadata table tokens, RID 0 is nil/invalid. However, for user string tokens (0x70), the "RID" is actually a heap offset, and offset 0 is a valid entry in the #US heap (the required null/empty byte per ECMA-335). The native IsValidToken would return TRUE for 0x70000000 when the heap is non-empty, while this implementation returns FALSE.

Fix: Move the rid == 0 check after the UserStringTokenType branch, or restrict it to table tokens only:

constintUserStringTokenType=0x70;if(tokenType==UserStringTokenType){intheapSize=_reader.GetHeapSize(HeapIndex.UserString);returnrid<heapSize?1:0;}if(rid==0)return0;// Only for table tokens

Advisory — unlikely to cause real-world issues since offset 0 is never referenced by IL, but it is a parity gap with native. Flagged by GPT-5.3-Codex, verified by primary reviewer.

⚠️ Lazy dictionary initialization without memory barriers (thread safety)

MetaDataImportImpl.cs:554,1336 — _interfaceImplToTypeDef and _paramToMethod use ??= for lazy initialization:

_interfaceImplToTypeDef??=BuildInterfaceImplLookup();

Since MetaDataImportImpl is a COM object callable from multiple threads (as acknowledged by the use of ConcurrentDictionary for _cdacEnumHandles), two threads could race on first access. The dictionaries are fully constructed before assignment and reference writes are atomic in .NET, so the practical risk is very low. However, the codebase convention states that ??= is not thread-safe and cross-thread field access should use Volatile or Interlocked.

Volatile.Write/Volatile.Read or LazyInitializer.EnsureInitialized would address this cleanly.

Advisory — not merge-blocking. The benign-race pattern here (two threads build independent dictionaries, one wins, both are equivalent) is safe in practice on x86/x64 and very likely safe on ARM given .NET's runtime guarantees. Flagged by all three models.

⚠️ GCHandle leak if CloseEnum is never called

MetaDataImportImpl.cs:66-73 — AllocEnum allocates GCHandle instances tracked in _cdacEnumHandles, but there's no finalizer or IDisposable to clean them up if the COM consumer forgets to call CloseEnum. While this matches the native IMetaDataImport contract (callers own enum lifetime), diagnostic tools may not always clean up on error paths.

Advisory — follows the same contract as native. A finalizer iterating _cdacEnumHandles.Keys and freeing outstanding handles would be a defensive improvement. Not merge-blocking since the native API has the same contract.

✅ COM ref counting — Correct

Both ICustomQueryInterface.GetInterface implementations (in MetaDataImportImpl.cs:47 and ClrDataModule.cs:111) correctly use ComInterfaceMarshaller<IMetaDataImport2>.ConvertToUnmanaged() which returns an already-AddRef'd pointer, consistent with COM QueryInterface semantics. The IMetaDataImport→IMetaDataImport2 vtable redirect for ClrMD compatibility is validated end-to-end by the test at line 964.

✅ GetUserString raw heap parsing — Correct

MetaDataImportImpl.cs:1188-1250 — Correctly reads from raw #US heap bytes rather than using MetadataReader.GetUserString(), avoiding potential discrepancies. The odd-size validation (blobSize % sizeof(char) == 0 → corrupt) correctly enforces the terminal byte requirement per ECMA-335 §II.24.2.4. pchString correctly returns character count without null terminator, matching native RegMeta semantics.

✅ Empty catch blocks in ClrDataModule.GetInterface — Acceptable

The two bare catch {} blocks (lines 85-87, 98-100) silently swallow exceptions when probing for metadata reader availability and legacy QI support. This is appropriate: both are optional capability probes. If both fail, the method returns CustomQueryInterfaceResult.NotHandled (line 103), which is the correct degradation path.

✅ DEBUG validation blocks — Excellent

The #if DEBUG blocks throughout MetaDataImportImpl comparing cDAC output against legacy DAC output (via Debug.ValidateHResult and per-field assertions) are a strong engineering practice for ensuring semantic parity. The ValidateBlobsEqual helper for byte-level blob comparison is thorough.

✅ Test coverage — Comprehensive

~1100 lines of unit tests covering: enum pagination, string truncation with CLDB_S_TRUNCATION, not-found error paths, GetMemberProps dispatch (method vs field table index), global method parent mapping (MapGlobalParentToken), constant value handling for both fields and params, vtable slot verification for the ClrMD compatibility fix, and enum lifecycle (CountEnum, ResetEnum, CloseEnum). The dump-based integration tests validate real-world metadata against actual process dumps.

💡 Static _testProvider field in tests

MetaDataImportImplTests.cs:161 — _testProvider is a shared static field used to prevent GC collection. Consider [ThreadStatic] for parallel test safety. Low practical risk since xUnit serializes tests within a class.

💡 _metaDataImportImpl lazy init race in ClrDataModule

ClrDataModule.cs:107 — _metaDataImportImpl ??= wrapper is a documented benign race. Both wrappers are functionally equivalent, the extra one is GC'd, and line 108 re-reads to ensure consistency. Acceptable pattern.

Generated by Code Review for issue #127028 · ◷

[noahfalk]

A few nits, but overall looks good to me!

[max-charlamb]

/ba-g previous test run passed. only nit style changes from comments

[max-charlamb]

Note

This PR description was AI/Copilot-generated.

Summary

Implements a managed [GeneratedComClass] wrapper (MetaDataImportImpl) that adapts System.Reflection.Metadata.MetadataReader to the IMetaDataImport, IMetaDataImport2, and IMetaDataAssemblyImport COM interfaces. This enables SOS and ClrMD to query metadata in cDAC mode, with optional legacy DAC fallback for methods not yet implemented in the managed layer.

Motivation

In cDAC no-fallback mode, ClrDataModule.GetInterface() returned NotHandled for IMetaDataImport QIs when _legacyModulePointer == 0, meaning diagnostic tools couldn't access type/method/field metadata. The cDAC already has access to MetadataReader via the EcmaMetadata contract, so a thin COM wrapper bridges the gap.

When a legacy DAC is available, the wrapper uses it for #if DEBUG validation (asserting that cDAC and DAC produce identical results) and as a fallback for the ~45 methods not yet implemented in managed code.

Changes

File	Description
IMetaDataImport.cs	Managed [GeneratedComInterface] definitions for IMetaDataImport (51 methods), IMetaDataImport2 (8 methods), IMetaDataAssemblyImport (14 methods), ASSEMBLYMETADATA struct, and internal CldbHResults constants
MetaDataImportImpl.cs	[GeneratedComClass] implementation — 28 full cDAC implementations via MetadataReader + ~45 legacy-delegated stubs. Uses explicit interface notation.
OutputBufferHelpers.cs	CopyStringToBuffer split into two overloads: void (for callers that don't check truncation) and out bool truncated (for MetaDataImportImpl)
ClrDataModule.cs	Wire up wrapper via ICustomQueryInterface — creates MetaDataImportImpl with both MetadataReader and optional legacy IMetaDataImport
MetaDataImportImplTests.cs	50 unit tests using synthetic metadata built with MetadataBuilder
MetaDataImportDumpTests.cs	3 dump-based integration tests verifying semantic parity against real metadata
MultiModule debuggee	Test debuggee with non-const fields, user strings, and methods for dump tests

Implemented methods (28 cDAC, ~45 legacy fallback)

Enum (cDAC):EnumInterfaceImpls, EnumFields, EnumGenericParams, CloseEnum, CountEnum, ResetEnum

Properties (cDAC):GetTypeDefProps, GetTypeRefProps, GetMethodProps, GetFieldProps, GetMemberProps, GetInterfaceImplProps, GetNestedClassProps, GetGenericParamProps, GetMemberRefProps, GetModuleRefProps, GetParamProps, GetClassLayout, GetUserString, GetParamForMethodIndex

Blob/token (cDAC):GetRVA, GetSigFromToken, GetTypeSpecFromToken, GetCustomAttributeByName, IsValidToken, FindTypeDefByName

Assembly (cDAC):GetAssemblyProps, GetAssemblyRefProps, GetAssemblyFromScope

Legacy fallback: All remaining methods delegate to _legacyImport/_legacyImport2/_legacyAssemblyImport or return E_NOTIMPL when no legacy is available.

Native parity behaviors

• <Module> parent mapping: GetMethodProps/GetFieldProps/GetMemberRefProps map TypeDef RID 1 to mdTypeDefNil (0x00000000) via MapGlobalParentToken, matching native RegMeta
• Constant defaults: GetFieldProps/GetParamProps return ELEMENT_TYPE_VOID when no constant is present
• User string heap: GetUserString uses raw #US heap byte parsing to exactly match native blob size validation (odd-length check, terminal byte stripping)
• Assembly flags: GetAssemblyProps ORs afPublicKey into flags when public key blob is non-empty
• Truncation: All string-returning methods return CLDB_S_TRUNCATION when buffer is too small
• Record not found: GetNestedClassProps/GetClassLayout/GetAssemblyProps return CLDB_E_RECORD_NOTFOUND for missing records

Design decisions

• Explicit interface implementation: All ~73 COM methods use explicit interface notation (int IMetaDataImport.Method(...)) to keep the public surface clean
• ICustomQueryInterface: QI for IMetaDataImport returns an IMetaDataImport2 vtable so callers always get the full interface
• HCORENUM pattern: Uses GCHandle.Alloc to box MetadataEnum objects; ConcurrentDictionary<nint, byte> tracks ownership for routing CloseEnum/CountEnum/ResetEnum between cDAC and legacy handles
• #if DEBUG validation: Every cDAC-implemented method (with 2 justified exceptions) cross-checks its output against the legacy DAC in debug builds
• CopyStringToBuffer overloads: void overload for callers that don't need truncation info; out bool truncated overload for MetaDataImportImpl callers that return CLDB_S_TRUNCATION

Testing

• 1845 unit tests pass (all cDAC tests including 50 MetaDataImportImpl tests)
• 214 dump-based tests pass locally (3 are MetaDataImport-specific)
• Tests cover: enum pagination, global/non-global parent mapping, constant handling, user string char counts, truncation, nested classes, generic params, assembly properties, invalid tokens, QI vtable correctness, and E_NOTIMPL fallback behavior

[Copilot]

Pull request overview

This PR adds a managed COM-compatible IMetaDataImport/IMetaDataImport2 implementation backed by System.Reflection.Metadata.MetadataReader, and wires it into cDAC’s ClrDataModule so diagnostic tools can query metadata when running in no-fallback mode (legacy DAC unavailable).

Changes:

• Introduces generated COM interface definitions for IMetaDataImport and IMetaDataImport2.
• Implements MetadataImportWrapper to adapt MetadataReader to the COM metadata import APIs (with a subset implemented and the rest stubbed).
• Updates ClrDataModule to provide IMetaDataImport in no-fallback mode and adds unit tests validating implemented behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/native/managed/cdac/tests/MetadataImportWrapperTests.cs	Adds unit tests that exercise the wrapper’s enumeration and property APIs over synthetic metadata.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetadataImportWrapper.cs	Implements the COM wrapper over MetadataReader including enum-handle handling and a set of metadata accessors.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IMetaDataImport.cs	Adds managed [GeneratedComInterface] declarations for IMetaDataImport/IMetaDataImport2 with vtable ordering.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Hooks IMetaDataImport QI to return the managed wrapper when legacy DAC is unavailable.

[jkoritzinsky]

You'll likely need to add support for IMetaDataAssemblyImport as well for assembly-level info

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

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

[github-actions]

🤖 Copilot Code Review — PR #127028

Note

This review was AI-generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, with Claude Sonnet 4.5 and GPT-5.3-Codex sub-agents).

Holistic Assessment

Motivation: Well-justified. A managed MetaDataImportImpl wrapping System.Reflection.Metadata.MetadataReader is essential for cDAC no-fallback mode where the legacy DAC is unavailable. SOS and ClrMD depend on IMetaDataImport for metadata queries, making this a required capability.

Approach: Sound. Implementing ~30 commonly-used methods with MetadataReader while delegating ~30 rarely-used ones to the legacy DAC is pragmatic. The ICustomQueryInterface vtable redirect to handle ClrMD's beyond-vtable IMetaDataImport2 slot access is clever and well-documented. The extensive #if DEBUG validation blocks comparing output against the legacy DAC are excellent for catching parity gaps during development.

Summary: ⚠️Needs Human Review. The code is well-structured, follows cDAC conventions, and handles COM interop correctly. There are a few thread-safety concerns with lazy initialization and a minor correctness gap in IsValidToken that warrant maintainer attention. None appear to be critical bugs in practice, but a human reviewer should evaluate whether they're acceptable for the diagnostics use case.

---

Detailed Findings

⚠️IsValidToken — RID 0 early rejection incorrectly covers user string tokens

MetaDataImportImpl.cs:819-838 — IsValidToken rejects RID 0 before checking the token type:

intrid=(int)(tk&0x00FFFFFF);inttokenType=(int)(tk>>24);if(rid==0)return0;// FALSE — but this runs before the UserString branch

For metadata table tokens, RID 0 is nil/invalid. However, for user string tokens (0x70), the "RID" is actually a heap offset, and offset 0 is a valid entry in the #US heap (the required null/empty byte per ECMA-335). The native IsValidToken would return TRUE for 0x70000000 when the heap is non-empty, while this implementation returns FALSE.

Fix: Move the rid == 0 check after the UserStringTokenType branch, or restrict it to table tokens only:

constintUserStringTokenType=0x70;if(tokenType==UserStringTokenType){intheapSize=_reader.GetHeapSize(HeapIndex.UserString);returnrid<heapSize?1:0;}if(rid==0)return0;// Only for table tokens

Advisory — unlikely to cause real-world issues since offset 0 is never referenced by IL, but it is a parity gap with native. Flagged by GPT-5.3-Codex, verified by primary reviewer.

⚠️ Lazy dictionary initialization without memory barriers (thread safety)

MetaDataImportImpl.cs:554,1336 — _interfaceImplToTypeDef and _paramToMethod use ??= for lazy initialization:

_interfaceImplToTypeDef??=BuildInterfaceImplLookup();

Since MetaDataImportImpl is a COM object callable from multiple threads (as acknowledged by the use of ConcurrentDictionary for _cdacEnumHandles), two threads could race on first access. The dictionaries are fully constructed before assignment and reference writes are atomic in .NET, so the practical risk is very low. However, the codebase convention states that ??= is not thread-safe and cross-thread field access should use Volatile or Interlocked.

Volatile.Write/Volatile.Read or LazyInitializer.EnsureInitialized would address this cleanly.

Advisory — not merge-blocking. The benign-race pattern here (two threads build independent dictionaries, one wins, both are equivalent) is safe in practice on x86/x64 and very likely safe on ARM given .NET's runtime guarantees. Flagged by all three models.

⚠️ GCHandle leak if CloseEnum is never called

MetaDataImportImpl.cs:66-73 — AllocEnum allocates GCHandle instances tracked in _cdacEnumHandles, but there's no finalizer or IDisposable to clean them up if the COM consumer forgets to call CloseEnum. While this matches the native IMetaDataImport contract (callers own enum lifetime), diagnostic tools may not always clean up on error paths.

Advisory — follows the same contract as native. A finalizer iterating _cdacEnumHandles.Keys and freeing outstanding handles would be a defensive improvement. Not merge-blocking since the native API has the same contract.

✅ COM ref counting — Correct

Both ICustomQueryInterface.GetInterface implementations (in MetaDataImportImpl.cs:47 and ClrDataModule.cs:111) correctly use ComInterfaceMarshaller<IMetaDataImport2>.ConvertToUnmanaged() which returns an already-AddRef'd pointer, consistent with COM QueryInterface semantics. The IMetaDataImport→IMetaDataImport2 vtable redirect for ClrMD compatibility is validated end-to-end by the test at line 964.

✅ GetUserString raw heap parsing — Correct

MetaDataImportImpl.cs:1188-1250 — Correctly reads from raw #US heap bytes rather than using MetadataReader.GetUserString(), avoiding potential discrepancies. The odd-size validation (blobSize % sizeof(char) == 0 → corrupt) correctly enforces the terminal byte requirement per ECMA-335 §II.24.2.4. pchString correctly returns character count without null terminator, matching native RegMeta semantics.

✅ Empty catch blocks in ClrDataModule.GetInterface — Acceptable

The two bare catch {} blocks (lines 85-87, 98-100) silently swallow exceptions when probing for metadata reader availability and legacy QI support. This is appropriate: both are optional capability probes. If both fail, the method returns CustomQueryInterfaceResult.NotHandled (line 103), which is the correct degradation path.

✅ DEBUG validation blocks — Excellent

The #if DEBUG blocks throughout MetaDataImportImpl comparing cDAC output against legacy DAC output (via Debug.ValidateHResult and per-field assertions) are a strong engineering practice for ensuring semantic parity. The ValidateBlobsEqual helper for byte-level blob comparison is thorough.

✅ Test coverage — Comprehensive

~1100 lines of unit tests covering: enum pagination, string truncation with CLDB_S_TRUNCATION, not-found error paths, GetMemberProps dispatch (method vs field table index), global method parent mapping (MapGlobalParentToken), constant value handling for both fields and params, vtable slot verification for the ClrMD compatibility fix, and enum lifecycle (CountEnum, ResetEnum, CloseEnum). The dump-based integration tests validate real-world metadata against actual process dumps.

💡 Static _testProvider field in tests

MetaDataImportImplTests.cs:161 — _testProvider is a shared static field used to prevent GC collection. Consider [ThreadStatic] for parallel test safety. Low practical risk since xUnit serializes tests within a class.

💡 _metaDataImportImpl lazy init race in ClrDataModule

ClrDataModule.cs:107 — _metaDataImportImpl ??= wrapper is a documented benign race. Both wrappers are functionally equivalent, the extra one is GC'd, and line 108 re-reads to ensure consistency. Acceptable pattern.

Generated by Code Review for issue #127028 · ◷

[noahfalk]

A few nits, but overall looks good to me!

[max-charlamb]

/ba-g previous test run passed. only nit style changes from comments