Integration of changes in shared files from runtimelab/NativeAOT #61655
Conversation
src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
Outdated
Show resolved
Hide resolved
| else if (type.IsSequentialLayout || type.IsEnum || type.Context.Target.Abi == TargetAbi.CppCodegen) | ||
| // Sequential layout has to be respected for blittable types only. We use approximation and respect it for | ||
| // all types without GC references (ie C# unmanaged types). Universal canonical instances might be blittable. | ||
| else if (type.IsSequentialLayout && (type.IsCanonicalSubtype(CanonicalFormKind.Universal) || !type.ContainsGCPointers)) |
There was a problem hiding this comment.
The field offsets in universal type have to match the field offsets of all non-universal types; or the field offset in universal type has to be undermined. This condition violates that invariant.
I am wondering whether it would be better to disable the tests for universal layout instead of adding this change that makes the tests work, but does not actually make the code work correctly.
There was a problem hiding this comment.
We can key off SupportsUniversalCanon to do the old thing. I think that's what we'll have to do if we ever activate universal canon here.
| @@ -92,6 +92,7 @@ protected internal virtual TypeDesc ConvertToCanon(TypeDesc typeToConvert, Canon | |||
| throw new NotSupportedException(); | |||
| } | |||
|
|
|||
| internal partial void InternalGetSupportsUniversalCanon(ref bool flag) => flag = SupportsUniversalCanon; | |||
There was a problem hiding this comment.
Because Canon support is in partial files, we don't force canon support on every user of the type system, and ILVerify doesn't include it.
There was a problem hiding this comment.
Would it better to fix this by moving the ComputeInstanceFieldLayout into the concrete type systems and leave it abstract in the base one?
There was a problem hiding this comment.
I thought about it, but that would basically mean that the change we're doing now (changing qualification for Sequential layout) would not be automatically reflected in unit tests because tests don't use the compiler type system context. So the logic would not be exercised at all unless we don't forget to keep it in sync.
There was a problem hiding this comment.
I think that it is ok. There is no one right way to do field layout, so it makes sense to allow it to be configurable and unit tests testing a subset of the possible configurations.
There was a problem hiding this comment.
Ok, in that case I'll redo this on the NativeAOT side. Test changes won't be needed because we can just keep using the old logic.
ghost
locked as resolved and limited conversation to collaborators
No description provided.