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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
