[cDAC] Add debug type validation for global reads#126849
[cDAC] Add debug type validation for global reads#126849max-charlamb wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR adds debug-only validation to cDAC global reads, aiming to catch mismatches between a global’s declared data-descriptor type (e.g., uint32, nuint, pointer) and the managed read API used (ReadGlobal<T> vs ReadGlobalPointer) during development.
Changes:
- Introduces
DataDescriptorTypeValidationdebug-only assertion helpers for validating global read type compatibility. - Hooks validation into
ContractDescriptorTarget.TryReadGlobal<T>andTryReadGlobalPointer(and adds an internal raw read helper for shared plumbing). - Refactors
TargetFieldExtensionsto keep its assertions self-contained (avoiding cross-project dependency on the new validation helper).
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/DataDescriptorTypeValidation.cs | Adds debug-only validation helpers for global/primitive/pointer type compatibility checks. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs | Integrates the new debug assertions into global read paths and factors out a raw global read helper. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/TargetFieldExtensions.cs | Keeps field-type validation local to the abstractions project and slightly refactors the assertion helpers. |
| DataDescriptorTypeValidation.AssertGlobalType<T>(type, name); | ||
| value = T.CreateChecked(rawValue.Value); |
There was a problem hiding this comment.
AssertGlobalType<T> will assert for globals declared as nuint/nint when callers currently use ReadGlobal<ulong>/ReadGlobal<long>. For example, CoreCLR’s datadescriptor declares CCWThisMask as T_NUINT, but BuiltInCOM_1 reads it via ReadGlobal<ulong>, which will now trip this debug assert. Either update affected call sites to use ReadGlobalPointer (and use .Value as needed) or relax the validation to treat nuint/nint as compatible with corresponding integral reads.
| /// <summary> | ||
| /// Debug-only helpers that validate cDAC type annotations match the C# read type. | ||
| /// In release builds, all methods are completely elided by the <see cref="ConditionalAttribute"/>. | ||
| /// </summary> |
There was a problem hiding this comment.
The XML doc claims all methods are “completely elided” in release builds via ConditionalAttribute, but the methods still exist in the assembly and only call sites are conditionally omitted. Also IsCompatiblePrimitiveType<T> is not conditional, so it is never “elided” at all. Please reword the documentation to reflect the actual behavior (or adjust the implementation to match the docs).
| /// <summary> | ||
| /// Assert that a global's declared type is compatible with <c>ReadGlobalPointer</c>. | ||
| /// Accepts pointer, nuint, nint, or absent type information. | ||
| /// </summary> | ||
| [Conditional("DEBUG")] | ||
| public static void AssertGlobalPointerType(string? typeName, string globalName) | ||
| { | ||
| Debug.Assert( | ||
| typeName is null or "" or "pointer" or "nuint" or "nint" or "string", | ||
| $"Type mismatch reading global '{globalName}' as pointer: declared as '{typeName}', expected pointer/nuint/nint"); |
There was a problem hiding this comment.
AssertGlobalPointerType’s XML doc says it accepts only pointer/nuint/nint (or absent type info), but the implementation also allows "string". Either update the documentation to include string (and why it’s allowed) or tighten the assertion to match the doc.
Add DataDescriptorTypeValidation as a shared helper in Abstractions for validating that data descriptor type annotations match the C# read type. Used by both field extensions and global reads. - AssertPrimitiveType<T> — validates primitive field/global types - AssertPointerType — validates pointer field types - AssertGlobalType<T> — validates ReadGlobal<T> calls; pointer-like globals (pointer, nuint, nint) must use ReadGlobalPointer instead - AssertGlobalPointerType — validates ReadGlobalPointer calls - TargetFieldExtensions delegates to shared helpers instead of duplicating validation logic - ContractDescriptorTarget uses TryReadGlobalRaw internally to separate raw reads from validated reads All methods are [Conditional(DEBUG)] and fully elided in release. Contributes to dotnet#126749 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
max-charlamb
force-pushed
the
dev/max-charlamb/cdac-typed-globals
branch
from
355bf0e to
0444fc3
Compare
Summary
Add debug-only type validation for global reads in the cDAC, catching type mismatches between the declared data descriptor type and the C# read type at development time.
Changes
New file:
DataDescriptorTypeValidation.cs(inMicrosoft.Diagnostics.DataContractReader)AssertGlobalType<T>— validatesReadGlobal<T>calls match the declared primitive type (uint8→byte, uint32→uint, etc.)AssertGlobalPointerType— validatesReadGlobalPointercalls are used for pointer/nuint/nint globalsReadGlobal<T>— they must useReadGlobalPointer[Conditional("DEBUG")]and fully elided in release buildsContractDescriptorTarget.csTryReadGlobal<T>validates viaAssertGlobalTypeTryReadGlobalPointervalidates viaAssertGlobalPointerTypeTryReadGlobalRawprovides unvalidated access for the pointer→ulong delegation pathTargetFieldExtensions.csContributes to #126749
Note
This PR was generated with the assistance of GitHub Copilot.