[v2,1/2] libstdc++: Handle extended alignment in std::get_temporary_buffer [PR105258]

Although it's only used from places where we are allocating a sensible
size, __detail::__get_temporary_buffer should really still check that
len * sizeof(T) doesn't wrap around to zero and allocate a buffer that's
smaller than expected.

This v2 patch is the same as v1 except for adding this check:

       inline _Tp*
       __get_temporary_buffer(ptrdiff_t __len) _GLIBCXX_NOTHROW
       {
+       if (__builtin_expect(__len > (size_t(-1) / sizeof(_Tp)), 0))
+         return 0;
+
 #if __cpp_aligned_new
        if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
          return (_Tp*) _GLIBCXX_OPERATOR_NEW(__len * sizeof(_Tp),

-- >8 --

This adds extended alignment support to std::get_temporary_buffer etc.
so that when std::stable_sort uses a temporary buffer it works for
overaligned types.

Also simplify the _Temporary_buffer type by using RAII for the
allocation, via a new data member. This simplifies the _Temporary_buffer
constructor and destructor by makingthem only responsible for
constructing and destroying the elements, not managing the memory.

libstdc++-v3/ChangeLog:

	PR libstdc++/105258
	* include/bits/stl_tempbuf.h (__detail::__get_temporary_buffer):
	New function to do allocation for get_temporary_buffer, with
	extended alignment support.
	(__detail::__return_temporary_buffer): Support extended
	alignment.
	(get_temporary_buffer): Use __get_temporary_buffer.
	(return_temporary_buffer): Support extended alignment. Add
	deprecated attribute.
	(_Temporary_buffer): Move allocation and deallocation into a
	subobject and remove try-catch block in constructor.
	(__uninitialized_construct_buf): Use argument deduction for
	value type.
	* testsuite/20_util/temporary_buffer.cc: Add dg-warning for new
	deprecated warning.
	* testsuite/25_algorithms/stable_sort/overaligned.cc: New test.
---
 libstdc++-v3/include/bits/stl_tempbuf.h       | 131 ++++++++++++------
 .../testsuite/20_util/temporary_buffer.cc     |   2 +-
 .../25_algorithms/stable_sort/overaligned.cc  |  29 ++++
 3 files changed, 116 insertions(+), 46 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/25_algorithms/stable_sort/overaligned.cc

On Sat, 1 Jun 2024 at 12:23, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> Although it's only used from places where we are allocating a sensible
> size, __detail::__get_temporary_buffer should really still check that
> len * sizeof(T) doesn't wrap around to zero and allocate a buffer that's
> smaller than expected.
>
> This v2 patch is the same as v1 except for adding this check:
>
>        inline _Tp*
>        __get_temporary_buffer(ptrdiff_t __len) _GLIBCXX_NOTHROW
>        {
> +       if (__builtin_expect(__len > (size_t(-1) / sizeof(_Tp)), 0))
> +         return 0;
> +
>  #if __cpp_aligned_new
>         if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
>           return (_Tp*) _GLIBCXX_OPERATOR_NEW(__len * sizeof(_Tp),


Pushed to trunk now.


> -- >8 --
>
> This adds extended alignment support to std::get_temporary_buffer etc.
> so that when std::stable_sort uses a temporary buffer it works for
> overaligned types.
>
> Also simplify the _Temporary_buffer type by using RAII for the
> allocation, via a new data member. This simplifies the _Temporary_buffer
> constructor and destructor by makingthem only responsible for
> constructing and destroying the elements, not managing the memory.
>
> libstdc++-v3/ChangeLog:
>
>         PR libstdc++/105258
>         * include/bits/stl_tempbuf.h (__detail::__get_temporary_buffer):
>         New function to do allocation for get_temporary_buffer, with
>         extended alignment support.
>         (__detail::__return_temporary_buffer): Support extended
>         alignment.
>         (get_temporary_buffer): Use __get_temporary_buffer.
>         (return_temporary_buffer): Support extended alignment. Add
>         deprecated attribute.
>         (_Temporary_buffer): Move allocation and deallocation into a
>         subobject and remove try-catch block in constructor.
>         (__uninitialized_construct_buf): Use argument deduction for
>         value type.
>         * testsuite/20_util/temporary_buffer.cc: Add dg-warning for new
>         deprecated warning.
>         * testsuite/25_algorithms/stable_sort/overaligned.cc: New test.
> ---
>  libstdc++-v3/include/bits/stl_tempbuf.h       | 131 ++++++++++++------
>  .../testsuite/20_util/temporary_buffer.cc     |   2 +-
>  .../25_algorithms/stable_sort/overaligned.cc  |  29 ++++
>  3 files changed, 116 insertions(+), 46 deletions(-)
>  create mode 100644 libstdc++-v3/testsuite/25_algorithms/stable_sort/overaligned.cc
>
> diff --git a/libstdc++-v3/include/bits/stl_tempbuf.h b/libstdc++-v3/include/bits/stl_tempbuf.h
> index 77b121460f9..fa03fd27704 100644
> --- a/libstdc++-v3/include/bits/stl_tempbuf.h
> +++ b/libstdc++-v3/include/bits/stl_tempbuf.h
> @@ -66,19 +66,58 @@ namespace std _GLIBCXX_VISIBILITY(default)
>  {
>  _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> +#if __has_builtin(__builtin_operator_new) >= 201802L
> +# define _GLIBCXX_OPERATOR_NEW __builtin_operator_new
> +# define _GLIBCXX_OPERATOR_DELETE __builtin_operator_delete
> +#else
> +# define _GLIBCXX_OPERATOR_NEW ::operator new
> +# define _GLIBCXX_OPERATOR_DELETE ::operator delete
> +#endif
> +
>    namespace __detail
>    {
> +    // Equivalent to std::get_temporary_buffer but won't return a smaller size.
> +    // It either returns a buffer of __len elements, or a null pointer.
> +    template<typename _Tp>
> +      inline _Tp*
> +      __get_temporary_buffer(ptrdiff_t __len) _GLIBCXX_NOTHROW
> +      {
> +       if (__builtin_expect(__len > (size_t(-1) / sizeof(_Tp)), 0))
> +         return 0;
> +
> +#if __cpp_aligned_new
> +       if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
> +         return (_Tp*) _GLIBCXX_OPERATOR_NEW(__len * sizeof(_Tp),
> +                                             align_val_t(alignof(_Tp)),
> +                                             nothrow_t());
> +#endif
> +       return (_Tp*) _GLIBCXX_OPERATOR_NEW(__len * sizeof(_Tp), nothrow_t());
> +      }
> +
> +    // Equivalent to std::return_temporary_buffer but with a size argument.
> +    // The size is the number of elements, not the number of bytes.
>      template<typename _Tp>
>        inline void
>        __return_temporary_buffer(_Tp* __p,
>                                 size_t __len __attribute__((__unused__)))
>        {
>  #if __cpp_sized_deallocation
> -       ::operator delete(__p, __len * sizeof(_Tp));
> +# define _GLIBCXX_SIZED_DEALLOC(T, p, n) (p), (n) * sizeof(T)
>  #else
> -       ::operator delete(__p);
> +# define _GLIBCXX_SIZED_DEALLOC(T, p, n) (p)
>  #endif
> +
> +#if __cpp_aligned_new
> +       if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
> +         {
> +           _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(_Tp, __p, __len),
> +                                    align_val_t(alignof(_Tp)));
> +           return;
> +         }
> +#endif
> +       _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(_Tp, __p, __len));
>        }
> +#undef _GLIBCXX_SIZED_DEALLOC
>    }
>
>    /**
> @@ -90,7 +129,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     *
>     *  This function tries to obtain storage for @c __len adjacent Tp
>     *  objects.  The objects themselves are not constructed, of course.
> -   *  A pair<> is returned containing <em>the buffer s address and
> +   *  A pair<> is returned containing <em>the buffer's address and
>     *  capacity (in the units of sizeof(_Tp)), or a pair of 0 values if
>     *  no storage can be obtained.</em>  Note that the capacity obtained
>     *  may be less than that requested if the memory is unavailable;
> @@ -110,13 +149,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>        while (__len > 0)
>         {
> -         _Tp* __tmp = static_cast<_Tp*>(::operator new(__len * sizeof(_Tp),
> -                                                       std::nothrow));
> -         if (__tmp != 0)
> -           return std::pair<_Tp*, ptrdiff_t>(__tmp, __len);
> +         if (_Tp* __tmp = __detail::__get_temporary_buffer<_Tp>(__len))
> +           return pair<_Tp*, ptrdiff_t>(__tmp, __len);
>           __len = __len == 1 ? 0 : ((__len + 1) / 2);
>         }
> -      return std::pair<_Tp*, ptrdiff_t>(static_cast<_Tp*>(0), 0);
> +      return pair<_Tp*, ptrdiff_t>();
>      }
>
>    /**
> @@ -127,9 +164,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     *  Frees the memory pointed to by __p.
>     */
>    template<typename _Tp>
> +    _GLIBCXX17_DEPRECATED
>      inline void
>      return_temporary_buffer(_Tp* __p)
> -    { ::operator delete(__p); }
> +    {
> +#if __cpp_aligned_new
> +      if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
> +       _GLIBCXX_OPERATOR_DELETE(__p, align_val_t(alignof(_Tp)));
> +      else
> +#endif
> +      _GLIBCXX_OPERATOR_DELETE(__p);
> +    }
> +
> +#undef _GLIBCXX_OPERATOR_DELETE
> +#undef _GLIBCXX_OPERATOR_NEW
>
>    /**
>     *  This class is used in two places: stl_algo.h and ext/memory,
> @@ -150,14 +198,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>      protected:
>        size_type  _M_original_len;
> -      size_type  _M_len;
> -      pointer    _M_buffer;
> +      struct _Impl
> +      {
> +       explicit
> +       _Impl(ptrdiff_t __original_len)
> +       {
> +         pair<pointer, size_type> __p(
> +           std::get_temporary_buffer<value_type>(__original_len));
> +         _M_len = __p.second;
> +         _M_buffer = __p.first;
> +       }
> +
> +       ~_Impl()
> +       { std::__detail::__return_temporary_buffer(_M_buffer, _M_len); }
> +
> +       size_type  _M_len;
> +       pointer    _M_buffer;
> +      } _M_impl;
>
>      public:
>        /// As per Table mumble.
>        size_type
>        size() const
> -      { return _M_len; }
> +      { return _M_impl._M_len; }
>
>        /// Returns the size requested by the constructor; may be >size().
>        size_type
> @@ -167,12 +230,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        /// As per Table mumble.
>        iterator
>        begin()
> -      { return _M_buffer; }
> +      { return _M_impl._M_buffer; }
>
>        /// As per Table mumble.
>        iterator
>        end()
> -      { return _M_buffer + _M_len; }
> +      { return _M_impl._M_buffer + _M_impl._M_len; }
>
>        /**
>         * Constructs a temporary buffer of a size somewhere between
> @@ -181,10 +244,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        _Temporary_buffer(_ForwardIterator __seed, size_type __original_len);
>
>        ~_Temporary_buffer()
> -      {
> -       std::_Destroy(_M_buffer, _M_buffer + _M_len);
> -       std::__detail::__return_temporary_buffer(_M_buffer, _M_len);
> -      }
> +      { std::_Destroy(_M_impl._M_buffer, _M_impl._M_buffer + _M_impl._M_len); }
>
>      private:
>        // Disable copy constructor and assignment operator.
> @@ -203,7 +263,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>          __ucr(_Pointer __first, _Pointer __last,
>               _ForwardIterator __seed)
>          {
> -         if (__first == __last)
> +         if (__builtin_expect(__first == __last, 0))
>             return;
>
>           _Pointer __cur = __first;
> @@ -243,17 +303,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>    // the same value when the algorithm finishes, unless one of the
>    // constructions throws.
>    //
> -  // Requirements: _Pointer::value_type(_Tp&&) is valid.
> -  template<typename _Pointer, typename _ForwardIterator>
> +  // Requirements:
> +  // _Tp is move constructible and constructible from std::move(*__seed).
> +  template<typename _Tp, typename _ForwardIterator>
>      inline void
> -    __uninitialized_construct_buf(_Pointer __first, _Pointer __last,
> +    __uninitialized_construct_buf(_Tp* __first, _Tp* __last,
>                                   _ForwardIterator __seed)
>      {
> -      typedef typename std::iterator_traits<_Pointer>::value_type
> -       _ValueType;
> -
>        std::__uninitialized_construct_buf_dispatch<
> -        __has_trivial_constructor(_ValueType)>::
> +       __has_trivial_constructor(_Tp)>::
>           __ucr(__first, __last, __seed);
>      }
>
> @@ -262,26 +320,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>    template<typename _ForwardIterator, typename _Tp>
>      _Temporary_buffer<_ForwardIterator, _Tp>::
>      _Temporary_buffer(_ForwardIterator __seed, size_type __original_len)
> -    : _M_original_len(__original_len), _M_len(0), _M_buffer(0)
> +    : _M_original_len(__original_len), _M_impl(__original_len)
>      {
> -      std::pair<pointer, size_type> __p(
> -               std::get_temporary_buffer<value_type>(_M_original_len));
> -
> -      if (__p.first)
> -       {
> -         __try
> -           {
> -             std::__uninitialized_construct_buf(__p.first, __p.first + __p.second,
> -                                                __seed);
> -             _M_buffer = __p.first;
> -             _M_len = __p.second;
> -           }
> -         __catch(...)
> -           {
> -             std::__detail::__return_temporary_buffer(__p.first, __p.second);
> -             __throw_exception_again;
> -           }
> -       }
> +      std::__uninitialized_construct_buf(begin(), end(), __seed);
>      }
>  #pragma GCC diagnostic pop
>
> diff --git a/libstdc++-v3/testsuite/20_util/temporary_buffer.cc b/libstdc++-v3/testsuite/20_util/temporary_buffer.cc
> index 155d19034e5..065739be29d 100644
> --- a/libstdc++-v3/testsuite/20_util/temporary_buffer.cc
> +++ b/libstdc++-v3/testsuite/20_util/temporary_buffer.cc
> @@ -44,7 +44,7 @@ int main(void)
>        VERIFY( results.first == 0 );
>    }
>
> -  std::return_temporary_buffer(results.first);
> +  std::return_temporary_buffer(results.first); // { dg-warning "deprecated" "" { target c++17 } }
>
>    return 0;
>  }
> diff --git a/libstdc++-v3/testsuite/25_algorithms/stable_sort/overaligned.cc b/libstdc++-v3/testsuite/25_algorithms/stable_sort/overaligned.cc
> new file mode 100644
> index 00000000000..3c200624617
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/25_algorithms/stable_sort/overaligned.cc
> @@ -0,0 +1,29 @@
> +// { dg-do run { target c++17 } }
> +// PR libstdc++/105258 std::get_temporary_buffer() does not respect alignment
> +
> +#include <algorithm>
> +#include <cstdint>
> +#include <testsuite_hooks.h>
> +
> +struct alignas(__STDCPP_DEFAULT_NEW_ALIGNMENT__ * 2) Overaligned
> +{
> +  ~Overaligned()
> +  {
> +    auto alignment = reinterpret_cast<std::uintptr_t>(this);
> +    VERIFY( (alignment % alignof(Overaligned)) == 0 );
> +  }
> +
> +  bool operator<(const Overaligned&) const { return false; }
> +};
> +
> +void
> +test_pr105258()
> +{
> +  Overaligned o[2];
> +  std::stable_sort(o, o+2);
> +}
> +
> +int main()
> +{
> +  test_pr105258();
> +}
> --
> 2.45.1
>

On 6/3/24 22:22, Jonathan Wakely wrote:
> Pushed to trunk now.

Just a heads-up that this started to cause Clang (at least 18/19) to 
emit a -Wdeprecated-declarations now,

> $ cat test.cc
> #include <algorithm>
> void f(int * p1, int * p2) { std::stable_sort(p1, p2); }

> $ clang++ --gcc-install-dir=/home/sberg/gcc/inst/lib/gcc/x86_64-pc-linux-gnu/15.0.0 -fsyntax-only test.cc
> In file included from test.cc:1:
> In file included from /home/sberg/gcc/inst/lib/gcc/x86_64-pc-linux-gnu/15.0.0/../../../../include/c++/15.0.0/algorithm:61:
> In file included from /home/sberg/gcc/inst/lib/gcc/x86_64-pc-linux-gnu/15.0.0/../../../../include/c++/15.0.0/bits/stl_algo.h:69:
> /home/sberg/gcc/inst/lib/gcc/x86_64-pc-linux-gnu/15.0.0/../../../../include/c++/15.0.0/bits/stl_tempbuf.h:207:11: warning: 'get_temporary_buffer<int>' is deprecated [-Wdeprecated-declarations]
>   207 |             std::get_temporary_buffer<value_type>(__original_len));
>       |                  ^
> /home/sberg/gcc/inst/lib/gcc/x86_64-pc-linux-gnu/15.0.0/../../../../include/c++/15.0.0/bits/stl_tempbuf.h:323:40: note: in instantiation of member function 'std::_Temporary_buffer<__gnu_cxx::__normal_iterator<int *, std::vector<int>>, int>::_Impl::_Impl' requested here
>   323 |     : _M_original_len(__original_len), _M_impl(__original_len)
>       |                                        ^
> /home/sberg/gcc/inst/lib/gcc/x86_64-pc-linux-gnu/15.0.0/../../../../include/c++/15.0.0/bits/stl_algo.h:4948:15: note: in instantiation of member function 'std::_Temporary_buffer<__gnu_cxx::__normal_iterator<int *, std::vector<int>>, int>::_Temporary_buffer' requested here
>  4948 |       _TmpBuf __buf(__first, (__last - __first + 1) / 2);
>       |               ^
> /home/sberg/gcc/inst/lib/gcc/x86_64-pc-linux-gnu/15.0.0/../../../../include/c++/15.0.0/bits/stl_algo.h:4993:23: note: in instantiation of function template specialization 'std::__stable_sort<__gnu_cxx::__normal_iterator<int *, std::vector<int>>, __gnu_cxx::__ops::_Iter_less_iter>' requested here
>  4993 |       _GLIBCXX_STD_A::__stable_sort(__first, __last,
>       |                       ^
> test.cc:3:37: note: in instantiation of function template specialization 'std::stable_sort<__gnu_cxx::__normal_iterator<int *, std::vector<int>>>' requested here
>     3 | void f(std::vector<int> & v) { std::stable_sort(v.begin(), v.end()); }
>       |                                     ^
> /home/sberg/gcc/inst/lib/gcc/x86_64-pc-linux-gnu/15.0.0/../../../../include/c++/15.0.0/bits/stl_tempbuf.h:141:5: note: 'get_temporary_buffer<int>' has been explicitly marked deprecated here
>   141 |     _GLIBCXX17_DEPRECATED
>       |     ^
> /home/sberg/gcc/inst/lib/gcc/x86_64-pc-linux-gnu/15.0.0/../../../../include/c++/15.0.0/x86_64-pc-linux-gnu/bits/c++config.h:123:34: note: expanded from macro '_GLIBCXX17_DEPRECATED'
>   123 | # define _GLIBCXX17_DEPRECATED [[__deprecated__]]
>       |                                  ^
> 1 warning generated.

which could be silenced with

> --- a/libstdc++-v3/include/bits/stl_tempbuf.h
> +++ b/libstdc++-v3/include/bits/stl_tempbuf.h
> @@ -203,8 +203,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>         explicit
>         _Impl(ptrdiff_t __original_len)
>         {
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
>           pair<pointer, size_type> __p(
>             std::get_temporary_buffer<value_type>(__original_len));
> +#pragma GCC diagnostic pop
>           _M_len = __p.second;
>           _M_buffer = __p.first;
>         }

(There already is another such pragma diagnostic ignored a bit further 
down in that file, so I assume that's the way to go there?)

On Tue, 18 Jun 2024 at 19:05, Stephan Bergmann <sberg.fun@gmail.com> wrote:
>
> On 6/3/24 22:22, Jonathan Wakely wrote:
> > Pushed to trunk now.
>
> Just a heads-up that this started to cause Clang (at least 18/19) to
> emit a -Wdeprecated-declarations now,

Yes, I saw this too.

> (There already is another such pragma diagnostic ignored a bit further
> down in that file, so I assume that's the way to go there?)

Yes, the call to get_temporary_buffer used to be in that function
further down in the file. I moved it elsewhere in the file, but didn't
move the pragma to go with it, oops.

So we can remove those later pragmas, and move them earlier in the
file. I'm already testing a patch locally.