From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26505 invoked by alias); 7 Oct 2015 19:38:49 -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 26466 invoked by uid 89); 7 Oct 2015 19:38:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_20,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-wi0-f180.google.com Received: from mail-wi0-f180.google.com (HELO mail-wi0-f180.google.com) (209.85.212.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 07 Oct 2015 19:38:44 +0000 Received: by wicfx3 with SMTP id fx3so43344647wic.0; Wed, 07 Oct 2015 12:38:41 -0700 (PDT) X-Received: by 10.194.184.136 with SMTP id eu8mr3464288wjc.151.1444246721240; Wed, 07 Oct 2015 12:38:41 -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 h8sm3904541wib.21.2015.10.07.12.38.39 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 07 Oct 2015 12:38:40 -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> <55F9C4F6.6030706@gmail.com> <20150916202953.GE2631@redhat.com> <55FD0F35.4010106@gmail.com> Cc: Jonathan Wakely , "libstdc++@gcc.gnu.org" , gcc-patches X-Enigmail-Draft-Status: N1110 Message-ID: <561574BE.1060005@gmail.com> Date: Wed, 07 Oct 2015 19:38: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: Content-Type: multipart/mixed; boundary="------------090104060502030008000307" X-SW-Source: 2015-10/txt/msg00748.txt.bz2 This is a multi-part message in MIME format. --------------090104060502030008000307 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-length: 3824 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 > >, std::__debug::vector > > (mutable iterator); state = dereferenceable (start-of-sequence); references sequence with type 'std::__debug::vector >' @ 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 >; } iterator "__position" @ 0x0x7fff60b1f860 { type = __gnu_cxx::__normal_iterator > >; } 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 and so: #include // for iterator_traits, // categories and _Iter_base #include // for __is_integer #include // 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 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. --------------090104060502030008000307 Content-Type: text/x-patch; name="vector_debug.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="vector_debug.patch" Content-length: 17537 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 #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,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 -# 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 +#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 #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 - 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 + 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 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 - inline bool - __check_singular(const _Iterator& __x) - { return __check_singular_aux(std::__addressof(__x)); } - - /** Non-NULL pointers are nonsingular. */ - template - 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 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 +// . + +// { dg-options "-D_GLIBCXX_ASSERTIONS" } +// { dg-do run { xfail *-*-* } } + +#include + +#include + +void +test01() +{ + std::vector v1, v2; + + v1.push_back(0); + + v1.insert(v1.begin() + 2, v2.begin(), v2.end()); +} + +int +main() +{ + test01(); +} --------------090104060502030008000307--