From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18026 invoked by alias); 16 Sep 2015 19:37:34 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 17999 invoked by uid 89); 16 Sep 2015 19:37:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-wi0-f172.google.com Received: from mail-wi0-f172.google.com (HELO mail-wi0-f172.google.com) (209.85.212.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 16 Sep 2015 19:37:31 +0000 Received: by wiclk2 with SMTP id lk2so85293041wic.1; Wed, 16 Sep 2015 12:37:28 -0700 (PDT) X-Received: by 10.180.96.166 with SMTP id dt6mr22502553wib.38.1442432248655; Wed, 16 Sep 2015 12:37:28 -0700 (PDT) Received: from [192.168.0.23] (arf62-1-82-237-250-248.fbx.proxad.net. [82.237.250.248]) by smtp.googlemail.com with ESMTPSA id p3sm6039642wib.16.2015.09.16.12.37.27 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 16 Sep 2015 12:37:27 -0700 (PDT) From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= Subject: Re: vector lightweight debug mode To: Jonathan Wakely References: <55F71189.8080006@gmail.com> <20150914195038.GQ2631@redhat.com> Cc: "libstdc++@gcc.gnu.org" , gcc-patches X-Enigmail-Draft-Status: N1110 Message-ID: <55F9C4F6.6030706@gmail.com> Date: Wed, 16 Sep 2015 19:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150914195038.GQ2631@redhat.com> Content-Type: multipart/mixed; boundary="------------070202010102030002090804" X-SW-Source: 2015-09/txt/msg01236.txt.bz2 This is a multi-part message in MIME format. --------------070202010102030002090804 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Content-length: 3754 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. > > --------------070202010102030002090804 Content-Type: text/x-patch; name="vector_debug.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="vector_debug.patch" Content-length: 14389 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 #endif +#include + 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 -# 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 +# include +#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 #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. --------------070202010102030002090804--