| Message ID | CAHVPgz=BsNLPv-Q=uvbF2L8qw1hG3mhJFpU1uudYURddxpxP-Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use __builtin_memmove for trivially copy assignable types | expand |
On Sat, 16 Jun 2018 at 07:01, Glen Fernandes wrote: > Use __builtin_memmove for trivially copy assignable types > I'll review the patch later, just a quick comment for now. Every use of memcpy, memmove etc. makes it harder to make everything in <algorithm> constexpr (as we're now required to do). But that's an existing problem, and probably needs to be solved by teaching the built-ins to expand inside constant expression evaluation.
On Sat, 16 Jun 2018, Glen Fernandes wrote: > Use __builtin_memmove for trivially copy assignable types > > 2018-06-14 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/25_algorithms/copy/58982.cc: Updated tests. > * testsuite/25_algorithms/copy_n/58982.cc: Likewise. > > Tested x86_64-pc-linux-gnu. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68350 looks related but for a different function. Hopefully the issues discussed there don't apply here...
On 16 June 2018 at 18:05, Marc Glisse <marc.glisse@inria.fr> wrote: > On Sat, 16 Jun 2018, Glen Fernandes wrote: > >> Use __builtin_memmove for trivially copy assignable types >> >> 2018-06-14 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/25_algorithms/copy/58982.cc: Updated tests. >> * testsuite/25_algorithms/copy_n/58982.cc: Likewise. >> >> Tested x86_64-pc-linux-gnu. > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68350 looks related but for a > different function. Hopefully the issues discussed there don't apply here... I would certainly prefer this sort of patches to check via tests that uninitialized_{copy,move} is unaffected.
On Mon, Jun 18, 2018 at 6:12 PM Ville Voutilainen wrote: > On 16 June 2018 at 18:05, Marc Glisse wrote: > > On Sat, 16 Jun 2018, Glen Fernandes wrote: > > > >> Use __builtin_memmove for trivially copy assignable types > >> > >> 2018-06-14 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/25_algorithms/copy/58982.cc: Updated tests. > >> * testsuite/25_algorithms/copy_n/58982.cc: Likewise. > >> > >> Tested x86_64-pc-linux-gnu. > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68350 looks related but for a > > different function. Hopefully the issues discussed there don't apply here... > > I would certainly prefer this sort of patches to check via tests that > uninitialized_{copy,move} is unaffected. Will do. Glen
Updated patch with a new test.
Use __builtin_memmove for trivially copy assignable types
2018-06-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 521df588d7a83c662c833c0460d171663c62fd9a
Author: Glen Fernandes <glenjofe@gmail.com>
Date: Fri Jun 15 07:33:07 2018 -0400
Use __builtin_memmove for trivially copy assignable types
2018-06-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 022a3f159..d9e1a7958 100644
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -74,6 +74,16 @@ namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_VERSION
+ template<typename _Tp>
+ struct __is_simple_copy_move
+ {
+#if __cplusplus < 201103L
+ enum { __value = __is_trivial(_Tp) };
+#else
+ enum { __value = is_trivially_copy_assignable<_Tp>::value };
+#endif
+ };
+
#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,
@@ -377,7 +387,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
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
&& __is_pointer<_II>::__value
&& __is_pointer<_OI>::__value
&& __are_same<_ValueTypeI, _ValueTypeO>::__value);
@@ -578,7 +588,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
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_trivial(_ValueType1)
+ const bool __simple = (__is_simple_copy_move<_ValueType1>::__value
&& __is_pointer<_BI1>::__value
&& __is_pointer<_BI2>::__value
&& __are_same<_ValueType1, _ValueType2>::__value);
diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc
new file mode 100644
index 000000000..437b4c2e9
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc
@@ -0,0 +1,38 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do compile { target c++11 } }
+
+#include <algorithm>
+#include <type_traits>
+
+struct T
+{
+ T() { }
+ T(const T&) = delete;
+};
+
+static_assert(std::is_trivially_copy_assignable<T>::value &&
+ !__is_trivial(T), "T is only trivially copy assignable");
+
+void
+test01(T* result)
+{
+ T t[1];
+ std::uninitialized_copy(t, t+1, result); // { dg-error "here" }
+}
+// { dg-prune-output "use of deleted function" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc b/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc
index 03d918344..324f0e3b5 100644
--- a/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc
@@ -38,4 +38,4 @@ test01(T* result)
T t[1];
std::copy(t, t+1, result); // { dg-error "here" }
}
-// { dg-prune-output "not assignable" }
+// { dg-prune-output "use of deleted function" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc
index dc488cd1f..3eb793218 100644
--- a/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc
@@ -38,4 +38,4 @@ test01(T* result)
T t[1];
std::copy_n(t, 1, result); // { dg-error "here" }
}
-// { dg-prune-output "not assignable" }
+// { dg-prune-output "use of deleted function" }
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
&& __is_pointer<_II>::__value
&& __is_pointer<_OI>::__value
&& __are_same<_ValueTypeI, _ValueTypeO>::__value);
return std::__copy_move<_IsMove, __simple,
@@ -591,11 +597,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__copy_move_backward_a(_BI1 __first, _BI1 __last, _BI2 __result)
{
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_trivial(_ValueType1)
+ const bool __simple = (__is_simple_copy_move<_ValueType1>::__value
&& __is_pointer<_BI1>::__value
&& __is_pointer<_BI2>::__value
&& __are_same<_ValueType1, _ValueType2>::__value);
return std::__copy_move_backward<_IsMove, __simple,
diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc
new file mode 100644
index 000000000..ec681879f
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc
@@ -0,0 +1,38 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do compile { target c++11 } }
+
+#include <memory>
+#include <type_traits>
+
+struct T
+{
+ T() { }
+ T(const T&) = delete;
+};
+
+static_assert(std::is_trivially_copy_assignable<T>::value &&
+ !__is_trivial(T), "T is only trivially copy assignable");
+
+void
+test01(T* result)
+{
+ T t[1];
+ std::uninitialized_copy(t, t+1, result); // { dg-error "here" }
+}
+// { dg-prune-output "use of deleted function" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc b/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc
index 03d918344..324f0e3b5 100644
--- a/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc
@@ -36,6 +36,6 @@ void
test01(T* result)
{
T t[1];
std::copy(t, t+1, result); // { dg-error "here" }
}
-// { dg-prune-output "not assignable" }
+// { dg-prune-output "use of deleted function" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc
index dc488cd1f..3eb793218 100644
--- a/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc
@@ -36,6 +36,6 @@ void
test01(T* result)
{
T t[1];
std::copy_n(t, 1, result); // { dg-error "here" }
}
-// { dg-prune-output "not assignable" }
+// { dg-prune-output "use of deleted function" }
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>.
On Thu, Jul 19, 2018 at 9:25 AM Jonathan Wakely <jwakely@redhat.com> wrote: > 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. I was still unclear about that, but I forwarded you an e-mail from Marshall with his answer when I asked whether libc++'s use of TriviallyCopyAssignable here was incorrect. Let me know if it applies here, and if not (and that interpretation of the standard is incorrect), I'll update the patch to do as you suggest and run the tests again. Glen
On Thu, Jul 19, 2018 at 10:01 AM Glen Fernandes wrote: > > I was still unclear about that, but I forwarded you an e-mail from > Marshall with his answer when I asked whether libc++'s use of > TriviallyCopyAssignable here was incorrect. Let me know if it applies > here, and if not (and that interpretation of the standard is > incorrect), I'll update the patch to do as you suggest and run the > tests again. > > Glen Attached: patch.txt Use __builtin_memmove for trivially copyable types 2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> * include/bits/stl_algobase.h (__copy_move_a): Used __is_trivially_copyable. (__copy_move_backward_a): Likewise. Tested x86_64-pc-linux-gnu. Glen commit 60bf5acca419df37752336c2008123558627ece7 Author: Glen Fernandes <glen.fernandes@gmail.com> Date: Thu Jul 19 10:27:54 2018 -0400 Use __builtin_memmove for trivially copyable types 2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> * include/bits/stl_algobase.h (__copy_move_a): Used __is_trivially_copyable. (__copy_move_backward_a): Likewise. diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h index 16a3f83b6..f0130bc41 100644 --- a/libstdc++-v3/include/bits/stl_algobase.h +++ b/libstdc++-v3/include/bits/stl_algobase.h @@ -389,11 +389,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_trivially_copyable(_ValueTypeI) && __is_pointer<_II>::__value && __is_pointer<_OI>::__value && __are_same<_ValueTypeI, _ValueTypeO>::__value); return std::__copy_move<_IsMove, __simple, @@ -591,11 +591,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __copy_move_backward_a(_BI1 __first, _BI1 __last, _BI2 __result) { 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_trivial(_ValueType1) + const bool __simple = (__is_trivially_copyable(_ValueType1) && __is_pointer<_BI1>::__value && __is_pointer<_BI2>::__value && __are_same<_ValueType1, _ValueType2>::__value); return std::__copy_move_backward<_IsMove, __simple,
On 19/07/18 10:01 -0400, Glen Fernandes wrote: >On Thu, Jul 19, 2018 at 9:25 AM Jonathan Wakely <jwakely@redhat.com> wrote: >> 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. > >I was still unclear about that, but I forwarded you an e-mail from >Marshall with his answer when I asked whether libc++'s use of >TriviallyCopyAssignable here was incorrect. Let me know if it applies >here, and if not (and that interpretation of the standard is >incorrect), I'll update the patch to do as you suggest and run the >tests again. While I sympathise with Marshall's position (that std::copy only cares about assignment not copying) that doesn't make it OK to use memmove here. Using memmove for a non-trivially copyable type is undefined. Period. The fact GCC warns that it's undefined also means GCC might start optimising based on the assumption that undefined behaviour isn't reached at runtime. So it could (for example) assume that the input range must be empty and remove the entire call to std::copy. For a non-trivially copyable, trivially assignable type I think we just have to rely on the compiler to transform the assignments into optimal code (which might end up being a memmove, ironically). Please do update the patch to use __is_trivially_copyable. I don't think we need the __is_simple_copy_move helper in that case, just change two uses of __is_trivial to __is_trivially_copyable.
On 19/07/18 10:32 -0400, Glen Fernandes wrote: >On Thu, Jul 19, 2018 at 10:01 AM Glen Fernandes wrote: >> >> I was still unclear about that, but I forwarded you an e-mail from >> Marshall with his answer when I asked whether libc++'s use of >> TriviallyCopyAssignable here was incorrect. Let me know if it applies >> here, and if not (and that interpretation of the standard is >> incorrect), I'll update the patch to do as you suggest and run the >> tests again. >> >> Glen > >Attached: patch.txt > >Use __builtin_memmove for trivially copyable types > >2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> > > * include/bits/stl_algobase.h > (__copy_move_a): Used __is_trivially_copyable. > (__copy_move_backward_a): Likewise. > >Tested x86_64-pc-linux-gnu. Ah, that was quick :-) Can we keep the new test you added in the previous patch? It seems useful to add anyway.
On Thu, Jul 19, 2018 at 10:40 AM Jonathan Wakely <jwakely@redhat.com> wrote: > On 19/07/18 10:32 -0400, Glen Fernandes wrote: > >Attached: patch.txt > >Use __builtin_memmove for trivially copyable types > >2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> > > * include/bits/stl_algobase.h > > (__copy_move_a): Used __is_trivially_copyable. > > (__copy_move_backward_a): Likewise. > >Tested x86_64-pc-linux-gnu. > > Ah, that was quick :-) > > Can we keep the new test you added in the previous patch? It seems > useful to add anyway. Affirmative. Attached: patch.txt Use __builtin_memmove for trivially copyable types 2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> * include/bits/stl_algobase.h (__copy_move_a): Used __is_trivially_copyable. (__copy_move_backward_a): Likewise. * testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc: New test. Glen commit f08d827c2e7e525f94b31d9ad5c22dab5a84e451 Author: Glen Fernandes <glen.fernandes@gmail.com> Date: Thu Jul 19 10:54:50 2018 -0400 Use __builtin_memmove for trivially copyable types 2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> * include/bits/stl_algobase.h (__copy_move_a): Used __is_trivially_copyable. (__copy_move_backward_a): Likewise. * testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc: New test. diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h index 16a3f83b6..f0130bc41 100644 --- a/libstdc++-v3/include/bits/stl_algobase.h +++ b/libstdc++-v3/include/bits/stl_algobase.h @@ -389,11 +389,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_trivially_copyable(_ValueTypeI) && __is_pointer<_II>::__value && __is_pointer<_OI>::__value && __are_same<_ValueTypeI, _ValueTypeO>::__value); return std::__copy_move<_IsMove, __simple, @@ -591,11 +591,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __copy_move_backward_a(_BI1 __first, _BI1 __last, _BI2 __result) { 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_trivial(_ValueType1) + const bool __simple = (__is_trivially_copyable(_ValueType1) && __is_pointer<_BI1>::__value && __is_pointer<_BI2>::__value && __are_same<_ValueType1, _ValueType2>::__value); return std::__copy_move_backward<_IsMove, __simple, diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc new file mode 100644 index 000000000..461d1f242 --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc @@ -0,0 +1,37 @@ +// Copyright (C) 2018 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// { dg-do compile { target c++11 } } + +#include <memory> + +struct T +{ + T() { } + T(const T&) = delete; +}; + +static_assert(__is_trivially_assignable(T&, const T&) && + !__is_trivial(T), "T is only trivially copy assignable"); + +void +test01(T* result) +{ + T t[1]; + std::uninitialized_copy(t, t+1, result); // { dg-error "here" } +} +// { dg-prune-output "use of deleted function" }
On Thu, Jul 19, 2018 at 10:40 AM Jonathan Wakely <jwakely@redhat.com> wrote: > On 19/07/18 10:32 -0400, Glen Fernandes wrote: > >Attached: patch.txt > >Use __builtin_memmove for trivially copyable types > >2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> > > * include/bits/stl_algobase.h > > (__copy_move_a): Used __is_trivially_copyable. > > (__copy_move_backward_a): Likewise. > >Tested x86_64-pc-linux-gnu. > > Ah, that was quick :-) > > Can we keep the new test you added in the previous patch? It seems > useful to add anyway. Affirmative. Attached: patch.txt Use __builtin_memmove for trivially copyable types 2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> * include/bits/stl_algobase.h (__copy_move_a): Used __is_trivially_copyable. (__copy_move_backward_a): Likewise. * testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc: New test. Glen commit f08d827c2e7e525f94b31d9ad5c22dab5a84e451 Author: Glen Fernandes <glen.fernandes@gmail.com> Date: Thu Jul 19 10:54:50 2018 -0400 Use __builtin_memmove for trivially copyable types 2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> * include/bits/stl_algobase.h (__copy_move_a): Used __is_trivially_copyable. (__copy_move_backward_a): Likewise. * testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc: New test. diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h index 16a3f83b6..f0130bc41 100644 --- a/libstdc++-v3/include/bits/stl_algobase.h +++ b/libstdc++-v3/include/bits/stl_algobase.h @@ -389,11 +389,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_trivially_copyable(_ValueTypeI) && __is_pointer<_II>::__value && __is_pointer<_OI>::__value && __are_same<_ValueTypeI, _ValueTypeO>::__value); return std::__copy_move<_IsMove, __simple, @@ -591,11 +591,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __copy_move_backward_a(_BI1 __first, _BI1 __last, _BI2 __result) { 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_trivial(_ValueType1) + const bool __simple = (__is_trivially_copyable(_ValueType1) && __is_pointer<_BI1>::__value && __is_pointer<_BI2>::__value && __are_same<_ValueType1, _ValueType2>::__value); return std::__copy_move_backward<_IsMove, __simple, diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc new file mode 100644 index 000000000..461d1f242 --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc @@ -0,0 +1,37 @@ +// Copyright (C) 2018 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// { dg-do compile { target c++11 } } + +#include <memory> + +struct T +{ + T() { } + T(const T&) = delete; +}; + +static_assert(__is_trivially_assignable(T&, const T&) && + !__is_trivial(T), "T is only trivially copy assignable"); + +void +test01(T* result) +{ + T t[1]; + std::uninitialized_copy(t, t+1, result); // { dg-error "here" } +} +// { dg-prune-output "use of deleted function" }
On 07/19/2018 08:39 AM, Jonathan Wakely wrote: > On 19/07/18 10:01 -0400, Glen Fernandes wrote: >> On Thu, Jul 19, 2018 at 9:25 AM Jonathan Wakely <jwakely@redhat.com> >> wrote: >>> 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. >> >> I was still unclear about that, but I forwarded you an e-mail from >> Marshall with his answer when I asked whether libc++'s use of >> TriviallyCopyAssignable here was incorrect. Let me know if it applies >> here, and if not (and that interpretation of the standard is >> incorrect), I'll update the patch to do as you suggest and run the >> tests again. > > While I sympathise with Marshall's position (that std::copy only cares > about assignment not copying) that doesn't make it OK to use memmove > here. > > Using memmove for a non-trivially copyable type is undefined. Period. > > The fact GCC warns that it's undefined also means GCC might start > optimising based on the assumption that undefined behaviour isn't > reached at runtime. So it could (for example) assume that the input > range must be empty and remove the entire call to std::copy. Right. In fact we have a pass which searches for a very small subset of undefined behavior (null pointer dereferences, division by zero) and when it finds them it replaces the offending operation with a trap and does the obvious CFG cleanups. While neither of those would affect this specific issue and we're pretty conservative about adding more cases to this pass, certainly the right thing to do is avoid undefined behavior :-) So I'm in total agreement with Jon here. jeff
On 19/07/18 11:04 -0400, Glen Fernandes wrote: >On Thu, Jul 19, 2018 at 10:40 AM Jonathan Wakely <jwakely@redhat.com> wrote: >> On 19/07/18 10:32 -0400, Glen Fernandes wrote: >> >Attached: patch.txt >> >Use __builtin_memmove for trivially copyable types >> >2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> >> > * include/bits/stl_algobase.h >> > (__copy_move_a): Used __is_trivially_copyable. >> > (__copy_move_backward_a): Likewise. >> >Tested x86_64-pc-linux-gnu. >> >> Ah, that was quick :-) >> >> Can we keep the new test you added in the previous patch? It seems >> useful to add anyway. > >Affirmative. Attached: patch.txt > >Use __builtin_memmove for trivially copyable types > >2018-07-19 Glen Joseph Fernandes <glenjofe@gmail.com> > > * include/bits/stl_algobase.h > (__copy_move_a): Used __is_trivially_copyable. > (__copy_move_backward_a): Likewise. > * testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc: > New test. Tested on x86_64-linux and committed to trunk - thanks!
diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h index 022a3f159..d9e1a7958 100644 --- a/libstdc++-v3/include/bits/stl_algobase.h +++ b/libstdc++-v3/include/bits/stl_algobase.h @@ -72,10 +72,20 @@ namespace std _GLIBCXX_VISIBILITY(default) { _GLIBCXX_BEGIN_NAMESPACE_VERSION + template<typename _Tp> + struct __is_simple_copy_move + { +#if __cplusplus < 201103L + enum { __value = __is_trivial(_Tp) }; +#else + enum { __value = is_trivially_copy_assignable<_Tp>::value }; +#endif + }; + #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> @@ -375,11 +385,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 && __is_pointer<_II>::__value && __is_pointer<_OI>::__value && __are_same<_ValueTypeI, _ValueTypeO>::__value); return std::__copy_move<_IsMove, __simple, @@ -576,11 +586,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __copy_move_backward_a(_BI1 __first, _BI1 __last, _BI2 __result) { 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_trivial(_ValueType1) + const bool __simple = (__is_simple_copy_move<_ValueType1>::__value && __is_pointer<_BI1>::__value && __is_pointer<_BI2>::__value && __are_same<_ValueType1, _ValueType2>::__value); return std::__copy_move_backward<_IsMove, __simple, diff --git a/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc b/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc index 03d918344..324f0e3b5 100644 --- a/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc +++ b/libstdc++-v3/testsuite/25_algorithms/copy/58982.cc @@ -36,6 +36,6 @@ void test01(T* result) { T t[1]; std::copy(t, t+1, result); // { dg-error "here" } } -// { dg-prune-output "not assignable" } +// { dg-prune-output "use of deleted function" } diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc index dc488cd1f..3eb793218 100644 --- a/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc +++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/58982.cc @@ -36,6 +36,6 @@ void test01(T* result) { T t[1]; std::copy_n(t, 1, result); // { dg-error "here" } } -// { dg-prune-output "not assignable" } +// { dg-prune-output "use of deleted function" }
Use __builtin_memmove for trivially copy assignable types 2018-06-14 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/25_algorithms/copy/58982.cc: Updated tests. * testsuite/25_algorithms/copy_n/58982.cc: Likewise. Tested x86_64-pc-linux-gnu. Glen commit 7ace2ad91fe7d2253b086aef8bfdb99f85d81f31 Author: Glen Fernandes <glenjofe@gmail.com> Date: Fri Jun 15 07:33:07 2018 -0400 Use __builtin_memmove for trivially copy assignable types 2018-06-14 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/25_algorithms/copy/58982.cc: Updated tests. * testsuite/25_algorithms/copy_n/58982.cc: Likewise.