Make max_align_t respect _Float128

The _FloatN, _FloatNx, _DecimalN and _DecimalNx types are specified in
such a way that they are basic types, meaning that max_align_t must be
at least as aligned as those types.

On 32-bit x86, max_align_t is currently 8-byte aligned, but
_Decimal128 and _Float128 are 16-byte aligned, so the alignment of
max_align_t needs to increase to meet the standard requirements for
these types.

This patch implements such an increase.  Because max_align_t needs to
be usable for C++ as well as for C, <stddef.h> can't actually refer to
_Float128, but needs to use __float128 (or some other notation for the
type) instead.  And since __float128 is architecture-specific, there
isn't a preprocessor conditional that means "__float128 is available"
(whereas one could test __FLT128_MANT_DIG__ to see if _Float128 is
available).  But I believe the only case that actually has an
alignment problem here is 32-bit x86, and <stddef.h> already has lots
of conditional specific to particular architectures or OSes, so this
patch uses a conditional on __i386__.  If any other architectures turn
out to have such an issue, it will show up as failures of one of the
testcases added by this patch.

Such an increase is of course an ABI change, but a reasonably safe
one, in that max_align_t doesn't tend to appear in library interfaces
(rather, it's something to use when writing allocators and similar
code; most matches found on codesearch.debian.net look like copies of
the gnulib stddef.h module rather than anything actually using
max_align_t at all).

(I think glibc malloc alignment should also increase to 16-byte on
32-bit x86 so it works for allocating objects of these types, which is
now straightforward given the fix made for 32-bit powerpc.)

Bootstrapped with no regressions on x86_64-pc-linux-gnu, and
spot-tested with -m32 that the new float128-align.c test now compiles
where previously it didn't.  OK to commit?

gcc:
2016-08-26  Joseph Myers  <joseph@codesourcery.com>

	* ginclude/stddef.h (max_align_t) [__i386__]: Add __float128
	element.

gcc/testsuite:
2016-08-26  Joseph Myers  <joseph@codesourcery.com>

	* gcc.dg/float128-align.c, gcc.dg/float128x-align.c,
	gcc.dg/float16-align.c, gcc.dg/float32-align.c,
	gcc.dg/float32x-align.c, gcc.dg/float64-align.c,
	gcc.dg/float64x-align.c, gcc.dg/floatn-align.h: New tests.

On Fri, 26 Aug 2016, Joseph Myers wrote:

> And since __float128 is architecture-specific, there
> isn't a preprocessor conditional that means "__float128 is available"

That's what __SIZEOF_FLOAT128__ was supposed to be for. I didn't add it to 
itanium because zombies, and it looks like powerpc didn't add the macro 
when they later gained __float128 support, but that would be easy to fix.

On Fri, 26 Aug 2016, Marc Glisse wrote:

> On Fri, 26 Aug 2016, Joseph Myers wrote:
> 
> > And since __float128 is architecture-specific, there
> > isn't a preprocessor conditional that means "__float128 is available"
> 
> That's what __SIZEOF_FLOAT128__ was supposed to be for. I didn't add it to
> itanium because zombies, and it looks like powerpc didn't add the macro when
> they later gained __float128 support, but that would be easy to fix.

Well, the patch could use __SIZEOF_FLOAT128__ just as well as __i386__ 
(the effect would be an extra union member that does nothing in the x86_64 
case) without the macro needing defining on other architectures.  What 
would be trickier is if a new architecture gets support for _Float128 (as 
a type increasing the alignment requirement) without the old __float128 
name, and then for C++ compatibility the header needs to use float 
__attribute__ ((__mode__ (__TF__))) as the type name on that architecture 
(but on powerpc, TFmode may not be the mode for this type).

Anyway, we need review of the principle of increasing the alignment, and 
given that can consider what the best choice of macro is.

On 08/26/2016 10:54 PM, Joseph Myers wrote:
> Such an increase is of course an ABI change, but a reasonably safe
> one, in that max_align_t doesn't tend to appear in library interfaces
> (rather, it's something to use when writing allocators and similar
> code; most matches found on codesearch.debian.net look like copies of
> the gnulib stddef.h module rather than anything actually using
> max_align_t at all).

The crucial question is whether GCC will start assuming that pointers 
returned by malloc (or attribute-malloc functions) have such an 
alignment.  That seems impossible.

I'm pretty sure it does not right now because few mallocs have this 
property (jemalloc, tcmalloc, and until recently glibc's malloc on 
32-bit powerpc and MIPS failed this requirement).  They only provide 
8-byte alignment even though the fundamental alignment is 16.  For the 
dl-minimal malloc in glibc, the glibc consensus was to ignore 
max_align_t and go with the regular malloc alignment.

Depending on what your malloc looks like, it can be very tempting *not* 
to provide 16-byte alignment for pointers returned from malloc (8) or 
strdup ("abc"), and practical evidence, gained with non-glibc malloc on 
x86_64 (where the fundamental alignment is 16), suggests that this is 
currently feasible.

In the end, max_align_t is ignored by allocators, and applications 
cannot rely on it as a result.

Florian

On 08/26/2016 01:54 PM, Joseph Myers wrote:
> Such an increase is of course an ABI change, but a reasonably safe
> one, in that max_align_t doesn't tend to appear in library interfaces
> (rather, it's something to use when writing allocators and similar
> code;

It should be fine, though I would like to mention a thin-ice area. Emacs 
uses max_align_t to infer whether malloc returns a pointer that is a 
multiple of 8, with macros like this:

   // in src/lisp.h:
   #define GCALIGNMENT 8

   // in src/alloc.c:
   #define MALLOC_IS_GC_ALIGNED (__alignof__ (max_align_t) % GCALIGNMENT 
== 0)

If __alignof__ (max_align_t) is greater than malloc alignment, the 
assumption behind the above code is wrong, i.e., MALLOC_IS_GC_ALIGNED 
might return 1 even though the correct value is 0. As it happens, Emacs 
will work on x86 since GCALIGNMENT is 8; but if GCALIGNMENT were 
increased to 16 the definition of MALLOC_IS_GC_ALIGNED would become 
incorrect with the proposed GCC patch.

> (I think glibc malloc alignment should also increase to 16-byte on
> 32-bit x86 so it works for allocating objects of these types, which is
> now straightforward given the fix made for 32-bit powerpc.)

Yes. I hope the above note helps explain why malloc alignment should be 
at least max_align_t.

> OK to commit?

For what it's worth, it looks good to me.


[in your followup email]

> Well, the patch could use __SIZEOF_FLOAT128__ just as well as __i386__
> (the effect would be an extra union member

s/union/struct/. Though I've always wondered why it is a struct and not 
a union. Maybe change it to union while we're doing an ABI change anyway?

On 08/26/2016 02:45 PM, Florian Weimer wrote:
> In the end, max_align_t is ignored by allocators, and applications 
> cannot rely on it as a result. 

Hmm, well, in that case I suppose I should change Emacs to ignore 
max_align_t as well. Thanks for the heads-up.

On Fri, 26 Aug 2016, Florian Weimer wrote:

> The crucial question is whether GCC will start assuming that pointers returned
> by malloc (or attribute-malloc functions) have such an alignment.  That seems
> impossible.

GCC's assumptions about the alignment of pointers returned by malloc are 
more conservative (MALLOC_ABI_ALIGNMENT, default BITS_PER_WORD) - those 
assumptions need to allow for malloc replacements as well as that in 
standard libc.  Of course any user code using malloc to allocate storage 
containing __float128 or _Decimal128 is implicitly requiring it to be 
sufficiently aligned (GCC will generate SSE loads / stores for such 
objects that require 16-byte alignment).  But most code avoids that issue 
just as most code wasn't hit by the powerpc problems (despite GCC being 
able to generate AltiVec loads / stores for long double on powerpc32).

On Fri, Aug 26, 2016 at 02:51:38PM -0700, Paul Eggert wrote:
> > Well, the patch could use __SIZEOF_FLOAT128__ just as well as __i386__
> > (the effect would be an extra union member
> 
> s/union/struct/. Though I've always wondered why it is a struct and not a
> union. Maybe change it to union while we're doing an ABI change anyway?
 
Yeah, me too.  The initial implementation is here
<https://gcc.gnu.org/ml/gcc-patches/2011-11/msg00841.html> but I don't see any
comments wrt max_align_t being a struct or a union there.

	Marek

On Mon, 29 Aug 2016, Marek Polacek wrote:

> On Fri, Aug 26, 2016 at 02:51:38PM -0700, Paul Eggert wrote:
> > > Well, the patch could use __SIZEOF_FLOAT128__ just as well as __i386__
> > > (the effect would be an extra union member
> > 
> > s/union/struct/. Though I've always wondered why it is a struct and not a
> > union. Maybe change it to union while we're doing an ABI change anyway?
>  
> Yeah, me too.  The initial implementation is here
> <https://gcc.gnu.org/ml/gcc-patches/2011-11/msg00841.html> but I don't see any
> comments wrt max_align_t being a struct or a union there.

I'm not aware of a specific reason for struct versus union (naming via a 
typedef without a struct tag, so that no identifiers from the tag 
namespace are used for C and so that the name for linkage purposes in C++ 
is max_align_t, is deliberate, however).

While the chance of any code's ABI being affected by the size of the type 
should be small, the minimum change is certainly the one that uses 
__i386__ and so doesn't affect the type at all except in the case where 
it's necessary to do so.

Ping.  This patch 
<https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01898.html> is pending 
review (with either __i386__ or __SIZEOF_FLOAT128__, although __i386__ is 
safest in that it minimizes the cases where there is any ABI change at 
all even in the size of the type).