From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2181) id E23E33858C53; Tue, 28 Feb 2023 09:50:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E23E33858C53 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1677577802; bh=SDB+Tjg514ikJCujiwtwFymg4zD7pUbV3go4Kvu++ms=; h=From:To:Subject:Date:From; b=K+2rWCyG5z66iJ5P8u1Wa8KIfxV/lQUYGpcD19s0xpXcg9I7xlyNKj3nOt4UM6zsl dYpIoUou4Nt9PVdIvFU9Cbpu1HTVBkLCVtMHLBLqQTZzsN6sILWIoiglU735jVcCld PosOILdfJsIiXKLLvoDhrRS7JzNeUw+hl1PBaeLs= MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Jonathan Wakely To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org Subject: [gcc r13-6372] libstdc++: Do not use memmove for 1-element ranges [PR108846] X-Act-Checkin: gcc X-Git-Author: Jonathan Wakely X-Git-Refname: refs/heads/master X-Git-Oldrev: a41a56dee5c2d48337739d60c43cab5074bcc8e7 X-Git-Newrev: 822a11a1e642e0abe92a996e7033a5066905a447 Message-Id: <20230228095002.E23E33858C53@sourceware.org> Date: Tue, 28 Feb 2023 09:50:02 +0000 (GMT) List-Id: https://gcc.gnu.org/g:822a11a1e642e0abe92a996e7033a5066905a447 commit r13-6372-g822a11a1e642e0abe92a996e7033a5066905a447 Author: Jonathan Wakely Date: Sat Feb 25 14:28:36 2023 +0000 libstdc++: Do not use memmove for 1-element ranges [PR108846] 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. Diff: --- 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(-) 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(); +}