mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] Alignment attribute in headers
@ 2024-04-21  3:51 Michael Forney
  2024-04-21  4:54 ` Markus Wichmann
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Michael Forney @ 2024-04-21  3:51 UTC (permalink / raw)
  To: musl

I'm looking at changing headers to use C11 alignment specifiers
when available instead of GNU attributes.

These are used in the following headers:

	arch/loongarch64/bits/signal.h
	arch/powerpc/bits/signal.h
	arch/powerpc/bits/user.h
	arch/powerpc64/bits/signal.h
	arch/powerpc64/bits/user.h
	arch/riscv32/bits/signal.h
	arch/riscv64/bits/signal.h
	arch/x32/bits/shm.h

In some of these cases (powerpc, powerpc64, x32), the attribute is
conditional on __GNUC__, which I think may result in improperly
aligned structs on compilers that don't define this.

For powerpc/powerpc64 user.h, the attribute is applied to a typedef
of a struct, but alignas can't be used with typedef. I think this
could be fixed by moving the attribute/alignment specifier to the
first (and only) struct member instead.

Similarly, x32 shm.h uses an attribute after a struct specifier,
which could be made compatible with an alignment specifier in the
same way.

I also noticed that arch/i386/bits/alltypes.h.in uses _Alignas(8)
for C, __attribute__((__aligned__(8))) for GNU C++, and alignas(8)
for non-GNU C++. Looking through commit history, this seems to be
a work around for a gcc 4.7 bug which claims C++11 support but
doesn't offer alignas.

Do we need to use this same approach for each of the instances above
to handle the three cases (C, GNU C++, non-GNU C++)?

I see that stdalign.h uses _Alignas conditional on C11 support, but
i386 alltypes.h uses it unconditionally on C. Should i386 alltypes.h
use __attribute__ when __STDC_VERSION__ < 201112L?

Something like

#if __STDC_VERSION__ >= 201112L
/* use _Alignas */
#elif defined(__cplusplus) && !defined(__GNUC__)
/* use alignas */
#else
/* use __attribute__((__aligned__(N))) */
#end

?

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

* Re: [musl] Alignment attribute in headers
  2024-04-21  3:51 [musl] Alignment attribute in headers Michael Forney
@ 2024-04-21  4:54 ` Markus Wichmann
  2024-04-21  7:16   ` Jₑₙₛ Gustedt
  2024-04-21 15:50 ` Thorsten Glaser
  2024-04-21 23:48 ` Rich Felker
  2 siblings, 1 reply; 18+ messages in thread
From: Markus Wichmann @ 2024-04-21  4:54 UTC (permalink / raw)
  To: musl

Am Sat, Apr 20, 2024 at 08:51:35PM -0700 schrieb Michael Forney:
> I'm looking at changing headers to use C11 alignment specifiers
> when available instead of GNU attributes.
>
> These are used in the following headers:
>
> 	arch/loongarch64/bits/signal.h
> 	arch/powerpc/bits/signal.h
> 	arch/powerpc/bits/user.h
> 	arch/powerpc64/bits/signal.h
> 	arch/powerpc64/bits/user.h
> 	arch/riscv32/bits/signal.h
> 	arch/riscv64/bits/signal.h
> 	arch/x32/bits/shm.h
>
> In some of these cases (powerpc, powerpc64, x32), the attribute is
> conditional on __GNUC__, which I think may result in improperly
> aligned structs on compilers that don't define this.
>

At least in the case of powerpc, the alignment directives are no-ops
regarding the struct layout. The fields in question are already
correctly aligned in all structures that contain them, all the way up to
ucontext_t. And the only objects of these types that matter usually are
allocated by the kernel on the signal stack, and so the matter is taken
care of there.

There is libucontext, though. It manipulates user-allocated ucontext_ts,
and is therefore dependent on the correct alignment being declared.
Though I don't know if they depend on libc's definitions.

Therefore, and because alignment directives are not portable to all
compilers we target, we should probably insert explicit padding where
necessary, so at least the structure layout is not affected by the
absence of these directives. Because of your prior mail, I can already
report that we need at least
- 12 bytes between uc_sigmask and uc_mcontext in riscv32's ucontext_t
- 8 bytes between uc_sigmask and uc_mcontext in riscv64's ucontext_t

unless I've miscounted.

> Do we need to use this same approach for each of the instances above
> to handle the three cases (C, GNU C++, non-GNU C++)?
>

Normally, public header files need to be compatible with all C and C++
compilers (C89 and C++98), so the cases are
- non-GNU pre-C11 (no support at all)
- GNU-C (__attribute__(__aligned__))
- C11 (_Alignas)
- non-GNU pre-C++11 (no support at all)
- GNU-C++ (__attribute__(__aligned__))
- C++11 (alignas)

We see that non-GNU pre-C11 and non-GNU pre-C++11 fall together, as do
the GNU-C and GNU-C++ cases. And because of the aforementioned bug, we
should probably prefer the GNU-C extension where available, so the logic
comes out to

#ifdef __GNUC__
/* use attribute */
#elif __STDC_VERSION >= 201100L /* I can never remember the month */
/* use _Alignas */
#elif __cplusplus >= 201100L
/* use alignas */
#endif

That should do it.

I think the i386 header file is just wrong.

Ciao,
Markus

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

* Re: [musl] Alignment attribute in headers
  2024-04-21  4:54 ` Markus Wichmann
@ 2024-04-21  7:16   ` Jₑₙₛ Gustedt
  2024-04-21  7:38     ` Markus Wichmann
  0 siblings, 1 reply; 18+ messages in thread
From: Jₑₙₛ Gustedt @ 2024-04-21  7:16 UTC (permalink / raw)
  To: Markus Wichmann; +Cc: musl

Hi,

on Sun, 21 Apr 2024 06:54:28 +0200 you (Markus Wichmann
<nullplan@gmx.net>) wrote:

> …

> #ifdef __GNUC__
> /* use attribute */
> #elif __STDC_VERSION >= 201100L /* I can never remember the month */
> /* use _Alignas */
> #elif __cplusplus >= 201100L
> /* use alignas */
> #endif

Since this is unified starting with C23 and I think we morally should
have C conformance first and fallbacks only if imperatively needed
I would go for

  #if __STDC_VERSION >= 202311L || __cplusplus >= 201100L
  /* use alignas */
  #elif __STDC_VERSION >= 201100L
  /* use _Alignas */
  #elif __GNUC__
  /* use attribute */
  #endif

Jₑₙₛ

-- 
:: ICube :::::::::::::::::::::::::::::: deputy director ::
:: Université de Strasbourg :::::::::::::::::::::: ICPS ::
:: INRIA Nancy Grand Est :::::::::::::::::::::::: Camus ::
:: :::::::::::::::::::::::::::::::::::: ☎ +33 368854536 ::
:: https://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

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

* Re: [musl] Alignment attribute in headers
  2024-04-21  7:16   ` Jₑₙₛ Gustedt
@ 2024-04-21  7:38     ` Markus Wichmann
  2024-04-21 10:23       ` Jₑₙₛ Gustedt
  0 siblings, 1 reply; 18+ messages in thread
From: Markus Wichmann @ 2024-04-21  7:38 UTC (permalink / raw)
  To: musl; +Cc: Jₑₙₛ Gustedt

Am Sun, Apr 21, 2024 at 09:16:05AM +0200 schrieb Jₑₙₛ Gustedt:
> Since this is unified starting with C23 and I think we morally should
> have C conformance first and fallbacks only if imperatively needed
> I would go for
>

Ugh. Let the bike shedding begin. I will tell you that moral arguments
about software don't make a lot of sense to me, though.

>   #if __STDC_VERSION >= 202311L || __cplusplus >= 201100L
>   /* use alignas */
>   #elif __STDC_VERSION >= 201100L
>   /* use _Alignas */
>   #elif __GNUC__
>   /* use attribute */
>   #endif
>

Did you not read the part about the GCC version that claims C11
conformance but doesn't have _Alignas? Yes, that's a bug. Yes, it was
fixed. No, musl can't break compatibility with it.

If not for that, I would have gone with this version too, but that's
like saying "your code would be perfect if it did work".

Ciao,
Markus


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

* Re: [musl] Alignment attribute in headers
  2024-04-21  7:38     ` Markus Wichmann
@ 2024-04-21 10:23       ` Jₑₙₛ Gustedt
  2024-04-21 10:31         ` Sam James
  2024-04-21 17:40         ` Markus Wichmann
  0 siblings, 2 replies; 18+ messages in thread
From: Jₑₙₛ Gustedt @ 2024-04-21 10:23 UTC (permalink / raw)
  To: Markus Wichmann; +Cc: musl

Markus,

on Sun, 21 Apr 2024 09:38:38 +0200 you (Markus Wichmann
<nullplan@gmx.net>) wrote:

> Am Sun, Apr 21, 2024 at 09:16:05AM +0200 schrieb Jₑₙₛ Gustedt:
> > Since this is unified starting with C23 and I think we morally
> > should have C conformance first and fallbacks only if imperatively
> > needed I would go for
> >  
> 
> Ugh. Let the bike shedding begin. I will tell you that moral arguments
> about software don't make a lot of sense to me, though.

I am not a native speaker, but I think this is generally used as
figure of speech for "there got reasons to do something".

> >   #if __STDC_VERSION >= 202311L || __cplusplus >= 201100L
> >   /* use alignas */
> >   #elif __STDC_VERSION >= 201100L
> >   /* use _Alignas */
> >   #elif __GNUC__
> >   /* use attribute */
> >   #endif
> >  
> 
> Did you not read the part about the GCC version that claims C11
> conformance but doesn't have _Alignas? Yes, that's a bug. Yes, it was
> fixed. No, musl can't break compatibility with it.

ah, sorry

then how about

   #if __GNUC__           /* or ifdef if preferable for some reason */
   /* use gcc attribute */
   #elif __STDC_VERSION >= 202311L || __cplusplus >= 201100L
   /* use alignas */
   #elif __STDC_VERSION >= 201100L
   /* use _Alignas */
   #endif


Thanks
Jₑₙₛ

-- 
:: ICube :::::::::::::::::::::::::::::: deputy director ::
:: Université de Strasbourg :::::::::::::::::::::: ICPS ::
:: INRIA Nancy Grand Est :::::::::::::::::::::::: Camus ::
:: :::::::::::::::::::::::::::::::::::: ☎ +33 368854536 ::
:: https://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

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

* Re: [musl] Alignment attribute in headers
  2024-04-21 10:23       ` Jₑₙₛ Gustedt
@ 2024-04-21 10:31         ` Sam James
  2024-04-21 17:40         ` Markus Wichmann
  1 sibling, 0 replies; 18+ messages in thread
From: Sam James @ 2024-04-21 10:31 UTC (permalink / raw)
  To: Jₑₙₛ Gustedt; +Cc: Markus Wichmann, musl

Jₑₙₛ Gustedt <jens.gustedt@inria.fr> writes:

> Markus,
>
> on Sun, 21 Apr 2024 09:38:38 +0200 you (Markus Wichmann
> <nullplan@gmx.net>) wrote:
>
>> Am Sun, Apr 21, 2024 at 09:16:05AM +0200 schrieb Jₑₙₛ Gustedt:
>> > Since this is unified starting with C23 and I think we morally
>> > should have C conformance first and fallbacks only if imperatively
>> > needed I would go for
>> >  
>> 
>> Ugh. Let the bike shedding begin. I will tell you that moral arguments
>> about software don't make a lot of sense to me, though.
>
> I am not a native speaker, but I think this is generally used as
> figure of speech for "there got reasons to do something".

I understood what you meant and it's an idiom I've seen before. Don't
worry.


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

* Re: [musl] Alignment attribute in headers
  2024-04-21  3:51 [musl] Alignment attribute in headers Michael Forney
  2024-04-21  4:54 ` Markus Wichmann
@ 2024-04-21 15:50 ` Thorsten Glaser
  2024-04-21 16:44   ` Alexander Monakov
  2024-04-21 17:20   ` Markus Wichmann
  2024-04-21 23:48 ` Rich Felker
  2 siblings, 2 replies; 18+ messages in thread
From: Thorsten Glaser @ 2024-04-21 15:50 UTC (permalink / raw)
  To: musl

Michael Forney dixit:

>Something like
>
>#if __STDC_VERSION__ >= 201112L
>/* use _Alignas */
>#elif defined(__cplusplus) && !defined(__GNUC__)
>/* use alignas */
>#else
>/* use __attribute__((__aligned__(N))) */
>#end

Something I noticed recently while doing m68k alignment work:

The C++ alignas is UB if the specified alignment is smaller
than what the structure would normally have, so adding cautious
alignments can explode in one’s face. ☹ Not only is this really
stupid, but makes it not generally usable, too.

GCC’s attribute, in contrast, (without __packed__) just gets
ignored for those cases.

I haven’t looked at the C11 one.

bye,
//mirabilos
-- 
“It is inappropriate to require that a time represented as
 seconds since the Epoch precisely represent the number of
 seconds between the referenced time and the Epoch.”
	-- IEEE Std 1003.1b-1993 (POSIX) Section B.2.2.2

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

* Re: [musl] Alignment attribute in headers
  2024-04-21 15:50 ` Thorsten Glaser
@ 2024-04-21 16:44   ` Alexander Monakov
  2024-04-21 17:20   ` Markus Wichmann
  1 sibling, 0 replies; 18+ messages in thread
From: Alexander Monakov @ 2024-04-21 16:44 UTC (permalink / raw)
  To: musl

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


On Sun, 21 Apr 2024, Thorsten Glaser wrote:

> Michael Forney dixit:
> 
> >Something like
> >
> >#if __STDC_VERSION__ >= 201112L
> >/* use _Alignas */
> >#elif defined(__cplusplus) && !defined(__GNUC__)
> >/* use alignas */
> >#else
> >/* use __attribute__((__aligned__(N))) */
> >#end
> 
> Something I noticed recently while doing m68k alignment work:
> 
> The C++ alignas is UB if the specified alignment is smaller
> than what the structure would normally have, so adding cautious
> alignments can explode in one’s face. ☹ Not only is this really
> stupid, but makes it not generally usable, too.
> 
> GCC’s attribute, in contrast, (without __packed__) just gets
> ignored for those cases.

This is inaccurate: you can use the attribute to decrease alignment
of scalar types (but you do need __packed__ when attaching the attribute
to a struct). As I recall, GCC documentation used to be misleading
or wrong about this, but the current wording is fairly clear to me:

    When used on a struct, or struct member, the aligned attribute can
    only increase the alignment; in order to decrease it, the packed
    attribute must be specified as well. When used as part of a typedef,
    the aligned attribute can both increase and decrease alignment, and
    specifying the packed attribute generates a warning. 

    https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html

Alexander

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

* Re: [musl] Alignment attribute in headers
  2024-04-21 15:50 ` Thorsten Glaser
  2024-04-21 16:44   ` Alexander Monakov
@ 2024-04-21 17:20   ` Markus Wichmann
  2024-04-21 18:23     ` Thorsten Glaser
  2024-04-21 19:04     ` Michael Forney
  1 sibling, 2 replies; 18+ messages in thread
From: Markus Wichmann @ 2024-04-21 17:20 UTC (permalink / raw)
  To: musl

Am Sun, Apr 21, 2024 at 03:50:31PM +0000 schrieb Thorsten Glaser:
> I haven’t looked at the C11 one.

C11's _Alignas can only raise alignment, not lower it. Alignment
specifications with a lower number than the field already has are
ignored. I can't believe the C++ guys screwed up so hard as to make
lower alignment UB.

Ciao,
Markus

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

* Re: [musl] Alignment attribute in headers
  2024-04-21 10:23       ` Jₑₙₛ Gustedt
  2024-04-21 10:31         ` Sam James
@ 2024-04-21 17:40         ` Markus Wichmann
  1 sibling, 0 replies; 18+ messages in thread
From: Markus Wichmann @ 2024-04-21 17:40 UTC (permalink / raw)
  To: musl

Am Sun, Apr 21, 2024 at 12:23:58PM +0200 schrieb Jₑₙₛ Gustedt:
> Markus,
>
> on Sun, 21 Apr 2024 09:38:38 +0200 you (Markus Wichmann
> <nullplan@gmx.net>) wrote:
>
> > Am Sun, Apr 21, 2024 at 09:16:05AM +0200 schrieb Jₑₙₛ Gustedt:
> > > Since this is unified starting with C23 and I think we morally
> > > should have C conformance first and fallbacks only if imperatively
> > > needed I would go for
> > >
> >
> > Ugh. Let the bike shedding begin. I will tell you that moral arguments
> > about software don't make a lot of sense to me, though.
>
> I am not a native speaker,

Me neither, but I do spend an embarassing amount of time on YouTube.

> but I think this is generally used as
> figure of speech for "there got reasons to do something".
>

I have only ever heard this phrase in technical discussions when the
pundit had run out of technical arguments. And usually only for personal
preferences. I personally also prefer standard solutions over
implementation-specific hacks (like the attribute syntax), but I cannot
blind myself to the requirements of the implementation.

Plus, I dislike conditional inclusion in general, but in this case, we
do require all three versions to be present anyway and are merely
arguing about precedence. Well, it won't make any difference to a human
reader, as they will have to process all seven lines anyway, and the
compiler doesn't care for any line except the one that does get included
in the end. I would much prefer to dispense with the entire attribute,
but that would break ABI. So the least bad form of this very bad code
would be my proposal.

BTW, in case it got missed earlier: The explicit padding is still
necessary for compat with non-GNU pre-C11 compilers! At least the layout
will be compatible this way.

Ciao,
Markus

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

* Re: [musl] Alignment attribute in headers
  2024-04-21 17:20   ` Markus Wichmann
@ 2024-04-21 18:23     ` Thorsten Glaser
  2024-04-21 19:04     ` Michael Forney
  1 sibling, 0 replies; 18+ messages in thread
From: Thorsten Glaser @ 2024-04-21 18:23 UTC (permalink / raw)
  To: musl

Markus Wichmann dixit:

>Am Sun, Apr 21, 2024 at 03:50:31PM +0000 schrieb Thorsten Glaser:
>> I haven’t looked at the C11 one.
>
>C11's _Alignas can only raise alignment, not lower it. Alignment
>specifications with a lower number than the field already has are
>ignored.

That’s sensible, and all I want when throwing in extra (mostly
32-bit, but also some 64-bit) alignments there (m68k does not
use “natural” alignment in its ABI but aligns to a max. of 16
bits, which comes from early design; in Debian, we’ll want to
change the ABI for Linux executables, but are not started yet,
but e.g. TOS/MiNT executables including bootloaders need to
stick with that).

>I can't believe the C++ guys screwed up so hard as to make
>lower alignment UB.

I “just” looked at cppreference.com so don’t take this as
definite but someone who has the standard might want to
look into that.

bye,
//mirabilos
-- 
15:41⎜<Lo-lan-do:#fusionforge> Somebody write a testsuite for helloworld :-)

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

* Re: [musl] Alignment attribute in headers
  2024-04-21 17:20   ` Markus Wichmann
  2024-04-21 18:23     ` Thorsten Glaser
@ 2024-04-21 19:04     ` Michael Forney
  1 sibling, 0 replies; 18+ messages in thread
From: Michael Forney @ 2024-04-21 19:04 UTC (permalink / raw)
  To: musl

Markus Wichmann <nullplan@gmx.net> wrote:
> Am Sun, Apr 21, 2024 at 03:50:31PM +0000 schrieb Thorsten Glaser:
> > I haven’t looked at the C11 one.
> 
> C11's _Alignas can only raise alignment, not lower it. Alignment
> specifications with a lower number than the field already has are
> ignored. I can't believe the C++ guys screwed up so hard as to make
> lower alignment UB.

It's true that _Alignas can't lower alignment, but it's not ignored
if you specify a weaker alignment, it's a constraint violation:

C17 6.7.5p5:
> The combined effect of all alignment specifiers in a declaration
> shall not specify an alignment that is less strict than the
> alignment that would otherwise be required for the type of the
> object or member being declared.

This doesn't matter for the proposed alignment specifiers since
they are in architecture-specific headers where we know that the
specified alignment is not less than is required for the type.

However, in general, I think Thorsten's concern is valid. If you
specify some alignment that your application needs, but the
implementation already requires a stricter alignment, your program
has an error.

You could work around it by specifying two alignment specifiers:

	_Alignas(N) _Alignas(T) T x;

But anyway, this has veered a bit off-topic.

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

* Re: [musl] Alignment attribute in headers
  2024-04-21  3:51 [musl] Alignment attribute in headers Michael Forney
  2024-04-21  4:54 ` Markus Wichmann
  2024-04-21 15:50 ` Thorsten Glaser
@ 2024-04-21 23:48 ` Rich Felker
  2024-04-24  0:45   ` Michael Forney
  2 siblings, 1 reply; 18+ messages in thread
From: Rich Felker @ 2024-04-21 23:48 UTC (permalink / raw)
  To: Michael Forney; +Cc: musl

On Sat, Apr 20, 2024 at 08:51:35PM -0700, Michael Forney wrote:
> I'm looking at changing headers to use C11 alignment specifiers
> when available instead of GNU attributes.
> 
> These are used in the following headers:
> 
> 	arch/loongarch64/bits/signal.h
> 	arch/powerpc/bits/signal.h
> 	arch/powerpc/bits/user.h
> 	arch/powerpc64/bits/signal.h
> 	arch/powerpc64/bits/user.h
> 	arch/riscv32/bits/signal.h
> 	arch/riscv64/bits/signal.h
> 	arch/x32/bits/shm.h
> 
> In some of these cases (powerpc, powerpc64, x32), the attribute is
> conditional on __GNUC__, which I think may result in improperly
> aligned structs on compilers that don't define this.
> 
> For powerpc/powerpc64 user.h, the attribute is applied to a typedef
> of a struct, but alignas can't be used with typedef. I think this
> could be fixed by moving the attribute/alignment specifier to the
> first (and only) struct member instead.
> 
> Similarly, x32 shm.h uses an attribute after a struct specifier,
> which could be made compatible with an alignment specifier in the
> same way.

Most of these I didn't remember, and were likely added begrudgingly.
There's really no point in having alignment specifiers on the
sigcontext/mcontext stuff, as it's kernel-allocated. If there are
places where explicit padding to match the layout is missing, these
are bugs and should be corrected. I tried to get explicit padding put
everywhere, asking folks doing new ports to add it (or refrain from
deleting it), but it's possible some places were missed. Let's fix
them.

For all of this, I think it's perfectly acceptable to just use the GNU
C attribute and condition it on __GNUC__. Once any missing padding is
fixed, the alignment attribute doesn't really matter.

> I also noticed that arch/i386/bits/alltypes.h.in uses _Alignas(8)
> for C, __attribute__((__aligned__(8))) for GNU C++, and alignas(8)
> for non-GNU C++. Looking through commit history, this seems to be
> a work around for a gcc 4.7 bug which claims C++11 support but
> doesn't offer alignas.

Note that max_align_t is only defined for C11 or C++11 and up, so all
of the branches of this conditional are assuming one of those
conditions is already true.

> Do we need to use this same approach for each of the instances above
> to handle the three cases (C, GNU C++, non-GNU C++)?
> 
> I see that stdalign.h uses _Alignas conditional on C11 support, but
> i386 alltypes.h uses it unconditionally on C. Should i386 alltypes.h
> use __attribute__ when __STDC_VERSION__ < 201112L?

This is sketchy; the header really should not be used at all with
pre-C11. I don't recall why we added it. Maybe to encourage use of the
stdalign.h and alignas over the GNU attribute if autoconf detects it's
available? I wouldn't be strongly opposed to removing the macro
definition for pre-C11.

> Something like
> 
> #if __STDC_VERSION__ >= 201112L
> /* use _Alignas */
> #elif defined(__cplusplus) && !defined(__GNUC__)
> /* use alignas */
> #else
> /* use __attribute__((__aligned__(N))) */
> #end

For arch-specific bits where the GNUC attribute can be considered part
of the psABI requirements, while I don't like this, my leaning is that
it's fine to use it directly and ignore the C11 and C++11 versions.

I think public interfaces that are not arch-specific should avoid
depending on it, but the only one of these is sys/fanotify.h, which is
very much a Linuxism feature and it doesn't seem that bad for it to
fail on weird compilers.

So I'm not sure any of this is much more than a bikeshed topic...

Rich

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

* Re: [musl] Alignment attribute in headers
  2024-04-21 23:48 ` Rich Felker
@ 2024-04-24  0:45   ` Michael Forney
  2024-04-24  2:54     ` Markus Wichmann
  2024-04-24  7:39     ` Jon Chesterfield
  0 siblings, 2 replies; 18+ messages in thread
From: Michael Forney @ 2024-04-24  0:45 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

Rich Felker <dalias@libc.org> wrote:
> Most of these I didn't remember, and were likely added begrudgingly.
> There's really no point in having alignment specifiers on the
> sigcontext/mcontext stuff, as it's kernel-allocated. If there are
> places where explicit padding to match the layout is missing, these
> are bugs and should be corrected. I tried to get explicit padding put
> everywhere, asking folks doing new ports to add it (or refrain from
> deleting it), but it's possible some places were missed. Let's fix
> them.

I checked the other instances, and I think the ones that Markus
identified (riscv32/riscv64 bits/signal.h) are the only ones that
are missing padding. I can prepare a patch if Markus doesn't plan
to.

> For all of this, I think it's perfectly acceptable to just use the GNU
> C attribute and condition it on __GNUC__. Once any missing padding is
> fixed, the alignment attribute doesn't really matter.

If there's really no point to the alignment specifiers for mcontext
sub-structures, should they just be removed completely?

If there is a point to the alignment specifiers, I think the structs
should be defined in such a way that their alignment doesn't depend
on the compiler you used.

My worry is that some application might use these structs as fields
inside their own structs, and save data from a signal handler in
them. Without the alignment specifier, they could potentially have
incorrect offsets, breaking ABI compatibility between GNU C and
non-GNU C compilers.

For x32 struct shm_info, the structure is allocated by the application.
Even if the kernel doesn't care when populating it, it seems plausible
to me that it might be part of a larger struct.

> > Something like
> > 
> > #if __STDC_VERSION__ >= 201112L
> > /* use _Alignas */
> > #elif defined(__cplusplus) && !defined(__GNUC__)
> > /* use alignas */
> > #else
> > /* use __attribute__((__aligned__(N))) */
> > #end
> 
> For arch-specific bits where the GNUC attribute can be considered part
> of the psABI requirements, while I don't like this, my leaning is that
> it's fine to use it directly and ignore the C11 and C++11 versions.
>
> I think public interfaces that are not arch-specific should avoid
> depending on it, but the only one of these is sys/fanotify.h, which is
> very much a Linuxism feature and it doesn't seem that bad for it to
> fail on weird compilers.

Why ignore the C11/C++11 versions? I know it's somewhat annoying
to handle the different cases, but that seems better to me than to
sacrifice correctness on compilers that don't support the attribute.

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

* Re: [musl] Alignment attribute in headers
  2024-04-24  0:45   ` Michael Forney
@ 2024-04-24  2:54     ` Markus Wichmann
  2024-04-24  7:39     ` Jon Chesterfield
  1 sibling, 0 replies; 18+ messages in thread
From: Markus Wichmann @ 2024-04-24  2:54 UTC (permalink / raw)
  To: musl

Am Tue, Apr 23, 2024 at 05:45:31PM -0700 schrieb Michael Forney:
> Rich Felker <dalias@libc.org> wrote:
> > Most of these I didn't remember, and were likely added begrudgingly.
> > There's really no point in having alignment specifiers on the
> > sigcontext/mcontext stuff, as it's kernel-allocated. If there are
> > places where explicit padding to match the layout is missing, these
> > are bugs and should be corrected. I tried to get explicit padding put
> > everywhere, asking folks doing new ports to add it (or refrain from
> > deleting it), but it's possible some places were missed. Let's fix
> > them.
>
> I checked the other instances, and I think the ones that Markus
> identified (riscv32/riscv64 bits/signal.h) are the only ones that
> are missing padding. I can prepare a patch if Markus doesn't plan
> to.
>

I don't plan to.

> > For all of this, I think it's perfectly acceptable to just use the GNU
> > C attribute and condition it on __GNUC__. Once any missing padding is
> > fixed, the alignment attribute doesn't really matter.
>
> If there's really no point to the alignment specifiers for mcontext
> sub-structures, should they just be removed completely?
>
> If there is a point to the alignment specifiers, I think the structs
> should be defined in such a way that their alignment doesn't depend
> on the compiler you used.
>

My worry is that someone is using something like libucontext,
manipulating ucontexts directly, and allocating them in the static data
section or on the stack. In that case, the alignment is important, or
else the getcontext()/setcontext() call will fail because the vector
register access will fail. A few ABIs have vector registers as preserved
registers (PowerPC with Altivec comes to mind).

Failure would be fine if it had just never worked, but we already have
versions out there that work right now. And updating your libc only to
find new unexplained crashes is not really a good way to spend a work
day.

> My worry is that some application might use these structs as fields
> inside their own structs, and save data from a signal handler in
> them. Without the alignment specifier, they could potentially have
> incorrect offsets, breaking ABI compatibility between GNU C and
> non-GNU C compilers.
>

That is also a good point.

Ciao,
Markus

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

* Re: [musl] Alignment attribute in headers
  2024-04-24  0:45   ` Michael Forney
  2024-04-24  2:54     ` Markus Wichmann
@ 2024-04-24  7:39     ` Jon Chesterfield
  2024-04-24  7:55       ` Jeffrey Walton
  1 sibling, 1 reply; 18+ messages in thread
From: Jon Chesterfield @ 2024-04-24  7:39 UTC (permalink / raw)
  To: musl

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

Re testing GNUC,

I'm not sure the macro means "targeting Linux", and it seems totally
legitimate that a C compiler which doesn't implement any GNU extensions
would not define that macro. Musl is quite a likely choice for a non-gnu
compiler that wants to compile code to run against the Linux kernel.

> only one of these is sys/fanotify.h, which is
> very much a Linuxism feature and it doesn't seem that bad for it to
> fail on weird compilers.

[-- Attachment #2: Type: text/html, Size: 841 bytes --]

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

* Re: [musl] Alignment attribute in headers
  2024-04-24  7:39     ` Jon Chesterfield
@ 2024-04-24  7:55       ` Jeffrey Walton
  2024-04-24  9:49         ` Jon Chesterfield
  0 siblings, 1 reply; 18+ messages in thread
From: Jeffrey Walton @ 2024-04-24  7:55 UTC (permalink / raw)
  To: musl

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

On Wed, Apr 24, 2024 at 3:40 AM Jon Chesterfield <
jonathanchesterfield@gmail.com> wrote:

> Re testing GNUC,
>
> I'm not sure the macro means "targeting Linux", and it seems totally
> legitimate that a C compiler which doesn't implement any GNU extensions
> would not define that macro. Musl is quite a likely choice for a non-gnu
> compiler that wants to compile code to run against the Linux kernel.
>

__GNUC__, __GNUC_MINOR__ and __GNUC_PATCHLEVEL__ only means the macros are
defined by GNU compilers that use the C preprocessor. See <
https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html>.

The macros don't mean "targeting Linux". They are also defined on OS X
(Darwin) and Windows.

Jeff

[-- Attachment #2: Type: text/html, Size: 1282 bytes --]

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

* Re: [musl] Alignment attribute in headers
  2024-04-24  7:55       ` Jeffrey Walton
@ 2024-04-24  9:49         ` Jon Chesterfield
  0 siblings, 0 replies; 18+ messages in thread
From: Jon Chesterfield @ 2024-04-24  9:49 UTC (permalink / raw)
  To: musl

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

Use in practice and use in documentation don't always align but it seems
you're agreeing that requiring GNU macros to use a Linux specific header is
invalid.

Jon

On Wed, 24 Apr 2024, 08:55 Jeffrey Walton, <noloader@gmail.com> wrote:

>
>
> On Wed, Apr 24, 2024 at 3:40 AM Jon Chesterfield <
> jonathanchesterfield@gmail.com> wrote:
>
>> Re testing GNUC,
>>
>> I'm not sure the macro means "targeting Linux", and it seems totally
>> legitimate that a C compiler which doesn't implement any GNU extensions
>> would not define that macro. Musl is quite a likely choice for a non-gnu
>> compiler that wants to compile code to run against the Linux kernel.
>>
>
> __GNUC__, __GNUC_MINOR__ and __GNUC_PATCHLEVEL__ only means the macros are
> defined by GNU compilers that use the C preprocessor. See <
> https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html>.
>
> The macros don't mean "targeting Linux". They are also defined on OS X
> (Darwin) and Windows.
>
> Jeff
>

[-- Attachment #2: Type: text/html, Size: 1924 bytes --]

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

end of thread, other threads:[~2024-04-24  9:49 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-21  3:51 [musl] Alignment attribute in headers Michael Forney
2024-04-21  4:54 ` Markus Wichmann
2024-04-21  7:16   ` Jₑₙₛ Gustedt
2024-04-21  7:38     ` Markus Wichmann
2024-04-21 10:23       ` Jₑₙₛ Gustedt
2024-04-21 10:31         ` Sam James
2024-04-21 17:40         ` Markus Wichmann
2024-04-21 15:50 ` Thorsten Glaser
2024-04-21 16:44   ` Alexander Monakov
2024-04-21 17:20   ` Markus Wichmann
2024-04-21 18:23     ` Thorsten Glaser
2024-04-21 19:04     ` Michael Forney
2024-04-21 23:48 ` Rich Felker
2024-04-24  0:45   ` Michael Forney
2024-04-24  2:54     ` Markus Wichmann
2024-04-24  7:39     ` Jon Chesterfield
2024-04-24  7:55       ` Jeffrey Walton
2024-04-24  9:49         ` Jon Chesterfield

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).