CoreCLR runtime seems to have a subtle bug in HFA flag calculation

[trylek]

While working on the issue

#80218

based on Anton's PR feedback I noticed what looks like a CoreCLR runtime bug w.r.t. HFA calculation: in the routine EEClass::CheckForHFA, the switch case for ELEMENT_TYPE_VALUETYPE seems to be missing the alignment check, so that it looks like

struct F2
{
    public double value;
}

[StructLayout(LayoutKind.Explicit)]
struct S
{
    [FieldOffset(0)] public double f1;
    [FieldOffset(3)] public F2 f2;
    [FieldOffset(8)] public double f3;
}

would be erroneously identified as HFA-eligible. /cc @jkotas and @janvorli to confirm whether I'm not just misreading the code, otherwise I guess it is worth fixing in .NET 8.

Note: If JIT logic is fixed, please also fix the corresponding managed ComputeHomogeneousAggregateCharacteristic function in MetadataFieldLayoutAlgorithm.cs.

[AntonLapounov]

It seems that discrepancy between JIT and Crossgen2 was introduced by dotnet/coreclr#22041. @jkoritzinsky, is that an oversight that JIT allows non-aligned wrapped floats/doubles for HFA? Also, why does the code require having a field with offset 0 while having a gap at, say, 8 is fine?

[jkoritzinsky]

Yes this is an oversight in the CoreCLR implementation. The CoreCLR implementation should check that the nested value type is aligned at the same alignment as the other fields and as it's HFA type requires.

The main reason behind the check for offset 0 is to ensure that we're consistent on what we define a gap at 0 bytes to be. We treat a gap at 0 bytes as though we have an array of unsigned char _gap[GAP_SIZE] at the start of the object, and it was easy to miss this case. I think the rest of the behavior was meant to match what we do for SysV (where we take the classification for the field prior to the gap in the eightbyte and use that for the gap, or treat the gap as mentioned above for a 0-offset gap). We should probably extend the test coverage and the code to cover gaps at other offsets as well.

[mangod9]

@trylek @jkoritzinsky is this something we plan on fixing in 8? Doesnt look like a regression.

[jkoritzinsky]

I think at this point, we should fix this in .NET 9.

[kg]

ComputeHomogeneousAggregateCharacteristic looks correct to me in main, I guess someone fixed it? Or am I misunderstanding the problem?

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