libstdc++: Remove __memmove wrapper for constexpr algorithms

The mutating sequence algorithms std::copy, std::copy_backward,
std::move and std::move_backward conditionally use __builtin_memmove
for trivially copyable types. However, because memmove isn't usable in
constant expressions the use of __builtin_memmove is wrapped in a
__memmove function which replaces __builtin_memmove with a handwritten
loop when std::is_constant_evaluated() is true.

This means we have a manual loop for non-trivially copyable cases, and a
different manual loop for trivially copyable but constexpr cases. The
latter loop has incorrect semantics for the {copy,move}_backward cases
and so isn't used for them. Until earlier today the latter loop also had
incorrect semantics for the std::move cases, trying to move from const
rvalues.

The approach taken by this patch is to remove the __memmove function
entirely and use the original (and correct) manual loops for the
constexpr cases as well as the non-trivially copyable cases. This was
already done for move_backward and copy_backward, but was incorrectly
turning copy_backward into move_backward, by failing to use the _IsMove
constant to select the right specialization. This patch also fixes that.

	* include/bits/ranges_algobase.h (__copy_or_move): Do not use memmove
	during constant evaluation. Call __builtin_memmove directly instead of
	__memmove.
	(__copy_or_move_backward): Likewise.
	* include/bits/stl_algobase.h (__memmove): Remove.
	(__copy_move<M, true, random_access_iterator_tag>::__copy_m)
	(__copy_move_backward<M, true, random_access_iterator_tag>::__copy_m):
	Use __builtin_memmove directly instead of __memmove.
	(__copy_move_a2): Do not use memmove during constant evaluation.
	(__copy_move_backward_a2): Use _IsMove constant to select correct
	__copy_move_backward specialization.
	* testsuite/25_algorithms/copy_backward/constexpr.cc: Check for copies
	begin turned into moves during constant evaluation.

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index 05195b4..2adf9cb 100644 (file)
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,5 +1,19 @@
 2020-02-25  Jonathan Wakely  <jwakely@redhat.com>
 
+       * include/bits/ranges_algobase.h (__copy_or_move): Do not use memmove
+       during constant evaluation. Call __builtin_memmove directly instead of
+       __memmove.
+       (__copy_or_move_backward): Likewise.
+       * include/bits/stl_algobase.h (__memmove): Remove.
+       (__copy_move<M, true, random_access_iterator_tag>::__copy_m)
+       (__copy_move_backward<M, true, random_access_iterator_tag>::__copy_m):
+       Use __builtin_memmove directly instead of __memmove.
+       (__copy_move_a2): Do not use memmove during constant evaluation.
+       (__copy_move_backward_a2): Use _IsMove constant to select correct
+       __copy_move_backward specialization.
+       * testsuite/25_algorithms/copy_backward/constexpr.cc: Check for copies
+       begin turned into moves during constant evaluation.
+
        * testsuite/25_algorithms/move_backward/93872.cc: Add test left out of
        previous commit.
 

diff --git a/libstdc++-v3/include/bits/ranges_algobase.h b/libstdc++-v3/include/bits/ranges_algobase.h
index 73f0205..feb6c57 100644 (file)
--- a/libstdc++-v3/include/bits/ranges_algobase.h
+++ b/libstdc++-v3/include/bits/ranges_algobase.h
@@ -248,37 +248,41 @@ namespace ranges
        }
       else if constexpr (sized_sentinel_for<_Sent, _Iter>)
        {
-         using _ValueTypeI = iter_value_t<_Iter>;
-         using _ValueTypeO = typename iterator_traits<_Out>::value_type;
-         constexpr bool __use_memmove
-           = (is_trivially_copyable_v<_ValueTypeI>
-              && is_same_v<_ValueTypeI, _ValueTypeO>
-              && is_pointer_v<_Iter>
-              && is_pointer_v<_Out>);
-
-         if constexpr (__use_memmove)
+#ifdef __cpp_lib_is_constant_evaluated
+         if (!std::is_constant_evaluated())
+#endif
            {
-             static_assert(_IsMove
-                           ? is_move_assignable_v<_ValueTypeI>
-                           : is_copy_assignable_v<_ValueTypeI>);
-             auto __num = __last - __first;
-             if (__num)
-               std::__memmove<_IsMove>(__result, __first, __num);
-             return {__first + __num, __result + __num};
-           }
-         else
-           {
-             for (auto __n = __last - __first; __n > 0; --__n)
+             using _ValueTypeI = iter_value_t<_Iter>;
+             using _ValueTypeO = typename iterator_traits<_Out>::value_type;
+             constexpr bool __use_memmove
+               = (is_trivially_copyable_v<_ValueTypeI>
+                   && is_same_v<_ValueTypeI, _ValueTypeO>
+                   && is_pointer_v<_Iter>
+                   && is_pointer_v<_Out>);
+
+             if constexpr (__use_memmove)
                {
-                 if constexpr (_IsMove)
-                   *__result = std::move(*__first);
-                 else
-                   *__result = *__first;
-                 ++__first;
-                 ++__result;
+                 static_assert(_IsMove
+                     ? is_move_assignable_v<_ValueTypeI>
+                     : is_copy_assignable_v<_ValueTypeI>);
+                 auto __num = __last - __first;
+                 if (__num)
+                   __builtin_memmove(__result, __first,
+                       sizeof(_ValueTypeI) * __num);
+                 return {__first + __num, __result + __num};
                }
-             return {std::move(__first), std::move(__result)};
            }
+
+         for (auto __n = __last - __first; __n > 0; --__n)
+           {
+             if constexpr (_IsMove)
+               *__result = std::move(*__first);
+             else
+               *__result = *__first;
+             ++__first;
+             ++__result;
+           }
+         return {std::move(__first), std::move(__result)};
        }
       else
        {
@@ -385,39 +389,43 @@ namespace ranges
        }
       else if constexpr (sized_sentinel_for<_Sent, _Iter>)
        {
-         using _ValueTypeI = iter_value_t<_Iter>;
-         using _ValueTypeO = typename iterator_traits<_Out>::value_type;
-         constexpr bool __use_memmove
-           = (is_trivially_copyable_v<_ValueTypeI>
-              && is_same_v<_ValueTypeI, _ValueTypeO>
-              && is_pointer_v<_Iter>
-              && is_pointer_v<_Out>);
-         if constexpr (__use_memmove)
-           {
-             static_assert(_IsMove
-                           ? is_move_assignable_v<_ValueTypeI>
-                           : is_copy_assignable_v<_ValueTypeI>);
-             auto __num = __last - __first;
-             if (__num)
-               std::__memmove<_IsMove>(__result - __num, __first, __num);
-             return {__first + __num, __result - __num};
-           }
-         else
+#ifdef __cpp_lib_is_constant_evaluated
+         if (!std::is_constant_evaluated())
+#endif
            {
-             auto __lasti = ranges::next(__first, __last);
-             auto __tail = __lasti;
-
-             for (auto __n = __last - __first; __n > 0; --__n)
+             using _ValueTypeI = iter_value_t<_Iter>;
+             using _ValueTypeO = typename iterator_traits<_Out>::value_type;
+             constexpr bool __use_memmove
+               = (is_trivially_copyable_v<_ValueTypeI>
+                   && is_same_v<_ValueTypeI, _ValueTypeO>
+                   && is_pointer_v<_Iter>
+                   && is_pointer_v<_Out>);
+             if constexpr (__use_memmove)
                {
-                 --__tail;
-                 --__result;
-                 if constexpr (_IsMove)
-                   *__result = std::move(*__tail);
-                 else
-                   *__result = *__tail;
+                 static_assert(_IsMove
+                     ? is_move_assignable_v<_ValueTypeI>
+                     : is_copy_assignable_v<_ValueTypeI>);
+                 auto __num = __last - __first;
+                 if (__num)
+                   __builtin_memmove(__result - __num, __first,
+                                     sizeof(_ValueTypeI) * __num);
+                 return {__first + __num, __result - __num};
                }
-             return {std::move(__lasti), std::move(__result)};
            }
+
+         auto __lasti = ranges::next(__first, __last);
+         auto __tail = __lasti;
+
+         for (auto __n = __last - __first; __n > 0; --__n)
+           {
+             --__tail;
+             --__result;
+             if constexpr (_IsMove)
+               *__result = std::move(*__tail);
+             else
+               *__result = *__tail;
+           }
+         return {std::move(__lasti), std::move(__result)};
        }
       else
        {

diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h
index c6b7148..2685693 100644 (file)
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -81,39 +81,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 578ed04..704dcf5 100644 (file)
--- 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());