Skip to content

[cDAC] Add typed field extension methods for Data classes#126713

Draft
max-charlamb wants to merge 2 commits intomainfrom
dev/max-charlamb-/runtime-cdac-managed-typing
Draft

[cDAC] Add typed field extension methods for Data classes#126713
max-charlamb wants to merge 2 commits intomainfrom
dev/max-charlamb-/runtime-cdac-managed-typing

Conversation

@max-charlamb
Copy link
Copy Markdown
Member

Summary

Add typed field extension methods for the cDAC managed reader and convert all IData<T> classes to use them. This builds on the native data descriptor type system (#126163) to add managed-side type validation.

Changes

TargetFieldExtensions.cs — New extension methods on Target:

  • ReadField<T> — reads a primitive field with Debug.Assert type validation
  • ReadPointerField / ReadPointerFieldOrNull — reads pointer fields (required/optional)
  • ReadNUIntField — reads native unsigned integer fields
  • ReadCodePointerField — reads code pointer fields
  • ReadDataField<T> / ReadDataFieldPointer<T> — reads inline/pointer-to Data struct fields
  • ReadFieldOrDefault<T> — reads optional primitive fields with default value

When the data descriptor includes type information (debug/checked builds), these methods assert that the declared field type is compatible with the C# read type.

All ~130 IData<T> classes converted to use the new extension methods, replacing direct target.Read<T>(address + (ulong)type.Fields[...].Offset) calls.

Bugs found and fixed

The type system caught several pre-existing issues:

  1. DynamicStaticsInfo.GCStatics/NonGCStatics — descriptor said uint32, native type is TADDR (pointer)
  2. EEClass.FieldDescList — used Read<ulong> for a pointer field (broken on 32-bit)
  3. ExceptionClause.ClassToken — read TypeHandle (nuint) as uint
  4. SimpleComCallWrapper.RefCount — property was ulong, native is LONGLONG (signed)
  5. NativeCodeVersionNode.NativeCode — descriptor was T_POINTER, actually CodePointer
  6. MethodTable.EEClassOrCanonMT — descriptor was T_NUINT, managed reads as pointer

Note

This content was generated with the assistance of GitHub Copilot.

Add TargetFieldExtensions with ReadField<T>, ReadPointerField,
ReadNUIntField, ReadCodePointerField, ReadDataField<T>, and
ReadDataFieldPointer<T> extension methods. Each method validates
the field type name from the data descriptor (when present) against
the C# read type via Debug.Assert.

Convert all ~110 IData<T> classes to use the new extension methods.

Fix pre-existing bugs found by the type system:
- DynamicStaticsInfo.GCStatics/NonGCStatics: was uint32, is TADDR (pointer)
- EEClass.FieldDescList: was Read<ulong>, should be ReadPointer
- ExceptionClause.ClassToken: was reading TypeHandle (nuint) as uint
- SimpleComCallWrapper.RefCount: was ulong, native is LONGLONG (signed)
- NativeCodeVersionNode.NativeCode: was T_POINTER, is PCODE (CodePointer)
- MethodTable.EEClassOrCanonMT: was T_NUINT, managed reads as pointer

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds managed-side typed field read helpers for cDAC Target and migrates cDAC IData<T> data readers to use them, enabling debug/checked build validation against native data descriptor field types.

Changes:

  • Introduces TargetFieldExtensions with typed field reads and Debug.Assert type compatibility checks.
  • Converts cDAC IData<T> data classes to use the new typed read helpers (replacing manual Read<T>(address + offset) patterns).
  • Adds descriptor/type verification infrastructure (test + generator + runtime helper) and updates select native descriptor field annotations.

Reviewed changes

Copilot reviewed 115 out of 115 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/native/managed/cdac/tests/ContractDescriptor/TypeVerificationTests.cs Adds a test intended to validate managed reads against a checked descriptor’s declared field types.
src/native/managed/cdac/tests/ContractDescriptor/checked-descriptor.json Adds a checked-build contract descriptor fixture containing field offsets + type names.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/DataDescriptorTypeVerification.cs Adds a DEBUG-only verification entrypoint and a noop Target wrapper for type-check instantiation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Generators/Microsoft.Diagnostics.DataContractReader.Generators.csproj Adds a new Roslyn generator project intended to generate verification code.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Generators/DataTypeVerificationGenerator.cs Implements a generator to discover IData<T> implementations and emit VerifyAllDataTypes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Microsoft.Diagnostics.DataContractReader.Contracts.csproj Adds InternalsVisibleTo for Microsoft.Diagnostics.DataContractReader.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/VirtualCallStubManager.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/UnwindInfo.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/TypeDesc.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/TLSIndex.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ThreadStressLog.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ThreadStore.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ThreadLocalData.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Thread.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ThisPtrRetBufPrecodeData.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/TableSegment.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/SystemDomain.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/SyncTableEntry.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/SyncBlockCache.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/SyncBlock.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/StubPrecodeData.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/String.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/StressLogModuleDesc.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/StressLogChunk.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/StressLog.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/SimpleComCallWrapper.cs Switches field reads to typed helper methods; adjusts RefCount type.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/RuntimeFunction.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/RealCodeHeader.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ReadyToRunSection.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ReadyToRunInfo.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ReadyToRunHeader.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ReadyToRunCoreInfo.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ReadyToRunCoreHeader.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/RCWCleanupList.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/RCW.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/RangeSectionFragment.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/RangeSection.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ProfControlBlock.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ProbeExtensionResult.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PrecodeMachineDescriptor.cs Switches field reads to typed helper methods (and modifies precode parsing logic).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PortableEntryPoint.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PlatformMetadata.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PEImageLayout.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PEImage.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PEAssembly.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PatchpointInfo.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ObjectHeader.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Object.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/NativeObjectWrapperObject.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/NativeCodeVersionNode.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ModuleLookupMap.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Module.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/MethodTableAuxiliaryData.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/MethodTable.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/MethodDescVersioningState.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/MethodDescCodeData.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/MethodDescChunk.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/MethodDesc.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ManagedObjectWrapperLayout.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ManagedObjectWrapperHolderObject.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/LoaderAllocator.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/InterfaceEntry.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/InflightTLSData.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ImageDataDirectory.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ILCodeVersionNode.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ILCodeVersioningState.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/IdDispenser.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/HashMap.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/HandleTableMap.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/HandleTableBucket.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/HandleTable.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/GenericsDictInfo.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/GCAllocContext.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/GC/OOMHistory.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/GC/HeapSegment.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/GC/Generation.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/GC/GCHeapSVR.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/TransitionBlock.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/TailCallFrame.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/StubDispatchFrame.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/SoftwareExceptionFrame.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/ResumableFrame.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/InlinedCallFrame.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/HijackFrame.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/HijackArgsAMD64.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/FuncEvalFrame.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/FramedMethodFrame.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/Frame.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Frames/DebuggerEval.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/FixupPrecodeData.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/FieldDesc.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ExceptionLookupTableEntry.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ExceptionInfo.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ExceptionClause.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Exception.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/EEJitManager.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/EEClass.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DynamicStaticsInfo.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DynamicMetadata.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DynamicILBlobEntry.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DacEnumerableHash.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/CtxEntry.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/CoreLibBinder.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ComMethodTable.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ComCallWrapper.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/CodeHeapListNode.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/CGrowableSymbolStream.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/AssemblyBinder.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Assembly.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ArrayListBase.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Array.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/AppDomain.cs Switches field reads to typed helper methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/BuiltInCOM_1.cs Adjusts SCCW refcount masking after RefCount signedness change.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/TargetFieldExtensions.cs Adds the new typed field read extensions + type compatibility asserts.
src/coreclr/vm/datadescriptor/datadescriptor.inc Updates select native descriptor field type annotations to match actual semantics.
Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PrecodeMachineDescriptor.cs:57

  • StubPrecodeSize is read using nameof(FixupStubPrecodeSize) (line 46), so it will fetch the wrong field offset and size when both fields exist. This will corrupt StubBytes/StubIgnoredBytes reads. Read StubPrecodeSize from its own field name and ensure the else branch resets the Stub* members (currently it resets Fixup* instead).

Comment on lines +80 to +83
private static void CreateData<T>(Target target) where T : IData<T>
{
T.Create(target, default);
}
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateData<T> is unused (the test uses TryCreate<T> everywhere). With warnings-as-errors enabled in this repo, unused members can break builds; remove it or use it.

Copilot uses AI. Check for mistakes.
@github-actions

This comment has been minimized.

@max-charlamb max-charlamb force-pushed the dev/max-charlamb-/runtime-cdac-managed-typing branch from ee38f27 to a291681 Compare April 9, 2026 17:00
Copilot AI review requested due to automatic review settings April 9, 2026 17:04
@max-charlamb max-charlamb force-pushed the dev/max-charlamb-/runtime-cdac-managed-typing branch from a291681 to c3a2982 Compare April 9, 2026 17:04
…sions

Convert Data classes in Frames/, GC/, and root Data/ that were missed
by the initial conversion (subdirectory files, string literal field keys,
underscore-prefixed variable names).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@max-charlamb max-charlamb force-pushed the dev/max-charlamb-/runtime-cdac-managed-typing branch from c3a2982 to 8ad41e4 Compare April 9, 2026 17:07
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 115 out of 115 changed files in this pull request and generated 2 comments.

Comment on lines 43 to 49

if (type.Fields.ContainsKey(nameof(StubPrecodeSize)))
{
StubPrecodeSize = target.Read<byte>(address + (ulong)type.Fields[nameof(FixupStubPrecodeSize)].Offset);
StubPrecodeSize = target.ReadField<byte>(address, type, nameof(FixupStubPrecodeSize));
StubBytes = new byte[StubPrecodeSize.Value];
target.ReadBuffer(address + (ulong)type.Fields[nameof(StubBytes)].Offset, StubBytes);
StubIgnoredBytes = new byte[StubPrecodeSize.Value];
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the StubPrecodeSize branch, StubPrecodeSize is read using nameof(FixupStubPrecodeSize) rather than nameof(StubPrecodeSize), so it will use the wrong field offset/value (and may throw if the fixup field isn't present). Also, the else branch for StubPrecodeSize currently nulls out FixupBytes/FixupIgnoredBytes instead of StubBytes/StubIgnoredBytes, which can leave the stub byte arrays in an incorrect state.

Copilot uses AI. Check for mistakes.
Comment on lines 21 to +24
{
GCCoverageInfo = target.ReadPointer(address + (ulong)type.Fields[nameof(GCCoverageInfo)].Offset);
GCCoverageInfo = target.ReadPointerField(address, type, nameof(GCCoverageInfo));
}
OptimizationTier = target.Read<uint>(address + (ulong)type.Fields[nameof(OptimizationTier)].Offset);
OptimizationTier = target.ReadField<uint>(address, type, nameof(OptimizationTier));
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OptimizationTier is still read via target.Read<uint>(address + ...) instead of the new ReadField<uint>(...) helper. Using ReadField here would keep this type’s field reads consistent and allow the descriptor type validation to apply to OptimizationTier as well.

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

🤖 Copilot Code Review — PR #126713

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: Well-justified. Introduces typed field extension methods that reduce boilerplate across ~110 Data classes and add debug-time type validation via Debug.Assert. The type validation already surfaced 6 real type mismatches in the existing code (e.g., reading a pointer as uint32, reading a signed LONGLONG as ulong). This is a meaningful improvement to maintainability and correctness.

Approach: Good. Extension methods on Target are idiomatic and discoverable. The [Conditional("DEBUG")] assertions have zero overhead in release builds. The conversion is mechanical and straightforward, keeping the behavioral change surface small.

Summary: ⚠️ Needs Human Review. The change is well-motivated and the mechanical conversion looks correct throughout. However, there is one pre-existing bug that is now more visible and should ideally be fixed in this PR, plus a few items for the author/reviewer to confirm.


Detailed Findings

✅ Type Bug Fixes — Correct based on native type declarations

All 6 type corrections mentioned in the commit message are verified against native C++ types:

Field Old (incorrect) New (correct) Verified against
DynamicStaticsInfo.GCStatics/NonGCStatics T_UINT32 / Read<uint> T_POINTER / ReadPointerField TADDR in methodtable.h:612-613
EEClass.FieldDescList Read<ulong> (always 8 bytes) ReadPointerField (pointer-size) T_POINTER in datadescriptor.inc
SimpleComCallWrapper.RefCount ulong long T_INT64 in datadescriptor.inc:1219
NativeCodeVersionNode.NativeCode T_POINTER TYPE(CodePointer) PCODE type semantics
MethodTable.EEClassOrCanonMT T_NUINT T_POINTER Union of DPTR(EEClass) / TADDR in methodtable.h:3980-3981
ExceptionClause.ClassToken Double-read of TypeHandle as uint Derives from NUInt value Avoids wrong-size read on 64-bit

The SimpleComCallWrapper.RefCount consumer in BuiltInCOM_1.cs was correctly updated: the ComRefCount enum was already long-based, and the & RefCountMask (0x7FFFFFFF) result is safely cast to (uint) before widening to the ulong property on SimpleComCallWrapperData.

✅ TargetFieldExtensions Design — Well structured

The API covers the key field read patterns (primitive, pointer, NUInt, CodePointer, inline data, data pointer) with optional/required variants. [Conditional("DEBUG")] on the assertion helpers is the right pattern.

✅ datadescriptor.inc Changes — Correct

The 3 changes to the native data descriptor match the actual C++ field types.

⚠️ Pre-existing Bug in PrecodeMachineDescriptor — Now more visible, should fix

PrecodeMachineDescriptor.cs line 46:

StubPrecodeSize = target.ReadField<byte>(address, type, nameof(FixupStubPrecodeSize));

This reads FixupStubPrecodeSize but assigns to StubPrecodeSize. This was a pre-existing bug (the old code had the same nameof(FixupStubPrecodeSize) mismatch), but the mechanical conversion preserved it. Since this PR touches exactly this line and the extension methods are specifically designed to make field names explicit, this would be a good opportunity to fix it to nameof(StubPrecodeSize).

This is flagged as a warning rather than an error because it's pre-existing and the fix could go into a follow-up if preferred.

💡 Incomplete Conversion of Inline Data Reads — Follow-up opportunity

Seven+ ProcessedData.GetOrAdd<T>(address + (ulong)type.Fields[...].Offset) patterns (in EEAllocContext, InflightTLSData, PEImage, ReadyToRunSection, RuntimeThreadLocals, GCHeapSVR, Generation) could use ReadDataField<T>. Only Thread.cs uses the new ReadDataFieldPointer<T>. The MaybeGetPrecodeType local function in PrecodeMachineDescriptor also still uses the old pattern.

This is fine as a follow-up since the methods were introduced to also benefit code outside Data classes, but worth noting for completeness.

💡 Some ContainsKey Patterns Could Use OrNull/OrDefault

Several if (type.Fields.ContainsKey(...)) ReadPointerField(...) patterns (in LoaderAllocator, GCHeapSVR, MethodDesc.GCCoverageInfo, InlinedCallFrame.SPAfterProlog) could use ReadPointerFieldOrNull. However, some of these have additional logic beyond a simple read, so the existing explicit patterns may be clearer. Author's judgment call.

Generated by Code Review for issue #126713 ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants