From 92078b2b254216780f5a7977fb80f37b47b038c2 Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Thu, 5 Jan 2023 02:31:35 +0100 Subject: [PATCH 1/2] Fix HFA detection in Crossgen2 According to customer feedback some WPF apps are crashing on arm64 at runtime in debug mode when compiled with Crossgen2. Based on the initial investigation by Anton Lapounov and with help from Jan Vorlicek I have managed to identify that the problem is caused by a mismatch between the native CoreCLR runtime and Crossgen2 w.r.t. identification of HFA types. This change puts Crossgen2 behavior in sync with the CoreCLR runtime. I have verified locally that this makes the GC ref map for the method System.Windows.Media.PathGeometry.GetPathBoundsAsRB identical with the runtime version and avoids the assertion failure that was previously triggered in debug CoreCLR builds due to this mismatch. Thanks Tomas --- .../Common/MetadataFieldLayoutAlgorithm.cs | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index a2d2eaccdae466..1dd6f58b323caa 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -960,12 +960,6 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte MetadataType metadataType = (MetadataType)type; - // No HAs with explicit layout. There may be cases where explicit layout may be still - // eligible for HA, but it is hard to tell the real intent. Make it simple and just - // unconditionally disable HAs for explicit layout. - if (metadataType.IsExplicitLayout) - return NotHA; - switch (metadataType.Category) { // These are the primitive types that constitute a HFA type @@ -977,12 +971,18 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte case TypeFlags.ValueType: // Find the common HA element type if any ValueTypeShapeCharacteristics haResultType = NotHA; + bool hasZeroOffsetField = false; foreach (FieldDesc field in metadataType.GetFields()) { if (field.IsStatic) continue; + if (field.Offset == LayoutInt.Zero) + { + hasZeroOffsetField = true; + } + // If a field isn't a DefType, then this type cannot be a HA type if (!(field.FieldType is DefType fieldType)) return NotHA; @@ -1006,8 +1006,12 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte } } - // If there are no instance fields, this is not a HA type - if (haResultType == NotHA) + // If the struct doesn't have a zero-offset field, it's not an HFA. + if (!hasZeroOffsetField) + return NotHA; + + // Types which are indeterminate in field size are not considered to be HA + if (type.InstanceFieldSize.IsIndeterminate) return NotHA; int haElementSize = haResultType switch @@ -1019,16 +1023,17 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte _ => throw new ArgumentOutOfRangeException() }; - // Types which are indeterminate in field size are not considered to be HA - if (type.InstanceFieldSize.IsIndeterminate) - return NotHA; - // Note that we check the total size, but do not perform any checks on number of fields: // - Type of fields can be HA valuetype itself. // - Managed C++ HA valuetypes have just one of type float to signal that // the valuetype is HA and explicitly specified size. - int maxSize = haElementSize * type.Context.Target.MaxHomogeneousAggregateElementCount; - if (type.InstanceFieldSize.AsInt > maxSize) + int totalSize = type.InstanceFieldSize.AsInt; + + if (totalSize % haElementSize != 0) + return NotHA; + + // On ARM, HFAs can have a maximum of four fields regardless of whether those are float or double. + if (totalSize > haElementSize * type.Context.Target.MaxHomogeneousAggregateElementCount) return NotHA; // All the tests passed. This is a HA type. From 9b43a8d61c1f335e6193738dd6a1fc67140e2969 Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Thu, 5 Jan 2023 21:49:57 +0100 Subject: [PATCH 2/2] Add modulus checks for fields and runtime reference comment per Anton's PR feedback --- .../Common/MetadataFieldLayoutAlgorithm.cs | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index 1dd6f58b323caa..f3b7b9442f5bbe 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -944,6 +944,14 @@ public override ValueTypeShapeCharacteristics ComputeValueTypeShapeCharacteristi return ComputeHomogeneousAggregateCharacteristic(type); } + /// + /// Identify whether a given type is a homogeneous floating-point aggregate. This code must be + /// kept in sync with the CoreCLR runtime method EEClass::CheckForHFA, as of this change it + /// can be found at + /// https://github.com/dotnet/runtime/blob/1928cd2b65c04ebe6fe528d4ebb581e46f1fed47/src/coreclr/vm/class.cpp#L1567 + /// + /// Type to analyze + /// HFA classification of the type parameter private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacteristic(DefType type) { // Use this constant to make the code below more laconic @@ -959,6 +967,7 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte return NotHA; MetadataType metadataType = (MetadataType)type; + int haElementSize = 0; switch (metadataType.Category) { @@ -996,6 +1005,15 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte { // If we hadn't yet figured out what form of HA this type might be, we've now found one case haResultType = haFieldType; + + haElementSize = haResultType switch + { + ValueTypeShapeCharacteristics.Float32Aggregate => 4, + ValueTypeShapeCharacteristics.Float64Aggregate => 8, + ValueTypeShapeCharacteristics.Vector64Aggregate => 8, + ValueTypeShapeCharacteristics.Vector128Aggregate => 16, + _ => throw new ArgumentOutOfRangeException() + }; } else if (haResultType != haFieldType) { @@ -1004,6 +1022,11 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte // be a HA type. return NotHA; } + + if (field.Offset.IsIndeterminate || field.Offset.AsInt % haElementSize != 0) + { + return NotHA; + } } // If the struct doesn't have a zero-offset field, it's not an HFA. @@ -1014,15 +1037,6 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte if (type.InstanceFieldSize.IsIndeterminate) return NotHA; - int haElementSize = haResultType switch - { - ValueTypeShapeCharacteristics.Float32Aggregate => 4, - ValueTypeShapeCharacteristics.Float64Aggregate => 8, - ValueTypeShapeCharacteristics.Vector64Aggregate => 8, - ValueTypeShapeCharacteristics.Vector128Aggregate => 16, - _ => throw new ArgumentOutOfRangeException() - }; - // Note that we check the total size, but do not perform any checks on number of fields: // - Type of fields can be HA valuetype itself. // - Managed C++ HA valuetypes have just one of type float to signal that