Wrong struct layout (alignment related), followup #4719

[JohanEngelen]

This is a followup from #4719.

The layout of struct Table (or Item) is wrong since LDC 1.31.

align(1) struct Item {
        KV v;
        uint i;
}
/+
// Also fails:
struct Item {
    align(1):
        KV v;
        uint i;
}+/

struct KV {
        align(1) S* s;
        uint k;
}

struct S {
        Table table;
}

struct Table {
        char a;
        Item v;
}

pragma(msg, Table.sizeof); // "17LU"
pragma(msg, Table.v.offsetof); // "1LU"

void foo(ref Table table, Item a) {
    // should write at offset 1, but LDC >= 1.31 writes at offset 4
    table.v = a;
}

In LLVM IR:

; LDC 1.30
%example.S = type { %example.Table }
%example.Table = type { i8, %example.Item }
%example.Item = type <{ %example.KV, i32 }>
%example.KV = type <{ %example.S*, i32 }>

; LDC >= 1.31
%example.S = type { %example.Table }
%example.Table = type { i8, %example.Item }
%example.Item = type { %example.KV, i32 }  ; < > missing here
%example.KV = type <{ %example.S*, i32 }>

[JohanEngelen]

Godbolt: https://d.godbolt.org/z/o77Kafhr6

[kinke]

I guess this is an instance of #4236, due to Item having that outer align(1), which just affects the tail padding, without making all members implicitly (potentially) misaligned.

Does this change things?

struct KV {
        align(4) S* s; // to prevent padding KV to 16 bytes on 64-bit targets
        uint k;
} // note: still an overall alignment of 4 as before, and size of 8/12 bytes

struct Item {
        KV v;
        uint i;
} // now an alignment of 4, and same size 12/16 bytes

struct Table {
        char a;
        align(1) Item v; // pack tightly, no member/tail padding
}

[kinke]

Oh sorry, just seen your

// Also fails:
struct Item {
    align(1):
        KV v;
        uint i;
}

now, so aligning the members instead apparently isn't enough.

[kinke]

This IR layout seems wrong:

%example.Table = type { i8, %example.Item }

%example.Item has a natural IR alignment of 4 due to the i32 member, but the field offset of Table.item is 1, i.e., unnatural. So Table should be IR-packed.

[kinke]

Weird, we only resort to the frontend type alignment (which would be 1, not matching the IR alignment) if the IR type isn't sized; we shouldn't be hitting this with the test case...

ldc/ir/irtypeaggr.cpp

Lines 215 to 224 in 6bd5d9a

	unsigned fieldAlignment, fieldSize;
	if (!llType->isSized()) {
	// forward reference in a cycle or similar, we need to trust the D type
	fieldAlignment = DtoAlignment(vd->type);
	fieldSize = af.size;
	} else {
	fieldAlignment = getABITypeAlign(llType);
	fieldSize = getTypeAllocSize(llType);
	assert(fieldSize <= af.size);
	}

[kinke]

Does this change things?

It does indeed, fixing the Table IR layout to packed. By 'accidentally' using a D type alignment that matches the IR one. So it looks like we are hitting some forward-ref issue indeed, e.g., some body-less IR type being declared first, then creating some other IR type that uses it as a member, and only then finalizing the members of the first IR type.

[JohanEngelen]

I'm now very wary of the changes related to IR packedness; it has already shown two regressions (fortunately caught by asserts in the code). Struct layout changes in the past almost lead to severe data loss, only prevented with a lot of work. It spawned a whole struct introspection framework at Weka (the change was detectable by compile-time introspection). The issue now is worse because frontend introspection gives different values than IR, so Weka's introspection hashing of struct layout does not catch these bugs.

[kinke]

There should at least be a simple brute-force fix, making the IR struct packed at the end if the D type alignment is less than the IR one, then we won't run into this, the D type alignment always being at least the IR one.

With opaque pointers now, we shouldn't need to forward-declare aggregate types anymore though, so I'd prefer defining them at once (so that we'll never have to resort to the D type info), but that's probably much harder.