[cDAC] Implement IsDiagnosticsHiddenOrLCGMethod for cDAC

[barosiak]

Summary

Implement IsDiagnosticsHiddenOrLCGMethod on DacDbiImpl in the cDAC, including the previously missing IsAsyncThunkMethod check for full parity with the native VM.

Changes

• DacDbiImpl.cs - IsDiagnosticsHiddenOrLCGMethod implementation
• datadescriptor.inc - exposed AsyncMethodData.Flags field
• Data/AsyncMethodData.cs - new data reader for AsyncMethodData
• MethodDescFlags_1.cs - added AsyncMethodFlags enum
• MethodDescOptionalSlots.cs - added HasAsyncMethodData, GetAddressOfAsyncMethodData, AsyncMethodDataOffset
• IRuntimeTypeSystem.cs / RuntimeTypeSystem_1.cs - added IsDiagnosticsHidden contract method with IsILStub, IsAsyncThunkMethod, IsWrapperStub; reorganized helpers
• dacdbiinterface.h - added [cDAC] contract annotation on DynamicMethodType enum
• RuntimeTypeSystem.md - updated contract design doc
• Tests - new tests covering IsDiagnosticsHidden and IsDynamicMethod across architectures

[dotnet-policy-service]

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

Implements IsDiagnosticsHiddenOrLCGMethod in the managed cDAC DBI layer by adding an IRuntimeTypeSystem.IsDiagnosticsHidden contract API and the supporting async-method metadata plumbing, plus tests to validate behavior across architectures.

Changes:

• Add IRuntimeTypeSystem.IsDiagnosticsHidden and implement it in RuntimeTypeSystem_1 using IL-stub, wrapper-stub, and async-thunk checks.
• Expose/read AsyncMethodData.Flags via data descriptors and a new managed data reader, and add optional-slot addressing for async method data.
• Add unit tests covering diagnostics-hidden behavior and LCG/IL-stub distinctions.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/MethodDescTests.cs	Adds tests for IsDiagnosticsHidden/IsDynamicMethod and wires AsyncMethodData.Flags into mock type info.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements IsDiagnosticsHiddenOrLCGMethod using cDAC RuntimeTypeSystem contract (with DEBUG parity check vs legacy DAC).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodDescOptionalSlots.cs	Adds HasAsyncMethodData and address calculation for async method data optional slot.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodDescFlags_1.cs	Adds AsyncMethodFlags (used to detect thunk methods).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/AsyncMethodData.cs	New data reader for AsyncMethodData (reads Flags).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Implements IsDiagnosticsHidden and async-thunk detection via AsyncMethodData.Flags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Adds the IsDiagnosticsHidden API to the contract interface.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Exposes AsyncMethodData.Flags in the runtime data descriptor.
src/coreclr/debug/inc/dacdbiinterface.h	Documents that cDAC depends on the DynamicMethodType enum values.
docs/design/datacontracts/RuntimeTypeSystem.md	Updates the design doc to include IsDiagnosticsHidden.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

[Copilot]

The test name IsDiagnosticsHidden_ReturnsCorrectValues is misleading: the assertions validate IsILStub / IsAsyncThunkMethod / IsWrapperStub / IsDynamicMethod, but don't directly assert a single “diagnostics hidden” decision (e.g., the IsDiagnosticsHiddenOrLCGMethod behavior). Consider renaming the test to reflect what it actually asserts, or add assertions that validate the combined hidden/LCG classification outcome.

Suggested change

	public void IsDiagnosticsHidden_ReturnsCorrectValues(MockTarget.Architecture arch)
	public void MethodClassificationFlags_ReturnCorrectValues(MockTarget.Architecture arch)

[github-actions]

Note

This review was generated by Copilot (Claude Opus 4.6), with additional input from gpt-5.3-codex and goldeneye.

🤖 Copilot Code Review — PR #127035

Holistic Assessment

Motivation: This PR addresses maintainer feedback from @jkotas that IsDiagnosticsHidden encodes debugger policy (what to hide) and does not belong on the IRuntimeTypeSystem contract. The type system should provide policy-free primitives, and the debugger should compose the policy. This is a well-justified architectural improvement.

Approach: The decomposition into IsAsyncThunkMethod and IsWrapperStub (alongside the pre-existing IsILStub) is the right approach. The caller DacDbiImpl.IsDiagnosticsHiddenOrLCGMethod now composes rts.IsILStub(md) || rts.IsAsyncThunkMethod(md) || rts.IsWrapperStub(md), which exactly mirrors the native VM's MethodDesc::IsDiagnosticsHidden() = IsILStub() || IsAsyncThunkMethod() || IsWrapperStub(). The #if DEBUG validation block in DacDbiImpl further guards behavioral parity against the legacy DAC.

Summary: ✅ LGTM. This is a clean refactoring that correctly decomposes a policy method into reusable primitives per maintainer direction. The behavior is preserved, the implementation matches the native VM, tests are thorough, and all review feedback from @jkotas and @rcj1 has been addressed. No blocking issues found.

---

Detailed Findings

✅ Correctness — Behavioral equivalence verified

The native VM's IsDiagnosticsHidden() (in src/coreclr/vm/method.inl:181) is IsILStub() || IsAsyncThunkMethod() || IsWrapperStub(). The cDAC caller now performs the identical composition at DacDbiImpl.cs:577. Each primitive's implementation in RuntimeTypeSystem_1.cs is unchanged from the prior version — only the organization changed (private helpers promoted to public contract methods, or inlined into existing public methods). The DynamicMethodType.kLCGMethod branch still uses rts.IsDynamicMethod(md) which corresponds to the native IsLCGMethod(). All correct.

✅ API Design — Good decomposition of policy vs. primitives

Removing IsDiagnosticsHidden from IRuntimeTypeSystem and exposing IsAsyncThunkMethod/IsWrapperStub as primitives is the right design. These methods have independent utility beyond diagnostics hiding (e.g., IsWrapperStub is already used internally by IsVersionable at line 1471). The debugger-specific composition now lives in DacDbiImpl where it belongs.

✅ Enum relocation — Consistent with reviewer feedback

• AsyncMethodFlags moved from MethodDescFlags_1.cs to RuntimeTypeSystem_1.cs per @rcj1's feedback that these aren't MethodDesc flags. The new location alongside DynamicMethodDescExtendedFlags is appropriate.
• DynamicMethodType moved from private in DacDbiImpl to public in IDacDbiInterface.cs per @rcj1's feedback that it's part of the COM interface contract. Explicit values (kNone = 0, etc.) are set, consistent with all other public types in that file.

✅ Test coverage — Thorough primitive testing

Tests in MethodDescTests.cs verify each primitive independently across all method categories: normal IL, IL stub, LCG, unboxing stub, instantiating stub, async thunk, async non-thunk, and async thunk with NativeCodeSlot. The #if DEBUG validation in DacDbiImpl provides an additional integration-level check against the legacy DAC for the composed behavior.

💡 Test method name — Minor discrepancy (non-blocking)

The test method at MethodDescTests.cs:452 is still named IsDiagnosticsHidden_ReturnsCorrectValues but now tests the individual primitives (IsILStub, IsAsyncThunkMethod, IsWrapperStub) rather than a combined IsDiagnosticsHidden. The name is still broadly accurate since the test verifies the building blocks of diagnostics-hidden classification, but a rename (e.g., DiagnosticsHiddenPrimitives_ReturnCorrectValues) would better reflect the test's current scope. Not blocking.

(Flagged by all three models: Claude Opus 4.6, gpt-5.3-codex, goldeneye)

💡 DacDbiImpl integration test — Optional follow-up

The tests validate each contract primitive but don't directly test the composed logic in DacDbiImpl.IsDiagnosticsHiddenOrLCGMethod. The #if DEBUG legacy-comparison block provides runtime validation, but a dedicated test for the composition could be valuable as a follow-up (especially if the #if DEBUG validation is ever removed). Not blocking.

(Flagged by gpt-5.3-codex and goldeneye)

Generated by Code Review for issue #127035 · ◷