Bug regarding inheritance in classes with explicit layout

[trylek]

Not fixing this for .NET 6 as we decided it's too risky to change runtime layouts now, it would also require bumping up the readytorun major version, but it's a bug nonetheless. For an explicit layout class with a base class, we place its fields as if the base class was twice its actual instance size. This is because we first take the base class instance size into account when calculating the layout itself in

runtime/src/coreclr/vm/classlayoutinfo.cpp

Line 138 in ffb095a

UINT32 cbCurOffset = parentSize;

and we add the parent class size a second time in MethodTableBuilder::HandleExplicitLayout in

runtime/src/coreclr/vm/methodtablebuilder.cpp

Line 8579 in ffb095a

S_UINT32 dwInstanceSliceOffset = S_UINT32(HasParent() ? GetParentMethodTable()->GetNumInstanceFieldBytes() : 0);

and

runtime/src/coreclr/vm/methodtablebuilder.cpp

Line 8631 in ffb095a

HRESULT hr = pTempFD->SetOffset(pTempFD->GetOffset_NoLogging() + dwInstanceSliceOffset.Value());

/cc @dotnet/crossgen-contrib, @dotnet/dotnet-coreclr

[trylek]

/cc @jkoritzinsky per JanK's comment in the PR discussion #53424

[trylek]

As an additional bit of fun, this logic is conditional on the following check referring to some bug I don't know where to find:

runtime/src/coreclr/vm/methodtablebuilder.cpp

Line 8563 in b1e20bf

if (IsBlittable() || IsManagedSequential())

It would be useful to understand what the original issue was, whether we can remove the conditional check, and whether it may be the case that the conditional check somehow compensates for the behavior tracked by this issue.

[trylek]

/cc @jkotas - I have found the "Bug 849333" in our internal DevDiv bug database and the Discussion is as follows:

Jan Kotas
commented Dec 21, 2013
 
Changeset 1134780 broke layout of blittable types with explicit layout by accident.

Could you please help shed light on this?

[trylek]

Attaching a simple repro Jan and I created as part of the abovementioned PR discussion.

using System;
using System.Runtime.InteropServices;

[StructLayout(LayoutKind.Explicit)]
class A1
{
   [FieldOffset(0)]
   public byte x;
   [FieldOffset(2)]
   public char x2;
}

[StructLayout(LayoutKind.Explicit)]
class A2 : A1
{
   [FieldOffset(0)]
   public int y;
}

[StructLayout(LayoutKind.Explicit)]
class A3 : A2
{
   [FieldOffset(0)]
   public byte z;
}

class My
{
    static unsafe void Main()
    {
        A3 a = new A3();
        fixed (byte *px = &a.x)
        fixed (byte *pz = &a.z)
        {
            Console.WriteLine(pz - px);
        }
    }
}

With proper packing in place, there should be a MethodTable pointer in front, followed by 4 bytes for each of the classes A1, A2, A3, so that the difference between the memory addresses a.x and a.z is 8 bytes (basically sizeof(A1) + sizeof(A2)). In reality, today .NET 6 Preview 6 reports 24 and according to JanK .NET Framework and .NET Core 3.1 report 19 as the difference. Original JanK's repro using a simpler formulation of the A1 class,

[StructLayout(LayoutKind.Explicit)]
class A1
{
   [FieldOffset(0)]
   public int x;
}

didn't hit this issue because it bailed out early in HandleExplicitLayout due to passing the if (IsBlittable() || IsManagedSequential()) check.

[trylek]

For the difference 24, the calculation of A2 doubles the instance size of A1 i.e. 2 * 4 = 8 and uses that as its base so that its own instance size is 12 and the calculation of A3doubles that to 2 * 12 = 24, that's where a.z is getting put.

[jkotas]

Not fixing this for .NET 6 as we decided it's too risky to change runtime layouts not

I do not see a problem with fixing this one in .NET 6 if we have time.

[jkotas]

I have found the "Bug 849333" in our internal DevDiv bug database and the Discussion is as follows:
Could you please help shed light on this?

There was a change made earlier that regressed the layout algorithm in observable way, bug 849333 was filled on it, and the bug was fixed.

Note that .NET Framework 2.0 prints 19 for the repro at top of this issue as well, so this bug is very old (prior 2005).

[trylek]

@jkotas - Hmm, so is the conditional statement still needed then? It looks like CoreCLR runtime currently has very fragile behavior where the check if (IsBlittable() || IsManagedSequential()) you yourself pointed out to be very complex decides whether HandleExplicitLayout updates the field offsets by doubling the base class size. If we're being serious about cleaning up the related code at some point, this jumps up as a sore thumb.

[jkotas]

Sure, I would love this condition to be simplifed to something less fragile, but we gave up on that for .NET 6 as too complex. Also, simlifying this condition would be R2R breaking change.

I see this issue as tracking just the double counting of the base offset with explicit non-blittable layout, nothing else. Fixing that should be much simpler and it won't be R2R breaking change (explicit layout for non-valuetypes is not part of the R2R contract).

[trylek]

Awesome, thanks Jeremy for fixing this!