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

[Jonathan Wakely <jwakely@redhat.com>]

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
>