* [PATCH] Add alloc_align attribute to memalign et al
@ 2021-09-30 10:19 Jonathan Wakely
2021-09-30 10:22 ` Jonathan Wakely
2021-09-30 10:42 ` Florian Weimer
0 siblings, 2 replies; 6+ messages in thread
From: Jonathan Wakely @ 2021-09-30 10:19 UTC (permalink / raw)
To: libc-alpha
[-- Attachment #1: Type: text/x-patch, Size: 210 bytes --]
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?
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 4388 bytes --]
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Add alloc_align attribute to memalign et al
2021-09-30 10:19 [PATCH] Add alloc_align attribute to memalign et al Jonathan Wakely
@ 2021-09-30 10:22 ` Jonathan Wakely
2021-09-30 10:42 ` Florian Weimer
1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Wakely @ 2021-09-30 10:22 UTC (permalink / raw)
To: libc-alpha
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Add alloc_align attribute to memalign et al
2021-09-30 10:19 [PATCH] Add alloc_align attribute to memalign et al Jonathan Wakely
2021-09-30 10:22 ` Jonathan Wakely
@ 2021-09-30 10:42 ` Florian Weimer
2021-10-06 13:10 ` Carlos O'Donell
1 sibling, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2021-09-30 10:42 UTC (permalink / raw)
To: Jonathan Wakely via Libc-alpha; +Cc: Jonathan Wakely
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Add alloc_align attribute to memalign et al
2021-09-30 10:42 ` Florian Weimer
@ 2021-10-06 13:10 ` Carlos O'Donell
2021-10-11 8:17 ` [PATCH v2] " Jonathan Wakely
0 siblings, 1 reply; 6+ messages in thread
From: Carlos O'Donell @ 2021-10-06 13:10 UTC (permalink / raw)
To: Florian Weimer, Jonathan Wakely via Libc-alpha; +Cc: Jonathan Wakely
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.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] Add alloc_align attribute to memalign et al
2021-10-06 13:10 ` Carlos O'Donell
@ 2021-10-11 8:17 ` Jonathan Wakely
2021-10-20 23:15 ` Carlos O'Donell
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Wakely @ 2021-10-11 8:17 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: Florian Weimer, Jonathan Wakely via Libc-alpha
[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]
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?
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 5404 bytes --]
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>
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.
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;
/* 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.
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__)
+# 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..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.
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;
#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.
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;
char *xasprintf (const char *format, ...)
__attribute__ ((format (printf, 1, 2), malloc)) __attr_dealloc_free
__returns_nonnull;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Add alloc_align attribute to memalign et al
2021-10-11 8:17 ` [PATCH v2] " Jonathan Wakely
@ 2021-10-20 23:15 ` Carlos O'Donell
0 siblings, 0 replies; 6+ messages in thread
From: Carlos O'Donell @ 2021-10-20 23:15 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: Florian Weimer, Jonathan Wakely via Libc-alpha
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;
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-20 23:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-30 10:19 [PATCH] Add alloc_align attribute to memalign et al Jonathan Wakely
2021-09-30 10:22 ` Jonathan Wakely
2021-09-30 10:42 ` Florian Weimer
2021-10-06 13:10 ` Carlos O'Donell
2021-10-11 8:17 ` [PATCH v2] " Jonathan Wakely
2021-10-20 23:15 ` Carlos O'Donell
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 read-only IMAP folder(s) and NNTP newsgroup(s).