From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id BDE6C395560D for ; Mon, 3 Jun 2024 20:22:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BDE6C395560D Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org BDE6C395560D Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717446159; cv=none; b=oQWKG3yzjfblufq1a/cx6tjL24XBlIihwxaBcAREe3wok76mdS6Ew8G97s4uFvIOORSJUTb+pZL6wxkgpAlbT12D6p+4JdkJcHb/TA3t360LLcPhAwTxx3t8QfwrtZVlRk4ZEhOFGK0kpFT8jdhUWxLP38Dac6Nna2FalnttUVQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717446159; c=relaxed/simple; bh=XdzSzE2ajZwCriW5UyfP8B8JqYRcEod0GSCfybPwgik=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=tby6FO03clEHmHX/RHvQc4OJRVypYEqFMfULhm19tHqJUMHgGYuiqQgq/G8AG2wqDtjkNN1KBLquSDn0jukwVzFap0k2Cv1Sn60/24ksNIraor656p/fvwO689J/UPkMXuOrIU/ij6XAbvEQ7HGMp4Smqs3+IVBOGfeFLoQ7nMc= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1717446156; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=quXJ9+rgL9ZdvyTuy2moe+BMW/tbcTlAhui9BjvtQAo=; b=JbCt70RTr0SUiZkP/JAYl/fTZms01bBBGRcXuvItsC0P6WNLjn1U7Bm+67cf1G9Z54PW1W Sz+iAM9EBLtxIGOMq4/gXYpgR9EjYdGdIMWAVLgMyB04xVBqG+uayLusAonH+YuG8JYR/O 5qzBUXJ10Hw8lNNucFsScuU/v6uMxgE= Received: from mail-yb1-f198.google.com (mail-yb1-f198.google.com [209.85.219.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-313-BMicgxiFNTmPwIJ_e2PUgw-1; Mon, 03 Jun 2024 16:22:35 -0400 X-MC-Unique: BMicgxiFNTmPwIJ_e2PUgw-1 Received: by mail-yb1-f198.google.com with SMTP id 3f1490d57ef6-dfa72779f04so5919629276.1 for ; Mon, 03 Jun 2024 13:22:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717446154; x=1718050954; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=quXJ9+rgL9ZdvyTuy2moe+BMW/tbcTlAhui9BjvtQAo=; b=cy7Z4G3NQk+tz053mMhWiH4SHEHv0sKzUx67a/jntkv7siTvsnoZrV+vcMoX0302wR E0Gh4WHc4jj68QmEw09w4aeYzYRhHpDlEtlL7+b9FkXlQVCADOOmlwUKfp8+XdLc5JI3 w4Dg3roF4Ch+49Yrz3P89J3sJTi1JXv8E534yy+ob/E9oGlbTyz5nXW4gWAJ0EXbWLgF 8YgZDtMflanyzO2ZwMK/ksgqSfQ5nwUigKqAnyItrYCM8v4x+RR9CpjRw5hkV8anvb1h 7fCkkf5OUAG+5LNmf4lDOxTCAJLl9VyMq387ZbY0jcQKoGD+zBHxWc354DqVdSxiw64r H+XQ== X-Gm-Message-State: AOJu0YyGF1q4RhOQqvTlPQA+NZOzHg6Q9PhA1upg9zo8Rg75VjnrumKl fGHTRWqATDrRGPuC1jff0SswG6SOxL2srhTVZAqygpElSyGbXDGXxkb0ZLBJTfJFVmTRo8ojnV1 SETaIFLQTKYpzpb1uU8DI0pciPLTUcHT/IbEf4LojkEC0+vyCCq/YQdcZB74poNg7x29WZvrE8W Ufow4r+y7OFYRoMz3/3RtG/T6YH7ulxkJvg1Y= X-Received: by 2002:a25:c750:0:b0:de6:896:26f0 with SMTP id 3f1490d57ef6-dfa73bcf60bmr10286550276.1.1717446154573; Mon, 03 Jun 2024 13:22:34 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEsWeBrR1Ar4nUUE08s4559sXjdTL2CJXdV8WR6h7ab8T7Oa3662OLx78IbebTLeivChB16y+ibA3RjM/TAhSA= X-Received: by 2002:a25:c750:0:b0:de6:896:26f0 with SMTP id 3f1490d57ef6-dfa73bcf60bmr10286531276.1.1717446154182; Mon, 03 Jun 2024 13:22:34 -0700 (PDT) MIME-Version: 1.0 References: <20240601102927.878453-1-jwakely@redhat.com> <20240601112310.893276-1-jwakely@redhat.com> In-Reply-To: <20240601112310.893276-1-jwakely@redhat.com> From: Jonathan Wakely Date: Mon, 3 Jun 2024 21:22:18 +0100 Message-ID: Subject: Re: [PATCH v2 1/2] libstdc++: Handle extended alignment in std::get_temporary_buffer [PR105258] To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Sat, 1 Jun 2024 at 12:23, Jonathan Wakely 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 > + 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 > 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 the buffer s address and > + * A pair<> is returned containing the buffer's address and > * capacity (in the units of sizeof(_Tp)), or a pair of 0 values if > * no storage can be obtained. 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 > + _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 __p( > + std::get_temporary_buffer(__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 > + // Requirements: > + // _Tp is move constructible and constructible from std::move(*__seed). > + template > 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 > _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 __p( > - std::get_temporary_buffer(_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 > +#include > +#include > + > +struct alignas(__STDCPP_DEFAULT_NEW_ALIGNMENT__ * 2) Overaligned > +{ > + ~Overaligned() > + { > + auto alignment = reinterpret_cast(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 >