[cDAC] Validate contract presence in interface returning functions#131009
Draft
hoyosjs wants to merge 6 commits into
Draft
[cDAC] Validate contract presence in interface returning functions#131009hoyosjs wants to merge 6 commits into
hoyosjs wants to merge 6 commits into
Conversation
…point)
Intermediate checkpoint of the cDAC 'policy by support' work:
- CdacHResults: CDAC_E_CONTRACT_{NOT_ADVERTISED,UNRECOGNIZED,UNSUPPORTED}
and CDAC_E_DESCRIPTOR_{NOT_FOUND,MALFORMED} (customer-bit failure codes).
- ContractNotAvailableException hierarchy (Missing/Unrecognized/Obsolete)
and ContractValidationException carrying the distinct HRESULT.
- ContractRegistry.TryValidate + CachingContractRegistry no-instantiation
resolution (safe on partial/triage dumps).
- CoreCLRContracts.ValidateForSos eager validation of the SOS surface,
run at CLRDataCreateInstance only when no legacy DAC is supplied.
- ContractDescriptorTarget descriptor-read failures mapped to
CDAC_E_DESCRIPTOR_* via FormatException.HResult.
- Design docs: contract-validation.md + consumer guide.
- Tests in TargetTests.cs.
Squash the two commits from dotnet#130856 into one isolated change so it can be reverted independently.
This reverts commit f1eba34.
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens cDAC target/interface creation by switching from “try” patterns to exception-based creation with cDAC-specific HRESULTs, and by eagerly validating that all contracts required for SOS/IXCLRDataProcess (and related interfaces) are available before publishing those interfaces.
Changes:
- Replace
ContractDescriptorTarget.TryCreateusage withCreate(...)and surface descriptor failures asFormatExceptionwith cDAC-specificCdacHResultscodes. - Add a contract-availability exception hierarchy plus registry support for distinguishing missing vs unrecognized vs intentionally-unsupported contract versions, and introduce
TryValidatefor no-instantiation validation. - Add
CoreCLRContracts.ValidateForDataAccess(...)and call it from interface-creation entrypoints; expand unit coverage for malformed descriptors and contract/version validation behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/ContractDescriptor/TargetTests.cs | Adds tests for malformed descriptors, contract missing/unrecognized/obsolete behaviors, and eager validation paths. |
| src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs | Updates test contract registry to return structured exceptions and track unsupported versions. |
| src/native/managed/cdac/tests/TestInfrastructure/DumpTestBase.cs | Switches dump tests to exception-based target creation. |
| src/native/managed/cdac/tests/TestInfrastructure/ContractDescriptor/ContractDescriptorBuilder.cs | Updates descriptor builder to emit contract versions and to create targets via Create(...) with a Try-wrapper. |
| src/native/managed/cdac/tests/DataGenerator/TestTarget.cs | Updates generator test registry to new TryGetContract signature and new abstract members. |
| src/native/managed/cdac/scripts/DumpHelpers.cs | Switches dump-inspection helper to exception-based target creation. |
| src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs | Uses Create(...) and adds eager ValidateForDataAccess gating for interface publication. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs | Implements exception-based descriptor read with CdacHResults classification and stricter malformed/not-found distinction. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs | Adds unsupported-version tracking and TryValidate that avoids instantiation/target reads. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/CoreCLRContracts.cs | Adds ValidateForDataAccess and maps validation failures to distinct CdacHResults. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs | Changes public contract retrieval signature and adds TryValidate/RegisterUnsupported. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractNotAvailableException.cs | Introduces public exceptions describing contract availability and validation failures. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/CdacHResults.cs | Introduces cDAC-specific HRESULT constants for descriptor/contract validation failures. |
Comment on lines
+140
to
+146
| throw failure switch | ||
| { | ||
| ContractObsoleteException obsolete => new ContractValidationException(CdacHResults.CDAC_E_CONTRACT_UNSUPPORTED, obsolete), | ||
| ContractUnrecognizedException unrecognized => new ContractValidationException(CdacHResults.CDAC_E_CONTRACT_UNRECOGNIZED, unrecognized), | ||
| ContractNotAvailableException notAvailable => new ContractValidationException(CdacHResults.CDAC_E_CONTRACT_NOT_ADVERTISED, notAvailable), | ||
| _ => failure, | ||
| }; |
Comment on lines
453
to
457
| if (hr != 0) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"{nameof(ICLRContractLocator)} failed to fetch the contract descriptor with HRESULT: 0x{hr:x}."); | ||
| } |
Comment on lines
+160
to
+166
| /// <param name="failureException"> | ||
| /// When this method returns <see langword="false"/>, contains the exception that describes why the contract could not be retrieved; otherwise, <see langword="null"/>. | ||
| /// </param> | ||
| /// <returns> | ||
| /// <see langword="true"/> if the requested contract is present and was retrieved successfully; <see langword="false"/> if the contract is not present or registered"/>. | ||
| /// </returns> | ||
| public abstract bool TryGetContract<TContract>([NotNullWhen(true)] out TContract contract, out string? failureReason) where TContract : IContract; | ||
| public abstract bool TryGetContract<TContract>([NotNullWhen(true)] out TContract contract, [NotNullWhen(false)] out System.Exception? failureException) where TContract : IContract; |
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.
No description provided.