Uh oh!
There was an error while loading. Please reload this page.
[cDAC] Implement GetFieldAlignment via EEClass layout alignment - #132646
[cDAC] Implement GetFieldAlignment via EEClass layout alignment#132646lewing wants to merge 6 commits into
Conversation
CdacTypeHandle.GetFieldAlignment threw NotImplementedException, so the shared ArgIterator could not reconstruct the stack layout of any value-type argument on WASM -- the Wasm32 case computes Math.Clamp(GetFieldAlignment(), 8, 16). Read the real alignment instead, mirroring CEEInfo::getClassAlignmentRequirementStatic (src/coreclr/vm/jitinterface.cpp), which is the same value the runtime's own wasm ArgIterator consumes via getClassAlignmentRequirementStatic in callingconvention.h: result = target pointer size if EEClass.HasLayout (VMFLAG_HASLAYOUT) and the layout is Sequential or blittable: result = EEClassLayoutInfo.AlignmentRequirement if result < 8 and RequiresAlign8: result = 8 // FEATURE_64BIT_ALIGNMENT (ARM, WASM) The result is unclamped; ArgIterator applies its own clamp, matching the runtime. Reading the alignment requires new data descriptors: EEClass.VMFlags (to test HasLayout), LayoutEEClass.LayoutInfo (LayoutEEClass derives from EEClass, so only the field offset is needed), and the EEClassLayoutInfo fields. Consumers must check HasLayout first -- reading LayoutInfo otherwise interprets unrelated memory. Notes: - The native implementation also handles the marshalled (native value type) view via TypeHandle::IsNativeValueType. That is a marshalling-only concept and is not reachable from the managed argument layout this contract serves, so it is not mirrored; this is called out in the code and the contract doc. - Adding EEClass.VMFlags required updating MockEEClass, since cDAC Data types read their fields eagerly. Fixes the cDAC side of the value-type argument layout gap; it now covers all value types rather than a single family. Full cDAC suite: 3041 passed / 0 failed / 16 skipped. clr.native builds clean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
…rgence - Add GetClassAlignmentRequirement_RequiresAlign8_BumpsToEight. The FEATURE_64BIT_ALIGNMENT branch was the one path with no coverage: a sequential layout reporting 4-byte alignment plus the RequiresAlign8 MethodTable flag must report 8. Mutation-verified (disabling the bump yields 4). - Document a second, previously unstated difference from the native helper: it starts from TypeHandle::GetMethodTable, which also resolves a MethodTable for a TypeDesc, whereas this reports the default for a non-MethodTable handle. Like the IsNativeValueType omission this is unreachable from the managed argument layout, since ArgIterator only consults alignment for ELEMENT_TYPE_VALUETYPE arguments. Full cDAC suite: 3045 passed / 0 failed / 16 skipped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
Update the revived alignment implementation for the current SignatureTypeInfo-based type handle, preserve compatibility with descriptors that predate EEClass.VMFlags, regenerate usage documentation from its source sidecars, and cover both bridge and fallback paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a7202220-ceeb-4fbf-a59f-8c096075c9d1
Keep comments focused on descriptor compatibility and unsafe layout invariants while relying on names and tests for straightforward behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a7202220-ceeb-4fbf-a59f-8c096075c9d1
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR extends the cDAC RuntimeTypeSystem contract and corresponding CoreCLR data descriptors to expose and consume runtime-computed value-type alignment (via EEClassLayoutInfo on LayoutEEClass), enabling CdacTypeHandle.GetFieldAlignment() to return a real alignment value for the Wasm32 ArgIterator path instead of throwing.
Changes:
- Add new cDAC contract surface
IRuntimeTypeSystem.GetClassAlignmentRequirement(ITypeHandle)and implement it inRuntimeTypeSystem_1, mirroringCEEInfo::getClassAlignmentRequirementStatic. - Extend the CoreCLR cDAC data descriptor to include
EEClass.VMFlags,LayoutEEClass.LayoutInfo, andEEClassLayoutInfofields/offsets; add corresponding managed data types and documentation. - Add unit tests covering layout/no-layout, sequential/blittable vs auto-layout, RequiresAlign8 bumping, missing-descriptor fallback, and
CdacTypeHandleintegration.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/SignatureTypeInfoProviderTests.cs | Adds a unit test ensuring CdacTypeHandle.GetFieldAlignment() uses runtime alignment when resolved, otherwise pointer-size fallback. |
| src/native/managed/cdac/tests/UnitTests/MockDescriptors/MockDescriptors.RuntimeTypeSystem.cs | Extends mocks with EEClassLayoutInfo and LayoutEEClass layouts plus VMFlags/HasLayout plumbing. |
| src/native/managed/cdac/tests/UnitTests/MethodTableTests.cs | Adds focused tests for GetClassAlignmentRequirement behavior across layout kinds, descriptor absence, and RequiresAlign8. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/DataType.cs | Adds new data type identifiers for LayoutEEClass and EEClassLayoutInfo. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/EEClassLayoutInfo.cs | Introduces managed representation of EEClassLayoutInfo fields needed by the contract. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/EEClass.cs | Adds optional VMFlags field read and HasLayout helper for safe layout-info probing. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs | Implements GetClassAlignmentRequirement by reading layout info when present and applying RequiresAlign8 bump. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CallingConvention/CdacTypeHandle.cs | Implements GetFieldAlignment() using the new runtime type system alignment API (or pointer-size fallback when unresolved). |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs | Adds the new contract method GetClassAlignmentRequirement. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Extends the runtime data descriptor with EEClass.VMFlags, LayoutEEClass.LayoutInfo, and EEClassLayoutInfo fields. |
| src/coreclr/vm/class.h | Adds cdac_data<> specializations and exposes required offsets for EEClassLayoutInfo and LayoutEEClass. |
| docs/design/datacontracts/RuntimeTypeSystem.md | Documents the new contract method and the added descriptor fields and algorithm. |
| docs/design/datacontracts/data-descriptor-overrides.json | Supplements descriptor typing for LayoutEEClass.LayoutInfo. |
| docs/design/datacontracts/data-descriptor-meanings.json | Adds meanings for the new fields/types. |
Uh oh!
There was an error while loading. Please reload this page.
Append the new layout data types so existing public enum ordinals remain unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a7202220-ceeb-4fbf-a59f-8c096075c9d1
noahfalk
commented
Aug 25, 2026
This feels reasonable to me but I'd like to hear @davidwrighton's feedback too. |
noahfalk
left a comment
There was a problem hiding this comment.
This seems nice to me. I think it highlights a few areas where @max-charlamb and I might want to refine guidance/conventions for dealing with optional fields. Don't consider any of those comments to Max as blocking for this PR, we can easily edit that stuff after checkin if needed.
Uh oh!
There was an error while loading. Please reload this page.
| | `EEClass` | `NumStaticFields` | `uint16` | Count of static fields of the EEClass | | ||
| | `EEClass` | `NumThreadStaticFields` | `uint16` | Count of threadstatic fields of the EEClass | | ||
| | `EEClass` | `OptionalFields` | `pointer` | Pointer to the `EEClassOptionalFields` for this type, or null if it has none | | ||
| | `EEClass` | `VMFlags` | `uint32` | Flags for the EEClass. Bit `0x40` (`VMFLAG_HASLAYOUT`) indicates the EEClass is a `LayoutEEClass` and its `LayoutInfo` may be read | |
There was a problem hiding this comment.
@max-charlamb - I don't think our generated docs currently disclose whether fields or globals are optional/required by the contract. It would be nice if they did for clarity.
| // Mirrors CEEInfo::getClassAlignmentRequirementStatic in src/coreclr/vm/jitinterface.cpp. | ||
| // result = target pointer size | ||
| // if the type is a MethodTable and has an EEClass: | ||
| // if EEClass.VMFlags has VMFLAG_HASLAYOUT: |
There was a problem hiding this comment.
@max-charlamb - I think we might want to tighten up our documented conventions for how we write pseudocode algorithm descriptions when dealing with optional fields. Right now we've got various algorithms elsewhere doing stuff like:
if (type.Fields.ContainsKey("Field"))
if (SomeType type has "Field" field)
if (/* SomeType::Field is present */)
I'd lean towards establishing a convention that:
- fields/globals are marked as optional in the data descriptor part of the doc
- pseudo-code that reads an optional field/global is considered to return default(FieldType) when the field isn't present.
- explicit field existence checks in the pseudo-code are fine, but hopefully aren't often needed because it can add a lot of verbosity
I'm open to alternate conventions too, I think the high order bit is that the convention is clear/consistent enough that an implementer reading the docs can understand where missing info is an error and where it falls back to a sentinel value.
Uh oh!
There was an error while loading. Please reload this page.
Use the established nullable-field pattern for optional VMFlags and describe alignment as optional functionality across runtime implementations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a7202220-ceeb-4fbf-a59f-8c096075c9d1
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
| // LayoutInfo aliases unrelated memory unless HasLayout is set. | ||
| if (eeClass.HasLayout) | ||
| { | ||
| Target.TypeInfo layoutClassType = _target.GetTypeInfo(DataType.LayoutEEClass); | ||
| TargetPointer layoutInfoPtr = eeClassPtr + (ulong)layoutClassType.Fields[LayoutInfoFieldName].Offset; | ||
| Data.EEClassLayoutInfo layoutInfo = _target.ProcessedData.GetOrAdd<Data.EEClassLayoutInfo>(layoutInfoPtr); | ||
| if (layoutInfo.LayoutType == (byte)Data.EEClassLayoutInfo.Type.Sequential || layoutInfo.IsBlittable) | ||
| { | ||
| result = layoutInfo.AlignmentRequirement; | ||
| } | ||
| } |
Fixes#131343.
Summary
The WASM
ArgIteratorcallsCdacTypeHandle.GetFieldAlignment()for value-type arguments, but that method currently throws. This change reads the runtime-computedEEClassLayoutInfoalignment, mirroringCEEInfo::getClassAlignmentRequirementStatic, then letsArgIteratorapply its existing WASM clamp.It adds the required
EEClass/LayoutEEClassdescriptor fields. The functionality is optional: runtimes that do not exposeEEClass.VMFlagsreturn the target pointer size. NewDataTypemembers are appended so existing public enum values remain unchanged.Coverage includes sequential, blittable, auto-layout,
RequiresAlign8, missing-descriptor, andCdacTypeHandleintegration paths.Design feedback requested
Is
GetClassAlignmentRequirement(ITypeHandle)the rightIRuntimeTypeSystemcontract surface, or should this remain a narrower internal implementation detail?The direction follows the #131328 review suggestion to implement
GetFieldAlignmentrather than add a WASM/SIMD-specific predicate. It also uses descriptor presence plus a safe default, following the additive-contract guidance discussed in #124448. The draft is intended to confirm that API shape before requesting final review.Testing
clr.nativeDebug buildNote
This pull request was authored with the assistance of GitHub Copilot.