[Bug libstdc++/63500] [4.9/5 Regression] bug in debug version of std::make_move_iterator?

[Bug libstdc++/63500] [4.9/5 Regression] bug in debug version of std::make_move_iterator?

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

Hi

     Here is a proposal to fix the issue with iterators which do not 
expose lvalue references when dereferenced. I simply chose to detect 
such an issue in c++11 mode thanks to the is_lvalue_reference meta function.

2014-10-15  François Dumont  <fdumont@gcc.gnu.org>

     PR libstdc++/63500
     * include/bits/cpp_type_traits.h (__true_type): Add __value constant.
     (__false_type): Likewise.
     * include/debug/functions.h (__foreign_iterator_aux2): Do not check for
     foreign iterators if input iterators returns rvalue reference.
     * testsuite/23_containers/vector/63500.cc: New.

Tested under Linux x86_64.

François


[-- Attachment #2: debug.patch --]
[-- Type: text/x-patch, Size: 3237 bytes --]

Index: include/bits/cpp_type_traits.h
===================================================================
--- include/bits/cpp_type_traits.h	(revision 216158)
+++ include/bits/cpp_type_traits.h	(working copy)
@@ -79,9 +79,12 @@
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
-  struct __true_type { };
-  struct __false_type { };
+  struct __true_type
+  { enum { __value = 1 }; };
 
+  struct __false_type
+  { enum { __value = 0 }; };
+
   template<bool>
     struct __truth_type
     { typedef __false_type __type; };
Index: include/debug/functions.h
===================================================================
--- include/debug/functions.h	(revision 216158)
+++ include/debug/functions.h	(working copy)
@@ -34,7 +34,7 @@
 					  // _Iter_base
 #include <bits/cpp_type_traits.h>	  // for __is_integer
 #include <bits/move.h>                    // for __addressof and addressof
-# include <bits/stl_function.h>		  // for less
+#include <bits/stl_function.h>		  // for less
 #if __cplusplus >= 201103L
 # include <type_traits>			  // for is_lvalue_reference and __and_
 #endif
@@ -252,8 +252,21 @@
 			    const _InputIterator& __other,
 			    const _InputIterator& __other_end)
     {
+#if __cplusplus >= 201103L
+      typedef std::iterator_traits<_InputIterator> _InputIteTraits;
+      typedef typename _InputIteTraits::reference _InputIteRefType;
+#endif
       return __foreign_iterator_aux3(__it, __other, __other_end,
+#if __cplusplus < 201103L
 				     _Is_contiguous_sequence<_Sequence>());
+#else
+      typename std::conditional<
+	std::__and_<std::integral_constant<
+	  bool, _Is_contiguous_sequence<_Sequence>::__value>,
+		    std::is_lvalue_reference<_InputIteRefType> >::value,
+	std::__true_type,
+	std::__false_type>::type());
+#endif
     }
 
   /* Handle the case where we aren't really inserting a range after all */
Index: testsuite/23_containers/vector/63500.cc
===================================================================
--- testsuite/23_containers/vector/63500.cc	(revision 0)
+++ testsuite/23_containers/vector/63500.cc	(working copy)
@@ -0,0 +1,39 @@
+// -*- C++ -*-
+
+// Copyright (C) 2014 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" }
+// { dg-do compile }
+
+#include <memory>
+#include <iterator>
+#include <debug/vector>
+
+class Foo
+{};
+
+void
+test01()
+{
+  __gnu_debug::vector<std::unique_ptr<Foo>> v;
+  __gnu_debug::vector<std::unique_ptr<Foo>> w;
+
+  v.insert(end(v),
+	   make_move_iterator(begin(w)),
+	   make_move_iterator(end(w)));
+}

Re: [Bug libstdc++/63500] [4.9/5 Regression] bug in debug version of std::make_move_iterator?

[Jonathan Wakely]

On 14/10/14 23:51 +0200, François Dumont wrote:
>Hi
>
>    Here is a proposal to fix the issue with iterators which do not 
>expose lvalue references when dereferenced. I simply chose to detect 
>such an issue in c++11 mode thanks to the is_lvalue_reference meta 
>function.
>
>2014-10-15  François Dumont  <fdumont@gcc.gnu.org>
>
>    PR libstdc++/63500
>    * include/bits/cpp_type_traits.h (__true_type): Add __value constant.
>    (__false_type): Likewise.
>    * include/debug/functions.h (__foreign_iterator_aux2): Do not check for
>    foreign iterators if input iterators returns rvalue reference.
>    * testsuite/23_containers/vector/63500.cc: New.
>
>Tested under Linux x86_64.
>
>François
>

>Index: include/bits/cpp_type_traits.h
>===================================================================
>--- include/bits/cpp_type_traits.h	(revision 216158)
>+++ include/bits/cpp_type_traits.h	(working copy)
>@@ -79,9 +79,12 @@
> {
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 
>-  struct __true_type { };
>-  struct __false_type { };
>+  struct __true_type
>+  { enum { __value = 1 }; };
> 
>+  struct __false_type
>+  { enum { __value = 0 }; };
>+
>   template<bool>
>     struct __truth_type
>     { typedef __false_type __type; };

This change isn't necessary with my suggestion below.

>Index: include/debug/functions.h
>===================================================================
>--- include/debug/functions.h	(revision 216158)
>+++ include/debug/functions.h	(working copy)
>@@ -34,7 +34,7 @@
> 					  // _Iter_base
> #include <bits/cpp_type_traits.h>	  // for __is_integer
> #include <bits/move.h>                    // for __addressof and addressof
>-# include <bits/stl_function.h>		  // for less
>+#include <bits/stl_function.h>		  // for less
> #if __cplusplus >= 201103L
> # include <type_traits>			  // for is_lvalue_reference and __and_
> #endif
>@@ -252,8 +252,21 @@
> 			    const _InputIterator& __other,
> 			    const _InputIterator& __other_end)
>     {
>+#if __cplusplus >= 201103L
>+      typedef std::iterator_traits<_InputIterator> _InputIteTraits;
>+      typedef typename _InputIteTraits::reference _InputIteRefType;
>+#endif
>       return __foreign_iterator_aux3(__it, __other, __other_end,
>+#if __cplusplus < 201103L
> 				     _Is_contiguous_sequence<_Sequence>());
>+#else
>+      typename std::conditional<
>+	std::__and_<std::integral_constant<
>+	  bool, _Is_contiguous_sequence<_Sequence>::__value>,
>+		    std::is_lvalue_reference<_InputIteRefType> >::value,
>+	std::__true_type,
>+	std::__false_type>::type());
>+#endif
>     }

I find this much easier to read:

#if __cplusplus < 201103L
      typedef _Is_contiguous_sequence<_Sequence> __tag;
#else
      using __lvalref = std::is_lvalue_reference<
        typename std::iterator_traits<_InputIterator>::reference>;
      using __contiguous = _Is_contiguous_sequence<_Sequence>;
      using __tag = typename std::conditional<__lvalref::value, __contiguous,
                                              std::__false_type>::type;
#endif
      return __foreign_iterator_aux3(__it, __other, __other_end, __tag());

It only has one preprocessor condition and it avoids mismatched
parentheses caused by opening the function parameter list once but
closing it twice in two different branches.

Re: [Bug libstdc++/63500] [4.9/5 Regression] bug in debug version of std::make_move_iterator?

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On 15/10/2014 13:10, Jonathan Wakely wrote:
>
> I find this much easier to read:
>
> #if __cplusplus < 201103L
>      typedef _Is_contiguous_sequence<_Sequence> __tag;
> #else
>      using __lvalref = std::is_lvalue_reference<
>        typename std::iterator_traits<_InputIterator>::reference>;
>      using __contiguous = _Is_contiguous_sequence<_Sequence>;
>      using __tag = typename std::conditional<__lvalref::value, 
> __contiguous,
> std::__false_type>::type;
> #endif
>      return __foreign_iterator_aux3(__it, __other, __other_end, __tag());
>
> It only has one preprocessor condition and it avoids mismatched
> parentheses caused by opening the function parameter list once but
> closing it twice in two different branches.
>
>
That's much better indeed.

     Shall we go with this ? Of course we are simply considering that we 
can't check for foreign iterators when some iterator adapters comes 
in-between. I hope one day to detect invalid usages even in this context.

2014-10-16  François Dumont  <fdumont@gcc.gnu.org>

     PR libstdc++/63500
     * include/debug/functions.h (__foreign_iterator_aux2): Do not check for
     foreign iterators if input iterators returns rvalue reference.
     * testsuite/23_containers/vector/63500.cc: New.

François


[-- Attachment #2: debug.patch --]
[-- Type: text/x-patch, Size: 2737 bytes --]

Index: include/debug/functions.h
===================================================================
--- include/debug/functions.h	(revision 216279)
+++ include/debug/functions.h	(working copy)
@@ -34,7 +34,7 @@
 					  // _Iter_base
 #include <bits/cpp_type_traits.h>	  // for __is_integer
 #include <bits/move.h>                    // for __addressof and addressof
-# include <bits/stl_function.h>		  // for less
+#include <bits/stl_function.h>		  // for less
 #if __cplusplus >= 201103L
 # include <type_traits>			  // for is_lvalue_reference and __and_
 #endif
@@ -252,8 +252,16 @@
 			    const _InputIterator& __other,
 			    const _InputIterator& __other_end)
     {
-      return __foreign_iterator_aux3(__it, __other, __other_end,
-				     _Is_contiguous_sequence<_Sequence>());
+#if __cplusplus < 201103L
+      typedef _Is_contiguous_sequence<_Sequence> __tag;
+#else
+      using __lvalref = std::is_lvalue_reference<
+	typename std::iterator_traits<_InputIterator>::reference>;
+      using __contiguous = _Is_contiguous_sequence<_Sequence>;
+      using __tag = typename std::conditional<__lvalref::value, __contiguous,
+					      std::__false_type>::type;
+#endif
+      return __foreign_iterator_aux3(__it, __other, __other_end, __tag());
     }
 
   /* Handle the case where we aren't really inserting a range after all */
Index: testsuite/23_containers/vector/63500.cc
===================================================================
--- testsuite/23_containers/vector/63500.cc	(revision 0)
+++ testsuite/23_containers/vector/63500.cc	(working copy)
@@ -0,0 +1,39 @@
+// -*- C++ -*-
+
+// Copyright (C) 2014 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" }
+// { dg-do compile }
+
+#include <memory>
+#include <iterator>
+#include <debug/vector>
+
+class Foo
+{};
+
+void
+test01()
+{
+  __gnu_debug::vector<std::unique_ptr<Foo>> v;
+  __gnu_debug::vector<std::unique_ptr<Foo>> w;
+
+  v.insert(end(v),
+	   make_move_iterator(begin(w)),
+	   make_move_iterator(end(w)));
+}

Re: [Bug libstdc++/63500] [4.9/5 Regression] bug in debug version of std::make_move_iterator?

[Jonathan Wakely]

On 15/10/14 22:06 +0200, François Dumont wrote:
>On 15/10/2014 13:10, Jonathan Wakely wrote:
>>
>>I find this much easier to read:
>>
>>#if __cplusplus < 201103L
>>     typedef _Is_contiguous_sequence<_Sequence> __tag;
>>#else
>>     using __lvalref = std::is_lvalue_reference<
>>       typename std::iterator_traits<_InputIterator>::reference>;
>>     using __contiguous = _Is_contiguous_sequence<_Sequence>;
>>     using __tag = typename std::conditional<__lvalref::value, 
>>__contiguous,
>>std::__false_type>::type;
>>#endif
>>     return __foreign_iterator_aux3(__it, __other, __other_end, __tag());
>>
>>It only has one preprocessor condition and it avoids mismatched
>>parentheses caused by opening the function parameter list once but
>>closing it twice in two different branches.
>>
>>
>That's much better indeed.
>
>    Shall we go with this ?

Yes, it looks good to me, thanks.

>Of course we are simply considering that 
>we can't check for foreign iterators when some iterator adapters comes 
>in-between. I hope one day to detect invalid usages even in this 
>context.

I agree that's OK.

Re: [Bug libstdc++/63500] [4.9/5 Regression] bug in debug version of std::make_move_iterator?

[Jonathan Wakely]

On 15/10/14 22:06 +0200, François Dumont wrote:
>On 15/10/2014 13:10, Jonathan Wakely wrote:
>>
>>I find this much easier to read:
>>
>>#if __cplusplus < 201103L
>>     typedef _Is_contiguous_sequence<_Sequence> __tag;
>>#else
>>     using __lvalref = std::is_lvalue_reference<
>>       typename std::iterator_traits<_InputIterator>::reference>;
>>     using __contiguous = _Is_contiguous_sequence<_Sequence>;
>>     using __tag = typename std::conditional<__lvalref::value, 
>>__contiguous,
>>std::__false_type>::type;
>>#endif
>>     return __foreign_iterator_aux3(__it, __other, __other_end, __tag());
>>
>>It only has one preprocessor condition and it avoids mismatched
>>parentheses caused by opening the function parameter list once but
>>closing it twice in two different branches.
>>
>>
>That's much better indeed.
>
>    Shall we go with this ? Of course we are simply considering that 
>we can't check for foreign iterators when some iterator adapters comes 
>in-between. I hope one day to detect invalid usages even in this 
>context.
>
>2014-10-16  François Dumont  <fdumont@gcc.gnu.org>
>
>    PR libstdc++/63500
>    * include/debug/functions.h (__foreign_iterator_aux2): Do not check for
>    foreign iterators if input iterators returns rvalue reference.
>    * testsuite/23_containers/vector/63500.cc: New.
>
>François
>

As this is a regression it should go on the 4.9 branch too.

>Index: include/debug/functions.h
>===================================================================
>--- include/debug/functions.h	(revision 216279)
>+++ include/debug/functions.h	(working copy)
>@@ -34,7 +34,7 @@
> 					  // _Iter_base
> #include <bits/cpp_type_traits.h>	  // for __is_integer
> #include <bits/move.h>                    // for __addressof and addressof
>-# include <bits/stl_function.h>		  // for less
>+#include <bits/stl_function.h>		  // for less
> #if __cplusplus >= 201103L
> # include <type_traits>			  // for is_lvalue_reference and __and_
> #endif
>@@ -252,8 +252,16 @@
> 			    const _InputIterator& __other,
> 			    const _InputIterator& __other_end)
>     {
>-      return __foreign_iterator_aux3(__it, __other, __other_end,
>-				     _Is_contiguous_sequence<_Sequence>());
>+#if __cplusplus < 201103L
>+      typedef _Is_contiguous_sequence<_Sequence> __tag;
>+#else
>+      using __lvalref = std::is_lvalue_reference<
>+	typename std::iterator_traits<_InputIterator>::reference>;
>+      using __contiguous = _Is_contiguous_sequence<_Sequence>;
>+      using __tag = typename std::conditional<__lvalref::value, __contiguous,
>+					      std::__false_type>::type;
>+#endif
>+      return __foreign_iterator_aux3(__it, __other, __other_end, __tag());
>     }
> 
>   /* Handle the case where we aren't really inserting a range after all */
>Index: testsuite/23_containers/vector/63500.cc
>===================================================================
>--- testsuite/23_containers/vector/63500.cc	(revision 0)
>+++ testsuite/23_containers/vector/63500.cc	(working copy)
>@@ -0,0 +1,39 @@
>+// -*- C++ -*-
>+
>+// Copyright (C) 2014 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" }
>+// { dg-do compile }
>+
>+#include <memory>
>+#include <iterator>
>+#include <debug/vector>
>+
>+class Foo
>+{};
>+
>+void
>+test01()
>+{
>+  __gnu_debug::vector<std::unique_ptr<Foo>> v;
>+  __gnu_debug::vector<std::unique_ptr<Foo>> w;
>+
>+  v.insert(end(v),
>+	   make_move_iterator(begin(w)),
>+	   make_move_iterator(end(w)));
>+}