ldc-developers/ldc

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

JohanEngelen opened this issue · 6 comments

This test case triggers the assert:

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.

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

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)

@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?

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.

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

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.