Re: [patch libstdc++] Fix assignability check in uninitialized_copy

[Jonathan Wakely <jwakely at redhat dot com>]

On 28/12/14 13:47 +0100, Eelis wrote:
> On 2014-12-28 00:18, Eelis wrote:
> > Trivial fix attached.
>
> Please don't commit this patch.
>
> I just noticed that the assignability test is wrong in an additional way: it should look at assignability of /output/ elements, not /input/ elements.
>
> As a result, this code is currently rejected (because uninitialized_copy doesn't notice in time that chars can't be assigned to Xs):
>
> 	struct X
> 	{
> 		X() = default;
> 		X(char) {}
> 		X(X const &) = default;
> 		X & operator=(X const&) = default;
>
> 		X & operator=(char) = delete;
> 	};
>
> 	#include <memory>
>
> 	int main()
> 	{
> 		char a[100];
> 		X b[100];
>
> 		std::uninitialized_copy(a, a+10, b);
> 	}
>
> Updated fix attached. With it, the code is accepted again. Testing as we speak.

> Index: stl_uninitialized.h
> ===================================================================
> --- stl_uninitialized.h	(revision 219070)
> +++ stl_uninitialized.h	(working copy)
> @@ -104,30 +104,30 @@
>   */
>   template<typename _InputIterator, typename _ForwardIterator>
>     inline _ForwardIterator
>     uninitialized_copy(_InputIterator __first, _InputIterator __last,
> 		       _ForwardIterator __result)
>     {
>       typedef typename iterator_traits<_InputIterator>::value_type
> 	_ValueType1;
>       typedef typename iterator_traits<_ForwardIterator>::value_type
> 	_ValueType2;
> #if __cplusplus < 201103L
>       const bool __assignable = true;
> #else
>       // trivial types can have deleted assignment
> -      typedef typename iterator_traits<_InputIterator>::reference _RefType;
> -      const bool __assignable = is_assignable<_ValueType1, _RefType>::value;
> +      typedef typename iterator_traits<_ForwardIterator>::reference _RefType;
> +      const bool __assignable = is_assignable<_RefType, _ValueType1>::value;
> #endif

This is still wrong, because it tests for assigning an rvalue to the
result sequence, but dereferencing the _InputIterator doesn't
necessarily produce an rvalue, and the output type might behave
differently for assignment from lvalues and rvalues.

My first attempt to fix it was simply:

#else
      // trivial types can have deleted assignment
      typedef typename iterator_traits<_InputIterator>::reference _RefType;
-      const bool __assignable = is_assignable<_ValueType1, _RefType>::value;
+      const bool __assignable = is_assignable<_ValueType2, _RefType>::value;
#endif


But even that is wrong if the target type has a ref-qualified
assignment from the source type. The attached patch should be correct.

Tested x86_64-linux, committed to trunk and 4.9.

commit 8ba322dd71cca830b3641002b4c1bf132f195741
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Jan 9 15:51:20 2015 +0000

    	PR libstdc++/64476
    	* include/bits/stl_uninitialized.h (uninitialized_copy): Fix
    	is_assignable arguments.
    	* testsuite/20_util/specialized_algorithms/uninitialized_copy/64476.cc:
    	New.

diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h
index 00659e9..61a1561 100644
--- a/libstdc++-v3/include/bits/stl_uninitialized.h
+++ b/libstdc++-v3/include/bits/stl_uninitialized.h
@@ -115,8 +115,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       const bool __assignable = true;
 #else
       // trivial types can have deleted assignment
-      typedef typename iterator_traits<_InputIterator>::reference _RefType;
-      const bool __assignable = is_assignable<_ValueType1, _RefType>::value;
+      typedef typename iterator_traits<_InputIterator>::reference _RefType1;
+      typedef typename iterator_traits<_ForwardIterator>::reference _RefType2;
+      const bool __assignable = is_assignable<_RefType2, _RefType1>::value;
 #endif
 
       return std::__uninitialized_copy<__is_trivial(_ValueType1)
diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/64476.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/64476.cc
new file mode 100644
index 0000000..6369b17
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/64476.cc
@@ -0,0 +1,65 @@
+// Copyright (C) 2015 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-options "-std=gnu++11" }
+
+#include <memory>
+#include <testsuite_hooks.h>
+
+struct X
+{
+  X() = default;
+  X(X const &) = default;
+  X& operator=(X const&) = delete;
+};
+
+static_assert(__is_trivial(X), "X is trivial");
+
+int constructed = 0;
+int assigned = 0;
+
+struct Y
+{
+  Y() = default;
+  Y(Y const &) = default;
+  Y& operator=(Y const&) = default;
+
+  Y(const X&) { ++constructed; }
+  Y& operator=(const X&)& { ++assigned; return *this; }
+  Y& operator=(const X&)&& = delete;
+  Y& operator=(X&&) = delete;
+};
+
+static_assert(__is_trivial(Y), "Y is trivial");
+
+void
+test01()
+{
+  X a[100];
+  Y b[100];
+
+  std::uninitialized_copy(a, a+10, b);
+
+  VERIFY(constructed == 0);
+  VERIFY(assigned == 10);
+}
+
+int
+main()
+{
+  test01();
+}