Incorrect LLVM type for struct with single align(1) member

[JohanEngelen]

This test case triggers the assert:

ldc/ir/irtypeaggr.cpp

Line 43 in 48c1dff

assert((m_offset & (fieldAlignment - 1)) == 0 && "Field is misaligned");

pragma(msg, TraceBuf.alignof); // prints 1
struct TraceBuf {
    align(1) uint args;
}

void foo() {
    byte[2] fixDescs;
    TraceBuf fixLog;

    auto dlg = delegate() {
        fixDescs[0] = 1;
        fixLog.args = 1;
    };
}

This is a minimized testcase, that no longer crashes LDC. The original case ran into the error
Error: variable ... overlaps previous field. This is an ICE, please file an LDC issue.

The reason is that the LLVM type is wrong since 1.31.0.

Used to be:
%example.TraceBuf = type <{ i32 }>
but now is:
%example.TraceBuf = type { i32 }, i.e. alignment is not 1 but 4.

[JohanEngelen]

Regression due to d16fba3 (note the changes in the gh2346.d testcase)

[JohanEngelen]

Seems to only go wrong for nested contexts of delegates, but I don't yet see where it actually goes wrong in codegen (i.e. for release builds without the assert, does it actually result in a crashing program)
The context allocation does not assume the packed alignment:

; LDC 1.30:
 %.frame = call i8* @_d_allocmemory(i64 6)
; LDC 1.38:
%.gc_frame = call ptr @_d_allocmemory(i64 8)

[JohanEngelen]

@kinke Do you remember whether there was anything bad about having %example.TraceBuf = type <{ i32 }> be packed that lead to your change in d16fba3? Seems like a good thing to keep D type alignment the same as LLVM IR type alignment in this case, no?

[kinke]

Do you remember whether there was anything bad about having %example.TraceBuf = type <{ i32 }> be packed that lead to your change in d16fba3? Seems like a good thing to keep D type alignment the same as LLVM IR type alignment in this case, no?

I think I came to the conclusion that the alignment cannot be represented for an IR type anyway, e.g., something exotic like struct S { align(2) int x; }. So I opted for only making containers packed, if their field offsets (or the overall aggregate size) aren't naturally aligned; not for types like that TraceBuf struct which aren't affected themselves by their alignment.

Looks like there's something missing for closures then. Note that making sure the heap-allocated closures themselves are aligned sufficiently (to the max of their member alignments) is something pretty recent; _d_allocmemory is extended by some bytes, and the pointer potentially aligned, all done by the compiler.

[kinke]

In the LDC v1.38 IR, the problem is that the closure type somehow isn't packed, although a struct Container with those 2 fields is:

%onlineapp.TraceBuf = type { i32 }
%nest.foo = type { [2 x i8], %onlineapp.TraceBuf }
%onlineapp.Container = type <{ [2 x i8], %onlineapp.TraceBuf }>

[kinke]

Ah, looks like that assertion in AggrTypeBuilder::addType() should just be converted to setting m_packed in that case - looks like this function is only used for stuff where we assumed (at least) natural alignment so far, incl. closure members.