WIP - Refactor fallback testing - #131619
Draft
noahfalk wants to merge 1 commit into
Draft
Conversation
I'm still working on this but posting it in case anyone was curious. The goal is to extract test code and test configuration out of our production binaries and isolate them. It creates a dedicated test validation shim that consumers can treat as if it was the cDAC binary. The shim loads both cDAC and DAC, calls APIs in both and validates the results agree. The SOS tests are modified to use it during the fallback/verify modes and it doesn't get used in the cDac mode. Aside from a bit of binary size improvement and simplification of the production code, this should reduce the risk that we accidentally fallback in production code where we didn't intend to. Not every place in the code that was manipulating the _legacyImpl interface reference was uniformly checking with LegacyFallbackHelper.CanFallback() prior to doing the fallback. The test shim also means we can do DAC vs cDAC comparison testing on retail builds of cDAC.
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors cDAC fallback/verification testing by removing legacy-DAC comparison/fallback logic from production binaries and introducing a dedicated test-only validation shim that loads both the production cDAC and the legacy DAC and compares results. It also updates the SOS test pipeline and documentation to use the shim in fallback/verify modes.
Changes:
- Adds a new on-demand build subset (
tools.cdacvalidationshim) and NativeAOT projectmscordaccore_cdac_validation_shim(never packaged) for cDAC vs DAC validation during SOS tests. - Removes legacy-implementation parameters / embedded validation paths from production cDAC and native DAC integration points (e.g.,
cdac_reader_create_*, managed entrypoints, and CoreCLR DAC codepaths). - Updates cDAC unit tests, pipelines, and docs to align with the new standalone production cDAC + validation-shim testing model.
Reviewed changes
Copilot reviewed 87 out of 91 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/compile-native.proj | Adds conditional build of the validation shim project. |
| eng/Subsets.props | Introduces Tools.CdacValidationShim subset and routes compile-native.proj with BuildCdacValidationShim. |
| eng/pipelines/runtime-diagnostics.yml | Builds and publishes shim artifacts; updates SOS legs to include shim build. |
| eng/pipelines/diagnostics/sos-test-leg.yml | Downloads shim artifacts for cdacfallback/cdacverify legs. |
| eng/pipelines/diagnostics/runtime-diag-job.yml | Wires cdacfallback/cdacverify args including shim + legacy DAC paths. |
| src/native/managed/cdac/README.md | Updates docs for new SOS integration modes and shim-based validation. |
| src/native/managed/cdac/cdac.slnx | Adds the validation shim project to the solution. |
| src/native/managed/cdac/inc/cdac_reader.h | Updates exported function signatures to remove legacy fallback parameters. |
| src/coreclr/vm/cdacstress.cpp | Updates cdac stress path to new cdac_reader_create_sos_interface signature. |
| src/coreclr/debug/daccess/request.cpp | Removes cDAC header include from DAC request code. |
| src/coreclr/debug/daccess/dacimpl.h | Removes cDAC member/include from ClrDataAccess. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Removes ENABLE_CDAC-based cDAC instantiation path from native DBI creation. |
| src/coreclr/debug/daccess/daccess.cpp | Removes ENABLE_CDAC-based cDAC instantiation path from CLRDataCreateInstance. |
| src/coreclr/debug/daccess/cdac.h | Updates CDAC::Create signature and removes legacy impl storage. |
| src/coreclr/debug/daccess/cdac.cpp | Updates CDAC creation and interface creation to no longer pass legacy impl. |
| src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs | Removes legacy fallback parameters/exports; keeps standalone creation paths. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Microsoft.Diagnostics.DataContractReader.Legacy.csproj | Suppresses unused parameter/value IDE warnings after removing legacy paths. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/README.md | Updates guidance: production cDAC is standalone; validation moved to shim. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.ICLRDataEnumMemoryRegions.cs | Removes legacy fallback and returns E_NOTIMPL. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.NativeCodeInfo.cs | Removes debug-only legacy validation helper code. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataValue.cs | Removes legacy implementation storage and debug comparison blocks. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTypeInstance.cs | Removes legacy fallback delegation; returns E_NOTIMPL consistently. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTypeDefinition.cs | Removes legacy fallback delegation; returns E_NOTIMPL consistently. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTask.cs | Removes legacy child pairing; constructs cDAC-only child objects. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataStackWalk.cs | Removes legacy stackwalk coupling and debug comparisons. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataExceptionState.cs | Removes legacy fallback/comparison and legacy child pairing. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataAppDomain.cs | Removes legacy comparison blocks and legacy impl storage. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/LegacyFallbackHelper.cs | Deletes legacy fallback helper (moved to shim). |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/DebugExtensions.cs | Deletes debug validation extensions (moved to shim). |
| src/native/managed/cdac/tests/UnitTests/SOSDacInterface8Tests.cs | Updates constructor usage after removing legacyObj parameter. |
| src/native/managed/cdac/tests/UnitTests/SOSDacInterface5Tests.cs | Updates SOSDacImpl construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/ObjectTests.cs | Updates SOSDacImpl construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/MethodTableTests.cs | Updates SOSDacImpl construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/MetaDataImportImplTests.cs | Updates MetaDataImportImpl construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/LoaderTests.cs | Updates SOSDacImpl/DacDbiImpl construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs | Updates SOSDacImpl construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/GetRegisterNameTests.cs | Updates SOSDacImpl construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/FunctionTableAccessTests.cs | Updates SOSDacImpl construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/DacDbiImplTests.cs | Updates DacDbiImpl construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/ClrDataTaskTests.cs | Updates ClrDataTask construction after signature change. |
| src/native/managed/cdac/tests/UnitTests/ClrDataRequestTests.cs | Updates request test object construction after signature changes. |
| src/native/managed/cdac/tests/UnitTests/ClrDataExceptionStateTests.cs | Updates ClrDataTask/ClrDataExceptionState construction after signature changes. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/mscordaccore_cdac_validation_shim.csproj | Adds new test-only NativeAOT shim project configuration (always DEBUG, never packaged). |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/README.md | Documents shim purpose/exports for SOS test usage. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Hosting/ShimEnvironment.cs | Shim env var parsing and module-path discovery for locating adjacent production cDAC. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Hosting/NativeModules.cs | Loads/pins production cDAC + legacy DAC and resolves required exports. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Hosting/LegacyFallbackHelper.cs | Implements strict/fallback allowlist behavior for shim-side delegation. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Hosting/Debug.cs | Shim-local debug/assert plumbing that logs mismatches while preserving original behavior. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Hosting/ValidationSession.cs | Provides per-instance pairing cache + handle registry and per-call recording state. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Hosting/ShimLog.cs | Centralizes shim stderr logging format. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Hosting/CorDebugDataTargetProxies.cs | Implements record/replay proxies for mutable CorDebug data target interactions. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Interop/CorElementType.cs | Duplicates CorElementType for interop declarations without referencing production assemblies. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/ShimProxy.cs | Base proxy behavior including QI-surface gating and unwrap helpers. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/ClrDataStackWalkProxy.cs | Adds paired proxy implementation for IXCLRDataStackWalk with comparison/replay behaviors. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/ClrDataModuleProxy.Metadata.cs | Adds custom QI pairing for metadata import objects returned by module QI. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/ClrDataAssemblyProxy.cs | Adds paired proxy for IXCLRDataAssembly. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/ClrDataAppDomainProxy.cs | Adds paired proxy for IXCLRDataAppDomain. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/SOSHandleEnumProxy.cs | Adds paired proxy for ISOSHandleEnum / ISOSEnum. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/SOSMemoryEnumProxy.cs | Adds paired proxy for ISOSMemoryEnum / ISOSEnum. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/SOSMethodEnumProxy.cs | Adds paired proxy for ISOSMethodEnum / ISOSEnum with debug validation. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/SOSStackRefEnumProxy.cs | Adds paired proxy for ISOSStackRefEnum / ISOSEnum. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/SOSStackRefErrorEnumProxy.cs | Adds paired proxy for ISOSStackRefErrorEnum / ISOSEnum. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/SOSStressLogMsgEnumProxy.cs | Adds paired proxy for ISOSStressLogMsgEnum / ISOSEnum. |
| src/native/managed/cdac/mscordaccore_cdac_validation_shim/Proxies/SOSStressLogThreadEnumProxy.cs | Adds paired proxy for ISOSStressLogThreadEnum / ISOSEnum. |
Comment on lines
+177
to
+188
| try | ||
| { | ||
| result = dladdr_libc(address, out info); | ||
| } | ||
| catch (EntryPointNotFoundException) | ||
| { | ||
| result = dladdr_libdl(address, out info); | ||
| } | ||
| catch (DllNotFoundException) | ||
| { | ||
| result = dladdr_libdl(address, out info); | ||
| } |
Comment on lines
+96
to
+104
| private static string? GetShimModulePath() | ||
| { | ||
| if (s_shimModulePathResolved) | ||
| return s_shimModulePath; | ||
|
|
||
| s_shimModulePath = ResolveShimModulePath(); | ||
| s_shimModulePathResolved = true; | ||
| return s_shimModulePath; | ||
| } |
max-charlamb
reviewed
Aug 3, 2026
Comment on lines
+8
to
+12
| Before the production decoupling, this comparison lived inside the cDAC itself: the in-box | ||
| DAC created the cDAC reader, handed it its own `ISOSDacInterface` and the cDAC compared | ||
| results in `#if DEBUG` blocks. That coupling is gone from the product. The comparison blocks | ||
| were moved here verbatim, so the validation behavior is preserved without the production cDAC | ||
| knowing that a legacy DAC exists. |
Member
There was a problem hiding this comment.
nit: before merging we should remove references to the 'before pr' state
Comment on lines
+183
to
+184
| #if DEBUG | ||
| if (_legacyImpl is not null) |
Member
There was a problem hiding this comment.
With this approach, can we remove the #if DEBUG defines?
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| namespace Microsoft.Diagnostics.DataContractReader.Contracts; |
| using System; | ||
| using System.Runtime.InteropServices.Marshalling; | ||
|
|
||
| namespace Microsoft.Diagnostics.DataContractReader.Legacy; |
Member
There was a problem hiding this comment.
I think it would be reasonable to put this all in a new namespace like: Microsoft.Diagnostics.DataContractReader.Legacy.Validation
| // The entire DBI interface is deferred — the cDAC does not implement ICorDebug data access yet. | ||
| private static readonly HashSet<string> s_fileAllowlist = new(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| "DacDbiImpl.cs", |
Member
There was a problem hiding this comment.
At some point, we should remove this blanket allowlist now that most Dbi APIs are implemented.
| /// <c>CDAC_NO_FALLBACK=1</c> behavior. | ||
| /// </summary> | ||
| Strict, | ||
| } |
Member
There was a problem hiding this comment.
nit: This seems like it belongs in a different file
Comment on lines
44
to
66
| @@ -66,24 +64,6 @@ int IXCLRDataFrame.GetContext( | |||
| hr = ex.HResult; | |||
| } | |||
|
|
|||
Member
There was a problem hiding this comment.
With this change, we should be able to remove the try/catch blocks in the cDAC implementation completely. We previously needed them to handle the validation, but that is handled by the shim.
Thrown exceptions are automatically handled by the GeneratedComInterface ExceptionAsHResultMarshaller<T>. We can override it if needed [GeneratedComInterface(ExceptionToUnmanagedMarshaller = typeof(MyExceptionMarshaller))]
Given the size, I think that would make sense as a follow-up
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm still working on this but posting it in case anyone was curious. The goal is to extract test code and test configuration out of our production binaries and isolate them. It creates a dedicated test validation shim that consumers can treat as if it was the cDAC binary. The shim loads both cDAC and DAC, calls APIs in both and validates the results agree. The SOS tests are modified to use it during the fallback/verify modes and it doesn't get used in the cDac mode. Aside from a bit of binary size improvement and simplification of the production code, this should reduce the risk that we accidentally fallback in production code where we didn't intend to. Not every place in the code that was manipulating the _legacyImpl interface reference was uniformly checking with LegacyFallbackHelper.CanFallback() prior to doing the fallback. The test shim also means we can do DAC vs cDAC comparison testing on retail builds of cDAC.