Re: [musl] Alignment attribute in headers

[Rich Felker <dalias@libc.org>]

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