* vector lightweight debug mode
@ 2015-09-14 18:30 François Dumont
2015-09-14 19:55 ` Jonathan Wakely
0 siblings, 1 reply; 18+ messages in thread
From: François Dumont @ 2015-09-14 18:30 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 466 bytes --]
Hi
Here is what I had in mind when talking about moving debug checks to
the lightweight debug checks.
Sometimes the checks have been simply moved resulting in a simpler
debug vector implementation (front, back...). Sometimes I copy the
checks in a simpler form and kept the debug one too to make sure
execution of the debug code is fine.
I plan to do the same for other containers.
I still need to run tests, ok if tests are fine ?
François
[-- Attachment #2: vector_debug.patch --]
[-- Type: text/x-patch, Size: 8395 bytes --]
diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 305d446..89a9aec 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -449,6 +449,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector&
operator=(vector&& __x) noexcept(_Alloc_traits::_S_nothrow_move())
{
+ __glibcxx_assert(this != &__x);
constexpr bool __move_storage =
_Alloc_traits::_S_propagate_on_move_assign()
|| _Alloc_traits::_S_always_equal();
@@ -778,7 +779,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
operator[](size_type __n) _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_assert(__n < size());
+ return *(this->_M_impl._M_start + __n);
+ }
/**
* @brief Subscript access to the data contained in the %vector.
@@ -793,7 +797,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_assert(__n < size());
+ return *(this->_M_impl._M_start + __n);
+ }
protected:
/// Safety check used only from at().
@@ -850,7 +857,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
front() _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_assert(!empty());
+ return *begin();
+ }
/**
* Returns a read-only (constant) reference to the data at the first
@@ -858,7 +868,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
front() const _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_assert(!empty());
+ return *begin();
+ }
/**
* Returns a read/write reference to the data at the last
@@ -866,7 +879,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
back() _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_assert(!empty());
+ return *(end() - 1);
+ }
/**
* Returns a read-only (constant) reference to the data at the
@@ -874,7 +890,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
back() const _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_assert(!empty());
+ return *(end() - 1);
+ }
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
@@ -949,6 +968,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
pop_back() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_assert(!empty());
--this->_M_impl._M_finish;
_Alloc_traits::destroy(this->_M_impl, this->_M_impl._M_finish);
}
@@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
insert(const_iterator __position, size_type __n, const value_type& __x)
{
+ __glibcxx_assert(__position >= cbegin() && __position <= cend());
difference_type __offset = __position - cbegin();
_M_fill_insert(begin() + __offset, __n, __x);
return begin() + __offset;
@@ -1071,7 +1092,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
void
insert(iterator __position, size_type __n, const value_type& __x)
- { _M_fill_insert(__position, __n, __x); }
+ {
+ __glibcxx_assert(__position >= begin() && __position <= end());
+ _M_fill_insert(__position, __n, __x);
+ }
#endif
#if __cplusplus >= 201103L
@@ -1096,6 +1120,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(const_iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_assert(__position >= cbegin() && __position <= cend());
difference_type __offset = __position - cbegin();
_M_insert_dispatch(begin() + __offset,
__first, __last, __false_type());
@@ -1121,6 +1146,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_assert(__position >= begin() && __position <= end());
// Check whether it's an integral type. If so, it's not an iterator.
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
_M_insert_dispatch(__position, __first, __last, _Integral());
@@ -1145,10 +1171,16 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
#if __cplusplus >= 201103L
erase(const_iterator __position)
- { return _M_erase(begin() + (__position - cbegin())); }
+ {
+ __glibcxx_assert(__position >= cbegin() && __position < cend());
+ return _M_erase(begin() + (__position - cbegin()));
+ }
#else
erase(iterator __position)
- { return _M_erase(__position); }
+ {
+ __glibcxx_assert(__position >= begin() && __position < end());
+ return _M_erase(__position);
+ }
#endif
/**
@@ -1173,13 +1205,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
#if __cplusplus >= 201103L
erase(const_iterator __first, const_iterator __last)
{
+ __glibcxx_assert(__first <= __last);
+ __glibcxx_assert(
+ __first == __last || __first >= cbegin() && __first < cend());
+ __glibcxx_assert(
+ __first == __last || __last > cbegin() && __last <= cend());
const auto __beg = begin();
const auto __cbeg = cbegin();
return _M_erase(__beg + (__first - __cbeg), __beg + (__last - __cbeg));
}
#else
erase(iterator __first, iterator __last)
- { return _M_erase(__first, __last); }
+ {
+ __glibcxx_assert(__first <= __last);
+ __glibcxx_assert(
+ __first == __last || __first >= begin() && __first < end());
+ __glibcxx_assert(
+ __first == __last || __last > begin() && __last <= end());
+ return _M_erase(__first, __last);
+ }
#endif
/**
@@ -1194,6 +1238,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
swap(vector& __x) _GLIBCXX_NOEXCEPT
{
+ __glibcxx_assert(_Alloc_traits::_S_propagate_on_swap() ||
+ this->get_allocator() == __x.get_allocator());
this->_M_impl._M_swap_data(__x._M_impl);
_Alloc_traits::_S_on_swap(_M_get_Tp_allocator(),
__x._M_get_Tp_allocator());
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index 34118a4..b38a15f 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -111,6 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(iterator __position, const value_type& __x)
#endif
{
+ __glibcxx_assert(__position >= begin() && __position <= end());
const size_type __n = __position - begin();
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
&& __position == end())
@@ -301,6 +302,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector<_Tp, _Alloc>::
emplace(const_iterator __position, _Args&&... __args)
{
+ __glibcxx_assert(__position >= begin() && __position <= end());
const size_type __n = __position - begin();
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
&& __position == end())
diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector
index fede4f0..7fccf28 100644
--- a/libstdc++-v3/include/debug/vector
+++ b/libstdc++-v3/include/debug/vector
@@ -406,49 +406,10 @@ namespace __debug
}
// element access:
- reference
- operator[](size_type __n) _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_subscript(__n);
- return _M_base()[__n];
- }
-
- const_reference
- operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_subscript(__n);
- return _M_base()[__n];
- }
-
+ using _Base::operator[];
using _Base::at;
-
- reference
- front() _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::front();
- }
-
- const_reference
- front() const _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::front();
- }
-
- reference
- back() _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::back();
- }
-
- const_reference
- back() const _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::back();
- }
+ using _Base::front;
+ using _Base::back;
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-09-14 18:30 vector lightweight debug mode François Dumont
@ 2015-09-14 19:55 ` Jonathan Wakely
2015-09-16 19:43 ` François Dumont
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Wakely @ 2015-09-14 19:55 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3019 bytes --]
On 14/09/15 20:27 +0200, François Dumont wrote:
>Hi
>
> Here is what I had in mind when talking about moving debug checks to
>the lightweight debug checks.
Ah yes, I hadn't thought about removing reundant checks from the
__gnu_debug containers, but that makes sense.
> Sometimes the checks have been simply moved resulting in a simpler
>debug vector implementation (front, back...). Sometimes I copy the
>checks in a simpler form and kept the debug one too to make sure
>execution of the debug code is fine.
>
> I plan to do the same for other containers.
>
> I still need to run tests, ok if tests are fine ?
>
>François
>
>diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
>index 305d446..89a9aec 100644
>--- a/libstdc++-v3/include/bits/stl_vector.h
>+++ b/libstdc++-v3/include/bits/stl_vector.h
>@@ -449,6 +449,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> vector&
> operator=(vector&& __x) noexcept(_Alloc_traits::_S_nothrow_move())
> {
>+ __glibcxx_assert(this != &__x);
Please don't do this, it fails in valid programs. The standard needs
to be fixed in this regard.
> constexpr bool __move_storage =
> _Alloc_traits::_S_propagate_on_move_assign()
> || _Alloc_traits::_S_always_equal();
>@@ -778,7 +779,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> */
> reference
> operator[](size_type __n) _GLIBCXX_NOEXCEPT
>- { return *(this->_M_impl._M_start + __n); }
>+ {
>+ __glibcxx_assert(__n < size());
>+ return *(this->_M_impl._M_start + __n);
>+ }
This could use __glibcxx_requires_subscript(__n), see the attached
patch.
>
> /**
> * @brief Subscript access to the data contained in the %vector.
>@@ -793,7 +797,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> */
> const_reference
> operator[](size_type __n) const _GLIBCXX_NOEXCEPT
>- { return *(this->_M_impl._M_start + __n); }
>+ {
>+ __glibcxx_assert(__n < size());
>+ return *(this->_M_impl._M_start + __n);
>+ }
>
> protected:
> /// Safety check used only from at().
>@@ -850,7 +857,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> */
> reference
> front() _GLIBCXX_NOEXCEPT
>- { return *begin(); }
>+ {
>+ __glibcxx_assert(!empty());
This is __glibcxx_requires_nonempty(), already defined for
_GLIBCXX_ASSERTIONS.
>@@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> iterator
> insert(const_iterator __position, size_type __n, const value_type& __x)
> {
>+ __glibcxx_assert(__position >= cbegin() && __position <= cend());
> difference_type __offset = __position - cbegin();
> _M_fill_insert(begin() + __offset, __n, __x);
> return begin() + __offset;
This is undefined behaviour, so I'd rather not add this check (I know
it's on the google branch, but it's still undefined behaviour).
We could do it with std::less, I suppose.
I've attached the simplest checks I thought we should add for vector
and deque.
[-- Attachment #2: debug.patch --]
[-- Type: text/plain, Size: 8027 bytes --]
commit c2b5d263b7553074c82a721dc59b71a2a4a84436
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Thu Sep 10 14:23:43 2015 +0100
Add cheap assertions to std::vector and std::deque.
PR libstdc++/56109
* include/bits/stl_deque.h (deque::operator[], deque::front,
deque::back, deque::pop_front, deque::pop_back, deque::swap): Assert
preconditions.
* include/bits/stl_vector.h (vector::operator[], vector::front,
vector::back, vector::pop_back, vector::swap): Likewise.
* include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define
__glibcxx_requires_subscript.
diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index e4fa6e3..fd38af9 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,3 +1,14 @@
+2015-09-10 Jonathan Wakely <jwakely@redhat.com>
+
+ PR libstdc++/56109
+ * include/bits/stl_deque.h (deque::operator[], deque::front,
+ deque::back, deque::pop_front, deque::pop_back, deque::swap): Assert
+ preconditions.
+ * include/bits/stl_vector.h (vector::operator[], vector::front,
+ vector::back, vector::pop_back, vector::swap): Likewise.
+ * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define
+ __glibcxx_requires_subscript.
+
2015-09-09 Jonathan Wakely <jwakely@redhat.com>
* doc/xml/manual/using.xml (_GLIBCXX_ASSERTIONS): Document.
diff --git a/libstdc++-v3/include/bits/stl_deque.h b/libstdc++-v3/include/bits/stl_deque.h
index f674245..3c8bb2e 100644
--- a/libstdc++-v3/include/bits/stl_deque.h
+++ b/libstdc++-v3/include/bits/stl_deque.h
@@ -1362,7 +1362,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
operator[](size_type __n) _GLIBCXX_NOEXCEPT
- { return this->_M_impl._M_start[difference_type(__n)]; }
+ {
+ __glibcxx_requires_subscript(__n);
+ return this->_M_impl._M_start[difference_type(__n)];
+ }
/**
* @brief Subscript access to the data contained in the %deque.
@@ -1377,7 +1380,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- { return this->_M_impl._M_start[difference_type(__n)]; }
+ {
+ __glibcxx_requires_subscript(__n);
+ return this->_M_impl._M_start[difference_type(__n)];
+ }
protected:
/// Safety check used only from at().
@@ -1434,7 +1440,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
front() _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+
+ return *begin();
+ }
/**
* Returns a read-only (constant) reference to the data at the first
@@ -1442,7 +1452,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
front() const _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+
+ return *begin();
+ }
/**
* Returns a read/write reference to the data at the last element of the
@@ -1451,6 +1465,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
reference
back() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
+
iterator __tmp = end();
--__tmp;
return *__tmp;
@@ -1463,6 +1479,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
const_reference
back() const _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
+
const_iterator __tmp = end();
--__tmp;
return *__tmp;
@@ -1546,6 +1564,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
pop_front() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
+
if (this->_M_impl._M_start._M_cur
!= this->_M_impl._M_start._M_last - 1)
{
@@ -1568,6 +1588,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
pop_back() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
+
if (this->_M_impl._M_finish._M_cur
!= this->_M_impl._M_finish._M_first)
{
@@ -1781,6 +1803,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
swap(deque& __x) _GLIBCXX_NOEXCEPT
{
+#if __cplusplus >= 201103L
+ __glibcxx_assert( _Alloc_traits::propagate_on_container_swap::value ||
+ _M_get_Tp_allocator() == __x._M_get_Tp_allocator() );
+#endif
_M_impl._M_swap_data(__x._M_impl);
_Alloc_traits::_S_on_swap(_M_get_Tp_allocator(),
__x._M_get_Tp_allocator());
diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 305d446..9cffcb9 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -778,7 +778,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
operator[](size_type __n) _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
/**
* @brief Subscript access to the data contained in the %vector.
@@ -793,7 +796,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
protected:
/// Safety check used only from at().
@@ -850,7 +856,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
front() _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read-only (constant) reference to the data at the first
@@ -858,7 +867,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
front() const _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read/write reference to the data at the last
@@ -866,7 +878,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
back() _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
/**
* Returns a read-only (constant) reference to the data at the
@@ -874,7 +889,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
back() const _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
@@ -949,6 +967,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
pop_back() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
+
--this->_M_impl._M_finish;
_Alloc_traits::destroy(this->_M_impl, this->_M_impl._M_finish);
}
@@ -1194,6 +1214,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
swap(vector& __x) _GLIBCXX_NOEXCEPT
{
+#if __cplusplus >= 201103L
+ __glibcxx_assert( _Alloc_traits::propagate_on_container_swap::value
+ || _M_get_Tp_allocator() == __x._M_get_Tp_allocator()
+ ;)
+#endif
this->_M_impl._M_swap_data(__x._M_impl);
_Alloc_traits::_S_on_swap(_M_get_Tp_allocator(),
__x._M_get_Tp_allocator());
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index b5935fe..d4ed4dc 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -84,12 +84,16 @@ namespace __gnu_debug
// Verify that [_First, _Last) forms a non-empty iterator range.
# define __glibcxx_requires_non_empty_range(_First,_Last) \
__glibcxx_assert(_First != _Last)
-// Verify that the container is nonempty
+// Verify that the container is nonempty.
# define __glibcxx_requires_nonempty() \
__glibcxx_assert(! this->empty())
+// Verify a subscript is in range.
+# define __glibcxx_requires_subscript(_N) \
+ __glibcxx_assert(_N < this->size())
#else
# define __glibcxx_requires_non_empty_range(_First,_Last)
# define __glibcxx_requires_nonempty()
+# define __glibcxx_requires_subscript(_N)
#endif
#else
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-09-14 19:55 ` Jonathan Wakely
@ 2015-09-16 19:43 ` François Dumont
2015-09-16 20:53 ` Jonathan Wakely
0 siblings, 1 reply; 18+ messages in thread
From: François Dumont @ 2015-09-16 19:43 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3756 bytes --]
On 14/09/2015 21:50, Jonathan Wakely wrote:
> On 14/09/15 20:27 +0200, François Dumont wrote:
>> Hi
>>
>> Here is what I had in mind when talking about moving debug checks to
>> the lightweight debug checks.
>
> Ah yes, I hadn't thought about removing reundant checks from the
> __gnu_debug containers, but that makes sense.
>
>> Sometimes the checks have been simply moved resulting in a simpler
>> debug vector implementation (front, back...). Sometimes I copy the
>> checks in a simpler form and kept the debug one too to make sure
>> execution of the debug code is fine.
>>
>> I plan to do the same for other containers.
>>
>> I still need to run tests, ok if tests are fine ?
>>
>> François
>>
>
>> diff --git a/libstdc++-v3/include/bits/stl_vector.h
>> b/libstdc++-v3/include/bits/stl_vector.h
>> index 305d446..89a9aec 100644
>> --- a/libstdc++-v3/include/bits/stl_vector.h
>> +++ b/libstdc++-v3/include/bits/stl_vector.h
>> @@ -449,6 +449,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>> vector&
>> operator=(vector&& __x) noexcept(_Alloc_traits::_S_nothrow_move())
>> {
>> + __glibcxx_assert(this != &__x);
>
> Please don't do this, it fails in valid programs. The standard needs
> to be fixed in this regard.
The debug mode check should be removed too then.
>
>> constexpr bool __move_storage =
>> _Alloc_traits::_S_propagate_on_move_assign()
>> || _Alloc_traits::_S_always_equal();
>> @@ -778,7 +779,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>> */
>> reference
>> operator[](size_type __n) _GLIBCXX_NOEXCEPT
>> - { return *(this->_M_impl._M_start + __n); }
>> + {
>> + __glibcxx_assert(__n < size());
>> + return *(this->_M_impl._M_start + __n);
>> + }
>
> This could use __glibcxx_requires_subscript(__n), see the attached
> patch.
I thought you didn't want to use anything from debug.h so I try to
do with only __glibcxx_assert coming from c++config. I think your patch
is missing include of debug.h.
But I was going to propose to use _Error_formatter also in this
mode, I do not see any reason to not do so. The attached patch does just
that.
>
>>
>> /**
>> * @brief Subscript access to the data contained in the %vector.
>> @@ -793,7 +797,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>> */
>> const_reference
>> operator[](size_type __n) const _GLIBCXX_NOEXCEPT
>> - { return *(this->_M_impl._M_start + __n); }
>> + {
>> + __glibcxx_assert(__n < size());
>> + return *(this->_M_impl._M_start + __n);
>> + }
>>
>> protected:
>> /// Safety check used only from at().
>> @@ -850,7 +857,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>> */
>> reference
>> front() _GLIBCXX_NOEXCEPT
>> - { return *begin(); }
>> + {
>> + __glibcxx_assert(!empty());
>
> This is __glibcxx_requires_nonempty(), already defined for
> _GLIBCXX_ASSERTIONS.
>
>> @@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>> iterator
>> insert(const_iterator __position, size_type __n, const
>> value_type& __x)
>> {
>> + __glibcxx_assert(__position >= cbegin() && __position <= cend());
>> difference_type __offset = __position - cbegin();
>> _M_fill_insert(begin() + __offset, __n, __x);
>> return begin() + __offset;
>
> This is undefined behaviour, so I'd rather not add this check (I know
> it's on the google branch, but it's still undefined behaviour).
Why ? Because of the >= operator usage ? Is the attached patch better ?
< and == operators are well defined for a random access iterator, no ?
>
> We could do it with std::less, I suppose.
>
> I've attached the simplest checks I thought we should add for vector
> and deque.
>
>
[-- Attachment #2: vector_debug.patch --]
[-- Type: text/x-patch, Size: 14389 bytes --]
diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 305d446..b84e653 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -63,6 +63,8 @@
#include <initializer_list>
#endif
+#include <debug/debug.h>
+
namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_CONTAINER
@@ -778,7 +780,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
operator[](size_type __n) _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
/**
* @brief Subscript access to the data contained in the %vector.
@@ -793,7 +798,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
protected:
/// Safety check used only from at().
@@ -850,7 +858,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
front() _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read-only (constant) reference to the data at the first
@@ -858,7 +869,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
front() const _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read/write reference to the data at the last
@@ -866,7 +880,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
back() _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
/**
* Returns a read-only (constant) reference to the data at the
@@ -874,7 +891,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
back() const _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
@@ -949,6 +969,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
pop_back() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
--this->_M_impl._M_finish;
_Alloc_traits::destroy(this->_M_impl, this->_M_impl._M_finish);
}
@@ -1051,6 +1072,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
insert(const_iterator __position, size_type __n, const value_type& __x)
{
+ __glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+ && (__position < cend() || !(cend() < __position)));
difference_type __offset = __position - cbegin();
_M_fill_insert(begin() + __offset, __n, __x);
return begin() + __offset;
@@ -1071,7 +1094,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
void
insert(iterator __position, size_type __n, const value_type& __x)
- { _M_fill_insert(__position, __n, __x); }
+ {
+ __glibcxx_assert((begin() < __position || !(__position < begin()))
+ && (__position < end() || !(end() < __position)));
+ _M_fill_insert(__position, __n, __x);
+ }
#endif
#if __cplusplus >= 201103L
@@ -1096,6 +1123,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(const_iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+ && (__position < cend() || !(cend() < __position)));
difference_type __offset = __position - cbegin();
_M_insert_dispatch(begin() + __offset,
__first, __last, __false_type());
@@ -1121,6 +1150,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_assert((begin() < __position || !(__position < begin()))
+ && (__position < end() || !(end() < __position)));
// Check whether it's an integral type. If so, it's not an iterator.
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
_M_insert_dispatch(__position, __first, __last, _Integral());
@@ -1145,10 +1176,18 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
#if __cplusplus >= 201103L
erase(const_iterator __position)
- { return _M_erase(begin() + (__position - cbegin())); }
+ {
+ __glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+ && __position < cend());
+ return _M_erase(begin() + (__position - cbegin()));
+ }
#else
erase(iterator __position)
- { return _M_erase(__position); }
+ {
+ __glibcxx_assert((begin() < __position || !(__position < begin()))
+ && __position < end());
+ return _M_erase(__position);
+ }
#endif
/**
@@ -1173,13 +1212,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
#if __cplusplus >= 201103L
erase(const_iterator __first, const_iterator __last)
{
+ __glibcxx_assert(__first < __last || !(__last < __first));
+ __glibcxx_assert((cbegin() < __first || !(__first < cbegin()))
+ && (__first < cend() || __first == __last));
+ __glibcxx_assert((cbegin() < __last || __first == __last)
+ && (__last < cend() || !(cend() < __last)));
const auto __beg = begin();
const auto __cbeg = cbegin();
return _M_erase(__beg + (__first - __cbeg), __beg + (__last - __cbeg));
}
#else
erase(iterator __first, iterator __last)
- { return _M_erase(__first, __last); }
+ {
+ __glibcxx_assert(__first < __last || !(__last < __first));
+ __glibcxx_assert((begin() < __first || !(__first < begin()))
+ && (__first < end() || __first == __last));
+ __glibcxx_assert((begin() < __last || __first == __last)
+ && (__last < end() || !(end() < __last)));
+ return _M_erase(__first, __last);
+ }
#endif
/**
@@ -1194,6 +1245,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
swap(vector& __x) _GLIBCXX_NOEXCEPT
{
+#if __cplusplus >= 201103L
+ __glibcxx_assert(_Alloc_traits::propagate_on_container_swap::value
+ || _M_get_Tp_allocator() == __x._M_get_Tp_allocator());
+#endif
this->_M_impl._M_swap_data(__x._M_impl);
_Alloc_traits::_S_on_swap(_M_get_Tp_allocator(),
__x._M_get_Tp_allocator());
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index 34118a4..b00a620 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -107,10 +107,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector<_Tp, _Alloc>::
#if __cplusplus >= 201103L
insert(const_iterator __position, const value_type& __x)
+ {
+ __glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+ && (__position < cend() || !(cend() < __position)));
#else
insert(iterator __position, const value_type& __x)
-#endif
{
+ __glibcxx_assert((begin() < __position || !(__position < begin()))
+ && (__position < end() || !(end() < __position)));
+#endif
const size_type __n = __position - begin();
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
&& __position == end())
@@ -301,6 +306,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector<_Tp, _Alloc>::
emplace(const_iterator __position, _Args&&... __args)
{
+ __glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+ && (__position < cend() || !(cend() < __position)));
const size_type __n = __position - begin();
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
&& __position == end())
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index b5935fe..ea9c0c2 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -74,68 +74,66 @@ namespace __gnu_debug
# define __glibcxx_requires_heap_pred(_First,_Last,_Pred)
# define __glibcxx_requires_string(_String)
# define __glibcxx_requires_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last)
# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)
# define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)
-#ifdef _GLIBCXX_ASSERTIONS
-// Verify that [_First, _Last) forms a non-empty iterator range.
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_assert(_First != _Last)
-// Verify that the container is nonempty
-# define __glibcxx_requires_nonempty() \
- __glibcxx_assert(! this->empty())
-#else
-# define __glibcxx_requires_non_empty_range(_First,_Last)
-# define __glibcxx_requires_nonempty()
#endif
+#ifndef _GLIBCXX_ASSERTIONS
+# define __glibcxx_requires_non_empty_range(_First,_Last)
+# define __glibcxx_requires_nonempty()
+# define __glibcxx_requires_subscript(_N)
#else
# include <debug/macros.h>
-# define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
-# define __glibcxx_requires_valid_range(_First,_Last) \
+# ifdef _GLIBCXX_DEBUG
+# define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
+# define __glibcxx_requires_valid_range(_First,_Last) \
__glibcxx_check_valid_range(_First,_Last)
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_check_non_empty_range(_First,_Last)
-# define __glibcxx_requires_sorted(_First,_Last) \
+# define __glibcxx_requires_sorted(_First,_Last) \
__glibcxx_check_sorted(_First,_Last)
-# define __glibcxx_requires_sorted_pred(_First,_Last,_Pred) \
+# define __glibcxx_requires_sorted_pred(_First,_Last,_Pred) \
__glibcxx_check_sorted_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_sorted_set(_First1,_Last1,_First2) \
+# define __glibcxx_requires_sorted_set(_First1,_Last1,_First2) \
__glibcxx_check_sorted_set(_First1,_Last1,_First2)
-# define __glibcxx_requires_sorted_set_pred(_First1,_Last1,_First2,_Pred) \
+# define __glibcxx_requires_sorted_set_pred(_First1,_Last1,_First2,_Pred) \
__glibcxx_check_sorted_set_pred(_First1,_Last1,_First2,_Pred)
-# define __glibcxx_requires_partitioned_lower(_First,_Last,_Value) \
+# define __glibcxx_requires_partitioned_lower(_First,_Last,_Value) \
__glibcxx_check_partitioned_lower(_First,_Last,_Value)
-# define __glibcxx_requires_partitioned_upper(_First,_Last,_Value) \
+# define __glibcxx_requires_partitioned_upper(_First,_Last,_Value) \
__glibcxx_check_partitioned_upper(_First,_Last,_Value)
-# define __glibcxx_requires_partitioned_lower_pred(_First,_Last,_Value,_Pred) \
+# define __glibcxx_requires_partitioned_lower_pred(_First,_Last,_Value,_Pred) \
__glibcxx_check_partitioned_lower_pred(_First,_Last,_Value,_Pred)
-# define __glibcxx_requires_partitioned_upper_pred(_First,_Last,_Value,_Pred) \
+# define __glibcxx_requires_partitioned_upper_pred(_First,_Last,_Value,_Pred) \
__glibcxx_check_partitioned_upper_pred(_First,_Last,_Value,_Pred)
-# define __glibcxx_requires_heap(_First,_Last) \
+# define __glibcxx_requires_heap(_First,_Last) \
__glibcxx_check_heap(_First,_Last)
-# define __glibcxx_requires_heap_pred(_First,_Last,_Pred) \
+# define __glibcxx_requires_heap_pred(_First,_Last,_Pred) \
__glibcxx_check_heap_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_nonempty() __glibcxx_check_nonempty()
-# define __glibcxx_requires_string(_String) __glibcxx_check_string(_String)
-# define __glibcxx_requires_string_len(_String,_Len) \
+# define __glibcxx_requires_string(_String) __glibcxx_check_string(_String)
+# define __glibcxx_requires_string_len(_String,_Len) \
__glibcxx_check_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
-# define __glibcxx_requires_irreflexive(_First,_Last) \
+# define __glibcxx_requires_irreflexive(_First,_Last) \
__glibcxx_check_irreflexive(_First,_Last)
-# define __glibcxx_requires_irreflexive2(_First,_Last) \
+# define __glibcxx_requires_irreflexive2(_First,_Last) \
__glibcxx_check_irreflexive2(_First,_Last)
-# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred) \
+# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred) \
__glibcxx_check_irreflexive_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred) \
+# define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred) \
__glibcxx_check_irreflexive_pred2(_First,_Last,_Pred)
-# include <debug/functions.h>
+# include <debug/functions.h>
+#endif
+
+# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
+# define __glibcxx_requires_non_empty_range(_First,_Last) \
+ __glibcxx_check_non_empty_range(_First,_Last)
+# define __glibcxx_requires_nonempty() __glibcxx_check_nonempty()
+
+# include <debug/formatter.h>
#endif
diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector
index fede4f0..7fccf28 100644
--- a/libstdc++-v3/include/debug/vector
+++ b/libstdc++-v3/include/debug/vector
@@ -406,49 +406,10 @@ namespace __debug
}
// element access:
- reference
- operator[](size_type __n) _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_subscript(__n);
- return _M_base()[__n];
- }
-
- const_reference
- operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_subscript(__n);
- return _M_base()[__n];
- }
-
+ using _Base::operator[];
using _Base::at;
-
- reference
- front() _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::front();
- }
-
- const_reference
- front() const _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::front();
- }
-
- reference
- back() _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::back();
- }
-
- const_reference
- back() const _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::back();
- }
+ using _Base::front;
+ using _Base::back;
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-09-16 19:43 ` François Dumont
@ 2015-09-16 20:53 ` Jonathan Wakely
[not found] ` <CA+jCFLueUi0Zo4m2D-scXNG5JLVSObKbJgXWaQRJrQ+notbXzg@mail.gmail.com>
2015-09-19 9:09 ` François Dumont
0 siblings, 2 replies; 18+ messages in thread
From: Jonathan Wakely @ 2015-09-16 20:53 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On 16/09/15 21:37 +0200, François Dumont wrote:
>On 14/09/2015 21:50, Jonathan Wakely wrote:
>> On 14/09/15 20:27 +0200, François Dumont wrote:
>>> diff --git a/libstdc++-v3/include/bits/stl_vector.h
>>> b/libstdc++-v3/include/bits/stl_vector.h
>>> index 305d446..89a9aec 100644
>>> --- a/libstdc++-v3/include/bits/stl_vector.h
>>> +++ b/libstdc++-v3/include/bits/stl_vector.h
>>> @@ -449,6 +449,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>> vector&
>>> operator=(vector&& __x) noexcept(_Alloc_traits::_S_nothrow_move())
>>> {
>>> + __glibcxx_assert(this != &__x);
>>
>> Please don't do this, it fails in valid programs. The standard needs
>> to be fixed in this regard.
>
>The debug mode check should be removed too then.
Yes.
>>
>>> constexpr bool __move_storage =
>>> _Alloc_traits::_S_propagate_on_move_assign()
>>> || _Alloc_traits::_S_always_equal();
>>> @@ -778,7 +779,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>> */
>>> reference
>>> operator[](size_type __n) _GLIBCXX_NOEXCEPT
>>> - { return *(this->_M_impl._M_start + __n); }
>>> + {
>>> + __glibcxx_assert(__n < size());
>>> + return *(this->_M_impl._M_start + __n);
>>> + }
>>
>> This could use __glibcxx_requires_subscript(__n), see the attached
>> patch.
>
> I thought you didn't want to use anything from debug.h so I try to
>do with only __glibcxx_assert coming from c++config. I think your patch
>is missing include of debug.h.
>
> But I was going to propose to use _Error_formatter also in this
>mode, I do not see any reason to not do so. The attached patch does just
>that.
That pulls in extra dependencies on I/O and fprintf and things, which
can cause code size to increase. Is it really worth it?
>>> @@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>> iterator
>>> insert(const_iterator __position, size_type __n, const
>>> value_type& __x)
>>> {
>>> + __glibcxx_assert(__position >= cbegin() && __position <= cend());
>>> difference_type __offset = __position - cbegin();
>>> _M_fill_insert(begin() + __offset, __n, __x);
>>> return begin() + __offset;
>>
>> This is undefined behaviour, so I'd rather not add this check (I know
>> it's on the google branch, but it's still undefined behaviour).
>
>Why ? Because of the >= operator usage ? Is the attached patch better ?
>< and == operators are well defined for a random access iterator, no ?
No, because it is undefined to compare iterators that belong to
different containers, or to compare pointers that point to different
arrays.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Fwd: vector lightweight debug mode
[not found] ` <CA+jCFLueUi0Zo4m2D-scXNG5JLVSObKbJgXWaQRJrQ+notbXzg@mail.gmail.com>
@ 2015-09-17 21:14 ` Christopher Jefferson
2015-09-19 9:12 ` Jonathan Wakely
0 siblings, 1 reply; 18+ messages in thread
From: Christopher Jefferson @ 2015-09-17 21:14 UTC (permalink / raw)
To: François Dumont, libstdc++, gcc-patches
---------- Forwarded message ----------
From: Christopher Jefferson <chris@bubblescope.net>
Date: 17 September 2015 at 18:59
Subject: Re: vector lightweight debug mode
To: Jonathan Wakely <jwakely@redhat.com>
On 16 September 2015 at 21:29, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 16/09/15 21:37 +0200, François Dumont wrote:
>
>>>> @@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>>> iterator
>>>> insert(const_iterator __position, size_type __n, const
>>>> value_type& __x)
>>>> {
>>>> + __glibcxx_assert(__position >= cbegin() && __position <= cend());
>>>> difference_type __offset = __position - cbegin();
>>>> _M_fill_insert(begin() + __offset, __n, __x);
>>>> return begin() + __offset;
>>>
>>>
>>> This is undefined behaviour, so I'd rather not add this check (I know
>>> it's on the google branch, but it's still undefined behaviour).
>>
>>
>> Why ? Because of the >= operator usage ? Is the attached patch better ?
>> < and == operators are well defined for a random access iterator, no ?
>
>
> No, because it is undefined to compare iterators that belong to
> different containers, or to compare pointers that point to different
> arrays.
While that's true, on the other hand it's defined behaviour when the
assert passes, and in the case where the thing it's trying to check
fails, we are off into undefined-land anyway.
A defined check would be to check if __offset is < 0 or > size(). Once
again if it's false we are undefined, but the assert line itself is
then defined behaviour.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-09-16 20:53 ` Jonathan Wakely
[not found] ` <CA+jCFLueUi0Zo4m2D-scXNG5JLVSObKbJgXWaQRJrQ+notbXzg@mail.gmail.com>
@ 2015-09-19 9:09 ` François Dumont
2015-09-19 9:48 ` Jonathan Wakely
1 sibling, 1 reply; 18+ messages in thread
From: François Dumont @ 2015-09-19 9:09 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
On 16/09/2015 22:29, Jonathan Wakely wrote:
>
>>>
>>>> constexpr bool __move_storage =
>>>> _Alloc_traits::_S_propagate_on_move_assign()
>>>> || _Alloc_traits::_S_always_equal();
>>>> @@ -778,7 +779,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>>> */
>>>> reference
>>>> operator[](size_type __n) _GLIBCXX_NOEXCEPT
>>>> - { return *(this->_M_impl._M_start + __n); }
>>>> + {
>>>> + __glibcxx_assert(__n < size());
>>>> + return *(this->_M_impl._M_start + __n);
>>>> + }
>>>
>>> This could use __glibcxx_requires_subscript(__n), see the attached
>>> patch.
>>
>> I thought you didn't want to use anything from debug.h so I try to
>> do with only __glibcxx_assert coming from c++config. I think your patch
>> is missing include of debug.h.
>>
>> But I was going to propose to use _Error_formatter also in this
>> mode, I do not see any reason to not do so. The attached patch does just
>> that.
>
> That pulls in extra dependencies on I/O and fprintf and things, which
> can cause code size to increase. Is it really worth it?
Not that much dependencies. We only need formatters.h in this mode which
has the following common includes:
#include <bits/c++config.h>
#include <bits/cpp_type_traits.h>
and if rtti is enabled the less common:
#include <typeinfo>
We would just leverage on the good job done to diagnose problems.
>
>
>>>> @@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>>> iterator
>>>> insert(const_iterator __position, size_type __n, const
>>>> value_type& __x)
>>>> {
>>>> + __glibcxx_assert(__position >= cbegin() && __position <= cend());
>>>> difference_type __offset = __position - cbegin();
>>>> _M_fill_insert(begin() + __offset, __n, __x);
>>>> return begin() + __offset;
>>>
>>> This is undefined behaviour, so I'd rather not add this check (I know
>>> it's on the google branch, but it's still undefined behaviour).
>>
>> Why ? Because of the >= operator usage ? Is the attached patch better ?
>> < and == operators are well defined for a random access iterator, no ?
>
> No, because it is undefined to compare iterators that belong to
> different containers, or to compare pointers that point to different
> arrays.
>
(Written before Christopher reply:)
At least program will compile only if iterator is coming from a vector
of the same type. So behavior is undefined only if user pass an invalid
iterator which is exactly what this check tries to detect, isn't it
paradoxical ? If this undefined behavior results in the program abortion
this is what should happen anyway. If it doesn't abort then the program
will definitely not behaves as expected so this check doesn't make
anything worst, no ?
François
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-09-17 21:14 ` Fwd: " Christopher Jefferson
@ 2015-09-19 9:12 ` Jonathan Wakely
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Wakely @ 2015-09-19 9:12 UTC (permalink / raw)
To: Christopher Jefferson; +Cc: François Dumont, libstdc++, gcc-patches
On 17 September 2015 at 21:52, Christopher Jefferson
<chris@bubblescope.net> wrote:
> ---------- Forwarded message ----------
> From: Christopher Jefferson <chris@bubblescope.net>
> Date: 17 September 2015 at 18:59
> Subject: Re: vector lightweight debug mode
> To: Jonathan Wakely <jwakely@redhat.com>
>
>
> On 16 September 2015 at 21:29, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 16/09/15 21:37 +0200, François Dumont wrote:
>>
>>>>> @@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>>>> iterator
>>>>> insert(const_iterator __position, size_type __n, const
>>>>> value_type& __x)
>>>>> {
>>>>> + __glibcxx_assert(__position >= cbegin() && __position <= cend());
>>>>> difference_type __offset = __position - cbegin();
>>>>> _M_fill_insert(begin() + __offset, __n, __x);
>>>>> return begin() + __offset;
>>>>
>>>>
>>>> This is undefined behaviour, so I'd rather not add this check (I know
>>>> it's on the google branch, but it's still undefined behaviour).
>>>
>>>
>>> Why ? Because of the >= operator usage ? Is the attached patch better ?
>>> < and == operators are well defined for a random access iterator, no ?
>>
>>
>> No, because it is undefined to compare iterators that belong to
>> different containers, or to compare pointers that point to different
>> arrays.
>
> While that's true, on the other hand it's defined behaviour when the
> assert passes, and in the case where the thing it's trying to check
> fails, we are off into undefined-land anyway.
>
> A defined check would be to check if __offset is < 0 or > size(). Once
> again if it's false we are undefined, but the assert line itself is
> then defined behaviour.
That's a good point, but it still means an optimiser could remove the
checks, because it is impossible for them to fail in a correct
program.
That would be no worse than not having the checks at all, but it could
make them unreliable.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-09-19 9:09 ` François Dumont
@ 2015-09-19 9:48 ` Jonathan Wakely
2015-09-19 12:00 ` Jonathan Wakely
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Wakely @ 2015-09-19 9:48 UTC (permalink / raw)
To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On 19 September 2015 at 08:31, François Dumont wrote:
> On 16/09/2015 22:29, Jonathan Wakely wrote:
>> No, because it is undefined to compare iterators that belong to
>> different containers, or to compare pointers that point to different
>> arrays.
>>
>
> (Written before Christopher reply:)
>
> At least program will compile only if iterator is coming from a vector
> of the same type. So behavior is undefined only if user pass an invalid
> iterator which is exactly what this check tries to detect, isn't it
> paradoxical ? If this undefined behavior results in the program abortion
> this is what should happen anyway. If it doesn't abort then the program
> will definitely not behaves as expected so this check doesn't make
> anything worst, no ?
The problem is that undefined behaviour can "travel backwards in time".
It's not as simple as saying that if the invalid check happens _then_
undefined behaviour happens afterwards.
However, Google seem to find these checks useful, and you and Chris
are in favour, so let's keep them.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-09-19 9:48 ` Jonathan Wakely
@ 2015-09-19 12:00 ` Jonathan Wakely
2015-10-07 19:38 ` François Dumont
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Wakely @ 2015-09-19 12:00 UTC (permalink / raw)
To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On 19 September 2015 at 10:12, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 19 September 2015 at 08:31, François Dumont wrote:
>> On 16/09/2015 22:29, Jonathan Wakely wrote:
>>> No, because it is undefined to compare iterators that belong to
>>> different containers, or to compare pointers that point to different
>>> arrays.
>>>
>>
>> (Written before Christopher reply:)
>>
>> At least program will compile only if iterator is coming from a vector
>> of the same type. So behavior is undefined only if user pass an invalid
>> iterator which is exactly what this check tries to detect, isn't it
>> paradoxical ? If this undefined behavior results in the program abortion
>> this is what should happen anyway. If it doesn't abort then the program
>> will definitely not behaves as expected so this check doesn't make
>> anything worst, no ?
>
> The problem is that undefined behaviour can "travel backwards in time".
>
> It's not as simple as saying that if the invalid check happens _then_
> undefined behaviour happens afterwards.
Just to be clear, I agree that it can't hurt a correct program (except
for the small cost of doing the checks).
My concern was that for an incorrect program (which is the entire
purpose of adding the checks) the results could be unpredictable. It
might abort, which is the desired behaviour, or it might do something
else and keep executing, and in that case it could be harmful for
debugging because users would look at the source and think "well my
iterators must be in range, otherwise that assertion would have
failed, so the bug must be elsewhere".
However ...
> However, Google seem to find these checks useful, and you and Chris
> are in favour, so let's keep them.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-09-19 12:00 ` Jonathan Wakely
@ 2015-10-07 19:38 ` François Dumont
2015-10-07 20:09 ` Jonathan Wakely
0 siblings, 1 reply; 18+ messages in thread
From: François Dumont @ 2015-10-07 19:38 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3828 bytes --]
Hi
I completed vector assertion mode. Here is the result of the new
test you will find in the attached patch.
With debug mode:
/home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:375:
Error: attempt to advance a dereferenceable (start-of-sequence) iterator 2
steps, which falls outside its valid range.
Objects involved in the operation:
iterator @ 0x0x7fff1c346760 {
type =
__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*,
std::__cxx1998::vector<int, std::allocator<int> > >,
std::__debug::vector<int, std::allocator<int> > > (mutable iterator);
state = dereferenceable (start-of-sequence);
references sequence with type 'std::__debug::vector<int,
std::allocator<int> >' @ 0x0x7fff1c3469a0
}
XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
With assertion mode:
/home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/bits/stl_vector.h:1124:
Error: invalid insert position outside container [begin, end) range.
Objects involved in the operation:
sequence "this" @ 0x0x7fff60b1f870 {
type = std::vector<int, std::allocator<int> >;
}
iterator "__position" @ 0x0x7fff60b1f860 {
type = __gnu_cxx::__normal_iterator<int const*, std::vector<int,
std::allocator<int> > >;
}
XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
As expected assertion mode doesn't detect invalid operation on the
iterator but only its usage later.
There is still one debug assertion I would like to use in assertion
mode: __valid_range. I could use it to check erase range and iterator
range used on the insert method. But for that it means including
<debug/helper_functions.h> and so:
#include <bits/stl_iterator_base_types.h> // for iterator_traits,
// categories and _Iter_base
#include <bits/cpp_type_traits.h> // for __is_integer
#include <bits/stl_pair.h> // for pair
That looks fine to me, what do you think ?
François
On 19/09/2015 11:47, Jonathan Wakely wrote:
> On 19 September 2015 at 10:12, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>> On 19 September 2015 at 08:31, François Dumont wrote:
>>> On 16/09/2015 22:29, Jonathan Wakely wrote:
>>>> No, because it is undefined to compare iterators that belong to
>>>> different containers, or to compare pointers that point to different
>>>> arrays.
>>>>
>>> (Written before Christopher reply:)
>>>
>>> At least program will compile only if iterator is coming from a vector
>>> of the same type. So behavior is undefined only if user pass an invalid
>>> iterator which is exactly what this check tries to detect, isn't it
>>> paradoxical ? If this undefined behavior results in the program abortion
>>> this is what should happen anyway. If it doesn't abort then the program
>>> will definitely not behaves as expected so this check doesn't make
>>> anything worst, no ?
>> The problem is that undefined behaviour can "travel backwards in time".
>>
>> It's not as simple as saying that if the invalid check happens _then_
>> undefined behaviour happens afterwards.
> Just to be clear, I agree that it can't hurt a correct program (except
> for the small cost of doing the checks).
>
> My concern was that for an incorrect program (which is the entire
> purpose of adding the checks) the results could be unpredictable. It
> might abort, which is the desired behaviour, or it might do something
> else and keep executing, and in that case it could be harmful for
> debugging because users would look at the source and think "well my
> iterators must be in range, otherwise that assertion would have
> failed, so the bug must be elsewhere".
>
> However ...
>
>> However, Google seem to find these checks useful, and you and Chris
>> are in favour, so let's keep them.
[-- Attachment #2: vector_debug.patch --]
[-- Type: text/x-patch, Size: 17537 bytes --]
diff --git libstdc++-v3/include/bits/stl_vector.h libstdc++-v3/include/bits/stl_vector.h
index 305d446..23e2e6a 100644
--- libstdc++-v3/include/bits/stl_vector.h
+++ libstdc++-v3/include/bits/stl_vector.h
@@ -63,6 +63,8 @@
#include <initializer_list>
#endif
+#include <debug/debug.h>
+
namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_CONTAINER
@@ -778,7 +780,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
operator[](size_type __n) _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
/**
* @brief Subscript access to the data contained in the %vector.
@@ -793,7 +798,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
protected:
/// Safety check used only from at().
@@ -850,7 +858,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
front() _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read-only (constant) reference to the data at the first
@@ -858,7 +869,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
front() const _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read/write reference to the data at the last
@@ -866,7 +880,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
back() _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
/**
* Returns a read-only (constant) reference to the data at the
@@ -874,7 +891,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
back() const _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
@@ -949,6 +969,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
pop_back() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
--this->_M_impl._M_finish;
_Alloc_traits::destroy(this->_M_impl, this->_M_impl._M_finish);
}
@@ -1051,6 +1072,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
insert(const_iterator __position, size_type __n, const value_type& __x)
{
+ __glibcxx_requires_valid_insert_position(__position);
difference_type __offset = __position - cbegin();
_M_fill_insert(begin() + __offset, __n, __x);
return begin() + __offset;
@@ -1071,7 +1093,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
void
insert(iterator __position, size_type __n, const value_type& __x)
- { _M_fill_insert(__position, __n, __x); }
+ {
+ __glibcxx_requires_valid_insert_position(__position);
+ _M_fill_insert(__position, __n, __x);
+ }
#endif
#if __cplusplus >= 201103L
@@ -1096,6 +1121,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(const_iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_requires_valid_insert_position(__position);
difference_type __offset = __position - cbegin();
_M_insert_dispatch(begin() + __offset,
__first, __last, __false_type());
@@ -1121,6 +1147,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_requires_valid_insert_position(__position);
// Check whether it's an integral type. If so, it's not an iterator.
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
_M_insert_dispatch(__position, __first, __last, _Integral());
@@ -1145,10 +1172,16 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
#if __cplusplus >= 201103L
erase(const_iterator __position)
- { return _M_erase(begin() + (__position - cbegin())); }
+ {
+ __glibcxx_requires_valid_erase_position(__position);
+ return _M_erase(begin() + (__position - cbegin()));
+ }
#else
erase(iterator __position)
- { return _M_erase(__position); }
+ {
+ __glibcxx_requires_valid_erase_position(__position);
+ return _M_erase(__position);
+ }
#endif
/**
@@ -1173,13 +1206,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
#if __cplusplus >= 201103L
erase(const_iterator __first, const_iterator __last)
{
+ __glibcxx_assert(__first < __last || !(__last < __first));
+ __glibcxx_assert((cbegin() < __first || !(__first < cbegin()))
+ && (__first < cend() || __first == __last));
+ __glibcxx_assert((cbegin() < __last || __first == __last)
+ && (__last < cend() || !(cend() < __last)));
const auto __beg = begin();
const auto __cbeg = cbegin();
return _M_erase(__beg + (__first - __cbeg), __beg + (__last - __cbeg));
}
#else
erase(iterator __first, iterator __last)
- { return _M_erase(__first, __last); }
+ {
+ __glibcxx_assert(__first < __last || !(__last < __first));
+ __glibcxx_assert((begin() < __first || !(__first < begin()))
+ && (__first < end() || __first == __last));
+ __glibcxx_assert((begin() < __last || __first == __last)
+ && (__last < end() || !(end() < __last)));
+ return _M_erase(__first, __last);
+ }
#endif
/**
@@ -1194,6 +1239,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
swap(vector& __x) _GLIBCXX_NOEXCEPT
{
+#if __cplusplus >= 201103L
+ __glibcxx_requires_cond(
+ _Alloc_traits::propagate_on_container_swap::value
+ || _M_get_Tp_allocator() == __x._M_get_Tp_allocator(),
+ _M_message(__gnu_debug::__msg_equal_allocs)
+ ._M_sequence(this, "this"));
+#endif
this->_M_impl._M_swap_data(__x._M_impl);
_Alloc_traits::_S_on_swap(_M_get_Tp_allocator(),
__x._M_get_Tp_allocator());
diff --git libstdc++-v3/include/bits/vector.tcc libstdc++-v3/include/bits/vector.tcc
index 34118a4..965dab3 100644
--- libstdc++-v3/include/bits/vector.tcc
+++ libstdc++-v3/include/bits/vector.tcc
@@ -111,6 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(iterator __position, const value_type& __x)
#endif
{
+ __glibcxx_requires_valid_insert_position(__position);
const size_type __n = __position - begin();
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
&& __position == end())
@@ -301,6 +302,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector<_Tp, _Alloc>::
emplace(const_iterator __position, _Args&&... __args)
{
+ __glibcxx_requires_valid_insert_position(__position);
const size_type __n = __position - begin();
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
&& __position == end())
diff --git libstdc++-v3/include/debug/debug.h libstdc++-v3/include/debug/debug.h
index b5935fe..8a3eb3b 100644
--- libstdc++-v3/include/debug/debug.h
+++ libstdc++-v3/include/debug/debug.h
@@ -74,33 +74,26 @@ namespace __gnu_debug
# define __glibcxx_requires_heap_pred(_First,_Last,_Pred)
# define __glibcxx_requires_string(_String)
# define __glibcxx_requires_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last)
# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)
# define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)
-#ifdef _GLIBCXX_ASSERTIONS
-// Verify that [_First, _Last) forms a non-empty iterator range.
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_assert(_First != _Last)
-// Verify that the container is nonempty
-# define __glibcxx_requires_nonempty() \
- __glibcxx_assert(! this->empty())
-#else
-# define __glibcxx_requires_non_empty_range(_First,_Last)
-# define __glibcxx_requires_nonempty()
#endif
+#ifndef _GLIBCXX_ASSERTIONS
+# define __glibcxx_requires_non_empty_range(_First,_Last)
+# define __glibcxx_requires_valid_insert_position(_Position)
+# define __glibcxx_requires_valid_erase_position(_Position)
+# define __glibcxx_requires_nonempty()
+# define __glibcxx_requires_subscript(_N)
#else
# include <debug/macros.h>
-# define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
+# ifdef _GLIBCXX_DEBUG
# define __glibcxx_requires_valid_range(_First,_Last) \
__glibcxx_check_valid_range(_First,_Last)
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_check_non_empty_range(_First,_Last)
# define __glibcxx_requires_sorted(_First,_Last) \
__glibcxx_check_sorted(_First,_Last)
# define __glibcxx_requires_sorted_pred(_First,_Last,_Pred) \
@@ -121,11 +114,9 @@ namespace __gnu_debug
__glibcxx_check_heap(_First,_Last)
# define __glibcxx_requires_heap_pred(_First,_Last,_Pred) \
__glibcxx_check_heap_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_nonempty() __glibcxx_check_nonempty()
# define __glibcxx_requires_string(_String) __glibcxx_check_string(_String)
# define __glibcxx_requires_string_len(_String,_Len) \
__glibcxx_check_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last) \
__glibcxx_check_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last) \
@@ -136,6 +127,19 @@ namespace __gnu_debug
__glibcxx_check_irreflexive_pred2(_First,_Last,_Pred)
# include <debug/functions.h>
+#endif
+
+# define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
+# define __glibcxx_requires_valid_insert_position(_Position) \
+ __glibcxx_check_insert2(_Position)
+# define __glibcxx_requires_valid_erase_position(_Position) \
+ __glibcxx_check_erase2(_Position)
+# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
+# define __glibcxx_requires_non_empty_range(_First,_Last) \
+ __glibcxx_check_non_empty_range(_First,_Last)
+# define __glibcxx_requires_nonempty() __glibcxx_check_nonempty()
+
+# include <debug/formatter.h>
#endif
diff --git libstdc++-v3/include/debug/formatter.h libstdc++-v3/include/debug/formatter.h
index 6e56c8f..921d1bc 100644
--- libstdc++-v3/include/debug/formatter.h
+++ libstdc++-v3/include/debug/formatter.h
@@ -47,8 +47,22 @@ namespace __gnu_debug
{
using std::type_info;
+ // An arbitrary iterator pointer is not singular.
+ inline bool
+ __check_singular_aux(const void*) { return false; }
+
+ // We may have an iterator that derives from _Safe_iterator_base but isn't
+ // a _Safe_iterator.
template<typename _Iterator>
- bool __check_singular(const _Iterator&);
+ inline bool
+ __check_singular(const _Iterator& __x)
+ { return __check_singular_aux(std::__addressof(__x)); }
+
+ /** Non-NULL pointers are nonsingular. */
+ template<typename _Tp>
+ inline bool
+ __check_singular(const _Tp* __ptr)
+ { return __ptr == 0; }
class _Safe_sequence_base;
diff --git libstdc++-v3/include/debug/functions.h libstdc++-v3/include/debug/functions.h
index 218092a..3437b22 100644
--- libstdc++-v3/include/debug/functions.h
+++ libstdc++-v3/include/debug/functions.h
@@ -51,23 +51,6 @@ namespace __gnu_debug
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; }
-
- // We may have an iterator that derives from _Safe_iterator_base but isn't
- // a _Safe_iterator.
- template<typename _Iterator>
- inline bool
- __check_singular(const _Iterator& __x)
- { return __check_singular_aux(std::__addressof(__x)); }
-
- /** Non-NULL pointers are nonsingular. */
- template<typename _Tp>
- inline bool
- __check_singular(const _Tp* __ptr)
- { return __ptr == 0; }
-
/** Assume that some arbitrary iterator is dereferenceable, because we
can't prove that it isn't. */
template<typename _Iterator>
diff --git libstdc++-v3/include/debug/macros.h libstdc++-v3/include/debug/macros.h
index c636663..71a3744 100644
--- libstdc++-v3/include/debug/macros.h
+++ libstdc++-v3/include/debug/macros.h
@@ -86,6 +86,25 @@ _GLIBCXX_DEBUG_VERIFY(_Position._M_attached_to(this), \
._M_sequence(*this, "this") \
._M_iterator(_Position, #_Position))
+/** The same as previous macro but when _Position is not a debug iterator. */
+#if __cplusplus >= 201103L
+# define __glibcxx_check_insert2(_Position) \
+_GLIBCXX_DEBUG_VERIFY((cbegin() < _Position || !(_Position < cbegin())) \
+ && (_Position < cend() || !(cend() < _Position)), \
+ _M_message("invalid insert position outside container" \
+ " [begin, end) range") \
+ ._M_sequence(*this, "this") \
+ ._M_iterator(_Position, #_Position))
+#else
+# define __glibcxx_check_insert2(_Position) \
+_GLIBCXX_DEBUG_VERIFY((begin() < _Position || !(_Position < begin())) \
+ && (_Position < end() || !(end() < _Position)), \
+ _M_message("invalid insert position outside container" \
+ " [begin, end) range") \
+ ._M_sequence(*this, "this") \
+ ._M_iterator(_Position, #_Position))
+#endif
+
/** Verify that we can insert into *this after the iterator _Position.
* Insertion into a container after a specific position requires that
* the iterator be nonsingular, either dereferenceable or before-begin,
@@ -152,6 +171,24 @@ _GLIBCXX_DEBUG_VERIFY(_Position._M_attached_to(this), \
._M_sequence(*this, "this") \
._M_iterator(_Position, #_Position))
+/** Same as above but for normal (non-debug) containers. */
+#if __cplusplus >= 201103L
+# define __glibcxx_check_erase2(_Position) \
+_GLIBCXX_DEBUG_VERIFY((cbegin() < _Position || !(_Position < cbegin())) \
+ && _Position < cend(), \
+ _M_message(__gnu_debug::__msg_erase_bad) \
+ ._M_sequence(*this, "this") \
+ ._M_iterator(_Position, #_Position));
+#else
+# define __glibcxx_check_erase2(_Position) \
+_GLIBCXX_DEBUG_VERIFY((begin() < _Position || !(_Position < begin())) \
+ && _Position < end(), \
+ _M_message(__gnu_debug::__msg_erase_bad) \
+ ._M_sequence(*this, "this") \
+ ._M_iterator(_Position, #_Position));
+#endif
+
+
/** Verify that we can erase the element after the iterator
* _Position. We can erase the element if the _Position iterator is
* before a dereferenceable one and references this sequence.
diff --git libstdc++-v3/include/debug/vector libstdc++-v3/include/debug/vector
index fede4f0..7fccf28 100644
--- libstdc++-v3/include/debug/vector
+++ libstdc++-v3/include/debug/vector
@@ -406,49 +406,10 @@ namespace __debug
}
// element access:
- reference
- operator[](size_type __n) _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_subscript(__n);
- return _M_base()[__n];
- }
-
- const_reference
- operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_subscript(__n);
- return _M_base()[__n];
- }
-
+ using _Base::operator[];
using _Base::at;
-
- reference
- front() _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::front();
- }
-
- const_reference
- front() const _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::front();
- }
-
- reference
- back() _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::back();
- }
-
- const_reference
- back() const _GLIBCXX_NOEXCEPT
- {
- __glibcxx_check_nonempty();
- return _Base::back();
- }
+ using _Base::front;
+ using _Base::back;
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
diff --git libstdc++-v3/testsuite/23_containers/vector/debug/insert8_neg.cc libstdc++-v3/testsuite/23_containers/vector/debug/insert8_neg.cc
new file mode 100644
index 0000000..5d915c2
--- /dev/null
+++ libstdc++-v3/testsuite/23_containers/vector/debug/insert8_neg.cc
@@ -0,0 +1,41 @@
+// -*- C++ -*-
+
+// 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 "-D_GLIBCXX_ASSERTIONS" }
+// { dg-do run { xfail *-*-* } }
+
+#include <iterator>
+
+#include <vector>
+
+void
+test01()
+{
+ std::vector<int> v1, v2;
+
+ v1.push_back(0);
+
+ v1.insert(v1.begin() + 2, v2.begin(), v2.end());
+}
+
+int
+main()
+{
+ test01();
+}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-10-07 19:38 ` François Dumont
@ 2015-10-07 20:09 ` Jonathan Wakely
2015-10-12 19:43 ` François Dumont
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Wakely @ 2015-10-07 20:09 UTC (permalink / raw)
To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On 07/10/15 21:38 +0200, François Dumont wrote:
>Hi
>
> I completed vector assertion mode. Here is the result of the new
>test you will find in the attached patch.
>
>With debug mode:
>/home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:375:
>Error: attempt to advance a dereferenceable (start-of-sequence) iterator 2
>steps, which falls outside its valid range.
>
>Objects involved in the operation:
> iterator @ 0x0x7fff1c346760 {
> type =
>__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*,
>std::__cxx1998::vector<int, std::allocator<int> > >,
>std::__debug::vector<int, std::allocator<int> > > (mutable iterator);
> state = dereferenceable (start-of-sequence);
> references sequence with type 'std::__debug::vector<int,
>std::allocator<int> >' @ 0x0x7fff1c3469a0
> }
>XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
>
>
>With assertion mode:
>/home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/bits/stl_vector.h:1124:
>Error: invalid insert position outside container [begin, end) range.
>
>Objects involved in the operation:
> sequence "this" @ 0x0x7fff60b1f870 {
> type = std::vector<int, std::allocator<int> >;
> }
> iterator "__position" @ 0x0x7fff60b1f860 {
> type = __gnu_cxx::__normal_iterator<int const*, std::vector<int,
>std::allocator<int> > >;
> }
>XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
I still don't like the formatted output for the lightweight mode, it
adds a dependency on I/O support in libc, which is a problem for
embedded systems.
The idea was to just add really cheap checks and abort :-(
Have you compared codegen with and without assertion mode? How much
more code is added to member functions like operator[] that must be
inlined for good performance? Is it likely to affect inlining
decisions?
I suspect it will have a much bigger impact than if we just use
__builtin_abort() as I made it do originally.
If these checks become more complex then people are not going to
enable them by default, and we probably won't want to make them
enabled by _FORTIFY_SOURCE.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-10-07 20:09 ` Jonathan Wakely
@ 2015-10-12 19:43 ` François Dumont
2015-11-15 21:12 ` François Dumont
0 siblings, 1 reply; 18+ messages in thread
From: François Dumont @ 2015-10-12 19:43 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3442 bytes --]
On 07/10/2015 22:09, Jonathan Wakely wrote:
> On 07/10/15 21:38 +0200, François Dumont wrote:
>> Hi
>>
>> I completed vector assertion mode. Here is the result of the new
>> test you will find in the attached patch.
>>
>> With debug mode:
>> /home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:375:
>>
>> Error: attempt to advance a dereferenceable (start-of-sequence)
>> iterator 2
>> steps, which falls outside its valid range.
>>
>> Objects involved in the operation:
>> iterator @ 0x0x7fff1c346760 {
>> type =
>> __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*,
>> std::__cxx1998::vector<int, std::allocator<int> > >,
>> std::__debug::vector<int, std::allocator<int> > > (mutable iterator);
>> state = dereferenceable (start-of-sequence);
>> references sequence with type 'std::__debug::vector<int,
>> std::allocator<int> >' @ 0x0x7fff1c3469a0
>> }
>> XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
>>
>>
>> With assertion mode:
>> /home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/bits/stl_vector.h:1124:
>>
>> Error: invalid insert position outside container [begin, end) range.
>>
>> Objects involved in the operation:
>> sequence "this" @ 0x0x7fff60b1f870 {
>> type = std::vector<int, std::allocator<int> >;
>> }
>> iterator "__position" @ 0x0x7fff60b1f860 {
>> type = __gnu_cxx::__normal_iterator<int const*, std::vector<int,
>> std::allocator<int> > >;
>> }
>> XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
>
> I still don't like the formatted output for the lightweight mode, it
> adds a dependency on I/O support in libc, which is a problem for
> embedded systems.
I thought you just meant I/O dependency in terms of included headers.
The __glibcxx_assert also has some I/O as in case of failure it calls:
inline void
__replacement_assert(const char* __file, int __line,
const char* __function, const char* __condition)
{
__builtin_printf("%s:%d: %s: Assertion '%s' failed.\n", __file, __line,
__function, __condition);
__builtin_abort();
}
but it is much more limited than the _GLIBCXX_DEBUG_VERIFY counterpart
which is calling fprintf to send to stderr.
So ok let's limit this mode to glibcxx_assert.
>
> The idea was to just add really cheap checks and abort :-(
>
> Have you compared codegen with and without assertion mode? How much
> more code is added to member functions like operator[] that must be
> inlined for good performance? Is it likely to affect inlining
> decisions?
>
> I suspect it will have a much bigger impact than if we just use
> __builtin_abort() as I made it do originally.
I think that impact on compiled code depends more on the assert
condition than on the code executed when this assertion happens to be
false. But I haven't check it and will try.
In the attached patch I eventually:
- Move assertion macros in debug/assertions.h, it sounds like the right
place for those.
- Complete implementation of assertion checks by using __valid_range
function. All checks I can think of are now in place. I still need to
compare with google branch.
Note that for the latter, condition is still evaluated in O(1).
__valid_range detects iterator issues without looping through them.
__valid_range, by considering iterator category, also make those macros
usable in any container.
François
[-- Attachment #2: vector_debug.patch --]
[-- Type: text/x-patch, Size: 16996 bytes --]
diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 305d446..04bc339 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -63,6 +63,8 @@
#include <initializer_list>
#endif
+#include <debug/assertions.h>
+
namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_CONTAINER
@@ -403,13 +405,18 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector(_InputIterator __first, _InputIterator __last,
const allocator_type& __a = allocator_type())
: _Base(__a)
- { _M_initialize_dispatch(__first, __last, __false_type()); }
+ {
+ __glibcxx_requires_valid_range(__first, __last);
+ _M_initialize_dispatch(__first, __last, __false_type());
+ }
#else
template<typename _InputIterator>
vector(_InputIterator __first, _InputIterator __last,
const allocator_type& __a = allocator_type())
: _Base(__a)
{
+ __glibcxx_requires_valid_range(__first, __last);
+
// Check whether it's an integral type. If so, it's not an iterator.
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
_M_initialize_dispatch(__first, __last, _Integral());
@@ -470,7 +477,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector&
operator=(initializer_list<value_type> __l)
{
- this->assign(__l.begin(), __l.end());
+ this->_M_assign_aux(__l.begin(), __l.end(),
+ random_access_iterator_tag());
return *this;
}
#endif
@@ -506,12 +514,17 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
typename = std::_RequireInputIter<_InputIterator>>
void
assign(_InputIterator __first, _InputIterator __last)
- { _M_assign_dispatch(__first, __last, __false_type()); }
+ {
+ __glibcxx_requires_valid_range(__first, __last);
+ _M_assign_dispatch(__first, __last, __false_type());
+ }
#else
template<typename _InputIterator>
void
assign(_InputIterator __first, _InputIterator __last)
{
+ __glibcxx_requires_valid_range(__first, __last);
+
// Check whether it's an integral type. If so, it's not an iterator.
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
_M_assign_dispatch(__first, __last, _Integral());
@@ -532,7 +545,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
void
assign(initializer_list<value_type> __l)
- { this->assign(__l.begin(), __l.end()); }
+ {
+ this->_M_assign_aux(__l.begin(), __l.end(),
+ random_access_iterator_tag());
+ }
#endif
/// Get a copy of the memory allocation object.
@@ -778,7 +794,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
operator[](size_type __n) _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
/**
* @brief Subscript access to the data contained in the %vector.
@@ -793,7 +812,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
protected:
/// Safety check used only from at().
@@ -850,7 +872,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
front() _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read-only (constant) reference to the data at the first
@@ -858,7 +883,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
front() const _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read/write reference to the data at the last
@@ -866,7 +894,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
back() _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
/**
* Returns a read-only (constant) reference to the data at the
@@ -874,7 +905,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
back() const _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
@@ -949,6 +983,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
pop_back() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
--this->_M_impl._M_finish;
_Alloc_traits::destroy(this->_M_impl, this->_M_impl._M_finish);
}
@@ -1051,6 +1086,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
insert(const_iterator __position, size_type __n, const value_type& __x)
{
+ __glibcxx_requires_valid_insert_position(__position);
difference_type __offset = __position - cbegin();
_M_fill_insert(begin() + __offset, __n, __x);
return begin() + __offset;
@@ -1071,7 +1107,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
void
insert(iterator __position, size_type __n, const value_type& __x)
- { _M_fill_insert(__position, __n, __x); }
+ {
+ __glibcxx_requires_valid_insert_position(__position);
+ _M_fill_insert(__position, __n, __x);
+ }
#endif
#if __cplusplus >= 201103L
@@ -1096,6 +1135,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(const_iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_requires_valid_insert_position(__position);
difference_type __offset = __position - cbegin();
_M_insert_dispatch(begin() + __offset,
__first, __last, __false_type());
@@ -1121,6 +1161,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_requires_valid_insert_position(__position);
// Check whether it's an integral type. If so, it's not an iterator.
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
_M_insert_dispatch(__position, __first, __last, _Integral());
@@ -1145,10 +1186,16 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
#if __cplusplus >= 201103L
erase(const_iterator __position)
- { return _M_erase(begin() + (__position - cbegin())); }
+ {
+ __glibcxx_requires_valid_erase_position(__position);
+ return _M_erase(begin() + (__position - cbegin()));
+ }
#else
erase(iterator __position)
- { return _M_erase(__position); }
+ {
+ __glibcxx_requires_valid_erase_position(__position);
+ return _M_erase(__position);
+ }
#endif
/**
@@ -1173,13 +1220,17 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
#if __cplusplus >= 201103L
erase(const_iterator __first, const_iterator __last)
{
+ __glibcxx_requires_valid_range(__first, __last);
const auto __beg = begin();
const auto __cbeg = cbegin();
return _M_erase(__beg + (__first - __cbeg), __beg + (__last - __cbeg));
}
#else
erase(iterator __first, iterator __last)
- { return _M_erase(__first, __last); }
+ {
+ __glibcxx_requires_valid_range(__first, __last);
+ return _M_erase(__first, __last);
+ }
#endif
/**
@@ -1194,6 +1245,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
swap(vector& __x) _GLIBCXX_NOEXCEPT
{
+#if __cplusplus >= 201103L
+ __glibcxx_assert(_Alloc_traits::propagate_on_container_swap::value
+ || _M_get_Tp_allocator() == __x._M_get_Tp_allocator());
+#endif
this->_M_impl._M_swap_data(__x._M_impl);
_Alloc_traits::_S_on_swap(_M_get_Tp_allocator(),
__x._M_get_Tp_allocator());
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index 34118a4..965dab3 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -111,6 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(iterator __position, const value_type& __x)
#endif
{
+ __glibcxx_requires_valid_insert_position(__position);
const size_type __n = __position - begin();
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
&& __position == end())
@@ -301,6 +302,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector<_Tp, _Alloc>::
emplace(const_iterator __position, _Args&&... __args)
{
+ __glibcxx_requires_valid_insert_position(__position);
const size_type __n = __position - begin();
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
&& __position == end())
diff --git a/libstdc++-v3/include/debug/assertions.h b/libstdc++-v3/include/debug/assertions.h
index 9b9a48c..5e29676 100644
--- a/libstdc++-v3/include/debug/assertions.h
+++ b/libstdc++-v3/include/debug/assertions.h
@@ -33,20 +33,48 @@
# define _GLIBCXX_DEBUG_ASSERT(_Condition)
# define _GLIBCXX_DEBUG_PEDASSERT(_Condition)
-# define _GLIBCXX_DEBUG_ONLY(_Statement) ;
+# define _GLIBCXX_DEBUG_ONLY(_Statement)
+#endif
+
+#ifndef _GLIBCXX_ASSERTIONS
+# define __glibcxx_requires_non_empty_range(_First,_Last)
+# define __glibcxx_requires_valid_range(_First,_Last)
+# define __glibcxx_requires_valid_insert_position(_Position)
+# define __glibcxx_requires_valid_erase_position(_Position)
+# define __glibcxx_requires_nonempty()
+# define __glibcxx_requires_subscript(_N)
#else
-#define _GLIBCXX_DEBUG_ASSERT(_Condition) __glibcxx_assert(_Condition)
+# include <debug/macros.h>
+
+# define __glibcxx_requires_valid_insert_position(_Position) \
+ __glibcxx_check_insert2(_Position)
+# define __glibcxx_requires_valid_erase_position(_Position) \
+ __glibcxx_check_erase2(_Position)
+# define __glibcxx_requires_subscript(_N) \
+ __glibcxx_assert(_N < this->size())
+# define __glibcxx_requires_non_empty_range(_First,_Last) \
+ __glibcxx_check_non_empty_range(_First,_Last)
+# define __glibcxx_requires_valid_range(_First,_Last) \
+ __glibcxx_assert(__gnu_debug::__valid_range(_First, _Last))
+# define __glibcxx_requires_nonempty() \
+ __glibcxx_assert(!this->empty())
+
+# include <debug/helper_functions.h>
-#ifdef _GLIBCXX_DEBUG_PEDANTIC
-# define _GLIBCXX_DEBUG_PEDASSERT(_Condition) _GLIBCXX_DEBUG_ASSERT(_Condition)
-#else
-# define _GLIBCXX_DEBUG_PEDASSERT(_Condition)
#endif
-# define _GLIBCXX_DEBUG_ONLY(_Statement) _Statement
+#ifdef _GLIBCXX_DEBUG
+# define _GLIBCXX_DEBUG_ASSERT(_Condition) __glibcxx_assert(_Condition)
+# ifdef _GLIBCXX_DEBUG_PEDANTIC
+# define _GLIBCXX_DEBUG_PEDASSERT(_Condition) _GLIBCXX_DEBUG_ASSERT(_Condition)
+# else
+# define _GLIBCXX_DEBUG_PEDASSERT(_Condition)
+# endif
+
+# define _GLIBCXX_DEBUG_ONLY(_Statement) _Statement
#endif
#endif // _GLIBCXX_DEBUG_ASSERTIONS
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index b5935fe..f233bc1 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -74,33 +74,16 @@ namespace __gnu_debug
# define __glibcxx_requires_heap_pred(_First,_Last,_Pred)
# define __glibcxx_requires_string(_String)
# define __glibcxx_requires_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last)
# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)
# define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)
-#ifdef _GLIBCXX_ASSERTIONS
-// Verify that [_First, _Last) forms a non-empty iterator range.
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_assert(_First != _Last)
-// Verify that the container is nonempty
-# define __glibcxx_requires_nonempty() \
- __glibcxx_assert(! this->empty())
#else
-# define __glibcxx_requires_non_empty_range(_First,_Last)
-# define __glibcxx_requires_nonempty()
-#endif
-
-#else
-
-# include <debug/macros.h>
# define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
# define __glibcxx_requires_valid_range(_First,_Last) \
__glibcxx_check_valid_range(_First,_Last)
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_check_non_empty_range(_First,_Last)
# define __glibcxx_requires_sorted(_First,_Last) \
__glibcxx_check_sorted(_First,_Last)
# define __glibcxx_requires_sorted_pred(_First,_Last,_Pred) \
@@ -121,11 +104,9 @@ namespace __gnu_debug
__glibcxx_check_heap(_First,_Last)
# define __glibcxx_requires_heap_pred(_First,_Last,_Pred) \
__glibcxx_check_heap_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_nonempty() __glibcxx_check_nonempty()
# define __glibcxx_requires_string(_String) __glibcxx_check_string(_String)
# define __glibcxx_requires_string_len(_String,_Len) \
__glibcxx_check_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last) \
__glibcxx_check_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last) \
diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h
index a2db00d..b628b06 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h
@@ -138,6 +138,7 @@ namespace __gnu_debug
return __dist.first >= 0;
}
+ // Can't tell so assume it is fine.
return true;
}
diff --git a/libstdc++-v3/include/debug/macros.h b/libstdc++-v3/include/debug/macros.h
index c636663..f1ad40f 100644
--- a/libstdc++-v3/include/debug/macros.h
+++ b/libstdc++-v3/include/debug/macros.h
@@ -86,6 +86,17 @@ _GLIBCXX_DEBUG_VERIFY(_Position._M_attached_to(this), \
._M_sequence(*this, "this") \
._M_iterator(_Position, #_Position))
+/** The same as previous macro but when _Position is not a debug iterator. */
+#if __cplusplus >= 201103L
+# define __glibcxx_check_insert2(_Position) \
+ __glibcxx_assert(__gnu_debug::__valid_range(this->cbegin(), _Position) \
+ && __gnu_debug::__valid_range(_Position, this->cend()))
+#else
+# define __glibcxx_check_insert2(_Position) \
+ __glibcxx_assert(__gnu_debug::__valid_range(this->begin(), _Position) \
+ && __gnu_debug::__valid_range(_Position, this->end()))
+#endif
+
/** Verify that we can insert into *this after the iterator _Position.
* Insertion into a container after a specific position requires that
* the iterator be nonsingular, either dereferenceable or before-begin,
@@ -152,6 +163,19 @@ _GLIBCXX_DEBUG_VERIFY(_Position._M_attached_to(this), \
._M_sequence(*this, "this") \
._M_iterator(_Position, #_Position))
+/** Same as above but for normal (non-debug) containers. */
+#if __cplusplus >= 201103L
+# define __glibcxx_check_erase2(_Position) \
+ __glibcxx_assert(__gnu_debug::__valid_range(this->cbegin(), _Position) && \
+ __gnu_debug::__valid_range(_Position, this->cend()) && \
+ _Position != this->cend())
+#else
+# define __glibcxx_check_erase2(_Position) \
+ __glibcxx_assert(__gnu_debug::__valid_range(this->begin(), _Position) && \
+ __gnu_debug::__valid_range(_Position, this->end()) && \
+ _Position != this->end())
+#endif
+
/** Verify that we can erase the element after the iterator
* _Position. We can erase the element if the _Position iterator is
* before a dereferenceable one and references this sequence.
diff --git a/libstdc++-v3/testsuite/23_containers/vector/debug/insert8_neg.cc b/libstdc++-v3/testsuite/23_containers/vector/debug/insert8_neg.cc
new file mode 100644
index 0000000..5d915c2
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/debug/insert8_neg.cc
@@ -0,0 +1,41 @@
+// -*- C++ -*-
+
+// 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 "-D_GLIBCXX_ASSERTIONS" }
+// { dg-do run { xfail *-*-* } }
+
+#include <iterator>
+
+#include <vector>
+
+void
+test01()
+{
+ std::vector<int> v1, v2;
+
+ v1.push_back(0);
+
+ v1.insert(v1.begin() + 2, v2.begin(), v2.end());
+}
+
+int
+main()
+{
+ test01();
+}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-10-12 19:43 ` François Dumont
@ 2015-11-15 21:12 ` François Dumont
2015-11-16 10:29 ` Jonathan Wakely
0 siblings, 1 reply; 18+ messages in thread
From: François Dumont @ 2015-11-15 21:12 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 4512 bytes --]
On 12/10/2015 21:42, François Dumont wrote:
> On 07/10/2015 22:09, Jonathan Wakely wrote:
>> On 07/10/15 21:38 +0200, François Dumont wrote:
>>> Hi
>>>
>>> I completed vector assertion mode. Here is the result of the new
>>> test you will find in the attached patch.
>>>
>>> With debug mode:
>>> /home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:375:
>>>
>>> Error: attempt to advance a dereferenceable (start-of-sequence)
>>> iterator 2
>>> steps, which falls outside its valid range.
>>>
>>> Objects involved in the operation:
>>> iterator @ 0x0x7fff1c346760 {
>>> type =
>>> __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*,
>>> std::__cxx1998::vector<int, std::allocator<int> > >,
>>> std::__debug::vector<int, std::allocator<int> > > (mutable iterator);
>>> state = dereferenceable (start-of-sequence);
>>> references sequence with type 'std::__debug::vector<int,
>>> std::allocator<int> >' @ 0x0x7fff1c3469a0
>>> }
>>> XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
>>>
>>>
>>> With assertion mode:
>>> /home/fdt/dev/gcc/build_git/x86_64-unknown-linux-gnu/libstdc++-v3/include/bits/stl_vector.h:1124:
>>>
>>> Error: invalid insert position outside container [begin, end) range.
>>>
>>> Objects involved in the operation:
>>> sequence "this" @ 0x0x7fff60b1f870 {
>>> type = std::vector<int, std::allocator<int> >;
>>> }
>>> iterator "__position" @ 0x0x7fff60b1f860 {
>>> type = __gnu_cxx::__normal_iterator<int const*, std::vector<int,
>>> std::allocator<int> > >;
>>> }
>>> XFAIL: 23_containers/vector/debug/insert8_neg.cc execution test
>> I still don't like the formatted output for the lightweight mode, it
>> adds a dependency on I/O support in libc, which is a problem for
>> embedded systems.
> I thought you just meant I/O dependency in terms of included headers.
> The __glibcxx_assert also has some I/O as in case of failure it calls:
>
> inline void
> __replacement_assert(const char* __file, int __line,
> const char* __function, const char* __condition)
> {
> __builtin_printf("%s:%d: %s: Assertion '%s' failed.\n", __file, __line,
> __function, __condition);
> __builtin_abort();
> }
>
> but it is much more limited than the _GLIBCXX_DEBUG_VERIFY counterpart
> which is calling fprintf to send to stderr.
>
> So ok let's limit this mode to glibcxx_assert.
>
>> The idea was to just add really cheap checks and abort :-(
>>
>> Have you compared codegen with and without assertion mode? How much
>> more code is added to member functions like operator[] that must be
>> inlined for good performance? Is it likely to affect inlining
>> decisions?
>>
>> I suspect it will have a much bigger impact than if we just use
>> __builtin_abort() as I made it do originally.
> I think that impact on compiled code depends more on the assert
> condition than on the code executed when this assertion happens to be
> false. But I haven't check it and will try.
>
> In the attached patch I eventually:
> - Move assertion macros in debug/assertions.h, it sounds like the right
> place for those.
> - Complete implementation of assertion checks by using __valid_range
> function. All checks I can think of are now in place. I still need to
> compare with google branch.
>
> Note that for the latter, condition is still evaluated in O(1).
> __valid_range detects iterator issues without looping through them.
> __valid_range, by considering iterator category, also make those macros
> usable in any container.
>
> François
>
Here is a last version I think.
I completed the debug light mode by adding some check on iterator
ranges.
Even if check are light I made some changes to make sure that
internally vector is not using methods instrumented with those checks.
This is to make sure checks are not done several times. Doing so also
simplify normal mode especially when using insert range, there is no
need to check if parameters are integers or not.
I also introduce some __builtin_expect to make sure compiler will
prefer the best path.
I didn't manage to check result on generated code. I am pretty sure
there will be an impact, you can't run more code without impact. But
that is a known drawback of debug mode, light or not, we just need to
minimize it. Mostly by making sure that checks are done only once.
It would be great to have it for gcc 6.0. I am working on the same
for other containers.
François
[-- Attachment #2: vector_debug.patch --]
[-- Type: text/x-patch, Size: 22001 bytes --]
diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 305d446..66b813d 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -63,6 +63,8 @@
#include <initializer_list>
#endif
+#include <debug/assertions.h>
+
namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_CONTAINER
@@ -320,7 +322,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector(const vector& __x)
: _Base(__x.size(),
_Alloc_traits::_S_select_on_copy(__x._M_get_Tp_allocator()))
- { this->_M_impl._M_finish =
+ {
+ this->_M_impl._M_finish =
std::__uninitialized_copy_a(__x.begin(), __x.end(),
this->_M_impl._M_start,
_M_get_Tp_allocator());
@@ -340,7 +343,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
/// Copy constructor with alternative allocator
vector(const vector& __x, const allocator_type& __a)
: _Base(__x.size(), __a)
- { this->_M_impl._M_finish =
+ {
+ this->_M_impl._M_finish =
std::__uninitialized_copy_a(__x.begin(), __x.end(),
this->_M_impl._M_start,
_M_get_Tp_allocator());
@@ -403,13 +407,18 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector(_InputIterator __first, _InputIterator __last,
const allocator_type& __a = allocator_type())
: _Base(__a)
- { _M_initialize_dispatch(__first, __last, __false_type()); }
+ {
+ __glibcxx_requires_valid_range(__first, __last);
+ _M_initialize_dispatch(__first, __last, __false_type());
+ }
#else
template<typename _InputIterator>
vector(_InputIterator __first, _InputIterator __last,
const allocator_type& __a = allocator_type())
: _Base(__a)
{
+ __glibcxx_requires_valid_range(__first, __last);
+
// Check whether it's an integral type. If so, it's not an iterator.
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
_M_initialize_dispatch(__first, __last, _Integral());
@@ -470,7 +479,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector&
operator=(initializer_list<value_type> __l)
{
- this->assign(__l.begin(), __l.end());
+ this->_M_assign_aux(__l.begin(), __l.end(),
+ random_access_iterator_tag());
return *this;
}
#endif
@@ -506,12 +516,17 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
typename = std::_RequireInputIter<_InputIterator>>
void
assign(_InputIterator __first, _InputIterator __last)
- { _M_assign_dispatch(__first, __last, __false_type()); }
+ {
+ __glibcxx_requires_valid_range(__first, __last);
+ _M_assign_dispatch(__first, __last, __false_type());
+ }
#else
template<typename _InputIterator>
void
assign(_InputIterator __first, _InputIterator __last)
{
+ __glibcxx_requires_valid_range(__first, __last);
+
// Check whether it's an integral type. If so, it's not an iterator.
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
_M_assign_dispatch(__first, __last, _Integral());
@@ -532,7 +547,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
void
assign(initializer_list<value_type> __l)
- { this->assign(__l.begin(), __l.end()); }
+ {
+ this->_M_assign_aux(__l.begin(), __l.end(),
+ random_access_iterator_tag());
+ }
#endif
/// Get a copy of the memory allocation object.
@@ -694,7 +712,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
resize(size_type __new_size, const value_type& __x)
{
if (__new_size > size())
- insert(end(), __new_size - size(), __x);
+ _M_fill_insert(end(), __new_size - size(), __x);
else if (__new_size < size())
_M_erase_at_end(this->_M_impl._M_start + __new_size);
}
@@ -714,7 +732,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
resize(size_type __new_size, value_type __x = value_type())
{
if (__new_size > size())
- insert(end(), __new_size - size(), __x);
+ _M_fill_insert(end(), __new_size - size(), __x);
else if (__new_size < size())
_M_erase_at_end(this->_M_impl._M_start + __new_size);
}
@@ -778,7 +796,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
operator[](size_type __n) _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
/**
* @brief Subscript access to the data contained in the %vector.
@@ -793,7 +814,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
protected:
/// Safety check used only from at().
@@ -850,7 +874,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
front() _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read-only (constant) reference to the data at the first
@@ -858,7 +885,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
front() const _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read/write reference to the data at the last
@@ -866,7 +896,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
back() _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
/**
* Returns a read-only (constant) reference to the data at the
@@ -874,7 +907,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
back() const _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
@@ -949,6 +985,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
pop_back() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
--this->_M_impl._M_finish;
_Alloc_traits::destroy(this->_M_impl, this->_M_impl._M_finish);
}
@@ -968,7 +1005,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
template<typename... _Args>
iterator
- emplace(const_iterator __position, _Args&&... __args);
+ emplace(const_iterator __position, _Args&&... __args)
+ {
+ __glibcxx_requires_valid_insert_position(__position);
+ return _M_emplace(__position, std::forward<_Args>(__args)...);
+ }
/**
* @brief Inserts given value into %vector before specified iterator.
@@ -982,7 +1023,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
* used the user should consider using std::list.
*/
iterator
- insert(const_iterator __position, const value_type& __x);
+ insert(const_iterator __position, const value_type& __x)
#else
/**
* @brief Inserts given value into %vector before specified iterator.
@@ -996,8 +1037,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
* used the user should consider using std::list.
*/
iterator
- insert(iterator __position, const value_type& __x);
+ insert(iterator __position, const value_type& __x)
#endif
+ {
+ __glibcxx_requires_valid_insert_position(__position);
+ return _M_insert(__position, __x);
+ }
#if __cplusplus >= 201103L
/**
@@ -1013,7 +1058,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
iterator
insert(const_iterator __position, value_type&& __x)
- { return emplace(__position, std::move(__x)); }
+ {
+ __glibcxx_requires_valid_insert_position(__position);
+ return _M_emplace(__position, std::move(__x));
+ }
/**
* @brief Inserts an initializer_list into the %vector.
@@ -1030,7 +1078,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
iterator
insert(const_iterator __position, initializer_list<value_type> __l)
- { return this->insert(__position, __l.begin(), __l.end()); }
+ {
+ __glibcxx_requires_valid_insert_position(__position);
+ auto __offset = __position - cbegin();
+ _M_range_insert(begin() + __offset, __l.begin(), __l.end(),
+ std::random_access_iterator_tag());
+ return begin() + __offset;
+ }
#endif
#if __cplusplus >= 201103L
@@ -1051,6 +1105,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
insert(const_iterator __position, size_type __n, const value_type& __x)
{
+ __glibcxx_requires_valid_insert_position(__position);
difference_type __offset = __position - cbegin();
_M_fill_insert(begin() + __offset, __n, __x);
return begin() + __offset;
@@ -1071,7 +1126,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
void
insert(iterator __position, size_type __n, const value_type& __x)
- { _M_fill_insert(__position, __n, __x); }
+ {
+ __glibcxx_requires_valid_insert_position(__position);
+ _M_fill_insert(__position, __n, __x);
+ }
#endif
#if __cplusplus >= 201103L
@@ -1096,6 +1154,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(const_iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_requires_valid_insert_position(__position);
+ __glibcxx_requires_valid_range(__first, __last);
+
difference_type __offset = __position - cbegin();
_M_insert_dispatch(begin() + __offset,
__first, __last, __false_type());
@@ -1121,6 +1182,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
insert(iterator __position, _InputIterator __first,
_InputIterator __last)
{
+ __glibcxx_requires_valid_insert_position(__position);
+
// Check whether it's an integral type. If so, it's not an iterator.
typedef typename std::__is_integer<_InputIterator>::__type _Integral;
_M_insert_dispatch(__position, __first, __last, _Integral());
@@ -1145,10 +1208,16 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
iterator
#if __cplusplus >= 201103L
erase(const_iterator __position)
- { return _M_erase(begin() + (__position - cbegin())); }
+ {
+ __glibcxx_requires_valid_erase_position(__position);
+ return _M_erase(begin() + (__position - cbegin()));
+ }
#else
erase(iterator __position)
- { return _M_erase(__position); }
+ {
+ __glibcxx_requires_valid_erase_position(__position);
+ return _M_erase(__position);
+ }
#endif
/**
@@ -1173,13 +1242,17 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
#if __cplusplus >= 201103L
erase(const_iterator __first, const_iterator __last)
{
+ __glibcxx_requires_valid_range(__first, __last);
const auto __beg = begin();
const auto __cbeg = cbegin();
return _M_erase(__beg + (__first - __cbeg), __beg + (__last - __cbeg));
}
#else
erase(iterator __first, iterator __last)
- { return _M_erase(__first, __last); }
+ {
+ __glibcxx_requires_valid_range(__first, __last);
+ return _M_erase(__first, __last);
+ }
#endif
/**
@@ -1194,6 +1267,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
swap(vector& __x) _GLIBCXX_NOEXCEPT
{
+#if __cplusplus >= 201103L
+ __glibcxx_assert(_Alloc_traits::propagate_on_container_swap::value
+ || _M_get_Tp_allocator() == __x._M_get_Tp_allocator());
+#endif
this->_M_impl._M_swap_data(__x._M_impl);
_Alloc_traits::_S_on_swap(_M_get_Tp_allocator(),
__x._M_get_Tp_allocator());
@@ -1353,6 +1430,21 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
// Internal insert functions follow.
+#if __cplusplus >= 201103L
+ template<typename... _Args>
+ iterator
+ _M_emplace(const_iterator __position, _Args&&... __args);
+
+ iterator
+ _M_insert(const_iterator __position, value_type&& __x)
+ { return _M_emplace(__position, std::move(__x)); }
+
+ iterator
+ _M_insert(const_iterator __position, const value_type& __x);
+#else
+ iterator
+ _M_insert(iterator __position, const value_type& __x);
+#endif
// Called by the range insert to implement [23.1.1]/9
@@ -1370,9 +1462,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_M_insert_dispatch(iterator __pos, _InputIterator __first,
_InputIterator __last, __false_type)
{
- typedef typename std::iterator_traits<_InputIterator>::
- iterator_category _IterCategory;
- _M_range_insert(__pos, __first, __last, _IterCategory());
+ _M_range_insert(__pos, __first, __last,
+ std::__iterator_category(__first));
}
// Called by the second insert_dispatch above
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index 34118a4..39a5133 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -106,9 +106,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
typename vector<_Tp, _Alloc>::iterator
vector<_Tp, _Alloc>::
#if __cplusplus >= 201103L
- insert(const_iterator __position, const value_type& __x)
+ _M_insert(const_iterator __position, const value_type& __x)
#else
- insert(iterator __position, const value_type& __x)
+ _M_insert(iterator __position, const value_type& __x)
#endif
{
const size_type __n = __position - begin();
@@ -256,7 +256,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
if (__first == __last)
_M_erase_at_end(__cur);
else
- insert(end(), __first, __last);
+ _M_range_insert(end(), __first, __last,
+ std::__iterator_category(__first));
}
template<typename _Tp, typename _Alloc>
@@ -299,7 +300,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
template<typename... _Args>
typename vector<_Tp, _Alloc>::iterator
vector<_Tp, _Alloc>::
- emplace(const_iterator __position, _Args&&... __args)
+ _M_emplace(const_iterator __position, _Args&&... __args)
{
const size_type __n = __position - begin();
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
@@ -605,7 +606,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
{
for (; __first != __last; ++__first)
{
- __pos = insert(__pos, *__first);
+ __pos = _M_insert(__pos, *__first);
++__pos;
}
}
diff --git a/libstdc++-v3/include/debug/assertions.h b/libstdc++-v3/include/debug/assertions.h
index 9b9a48c..f40dc61 100644
--- a/libstdc++-v3/include/debug/assertions.h
+++ b/libstdc++-v3/include/debug/assertions.h
@@ -33,10 +33,39 @@
# define _GLIBCXX_DEBUG_ASSERT(_Condition)
# define _GLIBCXX_DEBUG_PEDASSERT(_Condition)
-# define _GLIBCXX_DEBUG_ONLY(_Statement) ;
+# define _GLIBCXX_DEBUG_ONLY(_Statement)
+#endif
+
+#ifndef _GLIBCXX_ASSERTIONS
+# define __glibcxx_requires_non_empty_range(_First,_Last)
+# define __glibcxx_requires_valid_range(_First,_Last)
+# define __glibcxx_requires_valid_insert_position(_Position)
+# define __glibcxx_requires_valid_erase_position(_Position)
+# define __glibcxx_requires_nonempty()
+# define __glibcxx_requires_subscript(_N)
#else
+# include <debug/macros.h>
+
+# define __glibcxx_requires_valid_insert_position(_Position) \
+ __glibcxx_check_insert2(_Position)
+# define __glibcxx_requires_valid_erase_position(_Position) \
+ __glibcxx_check_erase2(_Position)
+# define __glibcxx_requires_subscript(_N) \
+ __glibcxx_assert(__builtin_expect(_N < this->size(), true))
+# define __glibcxx_requires_non_empty_range(_First,_Last) \
+ __glibcxx_check_non_empty_range(_First,_Last)
+# define __glibcxx_requires_valid_range(_First,_Last) \
+ __glibcxx_assert(__gnu_debug::__valid_range(_First, _Last))
+# define __glibcxx_requires_nonempty() \
+ __glibcxx_assert(__builtin_expect(!this->empty(), true))
+
+# include <debug/helper_functions.h>
+
+#endif
+
+#ifdef _GLIBCXX_DEBUG
# define _GLIBCXX_DEBUG_ASSERT(_Condition) __glibcxx_assert(_Condition)
# ifdef _GLIBCXX_DEBUG_PEDANTIC
@@ -46,7 +75,6 @@
# endif
# define _GLIBCXX_DEBUG_ONLY(_Statement) _Statement
-
#endif
#endif // _GLIBCXX_DEBUG_ASSERTIONS
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index b5935fe..f233bc1 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -74,33 +74,16 @@ namespace __gnu_debug
# define __glibcxx_requires_heap_pred(_First,_Last,_Pred)
# define __glibcxx_requires_string(_String)
# define __glibcxx_requires_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last)
# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)
# define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)
-#ifdef _GLIBCXX_ASSERTIONS
-// Verify that [_First, _Last) forms a non-empty iterator range.
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_assert(_First != _Last)
-// Verify that the container is nonempty
-# define __glibcxx_requires_nonempty() \
- __glibcxx_assert(! this->empty())
#else
-# define __glibcxx_requires_non_empty_range(_First,_Last)
-# define __glibcxx_requires_nonempty()
-#endif
-
-#else
-
-# include <debug/macros.h>
# define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
# define __glibcxx_requires_valid_range(_First,_Last) \
__glibcxx_check_valid_range(_First,_Last)
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_check_non_empty_range(_First,_Last)
# define __glibcxx_requires_sorted(_First,_Last) \
__glibcxx_check_sorted(_First,_Last)
# define __glibcxx_requires_sorted_pred(_First,_Last,_Pred) \
@@ -121,11 +104,9 @@ namespace __gnu_debug
__glibcxx_check_heap(_First,_Last)
# define __glibcxx_requires_heap_pred(_First,_Last,_Pred) \
__glibcxx_check_heap_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_nonempty() __glibcxx_check_nonempty()
# define __glibcxx_requires_string(_String) __glibcxx_check_string(_String)
# define __glibcxx_requires_string_len(_String,_Len) \
__glibcxx_check_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last) \
__glibcxx_check_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last) \
diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h
index a2db00d..b628b06 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h
@@ -138,6 +138,7 @@ namespace __gnu_debug
return __dist.first >= 0;
}
+ // Can't tell so assume it is fine.
return true;
}
diff --git a/libstdc++-v3/include/debug/macros.h b/libstdc++-v3/include/debug/macros.h
index c636663..f1ad40f 100644
--- a/libstdc++-v3/include/debug/macros.h
+++ b/libstdc++-v3/include/debug/macros.h
@@ -86,6 +86,17 @@ _GLIBCXX_DEBUG_VERIFY(_Position._M_attached_to(this), \
._M_sequence(*this, "this") \
._M_iterator(_Position, #_Position))
+/** The same as previous macro but when _Position is not a debug iterator. */
+#if __cplusplus >= 201103L
+# define __glibcxx_check_insert2(_Position) \
+ __glibcxx_assert(__gnu_debug::__valid_range(this->cbegin(), _Position) \
+ && __gnu_debug::__valid_range(_Position, this->cend()))
+#else
+# define __glibcxx_check_insert2(_Position) \
+ __glibcxx_assert(__gnu_debug::__valid_range(this->begin(), _Position) \
+ && __gnu_debug::__valid_range(_Position, this->end()))
+#endif
+
/** Verify that we can insert into *this after the iterator _Position.
* Insertion into a container after a specific position requires that
* the iterator be nonsingular, either dereferenceable or before-begin,
@@ -152,6 +163,19 @@ _GLIBCXX_DEBUG_VERIFY(_Position._M_attached_to(this), \
._M_sequence(*this, "this") \
._M_iterator(_Position, #_Position))
+/** Same as above but for normal (non-debug) containers. */
+#if __cplusplus >= 201103L
+# define __glibcxx_check_erase2(_Position) \
+ __glibcxx_assert(__gnu_debug::__valid_range(this->cbegin(), _Position) && \
+ __gnu_debug::__valid_range(_Position, this->cend()) && \
+ _Position != this->cend())
+#else
+# define __glibcxx_check_erase2(_Position) \
+ __glibcxx_assert(__gnu_debug::__valid_range(this->begin(), _Position) && \
+ __gnu_debug::__valid_range(_Position, this->end()) && \
+ _Position != this->end())
+#endif
+
/** Verify that we can erase the element after the iterator
* _Position. We can erase the element if the _Position iterator is
* before a dereferenceable one and references this sequence.
diff --git a/libstdc++-v3/testsuite/23_containers/vector/debug/insert8_neg.cc b/libstdc++-v3/testsuite/23_containers/vector/debug/insert8_neg.cc
new file mode 100644
index 0000000..5d915c2
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/debug/insert8_neg.cc
@@ -0,0 +1,41 @@
+// -*- C++ -*-
+
+// 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 "-D_GLIBCXX_ASSERTIONS" }
+// { dg-do run { xfail *-*-* } }
+
+#include <iterator>
+
+#include <vector>
+
+void
+test01()
+{
+ std::vector<int> v1, v2;
+
+ v1.push_back(0);
+
+ v1.insert(v1.begin() + 2, v2.begin(), v2.end());
+}
+
+int
+main()
+{
+ test01();
+}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-11-15 21:12 ` François Dumont
@ 2015-11-16 10:29 ` Jonathan Wakely
2015-11-17 19:49 ` François Dumont
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Wakely @ 2015-11-16 10:29 UTC (permalink / raw)
To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On 15/11/15 22:12 +0100, François Dumont wrote:
>Here is a last version I think.
>
> I completed the debug light mode by adding some check on iterator
>ranges.
>
> Even if check are light I made some changes to make sure that
>internally vector is not using methods instrumented with those checks.
>This is to make sure checks are not done several times. Doing so also
>simplify normal mode especially when using insert range, there is no
>need to check if parameters are integers or not.
Yes, I'd also observed that those improvements could be made, to avoid
dispatching when we already know we have iterators not integers.
> I also introduce some __builtin_expect to make sure compiler will
>prefer the best path.
>
> I didn't manage to check result on generated code. I am pretty sure
>there will be an impact, you can't run more code without impact. But
>that is a known drawback of debug mode, light or not, we just need to
>minimize it. Mostly by making sure that checks are done only once.
Not doing the checks is also an option. That minimizes the cost :-)
For the full debug mode we want to check everything we can, and accept
that has a cost.
For the lightweight one we need to evaluate the relative benefits. Is
it worth adding checks for errors that only happen rarely? Does the
benefit outweigh the cost?
I'm still not convinced that's the case for the "valid range" checks.
I'm willing to be convinced, but am not convinced yet.
> It would be great to have it for gcc 6.0. I am working on the same
>for other containers.
Please don't do the valid range checks for std::deque, the checks are
undefined for iterators into different containers and will not give a
reliable answer.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-11-16 10:29 ` Jonathan Wakely
@ 2015-11-17 19:49 ` François Dumont
2015-11-18 12:27 ` Jonathan Wakely
0 siblings, 1 reply; 18+ messages in thread
From: François Dumont @ 2015-11-17 19:49 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On 16/11/2015 11:29, Jonathan Wakely wrote:
> On 15/11/15 22:12 +0100, François Dumont wrote:
>> Here is a last version I think.
>>
>> I completed the debug light mode by adding some check on iterator
>> ranges.
>>
>> Even if check are light I made some changes to make sure that
>> internally vector is not using methods instrumented with those checks.
>> This is to make sure checks are not done several times. Doing so also
>> simplify normal mode especially when using insert range, there is no
>> need to check if parameters are integers or not.
>
> Yes, I'd also observed that those improvements could be made, to avoid
> dispatching when we already know we have iterators not integers.
I will keep those simplification even if I remove some checks.
>
>> I also introduce some __builtin_expect to make sure compiler will
>> prefer the best path.
>>
>> I didn't manage to check result on generated code. I am pretty sure
>> there will be an impact, you can't run more code without impact. But
>> that is a known drawback of debug mode, light or not, we just need to
>> minimize it. Mostly by making sure that checks are done only once.
>
> Not doing the checks is also an option. That minimizes the cost :-)
This is controlled by a macro, users already have this option.
>
> For the full debug mode we want to check everything we can, and accept
> that has a cost.
>
> For the lightweight one we need to evaluate the relative benefits. Is
> it worth adding checks for errors that only happen rarely? Does the
> benefit outweigh the cost?
>
> I'm still not convinced that's the case for the "valid range" checks.
> I'm willing to be convinced, but am not convinced yet.
Ok so I will remove this check. And what about insert position check ? I
guess this one too so I will remove it too. Note that will only remain
checks on the most basic operations that is to say those on which the
check will have the biggest impact proportionally.
I would like we push the simplest version so that people can start
experimenting.
I would also prefer concentrate on _GLIBCXX_DEBUG mode :-)
>
>> It would be great to have it for gcc 6.0. I am working on the same
>> for other containers.
>
> Please don't do the valid range checks for std::deque, the checks are
> undefined for iterators into different containers and will not give a
> reliable answer.
But debug mode is full of those checks, no ?
François
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-11-17 19:49 ` François Dumont
@ 2015-11-18 12:27 ` Jonathan Wakely
2015-11-18 12:34 ` Jonathan Wakely
2015-11-19 21:17 ` François Dumont
0 siblings, 2 replies; 18+ messages in thread
From: Jonathan Wakely @ 2015-11-18 12:27 UTC (permalink / raw)
To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On 17/11/15 20:49 +0100, François Dumont wrote:
>On 16/11/2015 11:29, Jonathan Wakely wrote:
>> Not doing the checks is also an option. That minimizes the cost :-)
>
>This is controlled by a macro, users already have this option.
True, but we're talking about maybe enabling these checks by default
when building linux distributions.
>>
>> For the full debug mode we want to check everything we can, and accept
>> that has a cost.
>>
>> For the lightweight one we need to evaluate the relative benefits. Is
>> it worth adding checks for errors that only happen rarely? Does the
>> benefit outweigh the cost?
>>
>> I'm still not convinced that's the case for the "valid range" checks.
>> I'm willing to be convinced, but am not convinced yet.
>
>Ok so I will remove this check. And what about insert position check ? I
>guess this one too so I will remove it too. Note that will only remain
>checks on the most basic operations that is to say those on which the
>check will have the biggest impact proportionally.
Yes, that's a good point.
But my unproven assumption is that it's more common to use operator[]
incorrectly, rather than pass invalid iterators to range insert, which
is a relatively "advanced" operation.
>I would like we push the simplest version so that people can start
>experimenting.
>
>I would also prefer concentrate on _GLIBCXX_DEBUG mode :-)
>
>>
>>> It would be great to have it for gcc 6.0. I am working on the same
>>> for other containers.
>>
>> Please don't do the valid range checks for std::deque, the checks are
>> undefined for iterators into different containers and will not give a
>> reliable answer.
>
>But debug mode is full of those checks, no ?
They're supposed to be guarded by checks for _M_can_compare, if they
aren't that's a regression. For debug mode we can tell whether two
iterators are comparable, because they store a pointer back to their
parent container. We can't check that in normal mode.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-11-18 12:27 ` Jonathan Wakely
@ 2015-11-18 12:34 ` Jonathan Wakely
2015-11-19 21:17 ` François Dumont
1 sibling, 0 replies; 18+ messages in thread
From: Jonathan Wakely @ 2015-11-18 12:34 UTC (permalink / raw)
To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On 18/11/15 12:27 +0000, Jonathan Wakely wrote:
>But my unproven assumption is that it's more common to use operator[]
>incorrectly, rather than pass invalid iterators to range insert, which
>is a relatively "advanced" operation.
It's worth noting that the google/integration branch doesn't have any
checks on range insert/erase, and they have entire teams who work on
analysing their code base to find common programming errors, and
identify ways to avoid problems.
That doesn't mean we shouldn't include any checks that aren't in the
google branch, and we also shouldn't use all their checks, but it's
useful to look at what they considered important to check.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: vector lightweight debug mode
2015-11-18 12:27 ` Jonathan Wakely
2015-11-18 12:34 ` Jonathan Wakely
@ 2015-11-19 21:17 ` François Dumont
1 sibling, 0 replies; 18+ messages in thread
From: François Dumont @ 2015-11-19 21:17 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2217 bytes --]
Thanks for the explanation, it is much clearer.
So here is a the more limited patch.
Tested under Linux x86_64.
Ok to commit ?
François
On 18/11/2015 13:27, Jonathan Wakely wrote:
> On 17/11/15 20:49 +0100, François Dumont wrote:
>> On 16/11/2015 11:29, Jonathan Wakely wrote:
>>> Not doing the checks is also an option. That minimizes the cost :-)
>>
>> This is controlled by a macro, users already have this option.
>
> True, but we're talking about maybe enabling these checks by default
> when building linux distributions.
>
>>>
>>> For the full debug mode we want to check everything we can, and accept
>>> that has a cost.
>>>
>>> For the lightweight one we need to evaluate the relative benefits. Is
>>> it worth adding checks for errors that only happen rarely? Does the
>>> benefit outweigh the cost?
>>>
>>> I'm still not convinced that's the case for the "valid range" checks.
>>> I'm willing to be convinced, but am not convinced yet.
>>
>> Ok so I will remove this check. And what about insert position check ? I
>> guess this one too so I will remove it too. Note that will only remain
>> checks on the most basic operations that is to say those on which the
>> check will have the biggest impact proportionally.
>
> Yes, that's a good point.
>
> But my unproven assumption is that it's more common to use operator[]
> incorrectly, rather than pass invalid iterators to range insert, which
> is a relatively "advanced" operation.
>
>
>> I would like we push the simplest version so that people can start
>> experimenting.
>>
>> I would also prefer concentrate on _GLIBCXX_DEBUG mode :-)
>>
>>>
>>>> It would be great to have it for gcc 6.0. I am working on the same
>>>> for other containers.
>>>
>>> Please don't do the valid range checks for std::deque, the checks are
>>> undefined for iterators into different containers and will not give a
>>> reliable answer.
>>
>> But debug mode is full of those checks, no ?
>
> They're supposed to be guarded by checks for _M_can_compare, if they
> aren't that's a regression. For debug mode we can tell whether two
> iterators are comparable, because they store a pointer back to their
> parent container. We can't check that in normal mode.
>
>
[-- Attachment #2: vector_debug.patch --]
[-- Type: text/x-patch, Size: 11341 bytes --]
diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 305d446..3b17fb4 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -63,6 +63,8 @@
#include <initializer_list>
#endif
+#include <debug/assertions.h>
+
namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_CONTAINER
@@ -320,7 +322,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector(const vector& __x)
: _Base(__x.size(),
_Alloc_traits::_S_select_on_copy(__x._M_get_Tp_allocator()))
- { this->_M_impl._M_finish =
+ {
+ this->_M_impl._M_finish =
std::__uninitialized_copy_a(__x.begin(), __x.end(),
this->_M_impl._M_start,
_M_get_Tp_allocator());
@@ -340,7 +343,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
/// Copy constructor with alternative allocator
vector(const vector& __x, const allocator_type& __a)
: _Base(__x.size(), __a)
- { this->_M_impl._M_finish =
+ {
+ this->_M_impl._M_finish =
std::__uninitialized_copy_a(__x.begin(), __x.end(),
this->_M_impl._M_start,
_M_get_Tp_allocator());
@@ -470,7 +474,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector&
operator=(initializer_list<value_type> __l)
{
- this->assign(__l.begin(), __l.end());
+ this->_M_assign_aux(__l.begin(), __l.end(),
+ random_access_iterator_tag());
return *this;
}
#endif
@@ -532,7 +537,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
void
assign(initializer_list<value_type> __l)
- { this->assign(__l.begin(), __l.end()); }
+ {
+ this->_M_assign_aux(__l.begin(), __l.end(),
+ random_access_iterator_tag());
+ }
#endif
/// Get a copy of the memory allocation object.
@@ -694,7 +702,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
resize(size_type __new_size, const value_type& __x)
{
if (__new_size > size())
- insert(end(), __new_size - size(), __x);
+ _M_fill_insert(end(), __new_size - size(), __x);
else if (__new_size < size())
_M_erase_at_end(this->_M_impl._M_start + __new_size);
}
@@ -714,7 +722,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
resize(size_type __new_size, value_type __x = value_type())
{
if (__new_size > size())
- insert(end(), __new_size - size(), __x);
+ _M_fill_insert(end(), __new_size - size(), __x);
else if (__new_size < size())
_M_erase_at_end(this->_M_impl._M_start + __new_size);
}
@@ -778,7 +786,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
operator[](size_type __n) _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
/**
* @brief Subscript access to the data contained in the %vector.
@@ -793,7 +804,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
operator[](size_type __n) const _GLIBCXX_NOEXCEPT
- { return *(this->_M_impl._M_start + __n); }
+ {
+ __glibcxx_requires_subscript(__n);
+ return *(this->_M_impl._M_start + __n);
+ }
protected:
/// Safety check used only from at().
@@ -850,7 +864,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
front() _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read-only (constant) reference to the data at the first
@@ -858,7 +875,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
front() const _GLIBCXX_NOEXCEPT
- { return *begin(); }
+ {
+ __glibcxx_requires_nonempty();
+ return *begin();
+ }
/**
* Returns a read/write reference to the data at the last
@@ -866,7 +886,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
reference
back() _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
/**
* Returns a read-only (constant) reference to the data at the
@@ -874,7 +897,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
const_reference
back() const _GLIBCXX_NOEXCEPT
- { return *(end() - 1); }
+ {
+ __glibcxx_requires_nonempty();
+ return *(end() - 1);
+ }
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 464. Suggestion for new member functions in standard containers.
@@ -949,6 +975,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
pop_back() _GLIBCXX_NOEXCEPT
{
+ __glibcxx_requires_nonempty();
--this->_M_impl._M_finish;
_Alloc_traits::destroy(this->_M_impl, this->_M_impl._M_finish);
}
@@ -1030,7 +1057,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
iterator
insert(const_iterator __position, initializer_list<value_type> __l)
- { return this->insert(__position, __l.begin(), __l.end()); }
+ {
+ auto __offset = __position - cbegin();
+ _M_range_insert(begin() + __offset, __l.begin(), __l.end(),
+ std::random_access_iterator_tag());
+ return begin() + __offset;
+ }
#endif
#if __cplusplus >= 201103L
@@ -1194,6 +1226,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
swap(vector& __x) _GLIBCXX_NOEXCEPT
{
+#if __cplusplus >= 201103L
+ __glibcxx_assert(_Alloc_traits::propagate_on_container_swap::value
+ || _M_get_Tp_allocator() == __x._M_get_Tp_allocator());
+#endif
this->_M_impl._M_swap_data(__x._M_impl);
_Alloc_traits::_S_on_swap(_M_get_Tp_allocator(),
__x._M_get_Tp_allocator());
@@ -1328,11 +1364,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
_M_assign_dispatch(_InputIterator __first, _InputIterator __last,
__false_type)
- {
- typedef typename std::iterator_traits<_InputIterator>::
- iterator_category _IterCategory;
- _M_assign_aux(__first, __last, _IterCategory());
- }
+ { _M_assign_aux(__first, __last, std::__iterator_category(__first)); }
// Called by the second assign_dispatch above
template<typename _InputIterator>
@@ -1351,7 +1383,6 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
_M_fill_assign(size_type __n, const value_type& __val);
-
// Internal insert functions follow.
// Called by the range insert to implement [23.1.1]/9
@@ -1370,9 +1401,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_M_insert_dispatch(iterator __pos, _InputIterator __first,
_InputIterator __last, __false_type)
{
- typedef typename std::iterator_traits<_InputIterator>::
- iterator_category _IterCategory;
- _M_range_insert(__pos, __first, __last, _IterCategory());
+ _M_range_insert(__pos, __first, __last,
+ std::__iterator_category(__first));
}
// Called by the second insert_dispatch above
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index 34118a4..db2ca71 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -256,7 +256,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
if (__first == __last)
_M_erase_at_end(__cur);
else
- insert(end(), __first, __last);
+ _M_range_insert(end(), __first, __last,
+ std::__iterator_category(__first));
}
template<typename _Tp, typename _Alloc>
diff --git a/libstdc++-v3/include/debug/assertions.h b/libstdc++-v3/include/debug/assertions.h
index 9b9a48c..6e5b58f 100644
--- a/libstdc++-v3/include/debug/assertions.h
+++ b/libstdc++-v3/include/debug/assertions.h
@@ -33,10 +33,27 @@
# define _GLIBCXX_DEBUG_ASSERT(_Condition)
# define _GLIBCXX_DEBUG_PEDASSERT(_Condition)
-# define _GLIBCXX_DEBUG_ONLY(_Statement) ;
+# define _GLIBCXX_DEBUG_ONLY(_Statement)
+#endif
+
+#ifndef _GLIBCXX_ASSERTIONS
+# define __glibcxx_requires_non_empty_range(_First,_Last)
+# define __glibcxx_requires_nonempty()
+# define __glibcxx_requires_subscript(_N)
#else
+// Verify that [_First, _Last) forms a non-empty iterator range.
+# define __glibcxx_requires_non_empty_range(_First,_Last) \
+ __glibcxx_assert(__builtin_expect(_First != _Last, true))
+# define __glibcxx_requires_subscript(_N) \
+ __glibcxx_assert(__builtin_expect(_N < this->size(), true))
+// Verify that the container is nonempty
+# define __glibcxx_requires_nonempty() \
+ __glibcxx_assert(__builtin_expect(!this->empty(), true))
+#endif
+
+#ifdef _GLIBCXX_DEBUG
# define _GLIBCXX_DEBUG_ASSERT(_Condition) __glibcxx_assert(_Condition)
# ifdef _GLIBCXX_DEBUG_PEDANTIC
@@ -46,7 +63,6 @@
# endif
# define _GLIBCXX_DEBUG_ONLY(_Statement) _Statement
-
#endif
#endif // _GLIBCXX_DEBUG_ASSERTIONS
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index b5935fe..b7e325e 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -74,24 +74,11 @@ namespace __gnu_debug
# define __glibcxx_requires_heap_pred(_First,_Last,_Pred)
# define __glibcxx_requires_string(_String)
# define __glibcxx_requires_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last)
# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)
# define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)
-#ifdef _GLIBCXX_ASSERTIONS
-// Verify that [_First, _Last) forms a non-empty iterator range.
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_assert(_First != _Last)
-// Verify that the container is nonempty
-# define __glibcxx_requires_nonempty() \
- __glibcxx_assert(! this->empty())
-#else
-# define __glibcxx_requires_non_empty_range(_First,_Last)
-# define __glibcxx_requires_nonempty()
-#endif
-
#else
#include <debug/macros.h>
@@ -99,8 +86,6 @@ namespace __gnu_debug
# define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
# define __glibcxx_requires_valid_range(_First,_Last) \
__glibcxx_check_valid_range(_First,_Last)
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
- __glibcxx_check_non_empty_range(_First,_Last)
# define __glibcxx_requires_sorted(_First,_Last) \
__glibcxx_check_sorted(_First,_Last)
# define __glibcxx_requires_sorted_pred(_First,_Last,_Pred) \
@@ -121,11 +106,9 @@ namespace __gnu_debug
__glibcxx_check_heap(_First,_Last)
# define __glibcxx_requires_heap_pred(_First,_Last,_Pred) \
__glibcxx_check_heap_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_nonempty() __glibcxx_check_nonempty()
# define __glibcxx_requires_string(_String) __glibcxx_check_string(_String)
# define __glibcxx_requires_string_len(_String,_Len) \
__glibcxx_check_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last) \
__glibcxx_check_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last) \
diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h
index a2db00d..b628b06 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h
@@ -138,6 +138,7 @@ namespace __gnu_debug
return __dist.first >= 0;
}
+ // Can't tell so assume it is fine.
return true;
}
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-11-19 21:17 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-14 18:30 vector lightweight debug mode François Dumont
2015-09-14 19:55 ` Jonathan Wakely
2015-09-16 19:43 ` François Dumont
2015-09-16 20:53 ` Jonathan Wakely
[not found] ` <CA+jCFLueUi0Zo4m2D-scXNG5JLVSObKbJgXWaQRJrQ+notbXzg@mail.gmail.com>
2015-09-17 21:14 ` Fwd: " Christopher Jefferson
2015-09-19 9:12 ` Jonathan Wakely
2015-09-19 9:09 ` François Dumont
2015-09-19 9:48 ` Jonathan Wakely
2015-09-19 12:00 ` Jonathan Wakely
2015-10-07 19:38 ` François Dumont
2015-10-07 20:09 ` Jonathan Wakely
2015-10-12 19:43 ` François Dumont
2015-11-15 21:12 ` François Dumont
2015-11-16 10:29 ` Jonathan Wakely
2015-11-17 19:49 ` François Dumont
2015-11-18 12:27 ` Jonathan Wakely
2015-11-18 12:34 ` Jonathan Wakely
2015-11-19 21:17 ` François Dumont
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).