* 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
[parent not found: <CA+jCFLueUi0Zo4m2D-scXNG5JLVSObKbJgXWaQRJrQ+notbXzg@mail.gmail.com>]
* 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-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-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-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).