Re: [PATCH] Use __builtin_memmove for trivially copy assignable types

[Jonathan Wakely <jwakely at redhat dot com>]

On 19/07/18 07:59 -0400, Glen Fernandes wrote:
> Updated patch to simplify the helper trait, and to include <memory>
> instead of <algorithm> in the unit test for copy_uninitialized:
>
> Use __builtin_memmove for trivially copy assignable types
>
> 2018-07-19  Glen Joseph Fernandes  <glenjofe@gmail.com>
>
>    * include/bits/stl_algobase.h
>    (__is_simple_copy_move): Defined helper.
>    (__copy_move_a): Used helper.
>    (__copy_move_backward_a): Likewise.
>    * testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc:
>    New test.
>    * testsuite/25_algorithms/copy/58982.cc: Updated tests.
>    * testsuite/25_algorithms/copy_n/58982.cc: Likewise.
>
> Attached: patch.txt
>
> Glen

> commit 1af8d465545fda2451928fe100901db37c3e632c
> Author: Glen Fernandes <glen.fernandes@gmail.com>
> Date:   Thu Jul 19 07:40:17 2018 -0400
>
>    Use __builtin_memmove for trivially copy assignable types
>
>    2018-07-19  Glen Joseph Fernandes  <glenjofe@gmail.com>
>
>        * include/bits/stl_algobase.h
>        (__is_simple_copy_move): Defined helper.
>        (__copy_move_a): Used helper.
>        (__copy_move_backward_a): Likewise.
>        * testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc:
>        New test.
>        * testsuite/25_algorithms/copy/58982.cc: Updated tests.
>        * testsuite/25_algorithms/copy_n/58982.cc: Likewise.
>
> diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h
> index 16a3f83b6..4488207f0 100644
> --- a/libstdc++-v3/include/bits/stl_algobase.h
> +++ b/libstdc++-v3/include/bits/stl_algobase.h
> @@ -72,10 +72,16 @@
>
> namespace std _GLIBCXX_VISIBILITY(default)
> {
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> +  template<typename _Tp>
> +    struct __is_simple_copy_move
> +    {
> +      enum { __value = __is_trivially_assignable(_Tp, const _Tp&) };
> +    };
> +
> #if __cplusplus < 201103L
>   // See http://gcc.gnu.org/ml/libstdc++/2004-08/msg00167.html: in a
>   // nutshell, we are partially implementing the resolution of DR 187,
>   // when it's safe, i.e., the value_types are equal.
>   template<bool _BoolType>
> @@ -389,11 +395,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     __copy_move_a(_II __first, _II __last, _OI __result)
>     {
>       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_trivial(_ValueTypeI)
> +      const bool __simple = (__is_simple_copy_move<_ValueTypeI>::__value

Sorry for the delay in reviewing this properly, as I've only just
realised that this introduces undefined behaviour, doesn't it?

It's undefined to use memmove for a type that is not trivially
copyable. All trivial types are trivially copyable, so __is_trivial
was too conservative, but safe (IIRC we used it because there was no
__is_trivially_copyable trait at the time, so __is_trivial was the
best we had).

There are types which are trivially assignable but not trivially
copyable, and it's undefined to use memmove for such types. With your
patch applied I get a warning for this code, where there was none
before:

#include <memory>
#include <type_traits>

struct T
{
 T() { }
 T(const T&) { }
};

static_assert(std::is_trivially_copy_assignable<T>::value
   && !std::is_trivially_copyable<T>::value,
   "T is only trivially copy assignable, not trivially copyable");

void
test01(T* result)
{
 T t[1];
 std::copy(t, t+1, result);
}


In file included from /home/jwakely/gcc/9/include/c++/9.0.0/memory:62,
                from copy.cc:1:
/home/jwakely/gcc/9/include/c++/9.0.0/bits/stl_algobase.h: In instantiation of 'static _Tp* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(const _Tp*, const _Tp*, _Tp*) [with _Tp = T; bool _IsMove = false]':
/home/jwakely/gcc/9/include/c++/9.0.0/bits/stl_algobase.h:406:30:   required from '_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = T*; _OI = T*]'
/home/jwakely/gcc/9/include/c++/9.0.0/bits/stl_algobase.h:443:30:   required from '_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = T*; _OI = T*]'
/home/jwakely/gcc/9/include/c++/9.0.0/bits/stl_algobase.h:476:7:   required from '_OI std::copy(_II, _II, _OI) [with _II = T*; _OI = T*]'
copy.cc:18:27:   required from here
/home/jwakely/gcc/9/include/c++/9.0.0/bits/stl_algobase.h:388:23: warning: 'void* __builtin_memmove(void*, const void*, long unsigned int)' writing to an object of non-trivially copyable type 'struct T'; use copy-assignment or copy-initialization instead [-Wclass-memaccess]
     __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
copy.cc:4:8: note: 'struct T' declared here
struct T
       ^


I think the best we can do here is simply replace __is_trivial with
__is_trivially_copyable, which will enable memmove for trivially
copyable types for which !is_trivially_default_constructible_v<T>.