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

[trylek]

While working on the issue

https://github.com/dotnet/runtime/pull/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;
                        }

[kg]

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

From looking into fixing this, I'm left wondering whether S should have a natural alignment of 8 or not. In clang if you pack(1) in order to be able to manually put a double at an offset of 3, it will cause the outer union's natural alignment to be 1. To avoid this you would need to define a separate pack1'd struct with 3 bytes of padding, and put it inside of a non-pack'd union which has natural alignment. That kind of struct/union nested layout feels extremely contrived to me, so it makes me wonder what the real-world scenario for this looks like.

Do we have an example of real C being p/invoke'd via explicit layout in this way? Obviously it's good for every part of CoreCLR to agree on whether S is an HFA and what its alignment is, but I wanted to make sure I understood all the rules here.