diff options
author | Giuseppe D'Angelo <giuseppe.dangelo@kdab.com> | 2024-08-23 15:05:54 +0200 |
---|---|---|
committer | Jonathan Wakely <redi@gcc.gnu.org> | 2024-09-13 13:50:29 +0100 |
commit | 5938e0681c3907b2771ce6717988416b0ddd2f54 (patch) | |
tree | 9a637dcabba0f05a0058b57c637a4cbe53c77549 | |
parent | 494d3c3faaee0dbde696ea334f8e242ae85ae2b5 (diff) | |
download | gcc-5938e0681c3907b2771ce6717988416b0ddd2f54.zip gcc-5938e0681c3907b2771ce6717988416b0ddd2f54.tar.gz gcc-5938e0681c3907b2771ce6717988416b0ddd2f54.tar.bz2 |
libstdc++: Do not use use memmove for 1-element ranges [PR108846,PR116471]
This commit ports the fixes already applied by r13-6372-g822a11a1e642e0
to the range-based versions of copy/move algorithms.
When doing so, a further bug (PR116471) was discovered in the
implementation of the range-based algorithms: although the algorithms
are already constrained by the indirectly_copyable/movable concepts,
there was a failing static_assert in the memmove path.
This static_assert checked that iterator's value type was assignable by
using the is_copy_assignable (move) type traits. However, this is a
problem, because the traits are too strict when checking for constness;
a type like
struct S { S& operator=(S &) = default; };
is trivially copyable (and thus could benefit of the memmove path),
but it does not satisfy is_copy_assignable because the operator takes
by non-const reference.
Now, the reason for the check to be there is because a type with
a deleted assignment operator like
struct E { E& operator=(const E&) = delete; };
is still trivially copyable, but not assignable. We don't want
algorithms like std::ranges::copy to compile because they end up
selecting the memmove path, "ignoring" the fact that E isn't even
copy assignable.
But the static_assert isn't needed here any longer: as noted before,
the ranges algorithms already have the appropriate constraints; and
even if they didn't, there's now a non-discarded codepath to deal with
ranges of length 1 where there is an explicit assignment operation.
Therefore, this commit removes it. (In fact, r13-6372-g822a11a1e642e0
removed the same static_assert from the non-ranges algorithms.)
libstdc++-v3/ChangeLog:
PR libstdc++/108846
PR libstdc++/116471
* include/bits/ranges_algobase.h (__assign_one): New helper
function.
(__copy_or_move): Remove a spurious static_assert; use
__assign_one for memcpyable ranges of length 1.
(__copy_or_move_backward): Likewise.
* testsuite/25_algorithms/copy/108846.cc: Extend to range-based
algorithms, and cover both memcpyable and non-memcpyable
cases.
* testsuite/25_algorithms/copy_backward/108846.cc: Likewise.
* testsuite/25_algorithms/copy_n/108846.cc: Likewise.
* testsuite/25_algorithms/move/108846.cc: Likewise.
* testsuite/25_algorithms/move_backward/108846.cc: Likewise.
Signed-off-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
6 files changed, 209 insertions, 29 deletions
diff --git a/libstdc++-v3/include/bits/ranges_algobase.h b/libstdc++-v3/include/bits/ranges_algobase.h index fd35b8b..9b45cbc 100644 --- a/libstdc++-v3/include/bits/ranges_algobase.h +++ b/libstdc++-v3/include/bits/ranges_algobase.h @@ -225,6 +225,16 @@ namespace ranges copy_backward_result<_Iter, _Out>> __copy_or_move_backward(_Iter __first, _Sent __last, _Out __result); + template<bool _IsMove, typename _Iter, typename _Out> + constexpr void + __assign_one(_Iter& __iter, _Out& __result) + { + if constexpr (_IsMove) + *__result = std::move(*__iter); + else + *__result = *__iter; + } + template<bool _IsMove, input_iterator _Iter, sentinel_for<_Iter> _Sent, weakly_incrementable _Out> @@ -279,23 +289,19 @@ namespace ranges if constexpr (__memcpyable<_Iter, _Out>::__value) { using _ValueTypeI = iter_value_t<_Iter>; - static_assert(_IsMove - ? is_move_assignable_v<_ValueTypeI> - : is_copy_assignable_v<_ValueTypeI>); auto __num = __last - __first; - if (__num) + if (__num > 1) [[likely]] __builtin_memmove(__result, __first, - sizeof(_ValueTypeI) * __num); + sizeof(_ValueTypeI) * __num); + else if (__num == 1) + ranges::__assign_one<_IsMove>(__first, __result); return {__first + __num, __result + __num}; } } for (auto __n = __last - __first; __n > 0; --__n) { - if constexpr (_IsMove) - *__result = std::move(*__first); - else - *__result = *__first; + ranges::__assign_one<_IsMove>(__first, __result); ++__first; ++__result; } @@ -305,10 +311,7 @@ namespace ranges { while (__first != __last) { - if constexpr (_IsMove) - *__result = std::move(*__first); - else - *__result = *__first; + ranges::__assign_one<_IsMove>(__first, __result); ++__first; ++__result; } @@ -414,13 +417,12 @@ namespace ranges if constexpr (__memcpyable<_Out, _Iter>::__value) { using _ValueTypeI = iter_value_t<_Iter>; - static_assert(_IsMove - ? is_move_assignable_v<_ValueTypeI> - : is_copy_assignable_v<_ValueTypeI>); auto __num = __last - __first; - if (__num) + if (__num > 1) [[likely]] __builtin_memmove(__result - __num, __first, sizeof(_ValueTypeI) * __num); + else if (__num == 1) + ranges::__assign_one<_IsMove>(__first, __result); return {__first + __num, __result - __num}; } } @@ -432,10 +434,7 @@ namespace ranges { --__tail; --__result; - if constexpr (_IsMove) - *__result = std::move(*__tail); - else - *__result = *__tail; + ranges::__assign_one<_IsMove>(__tail, __result); } return {std::move(__lasti), std::move(__result)}; } @@ -448,10 +447,7 @@ namespace ranges { --__tail; --__result; - if constexpr (_IsMove) - *__result = std::move(*__tail); - else - *__result = *__tail; + ranges::__assign_one<_IsMove>(__tail, __result); } return {std::move(__lasti), std::move(__result)}; } diff --git a/libstdc++-v3/testsuite/25_algorithms/copy/108846.cc b/libstdc++-v3/testsuite/25_algorithms/copy/108846.cc index 9640289..e3b722c 100644 --- a/libstdc++-v3/testsuite/25_algorithms/copy/108846.cc +++ b/libstdc++-v3/testsuite/25_algorithms/copy/108846.cc @@ -26,6 +26,10 @@ test_pr108846() // If this is optimized to memmove it will overwrite tail padding. std::copy(src, src+1, dst); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::copy(src, src+1, dst); + VERIFY(ddst.x == 3); +#endif } struct B2 { @@ -49,10 +53,44 @@ test_non_const_copy_assign() // Ensure the not-taken trivial copy path works for this type. std::copy(src, src+1, dst); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::copy(src, src+1, dst); + VERIFY(ddst.x == 3); +#endif +} + +struct B3 { + B3(int i, short j) : i(i), j(j) {} +#if __cplusplus >= 201103L + B3& operator=(B3& b) = default; +#endif + 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_non_const_copy_assign_trivial() +{ + D3 ddst(1, 2, 3); + D3 dsrc(4, 5, 6); + B3 *dst = &ddst; + B3 *src = &dsrc; + // If this is optimized to memmove it will overwrite tail padding. + std::copy(src, src+1, dst); + VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::copy(src, src+1, dst); + VERIFY(ddst.x == 3); +#endif } int main() { test_pr108846(); test_non_const_copy_assign(); + test_non_const_copy_assign_trivial(); } diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_backward/108846.cc b/libstdc++-v3/testsuite/25_algorithms/copy_backward/108846.cc index 84b3d5a..206748d 100644 --- a/libstdc++-v3/testsuite/25_algorithms/copy_backward/108846.cc +++ b/libstdc++-v3/testsuite/25_algorithms/copy_backward/108846.cc @@ -26,6 +26,10 @@ test_pr108846() // If this is optimized to memmove it will overwrite tail padding. std::copy_backward(src, src+1, dst+1); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::copy_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +#endif } struct B2 { @@ -49,10 +53,44 @@ test_non_const_copy_assign() // Ensure the not-taken trivial copy path works for this type. std::copy_backward(src, src+1, dst+1); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::copy_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +#endif +} + +struct B3 { + B3(int i, short j) : i(i), j(j) {} +#if __cplusplus >= 201103L + B3& operator=(B3& b) = default; +#endif + 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_non_const_copy_assign_trivial() +{ + D3 ddst(1, 2, 3); + D3 dsrc(4, 5, 6); + B3 *dst = &ddst; + B3 *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); +#if __cpp_lib_ranges >= 201911L + std::ranges::copy_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +#endif } int main() { test_pr108846(); test_non_const_copy_assign(); + test_non_const_copy_assign_trivial(); } diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/108846.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/108846.cc index 9ed43e1..50deb5d 100644 --- a/libstdc++-v3/testsuite/25_algorithms/copy_n/108846.cc +++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/108846.cc @@ -26,11 +26,15 @@ test_pr108846() // If this is optimized to memmove it will overwrite tail padding. std::copy_n(src, 1, dst); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::copy_n(src, 1, dst); + VERIFY(ddst.x == 3); +#endif } struct B2 { B2(int i, short j) : i(i), j(j) {} - B2& operator=(B2&) = default; + B2& operator=(B2& b) { i = b.i; j = b.j; return *this; } int i; short j; }; @@ -49,10 +53,42 @@ test_non_const_copy_assign() // Ensure the not-taken trivial copy path works for this type. std::copy_n(src, 1, dst); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::copy_n(src, 1, dst); + VERIFY(ddst.x == 3); +#endif +} + +struct B3 { + B3(int i, short j) : i(i), j(j) {} + B3& operator=(B3& b) = 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_non_const_copy_assign_trivial() +{ + D3 ddst(1, 2, 3); + D3 dsrc(4, 5, 6); + B3 *dst = &ddst; + B3 *src = &dsrc; + // If this is optimized to memmove it will overwrite tail padding. + std::copy_n(src, 1, dst); + VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::copy_n(src, 1, dst); + VERIFY(ddst.x == 3); +#endif } int main() { test_pr108846(); test_non_const_copy_assign(); + test_non_const_copy_assign_trivial(); } diff --git a/libstdc++-v3/testsuite/25_algorithms/move/108846.cc b/libstdc++-v3/testsuite/25_algorithms/move/108846.cc index 8248b87..2dd52a4 100644 --- a/libstdc++-v3/testsuite/25_algorithms/move/108846.cc +++ b/libstdc++-v3/testsuite/25_algorithms/move/108846.cc @@ -26,6 +26,37 @@ test_pr108846() // If this is optimized to memmove it will overwrite tail padding. std::move(src, src+1, dst); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::move(src, src+1, dst); + VERIFY(ddst.x == 3); +#endif +} + +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_move_only() +{ + 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::move(src, src+1, dst); + VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::move(src, src+1, dst); + VERIFY(ddst.x == 3); +#endif } struct B3 { @@ -40,19 +71,24 @@ struct D3 : B3 { }; void -test_move_only() +test_move_only_trivial() { 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. + // If this is optimized to memmove it will overwrite tail padding. std::move(src, src+1, dst); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::move(src, src+1, dst); + VERIFY(ddst.x == 3); +#endif } int main() { test_pr108846(); test_move_only(); + test_move_only_trivial(); } diff --git a/libstdc++-v3/testsuite/25_algorithms/move_backward/108846.cc b/libstdc++-v3/testsuite/25_algorithms/move_backward/108846.cc index 0357435..92ac0f5 100644 --- a/libstdc++-v3/testsuite/25_algorithms/move_backward/108846.cc +++ b/libstdc++-v3/testsuite/25_algorithms/move_backward/108846.cc @@ -26,6 +26,37 @@ test_pr108846() // If this is optimized to memmove it will overwrite tail padding. std::move_backward(src, src+1, dst+1); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::move_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +#endif +} + +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_move_only() +{ + 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::move_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::move_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +#endif } struct B3 { @@ -40,7 +71,7 @@ struct D3 : B3 { }; void -test_move_only() +test_move_only_trivial() { D3 ddst(1, 2, 3); D3 dsrc(4, 5, 6); @@ -49,10 +80,15 @@ test_move_only() // Ensure the not-taken trivial copy path works for this type. std::move_backward(src, src+1, dst+1); VERIFY(ddst.x == 3); +#if __cpp_lib_ranges >= 201911L + std::ranges::move_backward(src, src+1, dst+1); + VERIFY(ddst.x == 3); +#endif } int main() { test_pr108846(); test_move_only(); + test_move_only_trivial(); } |