On 25/02/20 12:40 +0000, Jonathan Wakely wrote:
The std::move and std::move_backward algorithms dispatch to the
std::__memmove helper when appropriate. That function uses a
pointer-to-const for the source values, preventing them from being
moved. The two callers of that function have the same problem.
Rather than altering __memmove and its callers to work with const or
non-const source pointers, this takes a more conservative approach of
casting away the const at the point where we want to do a move
assignment. This relies on the fact that we only use __memmove when the
type is trivially copyable, so we know the move assignment doesn't alter
the source anyway.
PR libstdc++/93872
* include/bits/stl_algobase.h (__memmove): Cast away const before
doing move assignment.
* testsuite/25_algorithms/move/93872.cc: New test.
* testsuite/25_algorithms/move_backward/93872.cc: New test.
I think what I'd really like to do is get rid of __memmove entirely.
We already have code that does the explicit assignment in a loop, for
the cases where we can't use __builtin_memmove because the type is not
trivially copyable.
We should just use that existing code during constant evaluation, i.e.
don't do the __builtin_memmove optimizations during constant
evaluation. It seems much cleaner to just not use the optimization
rather than wrap it to be usable in constant expressions.
We already have to do that for {copy,move}_backward anyway, because
__memmove doesn't correctly implement the std::memmove semantics for
overlapping ranges. But we do it **wrong** and turn copy_backward into
move_backward during constant evaluation.
Here's a patch that gets rid of __memmove and fixes that bug
(generated with 'git diff -b' so that the changes to the logic aren't
obscured by the whitespace changes caused by re-indenting).
Maybe I should just go ahead and do this now, since __memmove (and the
problems it causes) are new for GCC 10 anyway. That would revert
<bits/stl_algobase.h> to something closer to the GCC 9 version.
diff --git a/libstdc++-v3/include/bits/ranges_algobase.h b/libstdc++-v3/include/bits/ranges_algobase.h
index 73f0205ba7f..feb6c5723dd 100644
--- a/libstdc++-v3/include/bits/ranges_algobase.h
+++ b/libstdc++-v3/include/bits/ranges_algobase.h
@@ -248,6 +248,10 @@ namespace ranges
}
else if constexpr (sized_sentinel_for<_Sent, _Iter>)
{
+#ifdef __cpp_lib_is_constant_evaluated
+ if (!std::is_constant_evaluated())
+#endif
+ {
using _ValueTypeI = iter_value_t<_Iter>;
using _ValueTypeO = typename iterator_traits<_Out>::value_type;
constexpr bool __use_memmove
@@ -263,11 +267,12 @@ namespace ranges
: is_copy_assignable_v<_ValueTypeI>);
auto __num = __last - __first;
if (__num)
- std::__memmove<_IsMove>(__result, __first, __num);
+ __builtin_memmove(__result, __first,
+ sizeof(_ValueTypeI) * __num);
return {__first + __num, __result + __num};
}
- else
- {
+ }
+
for (auto __n = __last - __first; __n > 0; --__n)
{
if constexpr (_IsMove)
@@ -279,7 +284,6 @@ namespace ranges
}
return {std::move(__first), std::move(__result)};
}
- }
else
{
while (__first != __last)
@@ -385,6 +389,10 @@ namespace ranges
}
else if constexpr (sized_sentinel_for<_Sent, _Iter>)
{
+#ifdef __cpp_lib_is_constant_evaluated
+ if (!std::is_constant_evaluated())
+#endif
+ {
using _ValueTypeI = iter_value_t<_Iter>;
using _ValueTypeO = typename iterator_traits<_Out>::value_type;
constexpr bool __use_memmove
@@ -399,11 +407,12 @@ namespace ranges
: is_copy_assignable_v<_ValueTypeI>);
auto __num = __last - __first;
if (__num)
- std::__memmove<_IsMove>(__result - __num, __first, __num);
+ __builtin_memmove(__result - __num, __first,
+ sizeof(_ValueTypeI) * __num);
return {__first + __num, __result - __num};
}
- else
- {
+ }
+
auto __lasti = ranges::next(__first, __last);
auto __tail = __lasti;
@@ -418,7 +427,6 @@ namespace ranges
}
return {std::move(__lasti), std::move(__result)};
}
- }
else
{
auto __lasti = ranges::next(__first, __last);
diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h
index c6b7148b39c..268569336b0 100644
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -80,39 +80,6 @@ namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_VERSION
- /*
- * A constexpr wrapper for __builtin_memmove.
- * @param __num The number of elements of type _Tp (not bytes).
- */
- template<bool _IsMove, typename _Tp>
- _GLIBCXX14_CONSTEXPR
- inline void*
- __memmove(_Tp* __dst, const _Tp* __src, size_t __num)
- {
-#ifdef __cpp_lib_is_constant_evaluated
- if (std::is_constant_evaluated())
- {
- for(; __num > 0; --__num)
- {
- if constexpr (_IsMove)
- // This const_cast looks unsafe, but we only use this function
- // for trivially-copyable types, which means this assignment
- // is trivial and so doesn't alter the source anyway.
- // See PR 93872 for why it's needed.
- *__dst = std::move(*const_cast<_Tp*>(__src));
- else
- *__dst = *__src;
- ++__src;
- ++__dst;
- }
- return __dst;
- }
- else
-#endif
- return __builtin_memmove(__dst, __src, sizeof(_Tp) * __num);
- return __dst;
- }
-
/*
* A constexpr wrapper for __builtin_memcmp.
* @param __num The number of elements of type _Tp (not bytes).
@@ -453,7 +420,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
#endif
const ptrdiff_t _Num = __last - __first;
if (_Num)
- std::__memmove<_IsMove>(__result, __first, _Num);
+ __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
return __result + _Num;
}
};
@@ -492,9 +459,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
inline _OI
__copy_move_a2(_II __first, _II __last, _OI __result)
{
+ typedef typename iterator_traits<_II>::iterator_category _Category;
+#ifdef __cpp_lib_is_constant_evaluated
+ if (std::is_constant_evaluated())
+ return std::__copy_move<_IsMove, false, _Category>::
+ __copy_m(__first, __last, __result);
+#endif
typedef typename iterator_traits<_II>::value_type _ValueTypeI;
typedef typename iterator_traits<_OI>::value_type _ValueTypeO;
- typedef typename iterator_traits<_II>::iterator_category _Category;
const bool __simple = (__is_trivially_copyable(_ValueTypeI)
&& __is_pointer<_II>::__value
&& __is_pointer<_OI>::__value
@@ -719,7 +691,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
#endif
const ptrdiff_t _Num = __last - __first;
if (_Num)
- std::__memmove<_IsMove>(__result - _Num, __first, _Num);
+ __builtin_memmove(__result - _Num, __first, sizeof(_Tp) * _Num);
return __result - _Num;
}
};
@@ -729,20 +701,19 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
inline _BI2
__copy_move_backward_a2(_BI1 __first, _BI1 __last, _BI2 __result)
{
+ typedef typename iterator_traits<_BI1>::iterator_category _Category;
+#ifdef __cpp_lib_is_constant_evaluated
+ if (std::is_constant_evaluated())
+ return std::__copy_move_backward<_IsMove, false, _Category>::
+ __copy_move_b(__first, __last, __result);
+#endif
typedef typename iterator_traits<_BI1>::value_type _ValueType1;
typedef typename iterator_traits<_BI2>::value_type _ValueType2;
- typedef typename iterator_traits<_BI1>::iterator_category _Category;
const bool __simple = (__is_trivially_copyable(_ValueType1)
&& __is_pointer<_BI1>::__value
&& __is_pointer<_BI2>::__value
&& __are_same<_ValueType1, _ValueType2>::__value);
-#ifdef __cpp_lib_is_constant_evaluated
- if (std::is_constant_evaluated())
- return std::__copy_move_backward<true, false,
- _Category>::__copy_move_b(__first, __last,
- __result);
-#endif
return std::__copy_move_backward<_IsMove, __simple,
_Category>::__copy_move_b(__first,
__last,
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr.cc b/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr.cc
index 578ed042cce..704dcf513c0 100644
--- a/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr.cc
@@ -34,3 +34,21 @@ test()
}
static_assert(test());
+
+constexpr bool
+test02()
+{
+ struct X
+ {
+ X() = default;
+ X& operator=(const X&) = default;
+ constexpr X& operator=(X&& x) { i = x.i; x.i = 0; return *this; }
+ int i = 1;
+ };
+
+ X from[1], to[1];
+ std::copy_backward(std::begin(from), std::end(from), std::end(to));
+ return from[0].i == 1;
+}
+
+static_assert(test02());
