diff mbox series

libstdc++: Add align_alloc attribute to aligned operator new

Message ID 20241030212626.1323680-1-jwakely@redhat.com
State New
Headers show
Series libstdc++: Add align_alloc attribute to aligned operator new | expand

Commit Message

Jonathan Wakely Oct. 30, 2024, 9:24 p.m. UTC 
The aligned versions of operator new should use the align_alloc
attribute to help the compiler.

PR c++/86878 requests that the compiler would use the attribute to warn
about invalid attributes, so an XFAILed test is added for that.

libstdc++-v3/ChangeLog:

	* libsupc++/new (operator new): Add attribute align_alloc(2) to
	overloads taking a std::align_val_t argument.
	* testsuite/18_support/new_aligned_warn.cc: New test.
---
I think this makes sense, but maybe there's some property of the
attribute that means this isn't a good idea? I think the compiler can
use the value of align_val_t even though it's a scoped enumeration type.

Tested x86_64-linux.

 libstdc++-v3/libsupc++/new                          |  6 +++---
 .../testsuite/18_support/new_aligned_warn.cc        | 13 +++++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/18_support/new_aligned_warn.cc

Comments

Jonathan Wakely Oct. 30, 2024, 9:27 p.m. UTC | #1 
On Wed, 30 Oct 2024 at 21:26, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> The aligned versions of operator new should use the align_alloc
> attribute to help the compiler.
>
> PR c++/86878 requests that the compiler would use the attribute to warn
> about invalid attributes, so an XFAILed test is added for that.
>
> libstdc++-v3/ChangeLog:
>
>         * libsupc++/new (operator new): Add attribute align_alloc(2) to
>         overloads taking a std::align_val_t argument.
>         * testsuite/18_support/new_aligned_warn.cc: New test.
> ---
> I think this makes sense, but maybe there's some property of the
> attribute that means this isn't a good idea? I think the compiler can
> use the value of align_val_t even though it's a scoped enumeration type.
>
> Tested x86_64-linux.

I forgot to say that this is also available for review at:
https://forge.sourceware.org/gcc/gcc-TEST/pulls/12

>
>  libstdc++-v3/libsupc++/new                          |  6 +++---
>  .../testsuite/18_support/new_aligned_warn.cc        | 13 +++++++++++++
>  2 files changed, 16 insertions(+), 3 deletions(-)
>  create mode 100644 libstdc++-v3/testsuite/18_support/new_aligned_warn.cc
>
> diff --git a/libstdc++-v3/libsupc++/new b/libstdc++-v3/libsupc++/new
> index e9a3d9b49a3..4345030071b 100644
> --- a/libstdc++-v3/libsupc++/new
> +++ b/libstdc++-v3/libsupc++/new
> @@ -167,7 +167,7 @@ void operator delete[](void*, const std::nothrow_t&)
>  #if __cpp_aligned_new
>  _GLIBCXX_NODISCARD void* operator new(std::size_t, std::align_val_t)
>    _GLIBCXX_TXN_SAFE
> -  __attribute__((__externally_visible__, __alloc_size__ (1), __malloc__));
> +  __attribute__((__externally_visible__, __alloc_size__ (1), __alloc_align__ (2),  __malloc__));
>  _GLIBCXX_NODISCARD void* operator new(std::size_t, std::align_val_t, const std::nothrow_t&)
>     _GLIBCXX_TXN_SAFE
>    _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__, __alloc_size__ (1), __malloc__));
> @@ -178,10 +178,10 @@ void operator delete(void*, std::align_val_t, const std::nothrow_t&)
>    _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__));
>  _GLIBCXX_NODISCARD void* operator new[](std::size_t, std::align_val_t)
>    _GLIBCXX_TXN_SAFE
> -  __attribute__((__externally_visible__, __alloc_size__ (1), __malloc__));
> +  __attribute__((__externally_visible__, __alloc_size__ (1), __alloc_align__ (2), __malloc__));
>  _GLIBCXX_NODISCARD void* operator new[](std::size_t, std::align_val_t, const std::nothrow_t&)
>    _GLIBCXX_TXN_SAFE
> -  _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__, __alloc_size__ (1), __malloc__));
> +  _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__, __alloc_size__ (1), __alloc_align__ (2), __malloc__));
>  void operator delete[](void*, std::align_val_t) _GLIBCXX_TXN_SAFE
>    _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__));
>  void operator delete[](void*, std::align_val_t, const std::nothrow_t&)
> diff --git a/libstdc++-v3/testsuite/18_support/new_aligned_warn.cc b/libstdc++-v3/testsuite/18_support/new_aligned_warn.cc
> new file mode 100644
> index 00000000000..e9d374abe31
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/18_support/new_aligned_warn.cc
> @@ -0,0 +1,13 @@
> +// { dg-options "-Wattributes" }
> +// { dg-do compile { target c++17 } }
> +
> +#include <new>
> +
> +int main()
> +{
> +  // PR c++/86878 has a patch to make these warn.
> +  (void) operator new(1, std::align_val_t(3)); // { dg-warning "power of two" "" { xfail *-*-* } }
> +  (void) operator new[](1, std::align_val_t(10)); // { dg-warning "power of two" "" { xfail *-*-* } }
> +  (void) operator new(1, std::align_val_t(0), std::nothrow_t()); // { dg-warning "power of two" "" { xfail *-*-* } }
> +  (void) operator new[](1, std::align_val_t(-1), std::nothrow_t()); // { dg-warning "power of two" "" { xfail *-*-* } }
> +}
> --
> 2.47.0
>
Jakub Jelinek Oct. 30, 2024, 9:53 p.m. UTC | #2 
On Wed, Oct 30, 2024 at 09:24:05PM +0000, Jonathan Wakely wrote:
> The aligned versions of operator new should use the align_alloc
> attribute to help the compiler.
> 
> PR c++/86878 requests that the compiler would use the attribute to warn
> about invalid attributes, so an XFAILed test is added for that.
> 
> libstdc++-v3/ChangeLog:
> 
> 	* libsupc++/new (operator new): Add attribute align_alloc(2) to
> 	overloads taking a std::align_val_t argument.
> 	* testsuite/18_support/new_aligned_warn.cc: New test.
> ---
> I think this makes sense, but maybe there's some property of the
> attribute that means this isn't a good idea? I think the compiler can
> use the value of align_val_t even though it's a scoped enumeration type.

We document that alloc_align (and alloc_size etc.) arguments can have
integer or enumerated types, the implemented check is INTEGRAL_TYPE_P (t)
&& TREE_CODE (t) != BOOLEAN_TYPE.

And the meaning of the attribute is exactly what the C++ standard requires
from such operators, so the patch LGTM.

	Jakub
Jonathan Wakely Oct. 30, 2024, 9:58 p.m. UTC | #3 
On Wed, 30 Oct 2024 at 21:54, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Oct 30, 2024 at 09:24:05PM +0000, Jonathan Wakely wrote:
> > The aligned versions of operator new should use the align_alloc
> > attribute to help the compiler.
> >
> > PR c++/86878 requests that the compiler would use the attribute to warn
> > about invalid attributes, so an XFAILed test is added for that.
> >
> > libstdc++-v3/ChangeLog:
> >
> >       * libsupc++/new (operator new): Add attribute align_alloc(2) to
> >       overloads taking a std::align_val_t argument.
> >       * testsuite/18_support/new_aligned_warn.cc: New test.
> > ---
> > I think this makes sense, but maybe there's some property of the
> > attribute that means this isn't a good idea? I think the compiler can
> > use the value of align_val_t even though it's a scoped enumeration type.
>
> We document that alloc_align (and alloc_size etc.) arguments can have
> integer or enumerated types, the implemented check is INTEGRAL_TYPE_P (t)
> && TREE_CODE (t) != BOOLEAN_TYPE.

I suppose the "not implicitly convertible to an integer" rule for
scoped enums is just a language constraint, the compiler sees scoped
enumerations like any other integral value.

> And the meaning of the attribute is exactly what the C++ standard requires
> from such operators, so the patch LGTM.

Great, thanks, I'll push in the morning.
Jakub Jelinek Oct. 30, 2024, 10:46 p.m. UTC | #4 
On Wed, Oct 30, 2024 at 09:58:56PM +0000, Jonathan Wakely wrote:
> I suppose the "not implicitly convertible to an integer" rule for
> scoped enums is just a language constraint, the compiler sees scoped
> enumerations like any other integral value.

Sure.  This is used in the middle-end and for it the argument has just
ENUMERAL_TYPE, nothing cares if scoped or not.  In fact right now all that
the compiler checks is if an INTEGER_CST is passed to that argument
and then just uses its value.

	Jakub
diff mbox series

Patch

diff --git a/libstdc++-v3/libsupc++/new b/libstdc++-v3/libsupc++/new
index e9a3d9b49a3..4345030071b 100644
--- a/libstdc++-v3/libsupc++/new
+++ b/libstdc++-v3/libsupc++/new
@@ -167,7 +167,7 @@  void operator delete[](void*, const std::nothrow_t&)
 #if __cpp_aligned_new
 _GLIBCXX_NODISCARD void* operator new(std::size_t, std::align_val_t)
   _GLIBCXX_TXN_SAFE
-  __attribute__((__externally_visible__, __alloc_size__ (1), __malloc__));
+  __attribute__((__externally_visible__, __alloc_size__ (1), __alloc_align__ (2),  __malloc__));
 _GLIBCXX_NODISCARD void* operator new(std::size_t, std::align_val_t, const std::nothrow_t&)
    _GLIBCXX_TXN_SAFE
   _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__, __alloc_size__ (1), __malloc__));
@@ -178,10 +178,10 @@  void operator delete(void*, std::align_val_t, const std::nothrow_t&)
   _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__));
 _GLIBCXX_NODISCARD void* operator new[](std::size_t, std::align_val_t)
   _GLIBCXX_TXN_SAFE
-  __attribute__((__externally_visible__, __alloc_size__ (1), __malloc__));
+  __attribute__((__externally_visible__, __alloc_size__ (1), __alloc_align__ (2), __malloc__));
 _GLIBCXX_NODISCARD void* operator new[](std::size_t, std::align_val_t, const std::nothrow_t&)
   _GLIBCXX_TXN_SAFE
-  _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__, __alloc_size__ (1), __malloc__));
+  _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__, __alloc_size__ (1), __alloc_align__ (2), __malloc__));
 void operator delete[](void*, std::align_val_t) _GLIBCXX_TXN_SAFE
   _GLIBCXX_USE_NOEXCEPT __attribute__((__externally_visible__));
 void operator delete[](void*, std::align_val_t, const std::nothrow_t&)
diff --git a/libstdc++-v3/testsuite/18_support/new_aligned_warn.cc b/libstdc++-v3/testsuite/18_support/new_aligned_warn.cc
new file mode 100644
index 00000000000..e9d374abe31
--- /dev/null
+++ b/libstdc++-v3/testsuite/18_support/new_aligned_warn.cc
@@ -0,0 +1,13 @@ 
+// { dg-options "-Wattributes" }
+// { dg-do compile { target c++17 } }
+
+#include <new>
+
+int main()
+{
+  // PR c++/86878 has a patch to make these warn.
+  (void) operator new(1, std::align_val_t(3)); // { dg-warning "power of two" "" { xfail *-*-* } }
+  (void) operator new[](1, std::align_val_t(10)); // { dg-warning "power of two" "" { xfail *-*-* } }
+  (void) operator new(1, std::align_val_t(0), std::nothrow_t()); // { dg-warning "power of two" "" { xfail *-*-* } }
+  (void) operator new[](1, std::align_val_t(-1), std::nothrow_t()); // { dg-warning "power of two" "" { xfail *-*-* } }
+}