Remove functional cDAC fallbacks by noahfalk · Pull Request #132720 · dotnet/runtime

noahfalk · 2026-08-24T21:50:15Z

Keep legacy DAC interfaces only for Debug comparison and required lifetime management. Return E_NOTIMPL for unsupported cDAC operations. Fixed code paths where the behavior of the DAC comparison API call influenced the return value from cDAC. The FallbackHepler is now removed and most legacy interface pointers store NULL in release builds. DOTNET_ENABLE_CDAC no longer forwards the creation of ICLRDataEnumMemoryRegions to cDAC. If needed for testing this can be restored once the interface is implemented, but ideally we are moving towards eliminating DOTNET_ENABLE_CDAC env var and DAC->cDAC bootstrapping.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Keep legacy DAC interfaces only for Debug comparison and required lifetime management. Return E_NOTIMPL for unsupported cDAC operations. Fixed code paths where the behavior of the DAC comparison API call influenced the return value from cDAC. The FallbackHepler is now removed and most legacy interface pointers store NULL in release builds. DOTNET_ENABLE_CDAC no longer forwards the creation of ICLRDataEnumMemoryRegions to cDAC. If needed for testing this can be restored once the interface is implemented, but ideally we are moving towards eliminating DOTNET_ENABLE_CDAC env var and DAC->cDAC bootstrapping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f8812ce5-101b-43e9-915a-835174a9d2ad

azure-pipelines · 2026-08-24T21:51:03Z

Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run.

dotnet-policy-service · 2026-08-24T21:52:17Z

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 removes functional legacy-DAC fallback behavior in the managed cDAC layer, keeping legacy interfaces only for Debug cross-validation and lifetime rooting, and returning E_NOTIMPL for unsupported operations. It also adjusts CoreCLR’s ENABLE_CDAC path to avoid routing ICLRDataEnumMemoryRegions creation through cDAC (dump creation still relies on legacy memory enumeration).

Changes:

Remove legacy fallback helper and update many legacy-interop call sites so legacy failures no longer influence cDAC return values (Debug validation only).
Tighten cDAC/CoreCLR bootstrap behavior: validate required contracts before publishing interfaces and skip cDAC for ICLRDataEnumMemoryRegions.
Update tests/docs to reflect the new “no fallback” policy and E_NOTIMPL expectations.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/UnitTests/MetaDataImportImplTests.cs	Adds coverage for `IMetaDataImport2.GetModuleFromScope` returning `E_NOTIMPL`.
src/native/managed/cdac/README.md	Updates documentation to reflect “Debug validation only” legacy usage and release lifetime-root behavior.
src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs	Makes data-access contract validation unconditional before publishing interfaces; adjusts legacy-impl plumbing.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs	Stops functional legacy fallback for unsupported ops and tightens validation-only legacy sync patterns.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.ICLRDataEnumMemoryRegions.cs	Returns `E_NOTIMPL` for memory region enumeration (no legacy fallback).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs	Keeps legacy object rooted for lifetime; avoids using legacy for functional behavior in release builds.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/README.md	Replaces prior “delegation placement” guidance with the new “no fallback, Debug validation state only” policy.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetaDataImportImpl.cs	Removes legacy delegation for not-implemented IMetaDataImport/IMetaDataAssemblyImport members.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/LegacyFallbackHelper.cs	Deletes the allowlist-based fallback mechanism.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Nulls legacy DBI in release builds; preserves lifetime management and Debug validation patterns.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTypeInstance.cs	Removes functional fallback paths and limits legacy use to Debug comparisons.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTypeDefinition.cs	Removes functional fallback paths; keeps legacy sync for validation only.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTask.cs	Stops propagating legacy HRESULTs into cDAC behavior; retains Debug validation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataStackWalk.cs	Same: cDAC behavior no longer depends on legacy return values; Debug validates mismatches.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Removes fallback returns for multiple enumeration APIs; keeps sync for validation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodInstance.cs	Removes `LegacyFallbackHelper` gating and avoids returning legacy failures as cDAC failures.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs	Removes “fallback to legacy result” exception handling and keeps legacy in sync for validation only.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataFrame.cs	Returns `E_NOTIMPL` for unsupported operations instead of falling back.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataExceptionState.cs	Removes `LegacyFallbackHelper` gating and uses legacy only for validation.
src/native/managed/cdac/inc/cdac_reader.h	Updates API comments describing legacy impl usage (lifetime + Debug comparisons only).
src/coreclr/debug/daccess/daccess.cpp	Skips cDAC creation for `ICLRDataEnumMemoryRegions` under `ENABLE_CDAC`; handles CreateSosInterface failure without asserting.
src/coreclr/debug/daccess/cdac.h	Changes `CreateSosInterface` to return an `int` status.
src/coreclr/debug/daccess/cdac.cpp	Propagates `cdac_reader_create_sos_interface` failure code instead of asserting success.

Suppressed comments (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetaDataImportImpl.cs:160

ResetEnum is implemented for cDAC-owned enumeration handles; for an unknown handle, returning E_NOTIMPL is misleading. Return E_INVALIDARG to indicate the handle is invalid for this implementation.

        if (_cdacEnumHandles.ContainsKey(hEnum))
        {
            MetadataEnum e = GetEnum(hEnum);
            e.Position = (int)Math.Min(ulPos, (uint)e.Tokens.Count);
            return HResults.S_OK;
        }

        return HResults.E_NOTIMPL;

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Fix the cross-platform IID check that broke CI, harden SOS interface creation, and complete the remaining debug-validation and metadata enumeration review updates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e3ee7201-c7f9-4f0d-b979-fbff0c77208b

Copilot

🟡 Changes recommended

CDAC::CreateSosInterface can fail without reliably initializing the out-parameter, which is a correctness risk in the ENABLE_CDAC path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/native/managed/cdac/tests/UnitTests/MetaDataImportImplTests.cs:1080

Test method name is inconsistent with nearby tests (e.g., "..._ReturnsZero"); rename to "..._ReturnsInvalidArg" for consistency and readability.

Files reviewed: 23/23 changed files
Comments generated: 1
Review effort level: Lite

Validate and clear the SOS interface out parameter before resolving the managed cDAC export, and align the metadata enumeration test name with local conventions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e3ee7201-c7f9-4f0d-b979-fbff0c77208b

Copilot

🔵 Needs a closer look

There are correctness/reliability issues in the updated code/tests (notably output initialization for E_NOTIMPL paths and a potentially fragile test sentinel) that should be addressed before merging.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTypeInstance.cs:36

When returning E_NOTIMPL from a COM method with output pointer parameters, initialize outputs to a safe default (e.g., *handle = 0) to avoid callers accidentally using an uninitialized value in cleanup paths.
src/native/managed/cdac/tests/UnitTests/MetaDataImportImplTests.cs:1085
unknownHandle is set to 1, which can coincide with a valid GCHandle.ToIntPtr(...) value if any cDAC enum handle has been allocated in this test process, making this test potentially flaky. Use a non-trivial sentinel value that cannot realistically be a valid cDAC enum handle.

Files reviewed: 23/23 changed files
Comments generated: 0 new
Review effort level: Lite

Clear enumeration handles before returning E_NOTIMPL across the affected legacy wrappers, add regression coverage for cleanup safety, and use a robust invalid metadata handle sentinel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e3ee7201-c7f9-4f0d-b979-fbff0c77208b

Copilot

🟡 Changes recommended

CDAC::CreateDacDbiInterface remains assertion-based and can still AV in Release if the export is missing or fails, which is inconsistent with the newly-hardened CreateSosInterface path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 24/24 changed files
Comments generated: 1
Review effort level: Lite

Validate and clear the DBI output pointer, handle a missing managed export without calling through null, propagate the entrypoint HRESULT, and require successful creation at the caller. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e3ee7201-c7f9-4f0d-b979-fbff0c77208b

Copilot

🔵 Needs a closer look

It changes core debugger/DAC behavior across managed + native layers (fallback removal, lifetime/validation semantics), which has a large compatibility surface and warrants maintainer review.

Review details

Files reviewed: 25/25 changed files
Comments generated: 0 new
Review effort level: Lite

noahfalk · 2026-09-05T08:42:35Z

/azp run runtime-diagnostics

azure-pipelines · 2026-09-05T08:42:53Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot AI lite review requested due to automatic review settings August 24, 2026 21:50

github-actions Bot added the area-Diagnostics-cdac label Aug 24, 2026

Copilot started reviewing on behalf of noahfalk August 24, 2026 21:50 View session

dotnet-policy-service Bot assigned noahfalk Aug 24, 2026

Copilot AI reviewed Aug 24, 2026

View reviewed changes

Apply suggestions from code review

40889f9

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings August 24, 2026 22:45

Copilot started reviewing on behalf of noahfalk August 24, 2026 22:46 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

max-charlamb approved these changes Aug 24, 2026

View reviewed changes

Comment thread src/coreclr/debug/daccess/cdac.cpp Outdated

Comment thread .../managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs Outdated

Copilot AI review requested due to automatic review settings September 2, 2026 09:49

Copilot started reviewing on behalf of noahfalk September 2, 2026 09:50 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread src/coreclr/debug/daccess/cdac.cpp

Copilot AI review requested due to automatic review settings September 2, 2026 10:32

Copilot started reviewing on behalf of noahfalk September 2, 2026 10:33 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 2, 2026 18:24

Copilot started reviewing on behalf of noahfalk September 2, 2026 18:25 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread src/coreclr/debug/daccess/cdac.cpp

This was referenced Sep 3, 2026

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

Copilot AI review requested due to automatic review settings September 3, 2026 10:00

Copilot started reviewing on behalf of noahfalk September 3, 2026 10:00 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Uh oh!

Conversation

noahfalk commented Aug 24, 2026

Uh oh!

azure-pipelines Bot commented Aug 24, 2026

Uh oh!

dotnet-policy-service Bot commented Aug 24, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

noahfalk commented Sep 5, 2026

Uh oh!

azure-pipelines Bot commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants