[cDAC] Implement ClrDataMethodDefinition APIs by max-charlamb · Pull Request #127104 · dotnet/runtime

max-charlamb · 2026-04-18T02:27:31Z

Note

This PR was generated with the assistance of GitHub Copilot.

Summary

Implements 5 of 6 IXCLRDataMethodDefinition APIs in the cDAC's managed ClrDataMethodDefinition wrapper:

Method	Status
`GetName`	✅ Implemented — TypeNameBuilder + metadata fallback
`HasClassOrMethodInstantiation`	✅ Implemented — mirrors native exactly
`StartEnumInstances`	✅ Implemented — reuses `SOSDacImpl.EnumMethodInstances`
`EnumInstance`	✅ Implemented — with legacy sync + exception fallback
`EndEnumInstances`	✅ Implemented — proper cleanup inside try/catch
`SetCodeNotification`	⏳ Deferred — `INotifications` contract lacks JIT notification table write APIs

Details

GetName: Uses TypeNameBuilder with FormatSignature|FormatNamespace|FormatFullInst flags (matching native daccess.cpp:5268). Falls back to GetFullMethodNameFromMetadata when no MethodDesc is available.
HasClassOrMethodInstantiation: Resolves MethodDesc and delegates to IRuntimeTypeSystem contract methods, matching native task.cpp:3486-3516.
Start/Enum/EndEnumInstances: Reuses the existing SOSDacImpl.EnumMethodInstances infrastructure (same IEnum<MethodDescHandle> + GCHandle pattern).
All implemented methods include #if DEBUG cross-validation blocks against the legacy DAC.
Removes implemented APIs from the LegacyFallbackHelper allowlist.

Testing

All 1638 existing cDAC tests pass.
Two rounds of cDAC-specific review completed (initial + deep line-by-line parity audit).

Implement GetName, HasClassOrMethodInstantiation, StartEnumInstances, EnumInstance, and EndEnumInstances on ClrDataMethodDefinition. - GetName: TypeNameBuilder path for loaded methods, metadata fallback for unloaded methods - HasClassOrMethodInstantiation: mirrors native E_UNEXPECTED for null MethodDesc, boolean result for found MethodDesc - Start/Enum/EndEnumInstances: reuses SOSDacImpl.EnumMethodInstances with legacy delegation and fallback - SetCodeNotification: deferred (needs new contract APIs) - All methods include #if DEBUG cross-validation against legacy DAC Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

GetName, StartEnumInstances, HasClassOrMethodInstantiation are now implemented in the cDAC and no longer need legacy fallback. SetCodeNotification remains allowlisted (needs INotifications contract). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR implements most of the IXCLRDataMethodDefinition surface in the managed cDAC legacy-layer wrapper (ClrDataMethodDefinition), and updates the legacy fallback allowlist accordingly so these APIs no longer delegate by default.

Changes:

Implemented IXCLRDataMethodDefinition APIs for name retrieval, generic instantiation detection, and instance enumeration using cDAC contracts plus legacy cross-validation/fallback behavior.
Added a metadata-based fallback path for method names when a MethodDesc cannot be resolved.
Updated LegacyFallbackHelper allowlist to remove the newly-implemented APIs (leaving SetCodeNotification as the remaining fallback).

Reviewed changes

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

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/LegacyFallbackHelper.cs	Removes `IXCLRDataMethodDefinition` members from the fallback allowlist now that they’re implemented (keeps `SetCodeNotification`).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs	Adds implementations for `GetName`, `HasClassOrMethodInstantiation`, and Start/Enum/End instance enumeration using cDAC contracts with legacy sync/validation.

rcj1 · 2026-04-18T02:34:03Z

Who uses GetName?

max-charlamb · 2026-04-18T15:29:32Z

Who uses GetName?

https://github.com/dotnet/diagnostics/blob/598140e1d6e694d88daebd194652b6faeda643f4/src/SOS/Strike/strike.cpp#L6204

StartEnumInstances now properly cleans up the legacy enumeration handle when the cDAC side fails or returns S_FALSE, preventing resource leaks. The GCHandle is only allocated after emi.Start() succeeds. EndEnumInstances now calls IEnum.Dispose() to release the enumerator before freeing the GCHandle, matching ClrDataModule cleanup pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Apply the same fix as ClrDataMethodDefinition: only allocate the GCHandle after emi.Start() returns S_OK, and clean up the legacy handle in a finally block when ownership is not transferred. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Implements most of the IXCLRDataMethodDefinition surface in the managed cDAC legacy layer, aligning behavior with the native/legacy DAC and reducing reliance on the legacy fallback allowlist.

Changes:

Implemented IXCLRDataMethodDefinition APIs for name retrieval, generic instantiation detection, and instance enumeration (start/next/end) in ClrDataMethodDefinition.
Tightened StartEnumMethodInstancesByAddress handle/cleanup behavior to avoid orphaning legacy enumeration handles on failure paths.
Updated LegacyFallbackHelper allowlist to remove methods that are now implemented, leaving SetCodeNotification as deferred.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs	Ensures enum handle is only published on `S_OK` and reliably cleans up legacy handles when cDAC enum creation fails.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/LegacyFallbackHelper.cs	Removes now-implemented `IXCLRDataMethodDefinition` methods from the fallback allowlist; keeps `SetCodeNotification`.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs	Adds managed implementations for method definition APIs, including metadata-based naming fallback and instance enumeration with legacy sync.

- Add null bGeneric check in HasClassOrMethodInstantiation (throws E_POINTER) - Replace if/else pattern with throw for E_UNEXPECTED when methodDesc is null - Use Interop.BOOL.TRUE/FALSE instead of raw 1/0 - Add instance.IsNullRef guard in EnumInstance Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-04-18T23:15:23Z

🤖 Copilot Code Review — PR #127104

Note

This review was generated by Copilot using multi-model analysis (Claude Opus 4.6 primary, Claude Sonnet 4.5 + GPT-5.3-Codex secondary).

Holistic Assessment

Motivation: Justified — implements cDAC-side ClrDataMethodDefinition APIs (GetName, HasClassOrMethodInstantiation, StartEnumInstances/EnumInstance/EndEnumInstances) replacing legacy DAC delegation, and fixes real handle leaks in the StartEnumInstances and StartEnumMethodInstancesByAddress paths. This advances the cDAC migration and improves resource lifecycle management.

Approach: Sound — reuses the existing EnumMethodInstances infrastructure from SOSDacImpl, follows the established legacy-lockstep pattern with #if DEBUG cross-validation. The commit history shows iterative refinement (handle leak fixes, null checks, Interop.BOOL usage) that addressed real issues.

Summary: ⚠️ Needs Human Review. The implementation is correct and well-structured, matching established patterns. Two resource-management concerns were flagged by multiple models that merit human judgment on whether to address in this PR or as follow-ups.

Detailed Findings

⚠️ Missing `Dispose()` in pre-existing `EndEnumMethodInstancesByAddress` — Follow-up

SOSDacImpl.IXCLRDataProcess.cs, lines 467–483: The existing EndEnumMethodInstancesByAddress does not call ((IEnum<MethodDescHandle>)emi).Dispose() before gcHandle.Free(), leaking the inner IEnumerator<MethodDescHandle>. The new EndEnumInstances in ClrDataMethodDefinition.cs (line 239) correctly calls Dispose before Free, matching the pattern in ClrDataModule.EndEnumMethodDefinitionsByName (line 333).

This is a pre-existing issue not introduced by this PR, but since this PR works directly in this area and establishes the correct pattern, it would be a natural place to fix the sibling method for consistency. Not blocking — could be a follow-up.

⚠️ Potential COM object leak in `EnumInstance` when `MoveNext()` returns false — Advisory (flagged by all three models)

ClrDataMethodDefinition.cs, lines 179–199: When the cDAC enumerator finishes (MoveNext() returns false at line 192), the legacyMethod COM object obtained at line 186 is not assigned to any output or explicitly released. The RCW keeps the COM object alive until GC collection.

Mitigating factors: (1) In practice, the cDAC and legacy enumerations should stay in sync — the DEBUG ValidateHResult assertion catches mismatches — so when cDAC returns S_FALSE, the legacy should too, meaning legacyMethod would be null. (2) The sibling EnumMethodInstanceByAddress in SOSDacImpl.IXCLRDataProcess.cs (lines 420–429) has the same pattern. (3) GC/finalization will eventually release the RCW.

Not blocking, but worth noting for awareness.

✅ Handle lifecycle in `StartEnumInstances` — Correct

The ownership-transfer pattern is well implemented: legacyHandle = default on success (line 132), finally-block cleanup on failure (lines 141–151). This matches the fixed StartEnumMethodInstancesByAddress pattern and prevents the handle leaks described in the commit messages.

✅ `EndEnumInstances` access after Dispose/Free — Safe

ClrDataMethodDefinition.cs, lines 239–244: Accessing emi.LegacyHandle after emi.Dispose() and gcHandle.Free() is safe — the local emi variable keeps the object alive, Dispose() only disposes the inner enumerator, and LegacyHandle is a TargetPointer struct field. This pattern is identical to ClrDataModule.EndEnumMethodDefinitionsByName (lines 333–341).

✅ `HasClassOrMethodInstantiation` — Correct

Throws NullReferenceException (HResult = E_POINTER) for null bGeneric, and COMException(E_UNEXPECTED) for unresolved MethodDesc. Uses Interop.BOOL.TRUE/FALSE instead of raw 1/0. Logic matches the static helpers which are consistent with the instance methods in EnumMethodInstances.

✅ `GetName` with metadata fallback — Correct

Primary path uses TypeNameBuilder.AppendMethodInternal for loaded methods, falls back to GetFullMethodNameFromMetadata() for unresolved MethodDescs. The fallback builds Namespace.Type.Method format — this doesn't handle nested types, but the Debug validation block (lines 299–324) would catch any mismatch with the legacy DAC.

✅ Fallback allowlist update — Correct

LegacyFallbackHelper.cs: Three now-implemented methods removed from the allowlist. SetCodeNotification correctly retained (comment explains it needs INotifications contract).

✅ `StartEnumMethodInstancesByAddress` handle leak fix — Correct

SOSDacImpl.IXCLRDataProcess.cs: Moves GCHandle allocation inside the if (hr == S_OK) check and adds a finally block to clean up the orphaned legacy handle. This is a real bug fix — without it, a failed emi.Start() would leak both the GCHandle and the legacy enumeration handle.

Generated by Code Review for issue #127104 · ◷

Add IXCLRDataMethodDefinitionDumpTests with 8 test methods covering: - GetName: query length, full retrieval, truncation (S_FALSE), null termination - HasClassOrMethodInstantiation: non-generic (FALSE) and generic type (TRUE) - Start/Enum/EndEnumInstances: non-generic method returns exactly one instance - EndEnumInstances: zero handle returns error Tests use the StackWalk debuggee's full dump. The generic type test walks the crash thread stack to find a module with loaded generic type MethodDescs, avoiding ILoader.GetRootAssembly() which requires the AssemblyList data descriptor that is not always present in locally-generated dumps. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Implements most IXCLRDataMethodDefinition APIs in the managed cDAC legacy layer wrapper, aiming to match native DAC behavior while reusing existing cDAC enumeration infrastructure and keeping optional DEBUG-time parity validation.

Changes:

Implemented GetName, HasClassOrMethodInstantiation, and Start/Enum/EndEnumInstances on ClrDataMethodDefinition.
Added legacy-handle cleanup to prevent orphaned legacy enumerations when cDAC enum creation fails.
Updated the legacy fallback allowlist to remove newly-implemented IXCLRDataMethodDefinition members (leaving SetCodeNotification deferred).

Reviewed changes

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

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs	Prevents orphaned legacy method-instance enumerations by deferring handle publication and cleaning up in `finally`.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/LegacyFallbackHelper.cs	Removes implemented `IXCLRDataMethodDefinition` APIs from the fallback allowlist; keeps `SetCodeNotification` allowlisted.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs	Implements several `IXCLRDataMethodDefinition` APIs (name formatting, generic instantiation detection, and instance enumeration) using cDAC contracts with legacy sync/parity hooks.

Native ClrDataMethodDefinition::EnumInstance does not null-check the output pointer — it dereferences *instance directly, and if NULL the AV is caught by EX_CATCH. The managed equivalent naturally throws NullReferenceException from DacComNullableByRef.Interface setter, which maps to E_POINTER at the COM interop boundary — matching native behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

dotnet-policy-service · 2026-04-20T18:01:54Z

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Copilot

Pull request overview

This PR implements most of the IXCLRDataMethodDefinition surface in the cDAC managed wrapper, adds dump-based integration coverage for the new behavior, and tightens legacy fallback allowlisting to reflect the newly implemented APIs.

Changes:

Implement IXCLRDataMethodDefinition.GetName, HasClassOrMethodInstantiation, and method instance enumeration (StartEnumInstances/EnumInstance/EndEnumInstances) in ClrDataMethodDefinition.
Add dump-based tests validating name retrieval, generic-instantiation detection, and enumeration lifecycle.
Fix legacy-enumeration handle cleanup semantics for StartEnumMethodInstancesByAddress and update LegacyFallbackHelper allowlist accordingly.

Reviewed changes

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

File	Description
src/native/managed/cdac/tests/DumpTests/IXCLRDataMethodDefinitionDumpTests.cs	Adds dump-based integration tests for the newly implemented `IXCLRDataMethodDefinition` APIs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs	Adjusts handle publication timing and ensures legacy handle cleanup when cDAC enumeration setup fails.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/LegacyFallbackHelper.cs	Removes now-implemented `IXCLRDataMethodDefinition` members from the fallback allowlist (leaving `SetCodeNotification`).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs	Implements method name retrieval, generic-instantiation detection, and method-instance enumeration with debug cross-validation.

max-charlamb and others added 2 commits April 17, 2026 22:25

Copilot AI review requested due to automatic review settings April 18, 2026 02:27

github-actions Bot added the area-Diagnostics-coreclr label Apr 18, 2026

Copilot started reviewing on behalf of max-charlamb April 18, 2026 02:28 View session

dotnet-policy-service Bot assigned max-charlamb Apr 18, 2026

max-charlamb mentioned this pull request Apr 18, 2026

[cDAC] Track no-fallback allowlist — remaining unimplemented APIs #126760

Closed

Copilot AI reviewed Apr 18, 2026

View reviewed changes

max-charlamb and others added 2 commits April 18, 2026 11:54

Copilot AI review requested due to automatic review settings April 18, 2026 16:01

Copilot started reviewing on behalf of max-charlamb April 18, 2026 16:01 View session

Copilot AI reviewed Apr 18, 2026

View reviewed changes

Comment thread ...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs

Comment thread ...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs

This comment has been minimized.

Sign in to view

max-charlamb commented Apr 18, 2026

View reviewed changes

max-charlamb requested a review from Copilot April 19, 2026 02:12

Copilot started reviewing on behalf of max-charlamb April 19, 2026 02:13 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

max-charlamb requested review from barosiak, hoyosjs, noahfalk and rcj1 April 20, 2026 17:56

max-charlamb marked this pull request as ready for review April 20, 2026 17:56

Copilot AI review requested due to automatic review settings April 20, 2026 17:56

max-charlamb added area-Diagnostics-cdac and removed area-Diagnostics-coreclr labels Apr 20, 2026

Copilot started reviewing on behalf of max-charlamb April 20, 2026 17:57 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

noahfalk approved these changes Apr 21, 2026

View reviewed changes

max-charlamb merged commit 4959d07 into main Apr 21, 2026
74 checks passed

max-charlamb deleted the cdac/method-definition-apis branch April 21, 2026 15:43

This was referenced Apr 21, 2026

[main] Source code updates from dotnet/runtime dotnet/dotnet#6183

Merged

[main] Source code updates from dotnet/runtime dotnet/dotnet#6196

Merged

Conversation

max-charlamb commented Apr 18, 2026

Summary

Details

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rcj1 commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

max-charlamb commented Apr 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 18, 2026

🤖 Copilot Code Review — PR #127104

Holistic Assessment

Detailed Findings

⚠️ Missing Dispose() in pre-existing EndEnumMethodInstancesByAddress — Follow-up

⚠️ Potential COM object leak in EnumInstance when MoveNext() returns false — Advisory (flagged by all three models)

✅ Handle lifecycle in StartEnumInstances — Correct

✅ EndEnumInstances access after Dispose/Free — Safe

✅ HasClassOrMethodInstantiation — Correct

✅ GetName with metadata fallback — Correct

✅ Fallback allowlist update — Correct

✅ StartEnumMethodInstancesByAddress handle leak fix — Correct

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dotnet-policy-service Bot commented Apr 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rcj1 commented Apr 18, 2026 •

edited

Loading

⚠️ Missing `Dispose()` in pre-existing `EndEnumMethodInstancesByAddress` — Follow-up

⚠️ Potential COM object leak in `EnumInstance` when `MoveNext()` returns false — Advisory (flagged by all three models)

✅ Handle lifecycle in `StartEnumInstances` — Correct

✅ `EndEnumInstances` access after Dispose/Free — Safe

✅ `HasClassOrMethodInstantiation` — Correct

✅ `GetName` with metadata fallback — Correct

✅ `StartEnumMethodInstancesByAddress` handle leak fix — Correct