This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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>.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]