public inbox for gcc-cvs@sourceware.org
help / color / mirror / Atom feed
* [gcc r13-6372] libstdc++: Do not use memmove for 1-element ranges [PR108846]
@ 2023-02-28 9:50 Jonathan Wakely
0 siblings, 0 replies; only message in thread
From: Jonathan Wakely @ 2023-02-28 9:50 UTC (permalink / raw)
To: gcc-cvs, libstdc++-cvs
https://gcc.gnu.org/g:822a11a1e642e0abe92a996e7033a5066905a447
commit r13-6372-g822a11a1e642e0abe92a996e7033a5066905a447
Author: Jonathan Wakely <jwakely@redhat.com>
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<false, false, RA>)
Add __assign_one static member function.
(__copy_move<true, false, RA>): Likewise.
(__copy_move<IsMove, true, RA>): Do not use memmove for a single
value.
(__copy_move_backward<IsMove, true, RA>): 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<typename _Tp, typename _Up>
+ static void
+ __assign_one(_Tp* __to, _Up* __from)
+ { *__to = *__from; }
};
#if __cplusplus >= 201103L
@@ -411,27 +416,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
return __result;
}
+
+ template<typename _Tp, typename _Up>
+ static void
+ __assign_one(_Tp* __to, _Up* __from)
+ { *__to = std::move(*__from); }
};
#endif
template<bool _IsMove>
struct __copy_move<_IsMove, true, random_access_iterator_tag>
{
- template<typename _Tp>
+ template<typename _Tp, typename _Up>
_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<bool _IsMove>
struct __copy_move_backward<_IsMove, true, random_access_iterator_tag>
{
- template<typename _Tp>
+ template<typename _Tp, typename _Up>
_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 <algorithm>
+#include <testsuite_hooks.h>
+
+// 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 <algorithm>
+#include <testsuite_hooks.h>
+
+// 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 <algorithm>
+#include <testsuite_hooks.h>
+
+// 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 <algorithm>
+#include <testsuite_hooks.h>
+
+// 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 <algorithm>
+#include <testsuite_hooks.h>
+
+// 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();
+}
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2023-02-28 9:50 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-28 9:50 [gcc r13-6372] libstdc++: Do not use memmove for 1-element ranges [PR108846] Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).