On 19/03/14 23:38 +0100, Paolo Carlini wrote:
On 19/03/14 21:39 +0000, Jonathan Wakely wrote:
I think the safe thing to do is (as I suggested at the time) to have a
trait saying which iterator types refer to contiguous memory. Our
debug mode only supports our own containers, so the ones which are
contiguous are known. For 4.9.0 I think the right option is simply
to remove __foreign_iterator_aux3 and __foreign_iterator_aux4
completely. The fixed version of __foreign_iterator_aux2() can detect
when we have iterators referring to the same sequence, which is what
we really want to detect. That's what the attached patch does and what
I'm going to test.
With my suggested change we get an XPASS for
testsuite/23_containers/vector/debug/57779_neg.cc
An __is_contiguous trait would solve that.
Funny, I thought we already had it...
I think we might have added it and removed it at some point ... not
sure though.
This is what I plan to commit, it's almost a total rewrite of
__foreign_iterator() :-(
Dereferencing invalid iterators is avoided by passing in both
iterators representing the range, not just the first.
The suspect pointer arithmetic that tries to determine if a pointer
points inside a container is made unnecessary by adding
__gnu_debug::_Is_contiguous_sequence, currently only true for
std::vector<T,A> for all T except bool. We don't need to make it true
for std::string because that defines _Insert_range_from_self_is_safe
instead, so we can never reach the code that depends on contiguous
storage. As a benefit of the rewrite it no longer depends on C++11 and
so works in C++98 mode.
The overload resolution issue is solved by consistently using
const-refs for the parameters of __foreign_iterator_aux2. I also added
another overload for debug iterators into different debug container
types, so they can be identified as foreign sooner, without calling
__foreign_iterator_aux3.
Everything passes for "make check check-debug", so I plan to commit it
tomorrow, unless anyone points out a problem.
commit f1b942879722242f21e5b4d2babae0fddcfb7a90
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Thu Mar 20 19:23:23 2014 +0000
PR libstdc++/60587
* include/debug/functions.h (_Is_contiguous_sequence): Define.
(__foreign_iterator): Accept additional iterator. Do not dispatch on
iterator category.
(__foreign_iterator_aux2): Likewise. Add overload for iterators
from different types of debug container. Use _Is_contiguous_sequence
instead of is_lvalue_reference.
(__foreign_iterator_aux3): Accept additional iterator. Avoid
dereferencing past-the-end iterator.
(__foreign_iterator_aux4): Use const value_type* instead of
potentially user-defined const_pointer type.
* include/debug/macros.h (__glibcxx_check_insert_range): Fix comment
and pass end iterator to __gnu_debug::__foreign_iterator.
(__glibcxx_check_insert_range_after): Likewise.
(__glibcxx_check_max_load_factor): Fix comment.
* include/debug/vector (_Is_contiguous_sequence): Define partial
specializations.
* testsuite/23_containers/vector/debug/57779_neg.cc: Remove
-std=gnu++11 option and unused header.
* testsuite/23_containers/vector/debug/60587.cc: New.
* testsuite/23_containers/vector/debug/60587_neg.cc: New.
diff --git a/libstdc++-v3/include/debug/functions.h
b/libstdc++-v3/include/debug/functions.h
index 5dda0c3..b48c36d 100644
--- a/libstdc++-v3/include/debug/functions.h
+++ b/libstdc++-v3/include/debug/functions.h
@@ -34,8 +34,8 @@
// _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
#if __cplusplus >= 201103L
-# include <bits/stl_function.h> // for less and greater_equal
# include <type_traits> // for is_lvalue_reference
and __and_
#endif
#include <debug/formatter.h>
@@ -52,6 +52,9 @@ namespace __gnu_debug
struct _Insert_range_from_self_is_safe
{ enum { __value = 0 }; };
+ template<typename _Sequence>
+ struct _Is_contiguous_sequence : std::__false_type { };
+
// An arbitrary iterator pointer is not singular.
inline bool
__check_singular_aux(const void*) { return false; }
@@ -175,123 +178,112 @@ namespace __gnu_debug
return __first;
}
-#if __cplusplus >= 201103L
- // Default implementation.
+ /* Handle the case where __other is a pointer to _Sequence::value_type. */
template<typename _Iterator, typename _Sequence>
inline bool
__foreign_iterator_aux4(const _Safe_iterator<_Iterator, _Sequence>& __it,
- typename _Sequence::const_pointer __begin,
- typename _Sequence::const_pointer __other)
+ const typename _Sequence::value_type* __other)
{
- typedef typename _Sequence::const_pointer _PointerType;
- constexpr std::less<_PointerType> __l{};
+ typedef const typename _Sequence::value_type* _PointerType;
+ typedef std::less<_PointerType> _Less;
+#if __cplusplus >= 201103L
+ constexpr _Less __l{};
+#else
+ const _Less __l = _Less();
+#endif
+ const _Sequence* __seq = __it._M_get_sequence();
+ const _PointerType __begin = std::__addressof(*__seq->_M_base().begin());
+ const _PointerType __end = std::__addressof(*(__seq->_M_base().end()-1));
- return (__l(__other, __begin)
- || __l(std::addressof(*(__it._M_get_sequence()->_M_base().end()
- - 1)), __other));
+ // Check whether __other points within the contiguous storage.
+ return __l(__other, __begin) || __l(__end, __other);
}
- // Fallback when address type cannot be implicitely casted to sequence
- // const_pointer.
- template<typename _Iterator, typename _Sequence,
- typename _InputIterator>
+ /* Fallback overload for when we can't tell, assume it is valid. */
+ template<typename _Iterator, typename _Sequence>
inline bool
- __foreign_iterator_aux4(const _Safe_iterator<_Iterator, _Sequence>&,
- _InputIterator, ...)
+ __foreign_iterator_aux4(const _Safe_iterator<_Iterator, _Sequence>&, ...)
{ return true; }
+ /* Handle sequences with contiguous storage */
template<typename _Iterator, typename _Sequence, typename _InputIterator>
inline bool
__foreign_iterator_aux3(const _Safe_iterator<_Iterator, _Sequence>& __it,
- _InputIterator __other,
- std::true_type)
+ const _InputIterator& __other,
+ const _InputIterator& __other_end,
+ std::__true_type)
{
- // Only containers with all elements in contiguous memory can have their
- // elements passed through pointers.
- // Arithmetics is here just to make sure we are not dereferencing
- // past-the-end iterator.
- if (__it._M_get_sequence()->_M_base().begin()
- != __it._M_get_sequence()->_M_base().end())
- if (std::addressof(*(__it._M_get_sequence()->_M_base().end() - 1))
- - std::addressof(*(__it._M_get_sequence()->_M_base().begin()))
- == __it._M_get_sequence()->size() - 1)
- return (__foreign_iterator_aux4
- (__it,
- std::addressof(*(__it._M_get_sequence()->_M_base().begin())),
- std::addressof(*__other)));
- return true;
+ if (__other == __other_end)
+ return true; // inserting nothing is safe even if not foreign iters
+ if (__it._M_get_sequence()->begin() == __it._M_get_sequence()->end())
+ return true; // can't be self-inserting if self is empty
+ return __foreign_iterator_aux4(__it, std::__addressof(*__other));
}
-
- /* Fallback overload for which we can't say, assume it is valid. */
+
+ /* Handle non-contiguous containers, assume it is valid. */
template<typename _Iterator, typename _Sequence, typename _InputIterator>
inline bool
- __foreign_iterator_aux3(const _Safe_iterator<_Iterator, _Sequence>& __it,
- _InputIterator __other,
- std::false_type)
+ __foreign_iterator_aux3(const _Safe_iterator<_Iterator, _Sequence>&,
+ const _InputIterator&, const _InputIterator&,
+ std::__false_type)
{ return true; }
-#endif
- /** Checks that iterators do not belong to the same sequence. */
+ /** Handle debug iterators from the same type of container. */
template<typename _Iterator, typename _Sequence, typename _OtherIterator>
inline bool
__foreign_iterator_aux2(const _Safe_iterator<_Iterator, _Sequence>& __it,
const _Safe_iterator<_OtherIterator, _Sequence>& __other,
- std::input_iterator_tag)
+ const _Safe_iterator<_OtherIterator, _Sequence>&)
{ return __it._M_get_sequence() != __other._M_get_sequence(); }
-
-#if __cplusplus >= 201103L
- /* This overload detects when passing pointers to the contained elements
- rather than using iterators.
- */
+
+ /** Handle debug iterators from different types of container. */
+ template<typename _Iterator, typename _Sequence, typename _OtherIterator,
+ typename _OtherSequence>
+ inline bool
+ __foreign_iterator_aux2(const _Safe_iterator<_Iterator, _Sequence>& __it,
+ const _Safe_iterator<_OtherIterator, _OtherSequence>&,
+ const _Safe_iterator<_OtherIterator, _OtherSequence>&)
+ { return true; }
+
+ /* Handle non-debug iterators. */
template<typename _Iterator, typename _Sequence, typename _InputIterator>
inline bool
__foreign_iterator_aux2(const _Safe_iterator<_Iterator, _Sequence>& __it,
- _InputIterator __other,
- std::random_access_iterator_tag)
+ const _InputIterator& __other,
+ const _InputIterator& __other_end)
{
- typedef typename _Sequence::const_iterator _ItType;
- typedef typename std::iterator_traits<_ItType>::reference _Ref;
- return __foreign_iterator_aux3(__it, __other,
- std::is_lvalue_reference<_Ref>());
+ return __foreign_iterator_aux3(__it, __other, __other_end,
+ _Is_contiguous_sequence<_Sequence>());
}
-#endif
-
- /* Fallback overload for which we can't say, assume it is valid. */
- template<typename _Iterator, typename _Sequence, typename _InputIterator>
- inline bool
- __foreign_iterator_aux2(const _Safe_iterator<_Iterator, _Sequence>&,
- _InputIterator,
- std::input_iterator_tag)
- { return true; }
-
- template<typename _Iterator, typename _Sequence,
- typename _Integral>
+
+ /* Handle the case where we aren't really inserting a range after all */
+ template<typename _Iterator, typename _Sequence, typename _Integral>
inline bool
- __foreign_iterator_aux(const _Safe_iterator<_Iterator, _Sequence>& __it,
- _Integral __other,
+ __foreign_iterator_aux(const _Safe_iterator<_Iterator, _Sequence>&,
+ _Integral, _Integral,
std::__true_type)
{ return true; }
+ /* Handle all iterators. */
template<typename _Iterator, typename _Sequence,
typename _InputIterator>
inline bool
__foreign_iterator_aux(const _Safe_iterator<_Iterator, _Sequence>& __it,
- _InputIterator __other,
+ _InputIterator __other, _InputIterator __other_end,
std::__false_type)
{
- return (_Insert_range_from_self_is_safe<_Sequence>::__value
- || __foreign_iterator_aux2(__it, __other,
- std::__iterator_category(__it)));
+ return _Insert_range_from_self_is_safe<_Sequence>::__value
+ || __foreign_iterator_aux2(__it, __other, __other_end);
}
template<typename _Iterator, typename _Sequence,
typename _InputIterator>
inline bool
__foreign_iterator(const _Safe_iterator<_Iterator, _Sequence>& __it,
- _InputIterator __other)
+ _InputIterator __other, _InputIterator __other_end)
{
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
- return __foreign_iterator_aux(__it, __other, _Integral());
+ return __foreign_iterator_aux(__it, __other, __other_end, _Integral());
}
/** Checks that __s is non-NULL or __n == 0, and then returns __s. */
diff --git a/libstdc++-v3/include/debug/macros.h
b/libstdc++-v3/include/debug/macros.h
index 9be7641..7ce374c 100644
--- a/libstdc++-v3/include/debug/macros.h
+++ b/libstdc++-v3/include/debug/macros.h
@@ -99,14 +99,15 @@ _GLIBCXX_DEBUG_VERIFY(!_Position._M_is_end(),
\
* into a container at a specific position requires that the iterator
* be nonsingular (i.e., either dereferenceable or past-the-end),
* that it reference the sequence we are inserting into, and that the
- * iterator range [_First, Last) is a valid (possibly empty)
- * range. Note that this macro is only valid when the container is a
+ * iterator range [_First, _Last) is a valid (possibly empty)
+ * range which does not reference the sequence we are inserting into.
+ * Note that this macro is only valid when the container is a
* _Safe_sequence and the _Position iterator is a _Safe_iterator.
*/
#define __glibcxx_check_insert_range(_Position,_First,_Last) \
__glibcxx_check_valid_range(_First,_Last); \
__glibcxx_check_insert(_Position); \
-_GLIBCXX_DEBUG_VERIFY(__gnu_debug::__foreign_iterator(_Position,_First),\
+_GLIBCXX_DEBUG_VERIFY(__gnu_debug::__foreign_iterator(_Position,_First,_Last),\
_M_message(__gnu_debug::__msg_insert_range_from_self)\
._M_iterator(_First, #_First) \
._M_iterator(_Last, #_Last) \
@@ -117,18 +118,15 @@
_GLIBCXX_DEBUG_VERIFY(__gnu_debug::__foreign_iterator(_Position,_First),\
* into a container after a specific position requires that the iterator
* be nonsingular (i.e., either dereferenceable or past-the-end),
* that it reference the sequence we are inserting into, and that the
- * iterator range [_First, Last) is a valid (possibly empty)
- * range. Note that this macro is only valid when the container is a
- * _Safe_sequence and the iterator is a _Safe_iterator.
- *
- * @todo We would like to be able to check for noninterference of
- * _Position and the range [_First, _Last), but that can't (in
- * general) be done.
+ * iterator range [_First, _Last) is a valid (possibly empty)
+ * range which does not reference the sequence we are inserting into.
+ * Note that this macro is only valid when the container is a
+ * _Safe_sequence and the _Position iterator is a _Safe_iterator.
*/
#define __glibcxx_check_insert_range_after(_Position,_First,_Last) \
__glibcxx_check_valid_range(_First,_Last); \
-__glibcxx_check_insert_after(_Position);
\
-_GLIBCXX_DEBUG_VERIFY(__gnu_debug::__foreign_iterator(_Position,_First),\
+__glibcxx_check_insert_after(_Position); \
+_GLIBCXX_DEBUG_VERIFY(__gnu_debug::__foreign_iterator(_Position,_First,_Last),\
_M_message(__gnu_debug::__msg_insert_range_from_self)\
._M_iterator(_First, #_First) \
._M_iterator(_Last, #_Last) \
@@ -343,7 +341,7 @@ _GLIBCXX_DEBUG_VERIFY(this != &_Other,
\
_M_message(__gnu_debug::__msg_self_move_assign) \
._M_sequence(*this, "this"))
-// Verify that load factor is position
+// Verify that load factor is positive
#define __glibcxx_check_max_load_factor(_F) \
_GLIBCXX_DEBUG_VERIFY(_F > 0.0f, \
_M_message(__gnu_debug::__msg_valid_load_factor) \
diff --git a/libstdc++-v3/include/debug/vector
b/libstdc++-v3/include/debug/vector
index bcca520..2e9cd65 100644
--- a/libstdc++-v3/include/debug/vector
+++ b/libstdc++-v3/include/debug/vector
@@ -718,4 +718,17 @@ namespace __debug
} // namespace std
+namespace __gnu_debug
+{
+ template<typename _Tp, typename _Alloc>
+ struct _Is_contiguous_sequence<std::__debug::vector<_Tp, _Alloc> >
+ : std::__true_type
+ { };
+
+ template<typename _Alloc>
+ struct _Is_contiguous_sequence<std::__debug::vector<bool, _Alloc> >
+ : std::__false_type
+ { };
+}
+
#endif
diff --git a/libstdc++-v3/testsuite/23_containers/vector/debug/57779_neg.cc
b/libstdc++-v3/testsuite/23_containers/vector/debug/57779_neg.cc
index 4fa6847..a317a83 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/debug/57779_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/debug/57779_neg.cc
@@ -15,12 +15,10 @@
// with this library; see the file COPYING3. If not see
// <http://www.gnu.org/licenses/>.
//
-// { dg-options "-std=gnu++11" }
// { dg-require-debug-mode "" }
// { dg-do run { xfail *-*-* } }
#include <vector>
-#include <debug/checks.h>
void test01()
{
diff --git a/libstdc++-v3/testsuite/23_containers/vector/debug/60587.cc
b/libstdc++-v3/testsuite/23_containers/vector/debug/60587.cc
new file mode 100644
index 0000000..73b4dac
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/debug/60587.cc
@@ -0,0 +1,35 @@
+// 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-require-debug-mode "" }
+
+// PR libstdc++/60587
+
+#include <vector>
+
+int main() {
+ std::vector<int> a, b;
+ a.push_back(1);
+ a.insert(a.end(), b.begin(), b.end());
+ b.push_back(1L);
+ a.insert(a.end(), b.begin(), b.end());
+
+ std::vector<long> c;
+ a.insert(a.end(), c.begin(), c.end());
+ c.push_back(1L);
+ a.insert(a.end(), c.begin(), c.end());
+}
diff --git a/libstdc++-v3/testsuite/23_containers/vector/debug/60587_neg.cc
b/libstdc++-v3/testsuite/23_containers/vector/debug/60587_neg.cc
new file mode 100644
index 0000000..219271b
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/debug/60587_neg.cc
@@ -0,0 +1,29 @@
+// 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-require-debug-mode "" }
+// { dg-do run { xfail *-*-* } }
+
+// PR libstdc++/60587
+
+#include <vector>
+
+int main() {
+ std::vector<int> a;
+ a.push_back(1);
+ a.insert(a.end(), a.begin(), a.begin()); // Expected to abort here
+}
