Repository navigation
Align UnionExp storage to real_t - #23966
Conversation
UnionExp emplaces RealExp and ComplexExp into a union declared align(8), but on x86_64 posix targets real_t is the x87 80-bit type with 16-byte alignment, so those instances (and the complex_t field of ComplexExp) were placed at addresses only 8-aligned. A compiler that copies the complex_t field with its natural alignment emits movaps and faults.
|
This surfaced when fixing a bug in LDC causing alignment to be lost and revert to align(1), causing ridiculous codegen on platforms that can't do unaligned load/store. |
dkorpel
left a comment
dkorpel
left a comment
There was a problem hiding this comment.
Why is the ternary needed?
| private: | ||
| // Ensure that the union is suitably aligned. | ||
| align(8) union _AnonStruct_u | ||
| align(real_t.alignof > 8 ? real_t.alignof : 8) union _AnonStruct_u |
There was a problem hiding this comment.
| align(real_t.alignof > 8 ? real_t.alignof : 8) union _AnonStruct_u | |
| align(real_t.alignof) union _AnonStruct_u |
There was a problem hiding this comment.
real_t.alignof is 2 for Windows (at least for dmd).
There was a problem hiding this comment.
Yeah I guess... but it's not certain what real actually even means in D.
There was a problem hiding this comment.
real_t.alignof is 2 for Windows (at least for dmd).
Wow. Weird choices!
There was a problem hiding this comment.
real is for programmer enrichment. We needed a new endless source of bugs to keep them entertained after having eliminated variable-width basic integer types, so we added a poorly-specified, totally non-portable, variable-width floating point type. And also made it nearly impossible to avoid by mandating its use in CTFE and math functions (though sadly this latter point has been eroded, because "less bugs" is viewed as desirable. Ridiculous!). Oh, and it's always 80-bit in CTFE, because portability. Except when it's not, because we haven't implemented that yet.
So to answer your question, real means fun!
rainers
left a comment
rainers
left a comment
There was a problem hiding this comment.
I stumbled upon this, too, when adding alignment info to some allocations, but thought the impact would be low.
|
That last test seems to be pending for days, but it's marked "required"... do we just ignore it? |
UnionExp emplaces RealExp and ComplexExp into a union declared align(8), but on x86_64 posix targets real_t is the x87 80-bit type with 16-byte alignment, so those instances (and the complex_t field of ComplexExp) were placed at addresses only 8-aligned. Upstream: dlang/dmd#23966
5 participants
UnionExpemplaces expression class instances into_AnonStruct_u, which is a union ofchar[classInstanceSize]buffers with a hand-pickedalign(8). On x86_64 posix targetsreal_tis the x87 80-bit type with 16-byte alignment, soRealExpandComplexExpinstances (andComplexExp.value, acomplex_t) end up at addresses that are only 8-aligned.This is latent today because the copies happen to be emitted with alignment 1. It surfaced with ldc-developers/ldc#5279, which makes LDC carry the type alignment on struct copies: the self-hosted compiler then copies
complex_twithmovapsinto an 8-alignedUnionExpand segfaults inconstfold.Castduring CTFE.The union now aligns to
max(8, real_t.alignof).real_tis the only member type of these classes wider than a pointer.__traits(classInstanceAlignment)would state this more directly, but it is unavailable on the oldest supported host compilers. No change wherereal_tis at most 8-aligned (e.g. Windows).