[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 2 inline comments as done.
EricWF added a comment.

@jyknight wrote:

> And then use that to determine whether to add float128 to the union?


Note that `max_align_t` isn't a union of these types, but a struct containing all of them. This seems like a bug to me, but it's what GNU happens to do. God knows why?



================
Comment at: lib/Headers/__stddef_max_align_t.h:40
       __attribute__((__aligned__(__alignof__(long double))));
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3
----------------
jyknight wrote:
> uweigand wrote:
> > jyknight wrote:
> > > Can you fix clang to consistently define `__SIZEOF_FLOAT128__` in InitPreprocessor alongside the rest of the SIZEOF macros?
> > > 
> > > And then use that to determine whether to add float128 to the union? This change, as is, will break on any target which is i386 but doesn't define __float128, e.g. i386-unknown-unknown.
> > > 
> > > The only additional target which will be modified with that (that is: the only other target which has a float128 type, but for which max_align isn't already 16) is systemz-*-linux.
> > > 
> > > But I think that's actually indicating a pre-existing bug in the SystemZ config -- it's configured for a 16-byte long double, with 8-byte alignment, but the ABI doc seems to call for 16-byte alignment. +Ulrich for comment on that.
> > That's a bug in the ABI doc which we'll fix once we get around to releasing an updated version.
> > 
> > long double on SystemZ must be 8-byte aligned, which is the maximum alignment of all standard types on Z, and this was chosen because historically the ABI only guarantees an 8-byte stack alignment, so 16-byte aligned standard types would be awkward.
> Then perhaps it's a bug that `__alignof__(__float128)` returns 16 with `-target systemz-linux`?
@jyknight Ack. I'll add `__SIZEOF_FLOAT128__`.


================
Comment at: lib/Headers/__stddef_max_align_t.h:44
+#endif
 } max_align_t;
 #endif
----------------
rsmith wrote:
> I don't want to hold up the immediate fix in this patch for this, but... we should move the definition of this type from the header into clang itself, like we do for (say) `__builtin_va_list`, and here just define
> 
> `typedef __builtin_max_align_t max_align_t;`
> 
> That way Clang can synthesize a struct of whatever size and alignment appropriate from an ABI perspective (or can use the relevant builtin type for platforms that typedef `max_align_t` to a builtin type). That'd also remove the need for an awkward factored-out header file here.
I'll try to implement this over the weekend. As long as I can land the Clang fix required for libc++ cleanup before the next release.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55057/new/

https://reviews.llvm.org/D55057