Skip to content

[release/6.0] Correctly handle HFA structs when crossgen'd on Arm64 #80872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
carlossanlop merged 1 commit into from
Feb 8, 2023
Merged

[release/6.0] Correctly handle HFA structs when crossgen'd on Arm64 #80872

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Manual .NET 6 backport of #80218 fixing #79327 
Description:

This change puts HFA calculation in Crossgen2 in sync with native
CoreCLR runtime for value types with explicit layout. Previously
Crossgen2 had a shortcut in the routine deciding that structs with
explicit layouts are never marked as HFA that disagreed with the
CoreCLR runtime; consequently, on arm64, Crossgen2 disagreed with
the runtime on whether or not a function returning such a type
should allocate the stack slot for return value, basically messing
up the calling convention and GC refmap, resulting in various
random AVs and corruptions. This was first observed by an internal
customer in WPF apps where MilRectD is the type in question, later
JanK filed the issue 79327 for the same problem.

Customer impact:

Random runtime crashes on arm64.

Regression:

Nope, I believe the incomplete implementation was the original one,
this change just "improves it" by putting it in better sync with
the native runtime. I have also added a code comment mentioning
that these two need to be kept in sync.

Risk:

Low - the error in the previous implementation is obvious, R2RDump
and my new runtime diagnostics clearly show the GC refmap mismatch
caused by this problem and its fixing after applying the Crossgen2
fix.

Link to issue:

#79327

Link to PR against main:

#80218

Publishing impact:

In the particular case of the WPF app the problem was in the
PresentationCore.dll assembly. The assembly (or rather the entire
WPF) need to be recompiled with Crossgen2 with the fix applied
for this to take effect. For now I assume that is an automated part
of the servicing process.

Thanks

Tomas
  • Loading branch information
@trylek
trylek committed Jan 19, 2023
commit 5632210e1a0a71156d42553fe2d76b2b871f1c6c
62 changes: 39 additions & 23 deletions src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,15 @@ public override ValueTypeShapeCharacteristics ComputeValueTypeShapeCharacteristi
return result;
}

private ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacteristic(DefType type)
/// <summary>
/// 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
/// </summary>
/// <param name="type">Type to analyze</param>
/// <returns>HFA classification of the type parameter</returns>
private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacteristic(DefType type)
{
// Use this constant to make the code below more laconic
const ValueTypeShapeCharacteristics NotHA = ValueTypeShapeCharacteristics.None;
Expand All @@ -931,16 +939,8 @@ private ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacteristic(
if ((targetArch != TargetArchitecture.ARM) && (targetArch != TargetArchitecture.ARM64))
return NotHA;

if (type.Context.Target.Abi == TargetAbi.CoreRTArmel)
return NotHA;

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;
int haElementSize = 0;

switch (metadataType.Category)
{
Expand All @@ -953,12 +953,18 @@ private ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacteristic(
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;
Expand All @@ -972,6 +978,15 @@ private ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacteristic(
{
// 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)
{
Expand All @@ -980,21 +995,17 @@ private ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacteristic(
// be a HA type.
return NotHA;
}

if (field.Offset.IsIndeterminate || field.Offset.AsInt % haElementSize != 0)
{
return NotHA;
}
}

// 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;

int haElementSize = haResultType switch
{
ValueTypeShapeCharacteristics.Float32Aggregate => 4,
ValueTypeShapeCharacteristics.Float64Aggregate => 8,
ValueTypeShapeCharacteristics.Vector64Aggregate => 8,
ValueTypeShapeCharacteristics.Vector128Aggregate => 16,
_ => throw new ArgumentOutOfRangeException()
};

// Types which are indeterminate in field size are not considered to be HA
if (type.InstanceFieldSize.IsIndeterminate)
return NotHA;
Expand All @@ -1003,8 +1014,13 @@ private ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacteristic(
// - Type of fields can be HA valuetype itself.
// - Managed C++ HA valuetypes have just one <alignment member> 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.
Expand Down