[v2] Add alloc_align attribute to memalign et al

  On Wed, 6 Oct 2021 at 14:10, Carlos O'Donell wrote:
>
> On 9/30/21 06:42, Florian Weimer via Libc-alpha wrote:
> > * Jonathan Wakely via Libc-alpha:
> >
> >>     Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
> >
> > As a DCO contribution, this needs to update the copyright headers of the
> > changed files, according to:
> >
> > <https://sourceware.org/glibc/wiki/Contribution%20checklist#Update_copyright_information>
> >
> > |     If you are adding a change to a file and those changes do not have
> > |     their copyright assigned to the FSF (contribution via DCO), those
> > |     changes should add the following to the top of the file after the
> > |     1-line description:
> > |
> > |         Copyright The GNU Toolchain Authors.
>
> This is correct.
>
> > On the other hand, the string “Copyright The GNU Toolchain Authors”
> > presently does not appear in glibc, so I don't know the rule is actually
> > in place, sorry.
>
> This rule has been in effect since August 1st 2021 when we transitioned to accepting DCO.
>
> We should accept Jonathan's patch if it meets our Contribution Checklist.
>
> In this case Jonathan should submit a v2 with the copyright updated.

The attached v2 patch adds those copyright reminders as requested. For
the avoidance of doubt, the copyright in this patch is mine (not Red
Hat's) and my personal FSF assignment has been cancelled, which is why
this is a DCO contribution.

Tested on x86_64-linux (Fedora 34).

Summary of test results:
  4559 PASS
    23 UNSUPPORTED
    16 XFAIL
     6 XPASS


OK to push?
commit 0521d85d2ab89008e4fe9de4e6a81663bc880c60
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Oct 6 20:05:48 2021 +0100

    Add alloc_align attribute to memalign et al
    
    GCC 4.9.0 added the alloc_align attribute to say that a function
    argument specifies the alignment of the returned pointer. Clang supports
    the attribute too. Using the attribute can allow a compiler to generate
    better code if it knows the returned pointer has a minimum alignment.
    See https://gcc.gnu.org/PR60092 for more details.
    
    GCC implicitly knows the semantics of aligned_alloc and posix_memalign,
    but not the obsolete memalign. As a result, GCC generates worse code
    when memalign is used, compared to aligned_alloc.  Clang knows about
    aligned_alloc and memalign, but not posix_memalign.
    
    This change adds a new __attribute_alloc_align__ macro to <sys/cdefs.h>
    and then uses it on memalign (where it helps GCC) and aligned_alloc
    (where GCC and Clang already know the semantics, but it doesn't hurt)
    and xposix_memalign. It can't be used on posix_memalign because that
    doesn't return a pointer (the allocated pointer is returned via a void**
    parameter instead).
    
    Unlike the alloc_size attribute, alloc_align only allows a single
    argument. That means the new __attribute_alloc_align__ macro doesn't
    really need to be used with double parentheses to protect a comma
    between its arguments. For consistency with __attribute_alloc_size__
    this patch defines it the same way, so that double parentheses are
    required.
    
    Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
  

On 10/11/21 04:17, Jonathan Wakely wrote:
> On Wed, 6 Oct 2021 at 14:10, Carlos O'Donell wrote:
>>
>> On 9/30/21 06:42, Florian Weimer via Libc-alpha wrote:
>>> * Jonathan Wakely via Libc-alpha:
>>>
>>>>     Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
>>>
>>> As a DCO contribution, this needs to update the copyright headers of the
>>> changed files, according to:
>>>
>>> <https://sourceware.org/glibc/wiki/Contribution%20checklist#Update_copyright_information>
>>>
>>> |     If you are adding a change to a file and those changes do not have
>>> |     their copyright assigned to the FSF (contribution via DCO), those
>>> |     changes should add the following to the top of the file after the
>>> |     1-line description:
>>> |
>>> |         Copyright The GNU Toolchain Authors.
>>
>> This is correct.
>>
>>> On the other hand, the string “Copyright The GNU Toolchain Authors”
>>> presently does not appear in glibc, so I don't know the rule is actually
>>> in place, sorry.
>>
>> This rule has been in effect since August 1st 2021 when we transitioned to accepting DCO.
>>
>> We should accept Jonathan's patch if it meets our Contribution Checklist.
>>
>> In this case Jonathan should submit a v2 with the copyright updated.
> 
> The attached v2 patch adds those copyright reminders as requested. For
> the avoidance of doubt, the copyright in this patch is mine (not Red
> Hat's) and my personal FSF assignment has been cancelled, which is why
> this is a DCO contribution.
> 
> Tested on x86_64-linux (Fedora 34).
> 
> Summary of test results:
>   4559 PASS
>     23 UNSUPPORTED
>     16 XFAIL
>      6 XPASS
> 
> 
> OK to push?

OK to push.

Once this is pushed we'll pick this up into Fedora Rawhide sync next
week and start building with the changes and see if anything comes up.

CI/CD is 2/2 pass.

i686 and x86_64 build and test passed without regression for me.

Please include any accumulated tags you received e.g. Reviewed-by, Tested-by.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> commit 0521d85d2ab89008e4fe9de4e6a81663bc880c60
> Author: Jonathan Wakely <jwakely@redhat.com>
> Date:   Wed Oct 6 20:05:48 2021 +0100
> 
>     Add alloc_align attribute to memalign et al
>     
>     GCC 4.9.0 added the alloc_align attribute to say that a function
>     argument specifies the alignment of the returned pointer. Clang supports
>     the attribute too. Using the attribute can allow a compiler to generate
>     better code if it knows the returned pointer has a minimum alignment.
>     See https://gcc.gnu.org/PR60092 for more details.
>     
>     GCC implicitly knows the semantics of aligned_alloc and posix_memalign,
>     but not the obsolete memalign. As a result, GCC generates worse code
>     when memalign is used, compared to aligned_alloc.  Clang knows about
>     aligned_alloc and memalign, but not posix_memalign.
>     
>     This change adds a new __attribute_alloc_align__ macro to <sys/cdefs.h>
>     and then uses it on memalign (where it helps GCC) and aligned_alloc
>     (where GCC and Clang already know the semantics, but it doesn't hurt)
>     and xposix_memalign. It can't be used on posix_memalign because that
>     doesn't return a pointer (the allocated pointer is returned via a void**
>     parameter instead).
>     
>     Unlike the alloc_size attribute, alloc_align only allows a single
>     argument. That means the new __attribute_alloc_align__ macro doesn't
>     really need to be used with double parentheses to protect a comma
>     between its arguments. For consistency with __attribute_alloc_size__
>     this patch defines it the same way, so that double parentheses are
>     required.

At a high level this makes sense and is something we want to do.

The implementation of putting this in cdefs.h follows __attribute_alloc_size__.

The details correct and it passes testing.

>     Signed-off-by: Jonathan Wakely <jwakely@redhat.com>

OK.

> diff --git a/malloc/malloc.h b/malloc/malloc.h
> index 2df0b38050..0e8a0cf051 100644
> --- a/malloc/malloc.h
> +++ b/malloc/malloc.h
> @@ -1,5 +1,6 @@
>  /* Prototypes and definition for malloc implementation.
>     Copyright (C) 1996-2021 Free Software Foundation, Inc.
> +   Copyright The GNU Toolchain Authors.

OK. As per DCO.

>     This file is part of the GNU C Library.
>  
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -64,8 +65,8 @@ extern void free (void *__ptr) __THROW;
>  
>  /* Allocate SIZE bytes allocated to ALIGNMENT bytes.  */
>  extern void *memalign (size_t __alignment, size_t __size)
> -  __THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur
> -  __attr_dealloc_free;
> +  __THROW __attribute_malloc__ __attribute_alloc_align__ ((1))
> +  __attribute_alloc_size__ ((2)) __wur __attr_dealloc_free;

OK. memalign 1/3

>  
>  /* Allocate SIZE bytes on a page boundary.  */
>  extern void *valloc (size_t __size) __THROW __attribute_malloc__
> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index 4dac9d264d..a864eb81d5 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
> @@ -1,4 +1,5 @@
>  /* Copyright (C) 1992-2021 Free Software Foundation, Inc.
> +   Copyright The GNU Toolchain Authors.

OK. As per DCO. Installed header.

>     This file is part of the GNU C Library.
>  
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -243,6 +244,15 @@
>  # define __attribute_alloc_size__(params) /* Ignore.  */
>  #endif
>  
> +/* Tell the compiler which argument to an allocation function
> +   indicates the alignment of the allocation.  */
> +#if __GNUC_PREREQ (4, 9) || __glibc_has_attribute (__alloc_align__)

OK. >4.9 or if we have the attribute.

> +# define __attribute_alloc_align__(param) \
> +  __attribute__ ((__alloc_align__ param))
> +#else
> +# define __attribute_alloc_align__(param) /* Ignore.  */
> +#endif

OK. Follows the __attribute_alloc_size__ definition and logically placed in header.

> +
>  /* At some point during the gcc 2.96 development the `pure' attribute
>     for functions was introduced.  We don't want to use it unconditionally
>     (although this would be possible) since it generates warnings.  */
> diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
> index 0481c12355..e99c5a1b90 100644
> --- a/stdlib/stdlib.h
> +++ b/stdlib/stdlib.h
> @@ -1,4 +1,5 @@
>  /* Copyright (C) 1991-2021 Free Software Foundation, Inc.
> +   Copyright The GNU Toolchain Authors.

OK. As per DCO.

>     This file is part of the GNU C Library.
>  
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -589,7 +590,8 @@ extern int posix_memalign (void **__memptr, size_t __alignment, size_t __size)
>  #ifdef __USE_ISOC11
>  /* ISO C variant of aligned allocation.  */
>  extern void *aligned_alloc (size_t __alignment, size_t __size)
> -     __THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur;
> +     __THROW __attribute_malloc__ __attribute_alloc_align__ ((1))
> +     __attribute_alloc_size__ ((2)) __wur;

OK. aligned_alloc 2/3

>  #endif
>  
>  /* Abort execution and generate a core-dump.  */
> diff --git a/support/support.h b/support/support.h
> index 837a806531..13324712e7 100644
> --- a/support/support.h
> +++ b/support/support.h
> @@ -1,5 +1,6 @@
>  /* Common extra functions.
>     Copyright (C) 2016-2021 Free Software Foundation, Inc.
> +   Copyright The GNU Toolchain Authors.

OK. As per DCO.

>     This file is part of the GNU C Library.
>  
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -98,8 +99,8 @@ extern void *xrealloc (void *o, size_t n)
>  extern char *xstrdup (const char *) __attribute_malloc__ __attr_dealloc_free
>    __returns_nonnull;
>  void *xposix_memalign (size_t alignment, size_t n)
> -  __attribute_malloc__ __attribute_alloc_size__ ((2)) __attr_dealloc_free
> -  __returns_nonnull;
> +  __attribute_malloc__ __attribute_alloc_align__ ((1))
> +  __attribute_alloc_size__ ((2)) __attr_dealloc_free __returns_nonnull;

OK. xposix_memalign 3/3 (for testing).

>  char *xasprintf (const char *format, ...)
>    __attribute__ ((format (printf, 1, 2), malloc)) __attr_dealloc_free
>    __returns_nonnull;