[Headers] Make max_align_t match GCC's implementation.

[EricWF]

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.

[jyknight]

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]

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]

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]

lib/Headers/__stddef_max_align_t.h

40

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

[EricWF]

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

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]

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]

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]

lib/Headers/__stddef_max_align_t.h

40

Checked in as rev. 348247.

[uweigand]

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]

lib/Headers/__stddef_max_align_t.h

44

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

lib/Headers/__stddef_max_align_t.h

 typedef long double max_align_t;
 #else
 // Define 'max_align_t' to match the GCC definition.
 typedef struct {
   long long __clang_max_align_nonce1
       __attribute__((__aligned__(__alignof__(long long))));
   long double __clang_max_align_nonce2
       __attribute__((__aligned__(__alignof__(long double))));
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3
+    __attribute__((__aligned__(__alignof__(__float128))));
+#endif
 } max_align_t;
 #endif
 
 #endif

test/Headers/c11.c

 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 %s
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -fmodules -fmodules-cache-path=%t %s -D__STDC_WANT_LIB_EXT1__=1
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -ffreestanding %s
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -triple i686-pc-win32 -fms-compatibility-version=17.00 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -triple i386-unknown-linux-gnu %s
 
 noreturn int f(); // expected-error 1+{{}}
 
 #include <stdnoreturn.h>
 #include <stdnoreturn.h>
 #include <stdnoreturn.h>
 
 int g();

 #define __STDC_WANT_LIB_EXT1__ 1
 #include <stddef.h>
 rsize_t x = 0;
 _Static_assert(sizeof(max_align_t) >= sizeof(long long), "");
 _Static_assert(alignof(max_align_t) >= alignof(long long), "");
 _Static_assert(sizeof(max_align_t) >= sizeof(long double), "");
 _Static_assert(alignof(max_align_t) >= alignof(long double), "");
 
+
 #ifdef _MSC_VER
 _Static_assert(sizeof(max_align_t) == sizeof(double), "");
+_Static_assert(alignof(max_align_t) == alignof(double), "");
+#elif defined(__APPLE__)
+_Static_assert(sizeof(max_align_t) == sizeof(long double), "");
+_Static_assert(alignof(max_align_t) == alignof(long double), "");
+#else
+// Test compatibility with GCC's version
+_Static_assert(sizeof(max_align_t) >= sizeof(__float128), "");
+_Static_assert(alignof(max_align_t) >= alignof(__float128), "");
 #endif
 
 // If we are freestanding, then also check RSIZE_MAX (in a hosted implementation
 // we will use the host stdint.h, which may not yet have C11 support).
 #ifndef __STDC_HOSTED__
 #include <stdint.h>
 rsize_t x2 = RSIZE_MAX;
 #endif