From 490350a11f82ee214aa8dd50b1222f3e7cffe630 Mon Sep 17 00:00:00 2001 From: Jonathan Wakely Date: Tue, 25 Feb 2020 14:16:42 +0000 Subject: [PATCH] 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::__copy_m) (__copy_move_backward::__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. --- libstdc++-v3/ChangeLog | 14 ++ libstdc++-v3/include/bits/ranges_algobase.h | 120 ++++++++++-------- libstdc++-v3/include/bits/stl_algobase.h | 57 ++------- .../25_algorithms/copy_backward/constexpr.cc | 18 +++ 4 files changed, 110 insertions(+), 99 deletions(-) diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index 05195b46f1a..2adf9cba26f 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,5 +1,19 @@ 2020-02-25 Jonathan Wakely + * 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::__copy_m) + (__copy_move_backward::__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 73f0205ba7f..feb6c5723dd 100644 --- 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 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 - _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::__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()); -- 2.30.2