Message ID | alpine.DEB.2.20.1608262052110.1996@digraph.polyomino.org.uk |
---|---|
State | New |
Headers | show |
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).
Index: gcc/ginclude/stddef.h =================================================================== --- gcc/ginclude/stddef.h (revision 239784) +++ gcc/ginclude/stddef.h (working copy) @@ -426,6 +426,14 @@ typedef struct { long long __max_align_ll __attribute__((__aligned__(__alignof__(long long)))); long double __max_align_ld __attribute__((__aligned__(__alignof__(long double)))); + /* _Float128 is defined as a basic type, so max_align_t must be + sufficiently aligned for it. This code must work in C++, so we + use __float128 here; that is only available on some + architectures, but only on i386 is extra alignment needed for + __float128. */ +#ifdef __i386__ + __float128 __max_align_f128 __attribute__((__aligned__(__alignof(__float128)))); +#endif } max_align_t; #endif #endif /* C11 or C++11. */ Index: gcc/testsuite/gcc.dg/float128-align.c =================================================================== --- gcc/testsuite/gcc.dg/float128-align.c (nonexistent) +++ gcc/testsuite/gcc.dg/float128-align.c (working copy) @@ -0,0 +1,9 @@ +/* Test _Float128 alignment. */ +/* { dg-do compile } */ +/* { dg-options "" } */ +/* { dg-add-options float128 } */ +/* { dg-require-effective-target float128 } */ + +#define WIDTH 128 +#define EXT 0 +#include "floatn-align.h" Index: gcc/testsuite/gcc.dg/float128x-align.c =================================================================== --- gcc/testsuite/gcc.dg/float128x-align.c (nonexistent) +++ gcc/testsuite/gcc.dg/float128x-align.c (working copy) @@ -0,0 +1,9 @@ +/* Test _Float128 alignment. */ +/* { dg-do compile } */ +/* { dg-options "" } */ +/* { dg-add-options float128x } */ +/* { dg-require-effective-target float128x } */ + +#define WIDTH 128 +#define EXT 1 +#include "floatn-align.h" Index: gcc/testsuite/gcc.dg/float16-align.c =================================================================== --- gcc/testsuite/gcc.dg/float16-align.c (nonexistent) +++ gcc/testsuite/gcc.dg/float16-align.c (working copy) @@ -0,0 +1,9 @@ +/* Test _Float16 alignment. */ +/* { dg-do compile } */ +/* { dg-options "" } */ +/* { dg-add-options float16 } */ +/* { dg-require-effective-target float16 } */ + +#define WIDTH 16 +#define EXT 0 +#include "floatn-align.h" Index: gcc/testsuite/gcc.dg/float32-align.c =================================================================== --- gcc/testsuite/gcc.dg/float32-align.c (nonexistent) +++ gcc/testsuite/gcc.dg/float32-align.c (working copy) @@ -0,0 +1,9 @@ +/* Test _Float32 alignment. */ +/* { dg-do compile } */ +/* { dg-options "" } */ +/* { dg-add-options float32 } */ +/* { dg-require-effective-target float32 } */ + +#define WIDTH 32 +#define EXT 0 +#include "floatn-align.h" Index: gcc/testsuite/gcc.dg/float32x-align.c =================================================================== --- gcc/testsuite/gcc.dg/float32x-align.c (nonexistent) +++ gcc/testsuite/gcc.dg/float32x-align.c (working copy) @@ -0,0 +1,9 @@ +/* Test _Float32 alignment. */ +/* { dg-do compile } */ +/* { dg-options "" } */ +/* { dg-add-options float32x } */ +/* { dg-require-effective-target float32x } */ + +#define WIDTH 32 +#define EXT 1 +#include "floatn-align.h" Index: gcc/testsuite/gcc.dg/float64-align.c =================================================================== --- gcc/testsuite/gcc.dg/float64-align.c (nonexistent) +++ gcc/testsuite/gcc.dg/float64-align.c (working copy) @@ -0,0 +1,9 @@ +/* Test _Float64 alignment. */ +/* { dg-do compile } */ +/* { dg-options "" } */ +/* { dg-add-options float64 } */ +/* { dg-require-effective-target float64 } */ + +#define WIDTH 64 +#define EXT 0 +#include "floatn-align.h" Index: gcc/testsuite/gcc.dg/float64x-align.c =================================================================== --- gcc/testsuite/gcc.dg/float64x-align.c (nonexistent) +++ gcc/testsuite/gcc.dg/float64x-align.c (working copy) @@ -0,0 +1,9 @@ +/* Test _Float64 alignment. */ +/* { dg-do compile } */ +/* { dg-options "" } */ +/* { dg-add-options float64x } */ +/* { dg-require-effective-target float64x } */ + +#define WIDTH 64 +#define EXT 1 +#include "floatn-align.h" Index: gcc/testsuite/gcc.dg/floatn-align.h =================================================================== --- gcc/testsuite/gcc.dg/floatn-align.h (nonexistent) +++ gcc/testsuite/gcc.dg/floatn-align.h (working copy) @@ -0,0 +1,18 @@ +/* Tests for _FloatN / _FloatNx types: test max_align_t alignment. + Before including this file, define WIDTH as the value N; define EXT + to 1 for _FloatNx and 0 for _FloatN. */ + +#define CONCATX(X, Y) X ## Y +#define CONCAT(X, Y) CONCATX (X, Y) +#define CONCAT3(X, Y, Z) CONCAT (CONCAT (X, Y), Z) + +#if EXT +# define TYPE CONCAT3 (_Float, WIDTH, x) +#else +# define TYPE CONCAT (_Float, WIDTH) +#endif + +#include <stddef.h> + +_Static_assert (_Alignof (max_align_t) >= _Alignof (TYPE), + "max_align_t must be at least as aligned as _Float* types");