From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 70890 invoked by alias); 19 Nov 2015 21:17:39 -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 70790 invoked by uid 89); 19 Nov 2015 21:17:38 -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-wm0-f47.google.com Received: from mail-wm0-f47.google.com (HELO mail-wm0-f47.google.com) (74.125.82.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 19 Nov 2015 21:17:27 +0000 Received: by wmvv187 with SMTP id v187so45401895wmv.1; Thu, 19 Nov 2015 13:17:24 -0800 (PST) X-Received: by 10.28.212.18 with SMTP id l18mr19728411wmg.93.1447967844293; Thu, 19 Nov 2015 13:17:24 -0800 (PST) Received: from [192.168.0.23] ([82.237.250.248]) by smtp.googlemail.com with ESMTPSA id gl4sm9657369wjd.17.2015.11.19.13.17.22 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 19 Nov 2015 13:17:22 -0800 (PST) Subject: Re: vector lightweight debug mode To: Jonathan Wakely References: <20150916202953.GE2631@redhat.com> <55FD0F35.4010106@gmail.com> <561574BE.1060005@gmail.com> <20151007200917.GU12094@redhat.com> <561C0D3A.7010504@gmail.com> <5648F551.5030404@gmail.com> <20151116102923.GI2937@redhat.com> <564B84C3.3090900@gmail.com> <20151118122744.GC25025@redhat.com> Cc: "libstdc++@gcc.gnu.org" , gcc-patches From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= X-Enigmail-Draft-Status: N1110 Message-ID: <564E3C61.7060207@gmail.com> Date: Thu, 19 Nov 2015 21:17:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151118122744.GC25025@redhat.com> Content-Type: multipart/mixed; boundary="------------070005030307090503070609" X-SW-Source: 2015-11/txt/msg02421.txt.bz2 This is a multi-part message in MIME format. --------------070005030307090503070609 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Content-length: 2215 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. > > --------------070005030307090503070609 Content-Type: text/x-patch; name="vector_debug.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="vector_debug.patch" Content-length: 11341 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 #endif +#include + 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 __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 __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 __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 @@ -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 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 @@ -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; } --------------070005030307090503070609--