From patchwork Tue Feb 28 09:50:48 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 65729 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DAE58384FB6D for ; Tue, 28 Feb 2023 09:53:07 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DAE58384FB6D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1677577987; bh=jwjXhHVGjwOJ74dMu7o6PJ+spqqdHEkxs/51GXb02q4=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=epDBo3CePCQ5VvDsrx7pG/H/ZLTKKWvLXzhySSwd3+irbQ8P+Ml8Dl6i7Eq7tpApc XiXoCmpyE7cCUrJwUzyHTEFPHFg6J60DvESs9Y4J8qGPctevnMFXnx/x3kzLmhDCxl 02OIXInY+MRpt9mD9sgWOllIP1j8aplVPzY4wjOE= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org 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 A5B483857835 for ; Tue, 28 Feb 2023 09:50:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A5B483857835 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-266-i45pQyS8O0eiNGMI7xvzTA-1; Tue, 28 Feb 2023 04:50:49 -0500 X-MC-Unique: i45pQyS8O0eiNGMI7xvzTA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0B28D1C08782; Tue, 28 Feb 2023 09:50:49 +0000 (UTC) Received: from localhost (unknown [10.33.36.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id ABEF8404BEC6; Tue, 28 Feb 2023 09:50:48 +0000 (UTC) To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Do not use memmove for 1-element ranges [PR108846] Date: Tue, 28 Feb 2023 09:50:48 +0000 Message-Id: <20230228095048.1193075-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-12.1 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_H2, SPF_HELO_NONE, SPF_NONE, TXREP, URI_HEX autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" Tested powerpc64le-linux. Pushed to trunk. -- >8 -- This avoids overwriting tail padding when algorithms like std::copy are used to write a single value through a pointer to a base subobject. The pointer arithmetic on a Base* is valid for N==1, but the copy/move operation needs to be done using assignment, not a memmove or memcpy of sizeof(Base) bytes. Instead of putting a check for N==1 in all of copy, copy_n, move etc. this adds it to the __copy_move and __copy_move_backward partial specializations used for trivially copyable types. When N==1 those partial specializations dispatch to new static member functions of the partial specializations for non-trivial types, so that a copy/move assignment is done appropriately for the _IsMove constant. libstdc++-v3/ChangeLog: PR libstdc++/108846 * include/bits/stl_algobase.h (__copy_move) Add __assign_one static member function. (__copy_move): Likewise. (__copy_move): Do not use memmove for a single value. (__copy_move_backward): Likewise. * testsuite/25_algorithms/copy/108846.cc: New test. * testsuite/25_algorithms/copy_backward/108846.cc: New test. * testsuite/25_algorithms/copy_n/108846.cc: New test. * testsuite/25_algorithms/move/108846.cc: New test. * testsuite/25_algorithms/move_backward/108846.cc: New test. --- libstdc++-v3/include/bits/stl_algobase.h | 46 ++++++++------- .../testsuite/25_algorithms/copy/108846.cc | 58 +++++++++++++++++++ .../25_algorithms/copy_backward/108846.cc | 58 +++++++++++++++++++ .../testsuite/25_algorithms/copy_n/108846.cc | 58 +++++++++++++++++++ .../testsuite/25_algorithms/move/108846.cc | 58 +++++++++++++++++++ .../25_algorithms/move_backward/108846.cc | 58 +++++++++++++++++++ 6 files changed, 314 insertions(+), 22 deletions(-) create mode 100644 libstdc++-v3/testsuite/25_algorithms/copy/108846.cc create mode 100644 libstdc++-v3/testsuite/25_algorithms/copy_backward/108846.cc create mode 100644 libstdc++-v3/testsuite/25_algorithms/copy_n/108846.cc create mode 100644 libstdc++-v3/testsuite/25_algorithms/move/108846.cc create mode 100644 libstdc++-v3/testsuite/25_algorithms/move_backward/108846.cc diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h index a30fc299d26..4a6f8195d98 100644 --- a/libstdc++-v3/include/bits/stl_algobase.h +++ b/libstdc++-v3/include/bits/stl_algobase.h @@ -391,6 +391,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } return __result; } + + template + static void + __assign_one(_Tp* __to, _Up* __from) + { *__to = *__from; } }; #if __cplusplus >= 201103L @@ -411,27 +416,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } return __result; } + + template + static void + __assign_one(_Tp* __to, _Up* __from) + { *__to = std::move(*__from); } }; #endif template struct __copy_move<_IsMove, true, random_access_iterator_tag> { - template + template _GLIBCXX20_CONSTEXPR - static _Tp* - __copy_m(const _Tp* __first, const _Tp* __last, _Tp* __result) + static _Up* + __copy_m(_Tp* __first, _Tp* __last, _Up* __result) { -#if __cplusplus >= 201103L - using __assignable = __conditional_t<_IsMove, - is_move_assignable<_Tp>, - is_copy_assignable<_Tp>>; - // trivial types can have deleted assignment - static_assert( __assignable::value, "type must be assignable" ); -#endif const ptrdiff_t _Num = __last - __first; - if (_Num) + if (__builtin_expect(_Num > 1, true)) __builtin_memmove(__result, __first, sizeof(_Tp) * _Num); + else if (_Num == 1) + std::__copy_move<_IsMove, false, random_access_iterator_tag>:: + __assign_one(__result, __first); return __result + _Num; } }; @@ -732,21 +738,17 @@ _GLIBCXX_END_NAMESPACE_CONTAINER template struct __copy_move_backward<_IsMove, true, random_access_iterator_tag> { - template + template _GLIBCXX20_CONSTEXPR - static _Tp* - __copy_move_b(const _Tp* __first, const _Tp* __last, _Tp* __result) + static _Up* + __copy_move_b(_Tp* __first, _Tp* __last, _Up* __result) { -#if __cplusplus >= 201103L - using __assignable = __conditional_t<_IsMove, - is_move_assignable<_Tp>, - is_copy_assignable<_Tp>>; - // trivial types can have deleted assignment - static_assert( __assignable::value, "type must be assignable" ); -#endif const ptrdiff_t _Num = __last - __first; - if (_Num) + if (__builtin_expect(_Num > 1, true)) __builtin_memmove(__result - _Num, __first, sizeof(_Tp) * _Num); + else if (_Num == 1) + std::__copy_move<_IsMove, false, random_access_iterator_tag>:: + __assign_one(__result - 1, __first); return __result - _Num; } }; diff --git a/libstdc++-v3/testsuite/25_algorithms/copy/108846.cc b/libstdc++-v3/testsuite/25_algorithms/copy/108846.cc new file mode 100644 index 00000000000..964028901b8 --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/copy/108846.cc @@ -0,0 +1,58 @@ +// { dg-do run } + +#include +#include + +// PR libstdc++/108846 std::copy, std::copy_n and std::copy_backward +// on potentially overlapping subobjects + +struct B { + B(int i, short j) : i(i), j(j) {} + int i; + short j; +}; +struct D : B { + D(int i, short j, short x) : B(i, j), x(x) {} + short x; // Stored in tail padding of B +}; + +void +test_pr108846() +{ + D ddst(1, 2, 3); + D dsrc(4, 5, 6); + B *dst = &ddst; + B *src = &dsrc; + // If this is optimized to memmove it will overwrite tail padding. + std::copy(src, src+1, dst); + VERIFY(ddst.x == 3); +} + +struct B2 { + B2(int i, short j) : i(i), j(j) {} + B2& operator=(B2& b) { i = b.i; j = b.j; return *this; } + int i; + short j; +}; +struct D2 : B2 { + D2(int i, short j, short x) : B2(i, j), x(x) {} + short x; // Stored in tail padding of B2 +}; + +void +test_non_const_copy_assign() +{ + D2 ddst(1, 2, 3); + D2 dsrc(4, 5, 6); + B2 *dst = &ddst; + B2 *src = &dsrc; + // Ensure the not-taken trivial copy path works for this type. + std::copy(src, src+1, dst); + VERIFY(ddst.x == 3); +} + +int main() +{ + test_pr108846(); + test_non_const_copy_assign(); +} diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_backward/108846.cc b/libstdc++-v3/testsuite/25_algorithms/copy_backward/108846.cc new file mode 100644 index 00000000000..84b3d5a285b --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/copy_backward/108846.cc @@ -0,0 +1,58 @@ +// { dg-do run } + +#include +#include + +// PR libstdc++/108846 std::copy, std::copy_n and std::copy_backward +// on potentially overlapping subobjects + +struct B { + B(int i, short j) : i(i), j(j) {} + int i; + short j; +}; +struct D : B { + D(int i, short j, short x) : B(i, j), x(x) {} + short x; // Stored in tail padding of B +}; + +void +test_pr108846() +{ + D ddst(1, 2, 3); + D dsrc(4, 5, 6); + B *dst = &ddst; + B *src = &dsrc; + // If this is optimized to memmove it will overwrite tail padding. + std::copy_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +} + +struct B2 { + B2(int i, short j) : i(i), j(j) {} + B2& operator=(B2& b) { i = b.i; j = b.j; return *this; } + int i; + short j; +}; +struct D2 : B2 { + D2(int i, short j, short x) : B2(i, j), x(x) {} + short x; // Stored in tail padding of B2 +}; + +void +test_non_const_copy_assign() +{ + D2 ddst(1, 2, 3); + D2 dsrc(4, 5, 6); + B2 *dst = &ddst; + B2 *src = &dsrc; + // Ensure the not-taken trivial copy path works for this type. + std::copy_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +} + +int main() +{ + test_pr108846(); + test_non_const_copy_assign(); +} diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/108846.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/108846.cc new file mode 100644 index 00000000000..9ed43e154b7 --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/108846.cc @@ -0,0 +1,58 @@ +// { dg-do run { target c++11 } } + +#include +#include + +// PR libstdc++/108846 std::copy, std::copy_n and std::copy_backward +// on potentially overlapping subobjects + +struct B { + B(int i, short j) : i(i), j(j) {} + int i; + short j; +}; +struct D : B { + D(int i, short j, short x) : B(i, j), x(x) {} + short x; // Stored in tail padding of B +}; + +void +test_pr108846() +{ + D ddst(1, 2, 3); + D dsrc(4, 5, 6); + B *dst = &ddst; + B *src = &dsrc; + // If this is optimized to memmove it will overwrite tail padding. + std::copy_n(src, 1, dst); + VERIFY(ddst.x == 3); +} + +struct B2 { + B2(int i, short j) : i(i), j(j) {} + B2& operator=(B2&) = default; + int i; + short j; +}; +struct D2 : B2 { + D2(int i, short j, short x) : B2(i, j), x(x) {} + short x; // Stored in tail padding of B2 +}; + +void +test_non_const_copy_assign() +{ + D2 ddst(1, 2, 3); + D2 dsrc(4, 5, 6); + B2 *dst = &ddst; + B2 *src = &dsrc; + // Ensure the not-taken trivial copy path works for this type. + std::copy_n(src, 1, dst); + VERIFY(ddst.x == 3); +} + +int main() +{ + test_pr108846(); + test_non_const_copy_assign(); +} diff --git a/libstdc++-v3/testsuite/25_algorithms/move/108846.cc b/libstdc++-v3/testsuite/25_algorithms/move/108846.cc new file mode 100644 index 00000000000..8248b87d5e2 --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/move/108846.cc @@ -0,0 +1,58 @@ +// { dg-do run { target c++11 } } + +#include +#include + +// PR libstdc++/108846 std::copy, std::copy_n and std::copy_backward +// on potentially overlapping subobjects + +struct B { + B(int i, short j) : i(i), j(j) {} + int i; + short j; +}; +struct D : B { + D(int i, short j, short x) : B(i, j), x(x) {} + short x; // Stored in tail padding of B +}; + +void +test_pr108846() +{ + D ddst(1, 2, 3); + D dsrc(4, 5, 6); + B *dst = &ddst; + B *src = &dsrc; + // If this is optimized to memmove it will overwrite tail padding. + std::move(src, src+1, dst); + VERIFY(ddst.x == 3); +} + +struct B3 { + B3(int i, short j) : i(i), j(j) {} + B3& operator=(B3&&) = default; + int i; + short j; +}; +struct D3 : B3 { + D3(int i, short j, short x) : B3(i, j), x(x) {} + short x; // Stored in tail padding of B3 +}; + +void +test_move_only() +{ + D3 ddst(1, 2, 3); + D3 dsrc(4, 5, 6); + B3 *dst = &ddst; + B3 *src = &dsrc; + // Ensure the not-taken trivial copy path works for this type. + std::move(src, src+1, dst); + VERIFY(ddst.x == 3); +} + +int main() +{ + test_pr108846(); + test_move_only(); +} diff --git a/libstdc++-v3/testsuite/25_algorithms/move_backward/108846.cc b/libstdc++-v3/testsuite/25_algorithms/move_backward/108846.cc new file mode 100644 index 00000000000..035743560e0 --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/move_backward/108846.cc @@ -0,0 +1,58 @@ +// { dg-do run { target c++11 } } + +#include +#include + +// PR libstdc++/108846 std::copy, std::copy_n and std::copy_backward +// on potentially overlapping subobjects + +struct B { + B(int i, short j) : i(i), j(j) {} + int i; + short j; +}; +struct D : B { + D(int i, short j, short x) : B(i, j), x(x) {} + short x; // Stored in tail padding of B +}; + +void +test_pr108846() +{ + D ddst(1, 2, 3); + D dsrc(4, 5, 6); + B *dst = &ddst; + B *src = &dsrc; + // If this is optimized to memmove it will overwrite tail padding. + std::move_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +} + +struct B3 { + B3(int i, short j) : i(i), j(j) {} + B3& operator=(B3&&) = default; + int i; + short j; +}; +struct D3 : B3 { + D3(int i, short j, short x) : B3(i, j), x(x) {} + short x; // Stored in tail padding of B3 +}; + +void +test_move_only() +{ + D3 ddst(1, 2, 3); + D3 dsrc(4, 5, 6); + B3 *dst = &ddst; + B3 *src = &dsrc; + // Ensure the not-taken trivial copy path works for this type. + std::move_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +} + +int main() +{ + test_pr108846(); + test_move_only(); +}