Uh oh!
There was an error while loading. Please reload this page.
[cdac] SystemV-AMD64 struct-in-register classifier: enable ARGITER + zero-known-issues on Linux x64 - #130257
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Enables SystemV-AMD64 struct-in-register classification for cDAC ArgIterator so GCRefMap generation matches runtime behavior on Unix x64, and flips related stress/GCREFS gates to an inverse “unsupported platforms” list.
Changes:
- Add Unix x64 SystemV-AMD64 eightbyte struct classifier and wire it into
CdacTypeHandle. - Expand CallSignatures stress debuggee with SysV-focused signature shapes.
- Simplify platform gating logic by skipping only remaining unsupported architectures (RiscV64/LoongArch64/Wasm).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/StressTests/Debuggees/CallSignatures/Program.cs | Adds SysV category signatures to exercise Unix x64 struct-in-register classification. |
| src/native/managed/cdac/tests/StressTests/CdacStressTests.cs | Switches ARGITER stress gate to an inverse unsupported-arch list. |
| src/native/managed/cdac/tests/StressTests/CdacStressTestBase.cs | Switches zero-known-issues assertion gate to an inverse unsupported-arch list. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/GC/GcScanner.cs | Aligns deferred-frame gating with new inverse unsupported-arch list. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs | Adds IsIntrinsicType contract impl, refactors intrinsic vector detection, bumps HFA recursion cap. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CallingConvention/SystemVStructClassifier.cs | Introduces SystemV-AMD64 struct-in-register classifier implementation. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CallingConvention/CdacTypeHandle.cs | Plumbs the new classifier into the calling-convention surface. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs | Adds IsIntrinsicType(TypeHandle) to the contract. |
| docs/design/datacontracts/RuntimeTypeSystem.md | Documents the new IsIntrinsicType API and its implementation. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
fa16cf5 to
7460a2cCompare7460a2c to
00dc8d2CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
00dc8d2 to
b9a2860Compareb9a2860 to
aa33f63CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Uh oh!
There was an error while loading. Please reload this page.
Per David Wrighton's suggestion on PR dotnet#130257: instead of porting the ~430 LOC SystemV-AMD64 struct-in-register classifier from crossgen2/methodtable.cpp, read the pre-classified data that the runtime already caches on EEClass::m_eightByteRegistersInfo. The runtime's MethodTableBuilder::StoreEightByteClassification (called from SystemVAmd64CheckForPassStructInRegister at MethodTable build time) populates m_eightByteRegistersInfo inside EEClassOptionalFields with the enregisterable-struct classification: NumEightBytes, per-eightbyte classification tags, per-eightbyte sizes. Callers (arg iterator, JIT-EE interface) read that cached info via SystemVRegDescriptorFromSystemVEightByteRegistersInfo instead of re-running ClassifyEightBytes. The cDAC does the same. Non-enregisterable structs (too large, or classified as SSEUP/X87/Memory) never reach StoreEightByteClassification, so their m_eightByteRegistersInfo reads back with NumEightBytes == 0. That's the natural "passedInRegisters = false" signal the cDAC surfaces. Descriptor plumbing: * class.h: cdac_data<EEClass>::OptionalFields; cdac_data<EEClassOptionalFields> (UNIX_AMD64_ABI-guarded, includes the EightByteRegistersInfo offset). * methodtable.h: cdac_data<SystemVEightByteRegistersInfo> for NumEightBytes plus the two byte[2] arrays; friend struct on the class so offsetof can reach the private fields. * datadescriptor.inc: EEClass.OptionalFields field + new EEClassOptionalFields and SystemVEightByteRegistersInfo types, both under #ifdef UNIX_AMD64_ABI. Managed side: * DataType: new EEClassOptionalFields and SystemVEightByteRegistersInfo entries. * Data.EEClass: OptionalFields TargetPointer field. * Data.EEClassOptionalFields: [Field] SystemVEightByteRegistersInfo inline sub-IData. * Data.SystemVEightByteRegistersInfo: [Field] NumEightBytes; OnInit-populated arrays for the two eightbyte tables. * CdacTypeHandle.GetSystemVAmd64PassStructInRegisterDescriptor: reads EEClass -> OptionalFields -> EightByteRegistersInfo directly (~40 LOC inline; the standalone SystemVStructClassifier.cs file is deleted). * MockDescriptors.RuntimeTypeSystem: mock EEClass layout gains OptionalFields so unit tests build the same shape as the runtime. Advantages vs the classifier port: * Byte-for-byte matches ComputeCallRefMap by construction (the stress harness compares against the runtime path). * No more crossgen2-vs-runtime Int128/UInt128 divergence to reason about -- the classifier just doesn't run. * ~371 LOC net removed. * No maintenance burden as the runtime classifier evolves. Only activates when the target runtime is UNIX_AMD64_ABI; on other runtimes the descriptor entries are ifdef'd out and the classifier returns "not passed in registers" immediately via the TryGetTypeInfo probe. Windows unit tests: all 2646 cDAC tests still pass. Linux x64 stress: TBD (pushed for CI validation). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
252c8b1 to
3423ebcCompare3423ebc to
2b4cd5fCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Rather than reading the two eightbyte tables as byte[] arrays via ReadBuffer (indexed only by NumEightBytes, which is read from target memory without validation), split each table into two explicitly-named [Field] properties: EightByteClassification0/1 and EightByteSize0/1. Two benefits: 1. The Data class is fully declarative -- no OnInit, no arrays. The source generator emits an ordinary two-uint8 read per slot, so there's no code path that could index past slot 1 regardless of what NumEightBytes says. 2. Any consumer keyed off descriptor.eightByteCount is now safe against a corrupted / mis-read NumEightBytes: the previously Debug.Assert-only check in CdacTypeHandle is now a runtime guard that treats NumEightBytes > 2 as "not passed in registers", so downstream loops (e.g. ArgIterator, TransitionBlock) can't overrun a two-slot descriptor. Also adds a static_assert in methodtable.h that CLR_SYSTEMV_MAX_EIGHTBYTES_COUNT_TO_PASS_IN_REGISTERS is still 2, so if the runtime ever grows the descriptor the cDAC-side split gets a build break rather than silently truncating. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
* GcScanner.PromoteCallerStack: reinstate a cheap arch/OS support gate (now including Unix x64) so unsupported targets go straight to RecordDeferredFrame instead of hitting the NotImplementedException fallback inside CallingConvention_1.TryComputeArgGCRefMapBlob. Avoids exception-driven control flow (and first-chance exceptions under debuggers) on RiscV64 / LoongArch64 / Wasm targets. Tracks the same supported-platform list as the runtime stress gate; TODO points at dotnet#130008 for extending coverage. * CdacTypeHandle: reuse SYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTOR.SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES instead of a locally-defined 8 constant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
* DataType: move EEClassOptionalFields / SystemVEightByteRegistersInfo back next to EEClass so type-system entries stay grouped together. * CallingConvention_1.ComputeArgGCRefMapBlobCore: hoist the shared TransitionBlock to the top of the method so the SysV struct-in-regs branch and the pos/offset conversion loop reuse the same instance. * CallingConvention_1.GetArgumentLayout: unwrap the ArgLocDesc? at the call site with a throw-on-null instead of dereferencing Value later; makes the invariant explicit at the point it's required. * GcScanner.PromoteCallerStack: drop the arch/OS supported-target gate again -- TryComputeArgGCRefMapBlob's own NotImplementedException path is the sole decline point. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The Debug.Assert this using existed for was replaced with a throw-on-null in a prior commit; drop the now-unused import. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…zero-known-issues on Linux x64 (#130257) Fills in the SystemV-AMD64 struct-in-register classification path in the cDAC's ArgIterator port so `ICallingConvention.TryComputeArgGCRefMapBlob` produces correct output on Unix x64. Lights up **Linux x64** for both the ARGITER stress sub-check and the zero-known-issues GCREFS assertion. Follow-up to #130090. Advances the platform matrix tracked in #130008. ## Approach The runtime already pre-computes the eightbyte classification for every enregisterable managed value type at `MethodTable` construction time and caches it in `EEClass::m_eightByteRegistersInfo` (inside `EEClassOptionalFields`). Runtime callers -- `ArgIterator`, `getSystemVAmd64PassStructInRegisterDescriptor` -- read that cached info via `SystemVRegDescriptorFromSystemVEightByteRegistersInfo` rather than re-running `ClassifyEightBytes`. The cDAC now does the same. Non-enregisterable structs (too large, or classified as `SSEUP`/`X87`/`Memory`) never reach `StoreEightByteClassification`, so their descriptor reads back with `NumEightBytes == 0`. That's the natural `passedInRegisters = false` signal the cDAC surfaces. Native P/Invoke layouts are not cached here (the runtime classifies them on demand) -- the cDAC never walks native transition frames for GC scanning, so this is fine. Because the stress harness compares against `ComputeCallRefMap` -- which uses the runtime path -- reading the same cached info gives byte-for-byte parity by construction. No merge-rule / intrinsic-rejection / `Int128` divergence to reason about. macOS x64 uses the same SystemV path and works the same way if run locally, but we don't add `osx_x64` to the Helix stress matrix in this PR. ## Descriptor plumbing **C++:** - `class.h`: `cdac_data<EEClass>::OptionalFields`; `cdac_data<EEClassOptionalFields>` (UNIX_AMD64_ABI-guarded) exposing the `EightByteRegistersInfo` offset. Friend declaration on `EEClassOptionalFields`. - `methodtable.h`: `cdac_data<SystemVEightByteRegistersInfo>` for `NumEightBytes` plus the two `byte[2]` eightbyte tables. Friend struct. - `datadescriptor.inc`: `EEClass.OptionalFields` field + new `EEClassOptionalFields` and `SystemVEightByteRegistersInfo` types, both under `#ifdef UNIX_AMD64_ABI`. **Managed:** - `DataType`: new `EEClassOptionalFields` and `SystemVEightByteRegistersInfo` entries. - `Data.EEClass`: adds `OptionalFields`. - `Data.EEClassOptionalFields`: `[Field] SystemVEightByteRegistersInfo` inline sub-IData. - `Data.SystemVEightByteRegistersInfo`: `[Field] NumEightBytes`, `OnInit`-populated arrays. - `CdacTypeHandle.GetSystemVAmd64PassStructInRegisterDescriptor`: ~40 LOC inline. Reads `EEClass -> OptionalFields -> EightByteRegistersInfo`, then projects into `SYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTOR` with `eightByteOffsets` synthesized as `i * 8` (matches `SystemVRegDescriptorFromSystemVEightByteRegistersInfo`). ## Test coverage `CallSignatures` debuggee gains a `SysVCategory` exercising: - Single-eightbyte shapes: `IntPair` (Integer), `FloatPair` (SSE), `IntFloat` (merged Integer), `SingleRef` (IntegerReference) - Two-eightbyte shapes: `LongLong`, `LongDouble`, `DoubleDouble`, `RefInt` - Nested value type, ByRefLike (`Span<byte>`) - Rejection / stack-passed: 24-byte 3-ref struct, `Vector128<int>`, empty struct - `Int128Wrapper` (2 Integer eightbytes -- register-passed; runtime does NOT reject by name) - Unions (`LayoutKind.Explicit`), sub-eightbyte trailer, single-field wrapper Same debuggee runs everywhere; only exercises the new descriptor read on Unix x64 (Windows targets don't emit the descriptor entries, so the classifier returns `passedInRegisters = false` immediately via a `TryGetTypeInfo` probe). `AssertAllPassed` now folds `KnownIssues` into the `Failed` check unconditionally -- no more platform gate on the assertion side; any surviving known-issue count is a regression. ## Local validation - `dotnet build src/native/managed/cdac/cdac.slnx -c Release`: clean - Unit tests: 2646 pass / 0 fail / 16 skipped - **Windows x64 local stress smoke** (`RunStressTests.ps1`): - GCREFS `CallSignatures`: 6150 / 6150 pass, 0 known-issue - ARGITER `CallSignatures` (with the `SysVCategory`): 383 pass / 0 fail / 0 skip / 0 error The descriptor entries are `UNIX_AMD64_ABI`-only, so the descriptor-read path only activates on Linux/macOS x64 targets; no regression on any pre-existing platform. CI validates `linux-x64` end-to-end via the byte-for-byte ARGITER comparison against runtime `ComputeCallRefMap`. > [!NOTE] > This change was authored with assistance from GitHub Copilot. --------- Co-authored-by: Max Charlamb <maxcharlamb@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fills in the SystemV-AMD64 struct-in-register classification path in the cDAC's ArgIterator port so
ICallingConvention.TryComputeArgGCRefMapBlobproduces correct output on Unix x64. Lights up Linux x64 for both the ARGITER stress sub-check and the zero-known-issues GCREFS assertion.Follow-up to #130090. Advances the platform matrix tracked in #130008.
Approach
The runtime already pre-computes the eightbyte classification for every enregisterable managed value type at
MethodTableconstruction time and caches it inEEClass::m_eightByteRegistersInfo(insideEEClassOptionalFields). Runtime callers --ArgIterator,getSystemVAmd64PassStructInRegisterDescriptor-- read that cached info viaSystemVRegDescriptorFromSystemVEightByteRegistersInforather than re-runningClassifyEightBytes. The cDAC now does the same.Non-enregisterable structs (too large, or classified as
SSEUP/X87/Memory) never reachStoreEightByteClassification, so their descriptor reads back withNumEightBytes == 0. That's the naturalpassedInRegisters = falsesignal the cDAC surfaces. Native P/Invoke layouts are not cached here (the runtime classifies them on demand) -- the cDAC never walks native transition frames for GC scanning, so this is fine.Because the stress harness compares against
ComputeCallRefMap-- which uses the runtime path -- reading the same cached info gives byte-for-byte parity by construction. No merge-rule / intrinsic-rejection /Int128divergence to reason about.macOS x64 uses the same SystemV path and works the same way if run locally, but we don't add
osx_x64to the Helix stress matrix in this PR.Descriptor plumbing
C++:
class.h:cdac_data<EEClass>::OptionalFields;cdac_data<EEClassOptionalFields>(UNIX_AMD64_ABI-guarded) exposing theEightByteRegistersInfooffset. Friend declaration onEEClassOptionalFields.methodtable.h:cdac_data<SystemVEightByteRegistersInfo>forNumEightBytesplus the twobyte[2]eightbyte tables. Friend struct.datadescriptor.inc:EEClass.OptionalFieldsfield + newEEClassOptionalFieldsandSystemVEightByteRegistersInfotypes, both under#ifdef UNIX_AMD64_ABI.Managed:
DataType: newEEClassOptionalFieldsandSystemVEightByteRegistersInfoentries.Data.EEClass: addsOptionalFields.Data.EEClassOptionalFields:[Field] SystemVEightByteRegistersInfoinline sub-IData.Data.SystemVEightByteRegistersInfo:[Field] NumEightBytes,OnInit-populated arrays.CdacTypeHandle.GetSystemVAmd64PassStructInRegisterDescriptor: ~40 LOC inline. ReadsEEClass -> OptionalFields -> EightByteRegistersInfo, then projects intoSYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTORwitheightByteOffsetssynthesized asi * 8(matchesSystemVRegDescriptorFromSystemVEightByteRegistersInfo).Test coverage
CallSignaturesdebuggee gains aSysVCategoryexercising:IntPair(Integer),FloatPair(SSE),IntFloat(merged Integer),SingleRef(IntegerReference)LongLong,LongDouble,DoubleDouble,RefIntSpan<byte>)Vector128<int>, empty structInt128Wrapper(2 Integer eightbytes -- register-passed; runtime does NOT reject by name)LayoutKind.Explicit), sub-eightbyte trailer, single-field wrapperSame debuggee runs everywhere; only exercises the new descriptor read on Unix x64 (Windows targets don't emit the descriptor entries, so the classifier returns
passedInRegisters = falseimmediately via aTryGetTypeInfoprobe).AssertAllPassednow foldsKnownIssuesinto theFailedcheck unconditionally -- no more platform gate on the assertion side; any surviving known-issue count is a regression.Local validation
dotnet build src/native/managed/cdac/cdac.slnx -c Release: cleanRunStressTests.ps1):CallSignatures: 6150 / 6150 pass, 0 known-issueCallSignatures(with theSysVCategory): 383 pass / 0 fail / 0 skip / 0 errorThe descriptor entries are
UNIX_AMD64_ABI-only, so the descriptor-read path only activates on Linux/macOS x64 targets; no regression on any pre-existing platform. CI validateslinux-x64end-to-end via the byte-for-byte ARGITER comparison against runtimeComputeCallRefMap.Note
This change was authored with assistance from GitHub Copilot.