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.133.124]) by sourceware.org (Postfix) with ESMTPS id 9E3FD3882AE3 for ; Sat, 1 Jun 2024 11:23:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9E3FD3882AE3 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 9E3FD3882AE3 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717240998; cv=none; b=vGU3ODNzJeUtw2drQC7gJC0ZOHsyKa4NLbaT6qgCROgpHFEH8eYyIQpDj7GKqe5CdvAJavNKK0LCG9iKTMzSFvu2g0trnwujez1G/RwjmLXJ6oY+IWptpLNk8zQhbz9JT6YN3YXo7uhwZMof74rsgCdABlQoR7DOtVTPXPLdDqk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717240998; c=relaxed/simple; bh=yd+UAYn/PLcCwsU1+5/JBTrRLub6c102n6FnjFKj1ms=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=wgFIdBGFvopiqs5cWHLYxv44QajUbEc5gfdBzOrLK1U0gj0hA4dbsQ93y7A675QiZN3Xs5Me6+mkroPFlo+kE2FO7d0izYdnX0e9gXyxZRTMEfBv6gF9gQcV1QL/EkrExyYCOjeaofOH/VqC00jCHyeedB8a/azonWpFyyGhWlM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1717240994; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=erQUlUgpJP2SrcwSItpApp2ARxIPUVTRELp1es53jqk=; b=DDaV5r7hSIyTQ20KDQsrNXqQ5CkojZC7DXcnlQPdABSI/JyaOM4d7HtnGUjVgxkALYHJUC wJtbhLMJgtnkkxTXteGKcvgI6fiSyUcVE9h/eaLhUeJ65redz6srtIGFahjIKwyw/I0bmz FRy2ESEfnz9dVY6LD428sRCwGXmLO70= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-658-LBTpO13kOoiC-z8pq3U8fg-1; Sat, 01 Jun 2024 07:23:11 -0400 X-MC-Unique: LBTpO13kOoiC-z8pq3U8fg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 2369F81227E; Sat, 1 Jun 2024 11:23:11 +0000 (UTC) Received: from localhost (unknown [10.42.28.51]) by smtp.corp.redhat.com (Postfix) with ESMTP id CD266C15BB1; Sat, 1 Jun 2024 11:23:10 +0000 (UTC) From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [PATCH v2 1/2] libstdc++: Handle extended alignment in std::get_temporary_buffer [PR105258] Date: Sat, 1 Jun 2024 12:19:01 +0100 Message-ID: <20240601112310.893276-1-jwakely@redhat.com> In-Reply-To: <20240601102927.878453-1-jwakely@redhat.com> References: <20240601102927.878453-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.9 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=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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