diff mbox series

Add alloc_align attribute to memalign et al

Message ID YVWPOQoD8IJeIE7+@redhat.com
State Superseded
Headers
Series Add alloc_align attribute to memalign et al |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Jonathan Wakely Sept. 30, 2021, 10:19 a.m. UTC 
  commit ec065256995fc8756972291a79011320fa180649
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Sep 30 10:48:40 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>

diff --git a/malloc/malloc.h b/malloc/malloc.h
index 2df0b38050..78d3ad82b2 100644
--- a/malloc/malloc.h
+++ b/malloc/malloc.h
@@ -64,8 +64,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;
 
 /* 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..7b2a9bd3b0 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -243,6 +243,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__)
+# define __attribute_alloc_align__(param) \
+  __attribute__ ((__alloc_align__ param))
+#else
+# define __attribute_alloc_align__(param) /* Ignore.  */
+#endif
+
 /* 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..45df9a12c6 100644
--- a/stdlib/stdlib.h
+++ b/stdlib/stdlib.h
@@ -589,7 +589,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;
 #endif
 
 /* Abort execution and generate a core-dump.  */
diff --git a/support/support.h b/support/support.h
index 837a806531..91c7178f06 100644
--- a/support/support.h
+++ b/support/support.h
@@ -98,8 +98,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;
 char *xasprintf (const char *format, ...)
   __attribute__ ((format (printf, 1, 2), malloc)) __attr_dealloc_free
   __returns_nonnull;

Comments

Jonathan Wakely Sept. 30, 2021, 10:22 a.m. UTC | #1 
Ugh, sorry, I managed to change the content type of the email body to
text/x-patch, instead of the patch.

The email body was supposed to say ...

This change was inspired by the observation in
https://twitter.com/taviso/status/1443335323702816773 that GCC
produces worse code for memalign than for aligned_alloc.

Tested x86_64-pc-linux-gnu. OK to push?



>commit ec065256995fc8756972291a79011320fa180649
>Author: Jonathan Wakely <jwakely@redhat.com>
>Date:   Thu Sep 30 10:48:40 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>
>
>diff --git a/malloc/malloc.h b/malloc/malloc.h
>index 2df0b38050..78d3ad82b2 100644
>--- a/malloc/malloc.h
>+++ b/malloc/malloc.h
>@@ -64,8 +64,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;
>
> /* 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..7b2a9bd3b0 100644
>--- a/misc/sys/cdefs.h
>+++ b/misc/sys/cdefs.h
>@@ -243,6 +243,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__)
>+# define __attribute_alloc_align__(param) \
>+  __attribute__ ((__alloc_align__ param))
>+#else
>+# define __attribute_alloc_align__(param) /* Ignore.  */
>+#endif
>+
> /* 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..45df9a12c6 100644
>--- a/stdlib/stdlib.h
>+++ b/stdlib/stdlib.h
>@@ -589,7 +589,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;
> #endif
>
> /* Abort execution and generate a core-dump.  */
>diff --git a/support/support.h b/support/support.h
>index 837a806531..91c7178f06 100644
>--- a/support/support.h
>+++ b/support/support.h
>@@ -98,8 +98,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;
> char *xasprintf (const char *format, ...)
>   __attribute__ ((format (printf, 1, 2), malloc)) __attr_dealloc_free
>   __returns_nonnull;
Florian Weimer Sept. 30, 2021, 10:42 a.m. UTC | #2 
* 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.

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.

Thanks,
Florian
Carlos O'Donell Oct. 6, 2021, 1:10 p.m. UTC | #3 
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.
diff mbox series

Patch

This change was inspired by the observation in
https://twitter.com/taviso/status/1443335323702816773 that GCC
produces worse code for memalign than for aligned_alloc.

Tested x86_64-pc-linux-gnu. OK to push?