mailing list of musl libc
 help / color / mirror / code / Atom feed
* Re: [PATCH] add definition of max_align_t to stddef.h
@ 2014-05-06 10:35 Paweł Dziepak
  2014-05-07  3:13 ` Rich Felker
  0 siblings, 1 reply; 28+ messages in thread
From: Paweł Dziepak @ 2014-05-06 10:35 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

Rich Felker wrote:
>On Sun, May 04, 2014 at 04:36:03AM +0200, Paweł Dziepak wrote:
>> 2014-04-30 23:42 GMT+02:00 Szabolcs Nagy <nsz@...t70.net>:
>> > * Pawel Dziepak <pdziepak@...rnos.org> [2014-04-30 22:23:01 +0200]:
>> >>
>> >> +TYPEDEF union { long double ld; long long ll; } max_align_t;
>> >
>> > this is wrong
>> >
>> > - ld and ll identifiers are not reserved for the implementation
>> > (you could name them _ld, _ll or __ld, __ll etc)
>>
>> I will fix that. However, I must admit I don't see why members of the
>> union (or struct) have to use identifiers reserved for the
>> implementation. It's not like they can conflict with anything, isn't
>> it?
>
> #define ll 0
> #include <stddef.h>

Ah, I didn't thought about that. Thanks for clarification.

>> > and see previous max_align_t discussion
>> > http://www.openwall.com/lists/musl/2014/04/28/8
>> >
>> > - compiler implementations are non-conforming on some platforms
>> > (_Alignof returns inconsistent results for the same object type so
>> > reasoning about alignments is problematic, there are exceptions
>> > where this is allowed in c++11 but not in c11)
>> >
>> > - max_align_t is part of the abi and your solution is incompatible
>> > with gcc and clang (your definition gives 4 byte _Alignof(max_align_t)
>> > on i386 instead of 8)
>>
>> The behavior of _Alignof on x86 is indeed quite surprising. I actually
>
> It's also wrong. The correct alignment for max_align_t on i386 is 4,
> not 8. It's a bug that GCC ever returns 8 for alignof on i386. We
> really need to file a bug against GCC and explain this clearly,
> because I have a feeling they're going to be opposed to fixing it...
>
>> don't see why 8 is the right value and 4 isn't - System V ABI for x86
>> doesn't mention any type with alignment 8. Anyway, I agree that it
>
> You're completely right; GCC is wrong.
>
>> would be a good thing to mach the definition gcc and clang use, i.e.
>> something like that:
>>
>> union max_align_t {
>>     alignas(long long) long long _ll;
>>     alignas(long double) long double _ld;
>> };
>
> This should not give results different from omitting the "alignas".
> The only reason it does give different results is a bug in GCC, so we
> should not be copying this confusing mess that's a no-op for a correct
> compiler. (Applying alignas(T) to type T is always a no-op.)

I should have checked whether GCC 4.9 has changed before sending that.
As I said earlier, alignof in 4.9 seems to be fixed and on i386 for
fundamental types values <=4 are returned. alignof(max_align_t)
remains 8, though.

However, while 4, undobtedly, is the expected value of
alignof(max_align_t) I don't think that 8 is really wrong (well, from
the C11 point of view). The standard is not very specific about what
max_align_t really should be and if the compiler supports larger
alignment in all contexts there is no reason that alignof(max_align_t)
cannot be larger than alignof() of the type with the strictest
alignment requirements.
Obviously, since max_align_t is the part of ABI it is not like the
implementation can set alignof(max_align_t) to any value or it would
risk compatibility problems with binaries compiled with different
max_align_t. Since both GCC and Clang already define max_align_t so
that its alignment is 8 on i386 I think that Musl should do the same.

Paweł

PS Please, do not remove me from CC.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-06 10:35 [PATCH] add definition of max_align_t to stddef.h Paweł Dziepak
@ 2014-05-07  3:13 ` Rich Felker
  2014-05-07  4:14   ` Luca Barbato
  2014-05-07  9:28   ` Paweł Dziepak
  0 siblings, 2 replies; 28+ messages in thread
From: Rich Felker @ 2014-05-07  3:13 UTC (permalink / raw)
  To: musl

On Tue, May 06, 2014 at 12:35:55PM +0200, Paweł Dziepak wrote:
> >> would be a good thing to mach the definition gcc and clang use, i.e.
> >> something like that:
> >>
> >> union max_align_t {
> >>     alignas(long long) long long _ll;
> >>     alignas(long double) long double _ld;
> >> };
> >
> > This should not give results different from omitting the "alignas".
> > The only reason it does give different results is a bug in GCC, so we
> > should not be copying this confusing mess that's a no-op for a correct
> > compiler. (Applying alignas(T) to type T is always a no-op.)
> 
> I should have checked whether GCC 4.9 has changed before sending that.
> As I said earlier, alignof in 4.9 seems to be fixed and on i386 for
> fundamental types values <=4 are returned. alignof(max_align_t)
> remains 8, though.

Then GCC still has a bug. The above definition should give an
alignment of 4, not 8. Neither alignas(long long) nor alignas(long
double) should impose 8-byte alignment.

> However, while 4, undobtedly, is the expected value of
> alignof(max_align_t) I don't think that 8 is really wrong (well, from
> the C11 point of view). The standard is not very specific about what
> max_align_t really should be and if the compiler supports larger
> alignment in all contexts there is no reason that alignof(max_align_t)
> cannot be larger than alignof() of the type with the strictest
> alignment requirements.
> Obviously, since max_align_t is the part of ABI it is not like the
> implementation can set alignof(max_align_t) to any value or it would
> risk compatibility problems with binaries compiled with different
> max_align_t. Since both GCC and Clang already define max_align_t so
> that its alignment is 8 on i386 I think that Musl should do the same.

If we want to achieve an alignment of 8, the above definition is
wrong; it will no longer have alignment 8 once the bug is fixed.
However I'm not convinced it's the right thing to do. Defining it as 8
is tightening malloc's contract to always return 8-byte-aligned memory
(note that it presently returns at least 16-byte alignment anyway, but
this is an implementation detail that's not meant to be observable,
not part of the interface contract).

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-07  3:13 ` Rich Felker
@ 2014-05-07  4:14   ` Luca Barbato
  2014-05-07  4:29     ` Rich Felker
  2014-05-07  9:28   ` Paweł Dziepak
  1 sibling, 1 reply; 28+ messages in thread
From: Luca Barbato @ 2014-05-07  4:14 UTC (permalink / raw)
  To: musl

On 07/05/14 05:13, Rich Felker wrote:
> If we want to achieve an alignment of 8, the above definition is
> wrong; it will no longer have alignment 8 once the bug is fixed.
> However I'm not convinced it's the right thing to do. Defining it as 8
> is tightening malloc's contract to always return 8-byte-aligned memory
> (note that it presently returns at least 16-byte alignment anyway, but
> this is an implementation detail that's not meant to be observable,
> not part of the interface contract).

The current natural alignment shouldn't be 32 for AVX and 16 for SSE ?

Not sure how wasteful would be but it would be surely a boon for the
applications I'm mostly involved.

lu


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-07  4:14   ` Luca Barbato
@ 2014-05-07  4:29     ` Rich Felker
  2014-05-07  5:12       ` Luca Barbato
  0 siblings, 1 reply; 28+ messages in thread
From: Rich Felker @ 2014-05-07  4:29 UTC (permalink / raw)
  To: musl

On Wed, May 07, 2014 at 06:14:38AM +0200, Luca Barbato wrote:
> On 07/05/14 05:13, Rich Felker wrote:
> > If we want to achieve an alignment of 8, the above definition is
> > wrong; it will no longer have alignment 8 once the bug is fixed.
> > However I'm not convinced it's the right thing to do. Defining it as 8
> > is tightening malloc's contract to always return 8-byte-aligned memory
> > (note that it presently returns at least 16-byte alignment anyway, but
> > this is an implementation detail that's not meant to be observable,
> > not part of the interface contract).
> 
> The current natural alignment shouldn't be 32 for AVX and 16 for SSE ?
> 
> Not sure how wasteful would be but it would be surely a boon for the
> applications I'm mostly involved.

If you're working with data that needs additional alignment, you have
to use aligned_alloc (C11), posix_memalign (POSIX), or memalign
(legacy). Just assuming the result of malloc will be aligned beyond
the alignment requirements of any standard type is unsafe.

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-07  4:29     ` Rich Felker
@ 2014-05-07  5:12       ` Luca Barbato
  2014-05-07 22:48         ` Rich Felker
  0 siblings, 1 reply; 28+ messages in thread
From: Luca Barbato @ 2014-05-07  5:12 UTC (permalink / raw)
  To: musl

On 07/05/14 06:29, Rich Felker wrote:
> On Wed, May 07, 2014 at 06:14:38AM +0200, Luca Barbato wrote:
>> On 07/05/14 05:13, Rich Felker wrote:
>>> If we want to achieve an alignment of 8, the above definition is
>>> wrong; it will no longer have alignment 8 once the bug is fixed.
>>> However I'm not convinced it's the right thing to do. Defining it as 8
>>> is tightening malloc's contract to always return 8-byte-aligned memory
>>> (note that it presently returns at least 16-byte alignment anyway, but
>>> this is an implementation detail that's not meant to be observable,
>>> not part of the interface contract).
>>
>> The current natural alignment shouldn't be 32 for AVX and 16 for SSE ?
>>
>> Not sure how wasteful would be but it would be surely a boon for the
>> applications I'm mostly involved.
> 
> If you're working with data that needs additional alignment, you have

That's the part that is annoying, the larger register is 32byte in those
platforms.

> to use aligned_alloc (C11), posix_memalign (POSIX), or memalign
> (legacy). Just assuming the result of malloc will be aligned beyond
> the alignment requirements of any standard type is unsafe.

That we do already obviously, with the additional fun of not having a
realloc matching the mentioned functions in most platforms.

Having the memory functions 32-byte aligned and a mean to probe for it
would simplify a lot of code.

lu


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-07  3:13 ` Rich Felker
  2014-05-07  4:14   ` Luca Barbato
@ 2014-05-07  9:28   ` Paweł Dziepak
  2014-05-07 23:07     ` Rich Felker
  1 sibling, 1 reply; 28+ messages in thread
From: Paweł Dziepak @ 2014-05-07  9:28 UTC (permalink / raw)
  To: musl; +Cc: Pawel Dziepak

2014-05-07 5:13 GMT+02:00 Rich Felker <dalias@libc.org>:
> On Tue, May 06, 2014 at 12:35:55PM +0200, Paweł Dziepak wrote:
>> >> would be a good thing to mach the definition gcc and clang use, i.e.
>> >> something like that:
>> >>
>> >> union max_align_t {
>> >>     alignas(long long) long long _ll;
>> >>     alignas(long double) long double _ld;
>> >> };
>> >
>> > This should not give results different from omitting the "alignas".
>> > The only reason it does give different results is a bug in GCC, so we
>> > should not be copying this confusing mess that's a no-op for a correct
>> > compiler. (Applying alignas(T) to type T is always a no-op.)
>>
>> I should have checked whether GCC 4.9 has changed before sending that.
>> As I said earlier, alignof in 4.9 seems to be fixed and on i386 for
>> fundamental types values <=4 are returned. alignof(max_align_t)
>> remains 8, though.
>
> Then GCC still has a bug. The above definition should give an
> alignment of 4, not 8. Neither alignas(long long) nor alignas(long
> double) should impose 8-byte alignment.

To clarify: GCC defines max_align_t so its alignment is 8. My original
definition (without alignas) makes max_align_t 4-byte-aligned (both
GCC 4.8.2 and 4.9). My second definition (with alignas) results in
8-byte-aligned max_align_t on GCC 4.8.2 and bug in GCC 4.9
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=61053

GCC uses its own __alignof__ to define max_align_t. __alignof__
returns the recommended alignment (as oposed to minimal in case of
alignof), which in case of long long is 8.

>> However, while 4, undobtedly, is the expected value of
>> alignof(max_align_t) I don't think that 8 is really wrong (well, from
>> the C11 point of view). The standard is not very specific about what
>> max_align_t really should be and if the compiler supports larger
>> alignment in all contexts there is no reason that alignof(max_align_t)
>> cannot be larger than alignof() of the type with the strictest
>> alignment requirements.
>> Obviously, since max_align_t is the part of ABI it is not like the
>> implementation can set alignof(max_align_t) to any value or it would
>> risk compatibility problems with binaries compiled with different
>> max_align_t. Since both GCC and Clang already define max_align_t so
>> that its alignment is 8 on i386 I think that Musl should do the same.
>
> If we want to achieve an alignment of 8, the above definition is
> wrong; it will no longer have alignment 8 once the bug is fixed.
> However I'm not convinced it's the right thing to do. Defining it as 8
> is tightening malloc's contract to always return 8-byte-aligned memory
> (note that it presently returns at least 16-byte alignment anyway, but
> this is an implementation detail that's not meant to be observable,
> not part of the interface contract).

I've mentioned earlier that it seems that the only option is to use
GCC extensions (i.e. __alignof__) to match their definition of
max_align_t, just like it is done in this patch:
http://www.openwall.com/lists/musl/2014/04/28/3
It is not nice that GCC forces malloc to support "extended" alignment
but I don't think there is much that can be done about it.

Paweł


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-07  5:12       ` Luca Barbato
@ 2014-05-07 22:48         ` Rich Felker
  2014-05-08 12:07           ` Luca Barbato
  0 siblings, 1 reply; 28+ messages in thread
From: Rich Felker @ 2014-05-07 22:48 UTC (permalink / raw)
  To: musl

On Wed, May 07, 2014 at 07:12:39AM +0200, Luca Barbato wrote:
> On 07/05/14 06:29, Rich Felker wrote:
> > On Wed, May 07, 2014 at 06:14:38AM +0200, Luca Barbato wrote:
> >> On 07/05/14 05:13, Rich Felker wrote:
> >>> If we want to achieve an alignment of 8, the above definition is
> >>> wrong; it will no longer have alignment 8 once the bug is fixed.
> >>> However I'm not convinced it's the right thing to do. Defining it as 8
> >>> is tightening malloc's contract to always return 8-byte-aligned memory
> >>> (note that it presently returns at least 16-byte alignment anyway, but
> >>> this is an implementation detail that's not meant to be observable,
> >>> not part of the interface contract).
> >>
> >> The current natural alignment shouldn't be 32 for AVX and 16 for SSE ?
> >>
> >> Not sure how wasteful would be but it would be surely a boon for the
> >> applications I'm mostly involved.
> > 
> > If you're working with data that needs additional alignment, you have
> 
> That's the part that is annoying, the larger register is 32byte in those
> platforms.

And it will keep getting larger. Obviously changing the definition of
types and/or the ABI again and again is not the solution. The solution
is requesting the alignment you want.

BTW note that in the case of audio and video, depending on which
sample you start at, your data will not be aligned even if the start
of the buffer is aligned (think video filters working on a sub-image,
for example). So in cases like that your code should just handle the
misaligned head/tail parts separately. Note that GCC (and AFAIK
clang/llvm) already do this for you with -O3 and a -march that
supports vector ops.

> > to use aligned_alloc (C11), posix_memalign (POSIX), or memalign
> > (legacy). Just assuming the result of malloc will be aligned beyond
> > the alignment requirements of any standard type is unsafe.
> 
> That we do already obviously, with the additional fun of not having a
> realloc matching the mentioned functions in most platforms.

This is really a minor limitation. realloc cannot realistically be
expected to grow an object in-place in most cases; the only common
exception is when you're working with new memory at the top of the
heap and there's nothing else making small allocations that might get
placed right after your buffer that needs to grow.

In particular, if you're using a sane growth stratgey (geometric),
almost all calls to realloc are likely to move the buffer, so you're
just as well off calling malloc (or aligned_alloc) and free yourself.
The main exception to this might be for HUGE buffers where realloc can
use mremap with MREMAP_MAYMOVE.

> Having the memory functions 32-byte aligned and a mean to probe for it
> would simplify a lot of code.

I think it would complicate the code because now you'd have two cases
to maintain, the case for implementations that always give excessive
alignment and the case for ones that don't. And one code path would
likely bitrot and have bugs.

In any case, the overhead would be undesirable. If/when I make some
improvements to malloc and its strategy for returning memory for use
by other processes (freeing commit charge), I'm also hoping to drop
the granularity on 64-bit platforms from 32 down to 16 or maybe even
smaller. There's really no need to store a size_t in the headers for
chunks which are only used for allocation sizes up to 128k/256k. This
kind of thing potentially makes a big difference for bloated OO/C++
apps that are allocating tons of tiny objects like linked list nodes
that are just 3 pointers (next/prev/data) and short strings.

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-07  9:28   ` Paweł Dziepak
@ 2014-05-07 23:07     ` Rich Felker
  2014-05-08 10:57       ` Szabolcs Nagy
  2014-05-08 16:41       ` Paweł Dziepak
  0 siblings, 2 replies; 28+ messages in thread
From: Rich Felker @ 2014-05-07 23:07 UTC (permalink / raw)
  To: musl; +Cc: Pawel Dziepak

On Wed, May 07, 2014 at 11:28:58AM +0200, Paweł Dziepak wrote:
> 2014-05-07 5:13 GMT+02:00 Rich Felker <dalias@libc.org>:
> > On Tue, May 06, 2014 at 12:35:55PM +0200, Paweł Dziepak wrote:
> >> >> would be a good thing to mach the definition gcc and clang use, i.e.
> >> >> something like that:
> >> >>
> >> >> union max_align_t {
> >> >>     alignas(long long) long long _ll;
> >> >>     alignas(long double) long double _ld;
> >> >> };
> >> >
> >> > This should not give results different from omitting the "alignas".
> >> > The only reason it does give different results is a bug in GCC, so we
> >> > should not be copying this confusing mess that's a no-op for a correct
> >> > compiler. (Applying alignas(T) to type T is always a no-op.)
> >>
> >> I should have checked whether GCC 4.9 has changed before sending that.
> >> As I said earlier, alignof in 4.9 seems to be fixed and on i386 for
> >> fundamental types values <=4 are returned. alignof(max_align_t)
> >> remains 8, though.
> >
> > Then GCC still has a bug. The above definition should give an
> > alignment of 4, not 8. Neither alignas(long long) nor alignas(long
> > double) should impose 8-byte alignment.
> 
> To clarify: GCC defines max_align_t so its alignment is 8. My original
> definition (without alignas) makes max_align_t 4-byte-aligned (both
> GCC 4.8.2 and 4.9). My second definition (with alignas) results in
> 8-byte-aligned max_align_t on GCC 4.8.2 and bug in GCC 4.9
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=61053
> 
> GCC uses its own __alignof__ to define max_align_t. __alignof__
> returns the recommended alignment (as oposed to minimal in case of
> alignof), which in case of long long is 8.

That's very confusing, so thanks for clarifying.

BTW, is __alignof__(long double) really giving 8? If so that's utter
nonsense. sizeof(long double) is 12, and alignment must always divide
the size of the type...

> >> However, while 4, undobtedly, is the expected value of
> >> alignof(max_align_t) I don't think that 8 is really wrong (well, from
> >> the C11 point of view). The standard is not very specific about what
> >> max_align_t really should be and if the compiler supports larger
> >> alignment in all contexts there is no reason that alignof(max_align_t)
> >> cannot be larger than alignof() of the type with the strictest
> >> alignment requirements.
> >> Obviously, since max_align_t is the part of ABI it is not like the
> >> implementation can set alignof(max_align_t) to any value or it would
> >> risk compatibility problems with binaries compiled with different
> >> max_align_t. Since both GCC and Clang already define max_align_t so
> >> that its alignment is 8 on i386 I think that Musl should do the same.
> >
> > If we want to achieve an alignment of 8, the above definition is
> > wrong; it will no longer have alignment 8 once the bug is fixed.
> > However I'm not convinced it's the right thing to do. Defining it as 8
> > is tightening malloc's contract to always return 8-byte-aligned memory
> > (note that it presently returns at least 16-byte alignment anyway, but
> > this is an implementation detail that's not meant to be observable,
> > not part of the interface contract).
> 
> I've mentioned earlier that it seems that the only option is to use
> GCC extensions (i.e. __alignof__) to match their definition of
> max_align_t, just like it is done in this patch:
> http://www.openwall.com/lists/musl/2014/04/28/3
> It is not nice that GCC forces malloc to support "extended" alignment
> but I don't think there is much that can be done about it.

I don't see how GCC "forces" this. The definition of max_align_t from
glibc's stddef.h, which we do not use, is what forces it.

As I see it, we have a choice whether to use the "8" definition on
i386 or use the natural definition, which would yield "4" on i386.
This is not an ABI issue (it does not affect the ability to use
glibc-built object files/binaries/shared libraries with musl, nor the
C++ name mangling ABI) but an API issue.

Moreover, I can't see any easy way that code assuming GCC's definition
could cause breakage on musl if we went with the other definition. The
x86 instructions that require additional alignment require at least
16-byte alignment, not 8-byte, so aligning as max_align_t will not
produce objects that can be used with sse instructions directly.

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-07 23:07     ` Rich Felker
@ 2014-05-08 10:57       ` Szabolcs Nagy
  2014-05-08 14:11         ` Rich Felker
  2014-05-08 16:41       ` Paweł Dziepak
  1 sibling, 1 reply; 28+ messages in thread
From: Szabolcs Nagy @ 2014-05-08 10:57 UTC (permalink / raw)
  To: musl; +Cc: Pawel Dziepak

* Rich Felker <dalias@libc.org> [2014-05-07 19:07:29 -0400]:
> BTW, is __alignof__(long double) really giving 8? If so that's utter
> nonsense. sizeof(long double) is 12, and alignment must always divide
> the size of the type...

no, long long is 8 though

#include <stdio.h>
#define p(x) printf("%s == %d\n", #x, (int)(x))
int main()
{
        p(__alignof__(long long));
        p(__alignof__(long double));
        p(__alignof__(union{long long a; long double b;}));
        p(_Alignof(long long));
        p(_Alignof(long double));
        p(_Alignof(union{long long a; long double b;}));
}

gcc 4.8 on i386:
__alignof__(long long) == 8
__alignof__(long double) == 4
__alignof__(union{long long a; long double b;}) == 4
_Alignof(long long) == 8
_Alignof(long double) == 4
_Alignof(union{long long a; long double b;}) == 4

gcc 4.9 on i386:
__alignof__(long long) == 8
__alignof__(long double) == 4
__alignof__(union{long long a; long double b;}) == 4
_Alignof(long long) == 4
_Alignof(long double) == 4
_Alignof(union{long long a; long double b;}) == 4

gcc 4.9 on arm:
__alignof__(long long) == 8
__alignof__(long double) == 8
__alignof__(union{long long a; long double b;}) == 8
_Alignof(long long) == 8
_Alignof(long double) == 8
_Alignof(union{long long a; long double b;}) == 8


> As I see it, we have a choice whether to use the "8" definition on
> i386 or use the natural definition, which would yield "4" on i386.
> This is not an ABI issue (it does not affect the ability to use
> glibc-built object files/binaries/shared libraries with musl, nor the
> C++ name mangling ABI) but an API issue.

assuming max_align_t does not appear in a function prototype


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-07 22:48         ` Rich Felker
@ 2014-05-08 12:07           ` Luca Barbato
  2014-05-08 14:25             ` Rich Felker
  0 siblings, 1 reply; 28+ messages in thread
From: Luca Barbato @ 2014-05-08 12:07 UTC (permalink / raw)
  To: musl

On 08/05/14 00:48, Rich Felker wrote:
> On Wed, May 07, 2014 at 07:12:39AM +0200, Luca Barbato wrote:
>> On 07/05/14 06:29, Rich Felker wrote:
>>> On Wed, May 07, 2014 at 06:14:38AM +0200, Luca Barbato wrote:
>>>> On 07/05/14 05:13, Rich Felker wrote:
>>>>> If we want to achieve an alignment of 8, the above definition is
>>>>> wrong; it will no longer have alignment 8 once the bug is fixed.
>>>>> However I'm not convinced it's the right thing to do. Defining it as 8
>>>>> is tightening malloc's contract to always return 8-byte-aligned memory
>>>>> (note that it presently returns at least 16-byte alignment anyway, but
>>>>> this is an implementation detail that's not meant to be observable,
>>>>> not part of the interface contract).
>>>>
>>>> The current natural alignment shouldn't be 32 for AVX and 16 for SSE ?
>>>>
>>>> Not sure how wasteful would be but it would be surely a boon for the
>>>> applications I'm mostly involved.
>>>
>>> If you're working with data that needs additional alignment, you have
>>
>> That's the part that is annoying, the larger register is 32byte in those
>> platforms.
> 
> And it will keep getting larger. Obviously changing the definition of
> types and/or the ABI again and again is not the solution. The solution
> is requesting the alignment you want.

No, but having the correct value for new architectures would be sort of
more correct. Is not that those won't be used.

> BTW note that in the case of audio and video, depending on which
> sample you start at, your data will not be aligned even if the start
> of the buffer is aligned (think video filters working on a sub-image,
> for example).

I know quite well =)

> So in cases like that your code should just handle the
> misaligned head/tail parts separately. Note that GCC (and AFAIK
> clang/llvm) already do this for you with -O3 and a -march that
> supports vector ops.

That isn't the concern at all =)

>>> to use aligned_alloc (C11), posix_memalign (POSIX), or memalign
>>> (legacy). Just assuming the result of malloc will be aligned beyond
>>> the alignment requirements of any standard type is unsafe.
>>
>> That we do already obviously, with the additional fun of not having a
>> realloc matching the mentioned functions in most platforms.
> 
> This is really a minor limitation. realloc cannot realistically be
> expected to grow an object in-place in most cases; the only common
> exception is when you're working with new memory at the top of the
> heap and there's nothing else making small allocations that might get
> placed right after your buffer that needs to grow.
> 
> In particular, if you're using a sane growth stratgey (geometric),
> almost all calls to realloc are likely to move the buffer, so you're
> just as well off calling malloc (or aligned_alloc) and free yourself.
> The main exception to this might be for HUGE buffers where realloc can
> use mremap with MREMAP_MAYMOVE.

That's what we are doing but as you know quite well, less custom code
the better.

> I think it would complicate the code because now you'd have two cases
> to maintain, the case for implementations that always give excessive
> alignment and the case for ones that don't. And one code path would
> likely bitrot and have bugs.

Not really, we already have platforms doing that so the code is there
and exercised on those.

> In any case, the overhead would be undesirable. If/when I make some
> improvements to malloc and its strategy for returning memory for use
> by other processes (freeing commit charge), I'm also hoping to drop
> the granularity on 64-bit platforms from 32 down to 16 or maybe even
> smaller. There's really no need to store a size_t in the headers for
> chunks which are only used for allocation sizes up to 128k/256k.

I see, nice to know that's the plan =) As said it would had been a nice
to have if it comes for free.

lu


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-08 10:57       ` Szabolcs Nagy
@ 2014-05-08 14:11         ` Rich Felker
  0 siblings, 0 replies; 28+ messages in thread
From: Rich Felker @ 2014-05-08 14:11 UTC (permalink / raw)
  To: musl; +Cc: Pawel Dziepak

On Thu, May 08, 2014 at 12:57:22PM +0200, Szabolcs Nagy wrote:
> > As I see it, we have a choice whether to use the "8" definition on
> > i386 or use the natural definition, which would yield "4" on i386.
> > This is not an ABI issue (it does not affect the ability to use
> > glibc-built object files/binaries/shared libraries with musl, nor the
> > C++ name mangling ABI) but an API issue.
> 
> assuming max_align_t does not appear in a function prototype

As long as the union tag and size are the same, it should not matter.
I don't think alignments affect i386 argument passing convention at
all. Certainly the traditional va_arg macro does not account for them;
it just assumes every argument is aligned to 4 bytes.

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-08 12:07           ` Luca Barbato
@ 2014-05-08 14:25             ` Rich Felker
  0 siblings, 0 replies; 28+ messages in thread
From: Rich Felker @ 2014-05-08 14:25 UTC (permalink / raw)
  To: musl

On Thu, May 08, 2014 at 02:07:20PM +0200, Luca Barbato wrote:
> >> That's the part that is annoying, the larger register is 32byte in those
> >> platforms.
> > 
> > And it will keep getting larger. Obviously changing the definition of
> > types and/or the ABI again and again is not the solution. The solution
> > is requesting the alignment you want.
> 
> No, but having the correct value for new architectures would be sort of
> more correct. Is not that those won't be used.

I'm not sure what you mean by "the correct value". The definition with
the union should give the correct value (max alignment requirement of
any standard type) for all archs.

> > In any case, the overhead would be undesirable. If/when I make some
> > improvements to malloc and its strategy for returning memory for use
> > by other processes (freeing commit charge), I'm also hoping to drop
> > the granularity on 64-bit platforms from 32 down to 16 or maybe even
> > smaller. There's really no need to store a size_t in the headers for
> > chunks which are only used for allocation sizes up to 128k/256k.
> 
> I see, nice to know that's the plan =) As said it would had been a nice
> to have if it comes for free.

Yeah, not much comes for free though...

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-07 23:07     ` Rich Felker
  2014-05-08 10:57       ` Szabolcs Nagy
@ 2014-05-08 16:41       ` Paweł Dziepak
  2014-05-08 17:41         ` Rich Felker
  1 sibling, 1 reply; 28+ messages in thread
From: Paweł Dziepak @ 2014-05-08 16:41 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

2014-05-08 1:07 GMT+02:00 Rich Felker <dalias@libc.org>:
> On Wed, May 07, 2014 at 11:28:58AM +0200, Paweł Dziepak wrote:
>> 2014-05-07 5:13 GMT+02:00 Rich Felker <dalias@libc.org>:
>> > On Tue, May 06, 2014 at 12:35:55PM +0200, Paweł Dziepak wrote:
>> >> However, while 4, undobtedly, is the expected value of
>> >> alignof(max_align_t) I don't think that 8 is really wrong (well, from
>> >> the C11 point of view). The standard is not very specific about what
>> >> max_align_t really should be and if the compiler supports larger
>> >> alignment in all contexts there is no reason that alignof(max_align_t)
>> >> cannot be larger than alignof() of the type with the strictest
>> >> alignment requirements.
>> >> Obviously, since max_align_t is the part of ABI it is not like the
>> >> implementation can set alignof(max_align_t) to any value or it would
>> >> risk compatibility problems with binaries compiled with different
>> >> max_align_t. Since both GCC and Clang already define max_align_t so
>> >> that its alignment is 8 on i386 I think that Musl should do the same.
>> >
>> > If we want to achieve an alignment of 8, the above definition is
>> > wrong; it will no longer have alignment 8 once the bug is fixed.
>> > However I'm not convinced it's the right thing to do. Defining it as 8
>> > is tightening malloc's contract to always return 8-byte-aligned memory
>> > (note that it presently returns at least 16-byte alignment anyway, but
>> > this is an implementation detail that's not meant to be observable,
>> > not part of the interface contract).
>>
>> I've mentioned earlier that it seems that the only option is to use
>> GCC extensions (i.e. __alignof__) to match their definition of
>> max_align_t, just like it is done in this patch:
>> http://www.openwall.com/lists/musl/2014/04/28/3
>> It is not nice that GCC forces malloc to support "extended" alignment
>> but I don't think there is much that can be done about it.
>
> I don't see how GCC "forces" this. The definition of max_align_t from
> glibc's stddef.h, which we do not use, is what forces it.

It is GCC that provides stddef.h, not glibc.

> As I see it, we have a choice whether to use the "8" definition on
> i386 or use the natural definition, which would yield "4" on i386.
> This is not an ABI issue (it does not affect the ability to use
> glibc-built object files/binaries/shared libraries with musl, nor the
> C++ name mangling ABI) but an API issue.

What about something like this?

struct foobar {
    char foo;
    alignas(max_align_t) char bar;
};

The binary representation of this structure depends on the definition
of max_align_t and binaries compiled with different
alignof(max_align_t) won't be compatible.

Paweł


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-08 16:41       ` Paweł Dziepak
@ 2014-05-08 17:41         ` Rich Felker
  2014-05-08 18:45           ` Jens Gustedt
  2014-05-08 19:45           ` Paweł Dziepak
  0 siblings, 2 replies; 28+ messages in thread
From: Rich Felker @ 2014-05-08 17:41 UTC (permalink / raw)
  To: Paweł Dziepak; +Cc: musl

On Thu, May 08, 2014 at 06:41:19PM +0200, Paweł Dziepak wrote:
> > As I see it, we have a choice whether to use the "8" definition on
> > i386 or use the natural definition, which would yield "4" on i386.
> > This is not an ABI issue (it does not affect the ability to use
> > glibc-built object files/binaries/shared libraries with musl, nor the
> > C++ name mangling ABI) but an API issue.
> 
> What about something like this?
> 
> struct foobar {
>     char foo;
>     alignas(max_align_t) char bar;
> };
> 
> The binary representation of this structure depends on the definition
> of max_align_t and binaries compiled with different
> alignof(max_align_t) won't be compatible.

Indeed. This is not an ABI issue with libc.so, but it's an API
difference that translates into an ABI difference between third-party
translation units if they use max_align_t as you've described. Whether
we care, I'm not sure. At least historically there were other cases
like this in musl where type sizes differed in ways that didn't affect
libc ABI but did affect ABI between third-party programs (jmp_buf
comes to mind) but most if not all of these were changed.

I'm not strongly opposed to imposing the 8-byte alignment requirement
on malloc (it would be hard to make malloc work on smaller granularity
anyway, and most archs need 8-byte alignment anyway for long long and
for double) but I generally dislike the inconsistency of defining
max_align_t in a semi-absurd way on i386, as well as the issue of
having to use non-portable definitions and/or arch-specific ones.

BTW in your above example, it's not even clear to me if that use of
alignas is valid. It makes no sense for an object to have an alignment
that does not divide its type (imagine if you added [2] to the end of
bar) and in other places (like the contract of aligned_alloc)
alignments that do not divide the size are explicitly illegal. I'd
like to understand this before making a decision.

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-08 17:41         ` Rich Felker
@ 2014-05-08 18:45           ` Jens Gustedt
  2014-05-08 19:11             ` Paweł Dziepak
  2014-05-08 19:45           ` Paweł Dziepak
  1 sibling, 1 reply; 28+ messages in thread
From: Jens Gustedt @ 2014-05-08 18:45 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

Hello,

Am Donnerstag, den 08.05.2014, 13:41 -0400 schrieb Rich Felker:
> BTW in your above example, it's not even clear to me if that use of
> alignas is valid.

Besides the question of such a thing makes sense or noţ with the
current version of the standard it isn't syntactically valid. alignas
can't be applied to struct fields. So for the moment the whole
discussion in the standard about types with extended alignment is
pointless.

This issue has been raised as a defect report, and it seems that the
committee agrees to change this in a corrigendum.

http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_444.htm

Jens

-- 
:: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/   ::
:: AlGorille ::::::::::::::: office Nancy : +33 383593090   ::
:: ICube :::::::::::::: office Strasbourg : +33 368854536   ::
:: ::::::::::::::::::::::::::: gsm France : +33 651400183   ::
:: :::::::::::::::::::: gsm international : +49 15737185122 ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-08 18:45           ` Jens Gustedt
@ 2014-05-08 19:11             ` Paweł Dziepak
  2014-05-08 19:22               ` Jens Gustedt
  0 siblings, 1 reply; 28+ messages in thread
From: Paweł Dziepak @ 2014-05-08 19:11 UTC (permalink / raw)
  To: musl

2014-05-08 20:45 GMT+02:00 Jens Gustedt <jens.gustedt@inria.fr>:
> Hello,
>
> Am Donnerstag, den 08.05.2014, 13:41 -0400 schrieb Rich Felker:
>> BTW in your above example, it's not even clear to me if that use of
>> alignas is valid.
>
> Besides the question of such a thing makes sense or noţ with the
> current version of the standard it isn't syntactically valid. alignas
> can't be applied to struct fields. So for the moment the whole
> discussion in the standard about types with extended alignment is
> pointless.
>
> This issue has been raised as a defect report, and it seems that the
> committee agrees to change this in a corrigendum.
>
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_444.htm

My bad, it is legal in C++11 though.

Paweł


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-08 19:11             ` Paweł Dziepak
@ 2014-05-08 19:22               ` Jens Gustedt
  0 siblings, 0 replies; 28+ messages in thread
From: Jens Gustedt @ 2014-05-08 19:22 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

Am Donnerstag, den 08.05.2014, 21:11 +0200 schrieb Paweł Dziepak:
> 2014-05-08 20:45 GMT+02:00 Jens Gustedt <jens.gustedt@inria.fr>:
> > Hello,
> >
> > Am Donnerstag, den 08.05.2014, 13:41 -0400 schrieb Rich Felker:
> >> BTW in your above example, it's not even clear to me if that use of
> >> alignas is valid.
> >
> > Besides the question of such a thing makes sense or noţ with the
> > current version of the standard it isn't syntactically valid. alignas
> > can't be applied to struct fields. So for the moment the whole
> > discussion in the standard about types with extended alignment is
> > pointless.
> >
> > This issue has been raised as a defect report, and it seems that the
> > committee agrees to change this in a corrigendum.
> >
> > http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_444.htm
> 
> My bad,

no, no, the "bad" is clearly in the standard, this is by no means easy
to read nor to understand

all that max_align_t discussion shows that the alignment stuff in the
standard is underspecified and is missing semantics.

Jens


-- 
:: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/   ::
:: AlGorille ::::::::::::::: office Nancy : +33 383593090   ::
:: ICube :::::::::::::: office Strasbourg : +33 368854536   ::
:: ::::::::::::::::::::::::::: gsm France : +33 651400183   ::
:: :::::::::::::::::::: gsm international : +49 15737185122 ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-08 17:41         ` Rich Felker
  2014-05-08 18:45           ` Jens Gustedt
@ 2014-05-08 19:45           ` Paweł Dziepak
  2014-05-08 20:02             ` Rich Felker
  1 sibling, 1 reply; 28+ messages in thread
From: Paweł Dziepak @ 2014-05-08 19:45 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

2014-05-08 19:41 GMT+02:00 Rich Felker <dalias@libc.org>:
> On Thu, May 08, 2014 at 06:41:19PM +0200, Paweł Dziepak wrote:
>> > As I see it, we have a choice whether to use the "8" definition on
>> > i386 or use the natural definition, which would yield "4" on i386.
>> > This is not an ABI issue (it does not affect the ability to use
>> > glibc-built object files/binaries/shared libraries with musl, nor the
>> > C++ name mangling ABI) but an API issue.
>>
>> What about something like this?
>>
>> struct foobar {
>>     char foo;
>>     alignas(max_align_t) char bar;
>> };
>>
>> The binary representation of this structure depends on the definition
>> of max_align_t and binaries compiled with different
>> alignof(max_align_t) won't be compatible.
>
> Indeed. This is not an ABI issue with libc.so, but it's an API
> difference that translates into an ABI difference between third-party
> translation units if they use max_align_t as you've described. Whether
> we care, I'm not sure. At least historically there were other cases
> like this in musl where type sizes differed in ways that didn't affect
> libc ABI but did affect ABI between third-party programs (jmp_buf
> comes to mind) but most if not all of these were changed.
>
> I'm not strongly opposed to imposing the 8-byte alignment requirement
> on malloc (it would be hard to make malloc work on smaller granularity
> anyway, and most archs need 8-byte alignment anyway for long long and
> for double) but I generally dislike the inconsistency of defining
> max_align_t in a semi-absurd way on i386, as well as the issue of
> having to use non-portable definitions and/or arch-specific ones.

I agree. The question remains, though, whether the consequences of
defining max_align_t differently are acceptable.

> BTW in your above example, it's not even clear to me if that use of
> alignas is valid. It makes no sense for an object to have an alignment
> that does not divide its type (imagine if you added [2] to the end of
> bar) and in other places (like the contract of aligned_alloc)
> alignments that do not divide the size are explicitly illegal. I'd
> like to understand this before making a decision.

6.7.5 doesn't mention such requirement. _Alignas, obviously, cannot
reduce the alignment requirement and the specified alignment has to
has to be either a valid fundamental alignment or valid extended
alignment supported by the implementation. Moreover, 6.2.8 requires
that valid alignment is a nonnegative integral power of two. As for
the additional requirement in contract of aligned_alloc 7.22.3.1
states that the requested alignment has to be valid and divide size of
the requested memory block. I don't see how that would disallow using
in alignas alignment larger than the size of the object.

Paweł


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-08 19:45           ` Paweł Dziepak
@ 2014-05-08 20:02             ` Rich Felker
  2014-05-08 20:45               ` Paweł Dziepak
  0 siblings, 1 reply; 28+ messages in thread
From: Rich Felker @ 2014-05-08 20:02 UTC (permalink / raw)
  To: Paweł Dziepak; +Cc: musl

On Thu, May 08, 2014 at 09:45:39PM +0200, Paweł Dziepak wrote:
> 6.7.5 doesn't mention such requirement. _Alignas, obviously, cannot
> reduce the alignment requirement and the specified alignment has to
> has to be either a valid fundamental alignment or valid extended
> alignment supported by the implementation. Moreover, 6.2.8 requires
> that valid alignment is a nonnegative integral power of two. As for
> the additional requirement in contract of aligned_alloc 7.22.3.1
> states that the requested alignment has to be valid and divide size of
> the requested memory block. I don't see how that would disallow using
> in alignas alignment larger than the size of the object.

The alignment of a type must divide its size; this is fundamental to
the existence of arrays. It's possible that, for an ugly definition of
"alignment of an object" independent of an alignment associated with
the type, some objects could be aligned with more alignment than their
size, but I'm not convinced that the standard intends to allow such
nonsense. My point about aligned_alloc was that its interface
requirements reflect the notion that alignment always divides size.

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-08 20:02             ` Rich Felker
@ 2014-05-08 20:45               ` Paweł Dziepak
  0 siblings, 0 replies; 28+ messages in thread
From: Paweł Dziepak @ 2014-05-08 20:45 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

2014-05-08 22:02 GMT+02:00 Rich Felker <dalias@libc.org>:
> On Thu, May 08, 2014 at 09:45:39PM +0200, Paweł Dziepak wrote:
>> 6.7.5 doesn't mention such requirement. _Alignas, obviously, cannot
>> reduce the alignment requirement and the specified alignment has to
>> has to be either a valid fundamental alignment or valid extended
>> alignment supported by the implementation. Moreover, 6.2.8 requires
>> that valid alignment is a nonnegative integral power of two. As for
>> the additional requirement in contract of aligned_alloc 7.22.3.1
>> states that the requested alignment has to be valid and divide size of
>> the requested memory block. I don't see how that would disallow using
>> in alignas alignment larger than the size of the object.
>
> The alignment of a type must divide its size; this is fundamental to
> the existence of arrays. It's possible that, for an ugly definition of
> "alignment of an object" independent of an alignment associated with
> the type, some objects could be aligned with more alignment than their
> size, but I'm not convinced that the standard intends to allow such
> nonsense. My point about aligned_alloc was that its interface
> requirements reflect the notion that alignment always divides size.

I'm not sure how alignas() could cause problems with arrays. It cannot
be used in declarations of typedefs. When applied to structure member
or the whole structure (like struct alignas(N) foo { ... }, C++11
alows it) it changes the size of the structure itself so the alignment
remains less than or equal size of the object and in declarations like
this:

alignas(N) T foo[K];

alignas() applies to the array foo, not each individual element of that array.

Paweł


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-04 11:42     ` Paweł Dziepak
@ 2014-05-07  5:02       ` Jens Gustedt
  0 siblings, 0 replies; 28+ messages in thread
From: Jens Gustedt @ 2014-05-07  5:02 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

Hello,

Am Sonntag, den 04.05.2014, 13:42 +0200 schrieb Paweł Dziepak:
> The behavior in GCC 4.9 has changed. _Alignof(long long) now is always
> 4. _Alignof(max_align_t) remains 8 though.

just to be clear, is it that _Alignof(max_align_t) is 8 for their
version of max_align_t or for your version.

Rich is correct, if it would be for your version, this would be a
bug.

But if it would be for their version, this would be just a unilateral
decision about the API they have taken, and musl should then mimic
that behavior. The idea behind that might be that they consider that
some special type(s) that are C extensions have "usual allignment".

Jens

-- 
:: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/   ::
:: AlGorille ::::::::::::::: office Nancy : +33 383593090   ::
:: ICube :::::::::::::: office Strasbourg : +33 368854536   ::
:: ::::::::::::::::::::::::::: gsm France : +33 651400183   ::
:: :::::::::::::::::::: gsm international : +49 15737185122 ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-04-30 22:43   ` Rich Felker
  2014-05-04  2:52     ` Paweł Dziepak
@ 2014-05-04 11:42     ` Paweł Dziepak
  2014-05-07  5:02       ` Jens Gustedt
  1 sibling, 1 reply; 28+ messages in thread
From: Paweł Dziepak @ 2014-05-04 11:42 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

2014-05-01 0:43 GMT+02:00 Rich Felker <dalias@libc.org>:
> On Wed, Apr 30, 2014 at 11:42:51PM +0200, Szabolcs Nagy wrote:
>> * Pawel Dziepak <pdziepak@quarnos.org> [2014-04-30 22:23:01 +0200]:
>> >
>> > +TYPEDEF union { long double ld; long long ll; } max_align_t;
>>
>> this is wrong
>>
>> - ld and ll identifiers are not reserved for the implementation
>> (you could name them _ld, _ll or __ld, __ll etc)
>
> It's also not clear to me why this should go in alltypes.h. Unless it
> needs to be arch-specific, it could go directly in stddef.h.
>
>> and see previous max_align_t discussion
>> http://www.openwall.com/lists/musl/2014/04/28/8
>>
>> - compiler implementations are non-conforming on some platforms
>> (_Alignof returns inconsistent results for the same object type so
>> reasoning about alignments is problematic, there are exceptions
>> where this is allowed in c++11 but not in c11)
>
> Is there a bug filed against gcc yet?

The behavior in GCC 4.9 has changed. _Alignof(long long) now is always
4. _Alignof(max_align_t) remains 8 though. Because of this, the
solution I proposed in earlier post doesn't work anymore
(_Alignof(max_align_t) would be 4 if GCC 4.9 didn't complain that
_Alignas(long long) reduces alignment of long long which is weird but
probably doesn't matters much in this discussion) and I looks like the
only option is to use __attribute__((__aligned__(...))). I don't think
there is reason for me to send another version of this patch since
there has already been sent a patch which defines max_align_t in such
way.

Paweł


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-05-04  2:36   ` Paweł Dziepak
@ 2014-05-04  5:02     ` Rich Felker
  0 siblings, 0 replies; 28+ messages in thread
From: Rich Felker @ 2014-05-04  5:02 UTC (permalink / raw)
  To: musl

On Sun, May 04, 2014 at 04:36:03AM +0200, Paweł Dziepak wrote:
> 2014-04-30 23:42 GMT+02:00 Szabolcs Nagy <nsz@port70.net>:
> > * Pawel Dziepak <pdziepak@quarnos.org> [2014-04-30 22:23:01 +0200]:
> >>
> >> +TYPEDEF union { long double ld; long long ll; } max_align_t;
> >
> > this is wrong
> >
> > - ld and ll identifiers are not reserved for the implementation
> > (you could name them _ld, _ll or __ld, __ll etc)
> 
> I will fix that. However, I must admit I don't see why members of the
> union (or struct) have to use identifiers reserved for the
> implementation. It's not like they can conflict with anything, isn't
> it?

#define ll 0
#include <stddef.h>

> > and see previous max_align_t discussion
> > http://www.openwall.com/lists/musl/2014/04/28/8
> >
> > - compiler implementations are non-conforming on some platforms
> > (_Alignof returns inconsistent results for the same object type so
> > reasoning about alignments is problematic, there are exceptions
> > where this is allowed in c++11 but not in c11)
> >
> > - max_align_t is part of the abi and your solution is incompatible
> > with gcc and clang (your definition gives 4 byte _Alignof(max_align_t)
> > on i386 instead of 8)
> 
> The behavior of _Alignof on x86 is indeed quite surprising. I actually

It's also wrong. The correct alignment for max_align_t on i386 is 4,
not 8. It's a bug that GCC ever returns 8 for alignof on i386. We
really need to file a bug against GCC and explain this clearly,
because I have a feeling they're going to be opposed to fixing it...

> don't see why 8 is the right value and 4 isn't - System V ABI for x86
> doesn't mention any type with alignment 8. Anyway, I agree that it

You're completely right; GCC is wrong.

> would be a good thing to mach the definition gcc and clang use, i.e.
> something like that:
> 
> union max_align_t {
>     alignas(long long) long long _ll;
>     alignas(long double) long double _ld;
> };

This should not give results different from omitting the "alignas".
The only reason it does give different results is a bug in GCC, so we
should not be copying this confusing mess that's a no-op for a correct
compiler. (Applying alignas(T) to type T is always a no-op.)

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-04-30 22:43   ` Rich Felker
@ 2014-05-04  2:52     ` Paweł Dziepak
  2014-05-04 11:42     ` Paweł Dziepak
  1 sibling, 0 replies; 28+ messages in thread
From: Paweł Dziepak @ 2014-05-04  2:52 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

2014-05-01 0:43 GMT+02:00 Rich Felker <dalias@libc.org>:
> On Wed, Apr 30, 2014 at 11:42:51PM +0200, Szabolcs Nagy wrote:
>> * Pawel Dziepak <pdziepak@quarnos.org> [2014-04-30 22:23:01 +0200]:
>> >
>> > +TYPEDEF union { long double ld; long long ll; } max_align_t;
>>
>> this is wrong
>>
>> - ld and ll identifiers are not reserved for the implementation
>> (you could name them _ld, _ll or __ld, __ll etc)
>
> It's also not clear to me why this should go in alltypes.h. Unless it
> needs to be arch-specific, it could go directly in stddef.h.

I thought it would be better if types required by the standard are
defined in one file. It doesn't really look like alltypes.h is
reserved for arch-specific types. On the other hand, there indeed is
not any reason not to put that definition directly in stddef.h.

Paweł


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-04-30 21:42 ` Szabolcs Nagy
  2014-04-30 22:43   ` Rich Felker
@ 2014-05-04  2:36   ` Paweł Dziepak
  2014-05-04  5:02     ` Rich Felker
  1 sibling, 1 reply; 28+ messages in thread
From: Paweł Dziepak @ 2014-05-04  2:36 UTC (permalink / raw)
  To: Szabolcs Nagy; +Cc: musl

2014-04-30 23:42 GMT+02:00 Szabolcs Nagy <nsz@port70.net>:
> * Pawel Dziepak <pdziepak@quarnos.org> [2014-04-30 22:23:01 +0200]:
>>
>> +TYPEDEF union { long double ld; long long ll; } max_align_t;
>
> this is wrong
>
> - ld and ll identifiers are not reserved for the implementation
> (you could name them _ld, _ll or __ld, __ll etc)

I will fix that. However, I must admit I don't see why members of the
union (or struct) have to use identifiers reserved for the
implementation. It's not like they can conflict with anything, isn't
it?

> and see previous max_align_t discussion
> http://www.openwall.com/lists/musl/2014/04/28/8
>
> - compiler implementations are non-conforming on some platforms
> (_Alignof returns inconsistent results for the same object type so
> reasoning about alignments is problematic, there are exceptions
> where this is allowed in c++11 but not in c11)
>
> - max_align_t is part of the abi and your solution is incompatible
> with gcc and clang (your definition gives 4 byte _Alignof(max_align_t)
> on i386 instead of 8)

The behavior of _Alignof on x86 is indeed quite surprising. I actually
don't see why 8 is the right value and 4 isn't - System V ABI for x86
doesn't mention any type with alignment 8. Anyway, I agree that it
would be a good thing to mach the definition gcc and clang use, i.e.
something like that:

union max_align_t {
    alignas(long long) long long _ll;
    alignas(long double) long double _ld;
};

> there is probably not much choice and musl will have to copy the
> silly definition used in gcc/clang making max_align_t not very
> useful (it does not reflect malloc alignment supported by the libc
> nor the object alignments supported by the compiler)

Well, alignments supported by the compiler may be different from the
alignments supported by the libc and that depends on how the
implementation supports extended alignments. max_align_t specifies the
greatest fundamental alignment which is guaranteed to be supported in
all contexts (i.e. it's at least as strict as the strictest alignment
required by fundamental types).

Paweł


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-04-30 21:42 ` Szabolcs Nagy
@ 2014-04-30 22:43   ` Rich Felker
  2014-05-04  2:52     ` Paweł Dziepak
  2014-05-04 11:42     ` Paweł Dziepak
  2014-05-04  2:36   ` Paweł Dziepak
  1 sibling, 2 replies; 28+ messages in thread
From: Rich Felker @ 2014-04-30 22:43 UTC (permalink / raw)
  To: musl; +Cc: Pawel Dziepak

On Wed, Apr 30, 2014 at 11:42:51PM +0200, Szabolcs Nagy wrote:
> * Pawel Dziepak <pdziepak@quarnos.org> [2014-04-30 22:23:01 +0200]:
> >  
> > +TYPEDEF union { long double ld; long long ll; } max_align_t;
> 
> this is wrong
> 
> - ld and ll identifiers are not reserved for the implementation
> (you could name them _ld, _ll or __ld, __ll etc)

It's also not clear to me why this should go in alltypes.h. Unless it
needs to be arch-specific, it could go directly in stddef.h.

> and see previous max_align_t discussion
> http://www.openwall.com/lists/musl/2014/04/28/8
> 
> - compiler implementations are non-conforming on some platforms
> (_Alignof returns inconsistent results for the same object type so
> reasoning about alignments is problematic, there are exceptions
> where this is allowed in c++11 but not in c11)

Is there a bug filed against gcc yet?

> - max_align_t is part of the abi and your solution is incompatible
> with gcc and clang (your definition gives 4 byte _Alignof(max_align_t)
> on i386 instead of 8)

This is probably not very important, but I agree it's desirable to be
consistent.

> there is probably not much choice and musl will have to copy the
> silly definition used in gcc/clang making max_align_t not very
> useful (it does not reflect malloc alignment supported by the libc
> nor the object alignments supported by the compiler)

 The definition of max_align_t is very confusing:

    "an object type whose alignment is as great as is supported by the
    implementation in all contexts"

But as far as I can tell, malloc has nothing to do with max_align_t;
the latter only pertains to observable behavior, and the alignment
malloc produces is not observable. The fact that our malloc produces
addresses which are multiples of 16 (or 32) as an implementation
detail does not mean that alignments up to 16 (or 32) are "supported
by the implementation" in this context; they just happen to work. As
its public interface contract, malloc only guarantees sufficient
alignment for types not produced with _Alignas (or similar GNU C
attribute usage).

Morally, max_align_t should be an object whose alignment requirement
is equal to the max alignment requirement for all types that don't
involve _Alignas.

Rich


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] add definition of max_align_t to stddef.h
  2014-04-30 20:23 Pawel Dziepak
@ 2014-04-30 21:42 ` Szabolcs Nagy
  2014-04-30 22:43   ` Rich Felker
  2014-05-04  2:36   ` Paweł Dziepak
  0 siblings, 2 replies; 28+ messages in thread
From: Szabolcs Nagy @ 2014-04-30 21:42 UTC (permalink / raw)
  To: musl; +Cc: Pawel Dziepak

* Pawel Dziepak <pdziepak@quarnos.org> [2014-04-30 22:23:01 +0200]:
>  
> +TYPEDEF union { long double ld; long long ll; } max_align_t;

this is wrong

- ld and ll identifiers are not reserved for the implementation
(you could name them _ld, _ll or __ld, __ll etc)

and see previous max_align_t discussion
http://www.openwall.com/lists/musl/2014/04/28/8

- compiler implementations are non-conforming on some platforms
(_Alignof returns inconsistent results for the same object type so
reasoning about alignments is problematic, there are exceptions
where this is allowed in c++11 but not in c11)

- max_align_t is part of the abi and your solution is incompatible
with gcc and clang (your definition gives 4 byte _Alignof(max_align_t)
on i386 instead of 8)

there is probably not much choice and musl will have to copy the
silly definition used in gcc/clang making max_align_t not very
useful (it does not reflect malloc alignment supported by the libc
nor the object alignments supported by the compiler)


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH] add definition of max_align_t to stddef.h
@ 2014-04-30 20:23 Pawel Dziepak
  2014-04-30 21:42 ` Szabolcs Nagy
  0 siblings, 1 reply; 28+ messages in thread
From: Pawel Dziepak @ 2014-04-30 20:23 UTC (permalink / raw)
  To: musl; +Cc: Pawel Dziepak

max_align_t is an object type with the strictest alignment supported by
the implementation in all contexts. An union is used to "choose" the greater
of the two greatest scalar types: long long and long double.

Signed-off-by: Pawel Dziepak <pdziepak@quarnos.org>
---
 include/alltypes.h.in | 2 ++
 include/stddef.h      | 1 +
 2 files changed, 3 insertions(+)

diff --git a/include/alltypes.h.in b/include/alltypes.h.in
index c4ca5d5..ec3873f 100644
--- a/include/alltypes.h.in
+++ b/include/alltypes.h.in
@@ -18,6 +18,8 @@ TYPEDEF unsigned _Int64 uint64_t;
 TYPEDEF unsigned _Int64 u_int64_t;
 TYPEDEF unsigned _Int64 uintmax_t;
 
+TYPEDEF union { long double ld; long long ll; } max_align_t;
+
 TYPEDEF unsigned mode_t;
 TYPEDEF unsigned _Reg nlink_t;
 TYPEDEF _Int64 off_t;
diff --git a/include/stddef.h b/include/stddef.h
index 0a32919..cab34d9 100644
--- a/include/stddef.h
+++ b/include/stddef.h
@@ -7,6 +7,7 @@
 #define NULL ((void*)0)
 #endif
 
+#define __NEED_max_align_t
 #define __NEED_ptrdiff_t
 #define __NEED_size_t
 #define __NEED_wchar_t
-- 
1.9.0



^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2014-05-08 20:45 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-06 10:35 [PATCH] add definition of max_align_t to stddef.h Paweł Dziepak
2014-05-07  3:13 ` Rich Felker
2014-05-07  4:14   ` Luca Barbato
2014-05-07  4:29     ` Rich Felker
2014-05-07  5:12       ` Luca Barbato
2014-05-07 22:48         ` Rich Felker
2014-05-08 12:07           ` Luca Barbato
2014-05-08 14:25             ` Rich Felker
2014-05-07  9:28   ` Paweł Dziepak
2014-05-07 23:07     ` Rich Felker
2014-05-08 10:57       ` Szabolcs Nagy
2014-05-08 14:11         ` Rich Felker
2014-05-08 16:41       ` Paweł Dziepak
2014-05-08 17:41         ` Rich Felker
2014-05-08 18:45           ` Jens Gustedt
2014-05-08 19:11             ` Paweł Dziepak
2014-05-08 19:22               ` Jens Gustedt
2014-05-08 19:45           ` Paweł Dziepak
2014-05-08 20:02             ` Rich Felker
2014-05-08 20:45               ` Paweł Dziepak
  -- strict thread matches above, loose matches on Subject: below --
2014-04-30 20:23 Pawel Dziepak
2014-04-30 21:42 ` Szabolcs Nagy
2014-04-30 22:43   ` Rich Felker
2014-05-04  2:52     ` Paweł Dziepak
2014-05-04 11:42     ` Paweł Dziepak
2014-05-07  5:02       ` Jens Gustedt
2014-05-04  2:36   ` Paweł Dziepak
2014-05-04  5:02     ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).