This is an archive of the discontinued LLVM Phabricator instance.

[Headers] Make max_align_t match GCC's implementation.
Needs ReviewPublic

Authored by EricWF on Nov 29 2018, 8:40 AM.

Details

Reviewers
rsmith
jyknight
Summary

GCC also considers the __float128 type when constructing max_align_t. Clang's definition is intended to exactly match GCC's, so this patch make Clang do the same.

NOTE: This breaks the ABI of max_align_t by changing its size and alignment on 32 bit linux platforms.

Diff Detail

Event Timeline

EricWF created this revision.Nov 29 2018, 8:40 AM
jyknight added inline comments.Nov 29 2018, 1:05 PM
lib/Headers/__stddef_max_align_t.h
40

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.

uweigand added inline comments.Nov 29 2018, 1:52 PM
lib/Headers/__stddef_max_align_t.h
40

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.

rsmith added inline comments.Nov 29 2018, 2:08 PM
lib/Headers/__stddef_max_align_t.h
44

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.

jyknight added inline comments.Nov 29 2018, 3:07 PM
lib/Headers/__stddef_max_align_t.h
40

Then perhaps it's a bug that __alignof__(__float128) returns 16 with -target systemz-linux?

EricWF marked 2 inline comments as done.Nov 30 2018, 8:45 PM
@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?

lib/Headers/__stddef_max_align_t.h
40

@jyknight Ack. I'll add __SIZEOF_FLOAT128__.

44

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.

uweigand added inline comments.Dec 3 2018, 8:51 AM
lib/Headers/__stddef_max_align_t.h
40

Huh, really float128 should not be supported at all on SystemZ. It's not supported with GCC either (GCC never provides float128 on targets where long double is already IEEE-128). (GCC does support the new _Float128 on SystemZ, but that's not yet supported by clang anywhere.)

If it were supported, I agree it should be an alias for long double, and also have an alignof of 8.

jyknight added inline comments.Dec 3 2018, 2:36 PM
lib/Headers/__stddef_max_align_t.h
40

@uweigand : One of those fixes needs to land before this, so that systemz's max_align_t doesn't change to 16 in the meantime. I think your preference would be for it to be simply removed, right? Looks like the type was originally added in https://reviews.llvm.org/D19125 -- possibly in error, since the focus was x86_64.

uweigand added inline comments.Dec 4 2018, 2:48 AM
lib/Headers/__stddef_max_align_t.h
40

@jyknight : Yes, this seems to have been simply an error. I'll check in a patch to remove __float128 on SystemZ.

uweigand marked an inline comment as done.Dec 4 2018, 2:55 AM
uweigand added inline comments.
lib/Headers/__stddef_max_align_t.h
40

Checked in as rev. 348247.

As an aside, it would be nice if we had a test case that verifies the explicit values of alignof(max_align_t) on all supported platforms. This is an ABI property that should never change.

jyknight added inline comments.Jan 15 2020, 7:27 AM
lib/Headers/__stddef_max_align_t.h
44

Looks like this patch never landed. I think it still should be, right?

Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2020, 7:27 AM