[cDAC] RuntimeSignatureDecoder and centralized Signature contract (2/5)#127636
[cDAC] RuntimeSignatureDecoder and centralized Signature contract (2/5)#127636max-charlamb wants to merge 9 commits intodotnet:mainfrom
Conversation
- Move RuntimeSignatureDecoder to Contracts/Signature/ - Move GcSignatureTypeProvider to Contracts/Signature/ with module-scoped caching - Add DecodeMethodSignatureForGC(BlobHandle, ModuleHandle) to ISignatureDecoder - Add DecodeFieldSignature to RuntimeSignatureDecoder - Add BlobHandleSignatureReader for lazy blob reading - SignatureTypeProvider implements IRuntimeSignatureTypeProvider - Switch DecodeFieldSignature to use RuntimeSignatureDecoder - GcScanner uses contract API instead of direct decoder construction Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR is part of the cDAC signature-decoding refactor, introducing a custom RuntimeSignatureDecoder capable of handling runtime-internal signature element types (ELEMENT_TYPE_INTERNAL / ELEMENT_TYPE_CMOD_INTERNAL) and centralizing GC-oriented signature decoding behind the ISignatureDecoder contract.
Changes:
- Added
RuntimeSignatureDecoderplus signature-reader abstractions (ISignatureReader, span/blob-backed readers) to support decoding from different signature sources. - Extended the
ISignatureDecodercontract withDecodeMethodSignatureForGC(...)and promotedGcTypeKindto a public abstraction. - Updated stack GC scanning (
GcScanner) and signature type providers to route decoding through the centralized contract API.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/GC/GcScanner.cs | Switches caller-signature decoding to the ISignatureDecoder.DecodeMethodSignatureForGC contract API. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/SignatureTypeProvider.cs | Updates the type provider to implement IRuntimeSignatureTypeProvider and adds internal-type callbacks. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/SignatureReaders.cs | Introduces abstractions for reading signature bytes from spans and metadata blobs. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/SignatureDecoder_1.cs | Adds module-scoped GC provider caching and implements DecodeMethodSignatureForGC using RuntimeSignatureDecoder. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/RuntimeSignatureDecoder.cs | New runtime-aware ECMA-335-ish signature decoder with support for internal runtime element types. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/GcSignatureTypeProvider.cs | Moves/updates GC classification provider to implement the runtime-aware provider interface and classify internal types via RTS. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ISignatureDecoder.cs | Extends the public contract surface with GcTypeKind and DecodeMethodSignatureForGC. |
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/GcSignatureTypeProvider.cs:26
GcSignatureTypeProviderstores_targetand_moduleHandlebut never uses either of them (the implementation uses thetargetmethod parameter instead). This is dead state that should be removed, or the implementation should consistently use the stored fields if they're intended for future module/target-scoped behavior.
Rewrites RuntimeSignatureDecoder as a readonly struct that mirrors SRM's SignatureDecoder API (ref BlobReader per method, allowTypeSpecifications flag), with only ELEMENT_TYPE_INTERNAL (0x21) and CMOD_INTERNAL (0x22) added on top via the new IRuntimeSignatureTypeProvider interface. Drops the custom ISignatureReader/BlobHandleSignatureReader/SpanSignatureReader abstraction since BlobReader already provides lazy reading. Fixes two latent bugs vs SRM: TypeDefOrRefOrSpec tag=3 now throws (was incorrectly returning Object), and the leading element type code is read as a compressed integer rather than a raw byte. Moves GC-specific signature decoding out of the Signature contract into the StackWalk contract (Option B). GcSignatureTypeProvider and GcTypeKind move to Contracts/StackWalk/GC/ under the StackWalkHelpers namespace. ISignatureDecoder no longer exposes DecodeMethodSignatureForGC; the _gcProviders cache is removed from SignatureDecoder_1; GcScanner constructs RuntimeSignatureDecoder<GcTypeKind, object?> directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…adataReader Splits IRuntimeSignatureTypeProvider into its own file (matches cDAC convention for internal interfaces). Makes MetadataReader required on RuntimeSignatureDecoder (matches SRM exactly; both call sites already pass non-null) and removes the null-forgiving operators. Reorders constructor params to (provider, target, metadataReader, genericContext) to mirror SRM's parameter order. Cleanup: removes unused _mdhProviders dictionary and GetMethodDescHandleProvider from SignatureDecoder_1; removes unused _target/_moduleHandle fields and constructor from GcSignatureTypeProvider; renames _metadataReaderOpt to _metadataReader (Opt suffix is non-standard for a nullable-typed field); converts block comment to line comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drop the redundant `Target target` parameter from `IRuntimeSignatureTypeProvider.GetInternalType` and `GetInternalModifiedType`. Providers now capture the target in their own constructor when they need it, matching `SignatureTypeProvider<T>`. `GcSignatureTypeProvider` gains a `Target` constructor parameter. Update the cDAC documentation: * `SignatureDecoder.md` -- describe `RuntimeSignatureDecoder`, `IRuntimeSignatureTypeProvider`, and the runtime-only `ELEMENT_TYPE_INTERNAL` / `ELEMENT_TYPE_CMOD_INTERNAL` extensions; refresh the `DecodeFieldSignature` code sample. * `StackWalk.md` -- new Signature-Based Scanning section covering `GcSignatureTypeProvider`, the `PromoteCallerStack` algorithm, reserved-slot table, and limitations vs. native. Also revert a stray whitespace change in `ISignatureDecoder.cs`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/SignatureTypeProvider.cs:25
- The private field
_targetis assigned in the constructor but never read in this type, which will trigger CS0414 (and in this repo, warnings are treated as errors by default). Please remove_target(and its assignment) or start using it (e.g., for validation) to avoid build breaks.
private readonly Target _target;
private readonly Contracts.ModuleHandle _moduleHandle;
private readonly Contracts.ILoader _loader;
private readonly Contracts.IRuntimeTypeSystem _runtimeTypeSystem;
public SignatureTypeProvider(Target target, Contracts.ModuleHandle moduleHandle)
{
_target = target;
_moduleHandle = moduleHandle;
_loader = target.Contracts.Loader;
_runtimeTypeSystem = target.Contracts.RuntimeTypeSystem;
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/GC/GcScanner.cs:380
isValueTypeThisis always initialized tofalseand never updated, sothisis never reported withGC_CALL_INTERIOReven for value-type instance methods. Since you already resolve the declaringTypeHandleearlier, it seems like this should be computed (e.g., viaIRuntimeTypeSystem.IsValueType(typeHandle)) and included in the existing try/catch alongsideRequiresInstArg/IsAsyncMethod.
bool hasThis = methodSig.Header.IsInstance;
bool hasRetBuf = methodSig.ReturnType is GcTypeKind.Other;
bool requiresInstArg = false;
bool isAsync = false;
bool isValueTypeThis = false;
try
{
requiresInstArg = rts.RequiresInstArg(mdh);
isAsync = rts.IsAsyncMethod(mdh);
}
catch
{
}
PromoteCallerStackHelper(transitionBlock, methodSig, hasThis, hasRetBuf,
requiresInstArg, isAsync, isValueTypeThis, scanContext);
}
…rovider Match native MethodDesc::GetSig in GcScanner.PromoteCallerStack: try IsStoredSigMethodDesc first (dynamic, EEImpl, and array method descs) and read the stored signature via an inline pinned BlobReader before falling back to the metadata token lookup. Cache the GcSignatureTypeProvider on the GcScanner so it is allocated once instead of per call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…mitive GcSignatureTypeProvider now classifies type parameters and TypeDef/TypeRef tokens using the loaded TypeHandle, matching native SigTypeContext-driven PeekElemTypeNormalized behavior: - VAR/MVAR placeholders are resolved against the method's actual class / method instantiation via a new GcSignatureContext(TypeHandle, MethodDescHandle) passed as the SRM generic context. - TypeDef/TypeRef tokens are resolved via the module's TypeDefToMethodTable / TypeRefToMethodTable lookup tables; enums collapse to their underlying primitive (None) via IRuntimeTypeSystem.IsEnum, matching MethodTable::GetInternalCorElementType. The provider is now module-scoped, so it is constructed per PromoteCallerStack call rather than cached on GcScanner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- IRuntimeSignatureTypeProvider: internal -> public to match SignatureTypeProvider's visibility (avoids the "public class implements internal interface" warning while keeping the provider type available to downstream consumers). - RuntimeSignatureDecoder: drop the unused _target field and the redundant ArgumentNullException.ThrowIfNull(provider/metadataReader) calls; the file is in a nullable-enabled context, so the type system already enforces non-null. - SignatureDecoder.md: drop the bogus `target,` argument from the GetInternalType / GetInternalModifiedType bullets, and reword GetTypeFromDefinition / GetTypeFromReference's "returns null" to "returns a default TypeHandle (Address == TargetPointer.Null)" since the API returns a struct value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| ## Version 1 | ||
|
|
||
| In version 1 of the SignatureDecoder contract we take advantage of the System.Reflection.Metadata signature decoding. We implement a SignatureTypeProvider that inherits from System.Reflection.Metadata ISignatureTypeProvider. | ||
| In version 1 of the SignatureDecoder contract we use a cDAC-internal `RuntimeSignatureDecoder` that closely mirrors the API and behavior of `System.Reflection.Metadata.SignatureDecoder`. The cDAC decoder exists because runtime-internal signatures may contain `ELEMENT_TYPE_INTERNAL` (`0x21`) and `ELEMENT_TYPE_CMOD_INTERNAL` (`0x22`) elements -- target-pointer references to runtime type handles that are not part of ECMA-335 and that SRM does not understand. |
There was a problem hiding this comment.
This talks a lot about the APIs. The data contract description should be centered on the data contract (what the bytes in memory mean). In this case, it should say that it is the ECMA-335 signature format with two additional internal element types.
The APIs are secondary for the data contract docs. We use the APIs and pseudo in the data contract docs just because of it is easier to use for explanation than English. The APIs do not have to be documented in detail.
There was a problem hiding this comment.
I reworked the entire markdown to follow these guidelines
|
|
||
| ### RuntimeSignatureDecoder | ||
|
|
||
| `RuntimeSignatureDecoder<TType, TGenericContext>` is a cDAC polyfill that mirrors `System.Reflection.Metadata.SignatureDecoder<TType, TGenericContext>`: |
There was a problem hiding this comment.
I am not sure "polyfill" is the right term to use. Polyfill implements a new feature for an old version of the underlying system, so that you can use the same API regardless of whether your app runs on old or new version.
I would call it "clone with added support for the internal element types".
|
|
||
| The decoder additionally recognizes: | ||
|
|
||
| * `ELEMENT_TYPE_INTERNAL` (`0x21`): followed by a target-sized pointer to a runtime `TypeHandle`. Provider returns a type via `GetInternalType(typeHandlePointer)`. |
There was a problem hiding this comment.
The comment on these says "[cDAC] [RuntimeTypeSystem]: Contract depends on the values of ELEMENT_TYPE_INTERNAL and ELEMENT_TYPE_CMOD_INTERNAL.". Should it be updated to mention this contract?
There was a problem hiding this comment.
Added [Signature] in addition to the existing comment.
| @@ -10,7 +10,7 @@ TypeHandle DecodeFieldSignature(BlobHandle blobHandle, ModuleHandle moduleHandle | |||
|
|
|||
There was a problem hiding this comment.
Naming nit: Should this be called "Signature" or "RuntimeSignature"?
Other data contract names describe the data, not the thing used to crack the data. For example, the GC info data contract is called GCInfo. It is not called GCInfoDecoder (a component that is used to crack the data).
There was a problem hiding this comment.
Renamed to "Signature"
Pure rename + cascading symbol updates per review feedback on PR dotnet#127636. The contract is named after the data it describes, not the API that decodes it (consistent with GCInfo, EcmaMetadata, etc.). - docs/design/datacontracts/SignatureDecoder.md -> Signature.md - ISignatureDecoder.cs -> ISignature.cs (interface + default struct) - SignatureDecoder_1.cs -> Signature_1.cs (impl class, comment xref) - ContractRegistry.cs: SignatureDecoder property -> Signature - CoreCLRContracts.cs: registration uses ISignature/Signature_1 - SOSDacImpl.cs: signatureDecoder local -> signatureContract - datadescriptor.inc: CDAC_GLOBAL_CONTRACT(Signature, c1) - StackWalk.md: cross-link updated to new name + path No behavioral or content change in this commit. Doc rewrite and style standards follow in a subsequent commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rewrite Signature.md data-first per Jan's j1 review: lead with byte-level meaning of ELEMENT_TYPE_INTERNAL/CMOD_INTERNAL, treat the C# API as illustrative spec material rather than the subject. - Add the Signature contract to corhdr.h's [cDAC] cross-reference tag covering ELEMENT_TYPE_INTERNAL and ELEMENT_TYPE_CMOD_INTERNAL, so a future change to either tag value is reviewed against this contract. - Replace "polyfill" prose with "clone with added support" -- the cDAC's RuntimeSignatureDecoder doesn't fill a gap in older SRM, it adds runtime-internal element types absent from ECMA-335. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ISignature.cs:16
- The PR description still refers to "ISignatureDecoder" as the contract surface, but the code here renames the contract to
ISignature/Signature. Please update the PR description (and any other narrative docs outside this repo, if applicable) to match the new contract name to avoid confusion for reviewers and downstream consumers.
| public RuntimeSignatureDecoder( | ||
| IRuntimeSignatureTypeProvider<TType, TGenericContext> provider, | ||
| Target target, | ||
| MetadataReader metadataReader, | ||
| TGenericContext genericContext) | ||
| { | ||
| _provider = provider; | ||
| _metadataReader = metadataReader; | ||
| _genericContext = genericContext; | ||
| _pointerSize = target.PointerSize; | ||
| } |
| Tag `3` in the `TypeDefOrRefOrSpec` encoding (ECMA-335 §II.23.2.8) is reserved and decoders throw `BadImageFormatException` when they encounter it. | ||
|
|
There was a problem hiding this comment.
| Tag `3` in the `TypeDefOrRefOrSpec` encoding (ECMA-335 §II.23.2.8) is reserved and decoders throw `BadImageFormatException` when they encounter it. |
This is duplicating ECMA-335 §II.23.2
| @@ -0,0 +1,73 @@ | |||
| # Contract Signature | |||
|
|
|||
| This contract describes the format of method, field, and local-variable signatures stored in target memory. Signatures use the ECMA-335 §II.23.2 format with two CoreCLR-internal element types added by the runtime. | |||
There was a problem hiding this comment.
| This contract describes the format of method, field, and local-variable signatures stored in target memory. Signatures use the ECMA-335 §II.23.2 format with two CoreCLR-internal element types added by the runtime. | |
| This contract describes the format of method, field, and local-variable signatures stored in target memory. Signatures use the ECMA-335 §II.23.2 format with CoreCLR-internal element types added by the runtime. |
Chances are we are going to add a third one. Avoid hardcoding "two" in the text to make future proof.
| * It does not enumerate embedded GC refs inside large value types passed by value (a `GcTypeKind.Other` parameter is silently skipped). | ||
| * It does not yet apply native's `ArgIterator`-driven multi-slot / HFA layout, nor does it model `String` constructors or `SuppressParamTypeArg`. | ||
|
|
||
| These limitations are visible to the cDAC GC stress verification harness, which compares cDAC and native walks; they may be tightened in future versions of this contract. |
There was a problem hiding this comment.
Should we track these TODOs as issues?
Summary
Part 2 of 5 stacked PRs splitting #126408. Builds on #127395 (merged).
This PR adds a cDAC ECMA-335 signature decoder that closely mirrors
System.Reflection.Metadata.SignatureDecoderand supports the two runtime-only extensions used in CoreCLR-internal signatures:ELEMENT_TYPE_INTERNAL(0x21) andELEMENT_TYPE_CMOD_INTERNAL(0x22). The decoder is then used by both the Signature contract (for field signatures) and the StackWalk contract (for signature-based GC reference scanning of transition frames).What this PR contains
RuntimeSignatureDecoder(SRM-aligned polyfill):RuntimeSignatureDecoder<TType, TGenericContext>-- readonly struct mirroring SRM'sSignatureDecoder<TType, TGenericContext>(DecodeType,DecodeFieldSignature,DecodeMethodSignature,DecodeLocalSignature, all takingref BlobReader).IRuntimeSignatureTypeProvider<TType, TGenericContext>-- superset of SRM'sISignatureTypeProvideraddingGetInternalType(TargetPointer)andGetInternalModifiedType(TargetPointer, TType, bool)for the two runtime-only encodings.SignatureTypeProvider(the existing field-signature provider) implementsIRuntimeSignatureTypeProviderso internal types in field signatures resolve viaRuntimeTypeSystem.GetTypeHandle.Signature-based GC reference scanning in StackWalk:
GcSignatureTypeProvider(internal, inStackWalkHelpers) classifies each method-signature parameter asRef,Interior,Other(value type or larger-than-slot), orNone.GcSignatureContext(TypeHandle classContext, MethodDescHandle methodContext)record struct is plumbed throughRuntimeSignatureDecoder<GcTypeKind, GcSignatureContext>soELEMENT_TYPE_VAR/ELEMENT_TYPE_MVARplaceholders resolve to the method's actual class / method instantiation -- matching nativeSigTypeContext-drivenPeekElemTypeNormalizedbehavior.GcScanner.PromoteCallerStackconstructs the provider per call and walks theTransitionBlockusing a reserved-slot count derived fromIsInstance/ return-buffer /RequiresInstArg/IsAsyncMethod/ ARM64x8, reporting each parameter slot as a GC reference, interior pointer, or skip.MethodDesc::GetSig: prefersIsStoredSigMethodDesc(dynamic, EEImpl, array methods) before falling back to the metadata token, so caller-stack roots of dynamic and array-method transition frames are handled. Stored sigs are pinned with an inlinefixedblock and read via aBlobReader, matching the existingSigFormat.cspattern.TransitionFrame::PromoteCallerStackand is used forPrestubMethodFrame,CallCountingHelperFrame, and theStubDispatchFrame/ExternalMethodFramefallback when no GCRefMap is available.Signature contract surface stays minimal:
ISignatureDecodercontinues to expose onlyDecodeFieldSignature(BlobHandle, ModuleHandle, TypeHandle).cDAC documentation:
docs/design/datacontracts/SignatureDecoder.md-- describesRuntimeSignatureDecoder,IRuntimeSignatureTypeProvider, and theELEMENT_TYPE_INTERNAL/CMOD_INTERNALextensions; refreshedDecodeFieldSignaturecode sample.docs/design/datacontracts/StackWalk.md-- new "Signature-Based Scanning" section under GC stack reference scanning, coveringGcSignatureTypeProvider(with module scoping,GcSignatureContext, and enum normalization), thePromoteCallerStackalgorithm, the reserved-slot table, and limitations vs. native.Testing
Note
This PR description was created with AI assistance from Copilot.