From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 0CBC53943540 for ; Thu, 8 Apr 2021 13:08:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0CBC53943540 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-303-noazH3qDNpWhI1XiwAse6g-1; Thu, 08 Apr 2021 09:08:11 -0400 X-MC-Unique: noazH3qDNpWhI1XiwAse6g-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8ABEA10082F6; Thu, 8 Apr 2021 13:07:50 +0000 (UTC) Received: from localhost (unknown [10.33.36.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id DD9CF60864; Thu, 8 Apr 2021 13:07:49 +0000 (UTC) Date: Thu, 8 Apr 2021 14:07:48 +0100 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: [Bug libstdc++/99402] [10/11 Regression] std::copy creates _GLIBCXX_DEBUG false positive for attempt to subscript a dereferenceable (start-of-sequence) iterator Message-ID: <20210408130748.GF3008@redhat.com> References: <5a5ae1ac-f3e3-70a4-c37d-80dd7baf6d05@gmail.com> MIME-Version: 1.0 In-Reply-To: <5a5ae1ac-f3e3-70a4-c37d-80dd7baf6d05@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Apr 2021 13:08:15 -0000 On 11/03/21 18:51 +0100, François Dumont via Libstdc++ wrote: >I eventually prefer to propose this version. > >Compared to the previous one I have the _M_can_advance calling the >former one with correct number of elements to check for advance. And >the former _M_can_advance is also properly making use of the >__dp_sign_max_size precision. > >Here is the revisited git log: > >    libstdc++: [_GLIBCXX_DEBUG] Fix management of __dp_sign_max_size >[PR 99402] > >    __dp_sign precision indicates that we found out what iterator >comes first or >    last in the range. __dp_sign_max_size is the same plus it gives >the information >    of the max size of the range that is to say the max_size value >such that >    distance(lhs, rhs) < max_size. >    Thanks to this additional information we are able to tell when a >copy of n elements >    to that range will fail even if we do not know exactly how large it is. > >    This patch makes sure that we are properly using this information. > >    libstdc++-v3/ChangeLog: > >            PR libstdc++/99402 >            * include/debug/helper_functions.h >(__can_advance(_InputIterator, >            const std::pair<_Diff, _Distance_precision>&, int)): New. >            (__can_advance(const _Safe_iterator<>&, >            const std::pair<_Diff, _Distance_precision>&, int)): New. >            * include/debug/macros.h >(__glibcxx_check_can_increment_dist): New, >            use latter. >            (__glibcxx_check_can_increment_range): Adapt to use latter. >            (__glibcxx_check_can_decrement_range): Likewise. >            * include/debug/safe_iterator.h >            (_Safe_iterator<>::_M_can_advance(const std::pair<_Diff, >_Distance_precision>&, >            int)): New. >            (__can_advance(const _Safe_iterator<>&, >            const std::pair<_Diff, _Distance_precision>&, int)): New. >            * include/debug/safe_iterator.tcc >            (_Safe_iterator<>::_M_can_advance(const std::pair<_Diff, >_Distance_precision>&, >            int)): New. >            (_Safe_iterator<>::_M_valid_range(const _Safe_iterator<>&, >            std::pair&, bool)): >Adapt for >            __dp_sign_max_size. >            (__copy_move_a): Adapt to use >__glibcxx_check_can_increment_dist. >            (__copy_move_backward_a): Likewise. >            (__equal_aux): Likewise. >            * include/debug/stl_iterator.h (__can_advance(const >std::reverse_iterator<>&, >            const std::pair<_Diff, _Distance_precision>&, int)): New. >            (__can_advance(const std::move_iterator<>&, >            const std::pair<_Diff, _Distance_precision>&, int)): New. >            * testsuite/25_algorithms/copy/debug/99402.cc: New test. > >Ok to commit if tests are all PASS ? > >François > >On 07/03/21 10:30 pm, François Dumont wrote: >>Here is the patch to correctly deal with the new __dp_sign_max_size. >> >>I prefer to introduce new __can_advance overloads for this to >>correctly deal with the _Distance_precision in it. _M_valid_range >>was also ignoring __dp_sign_max_size. >> >>    libstdc++: [_GLIBCXX_DEBUG] Fix management of __dp_sign_max_size >>[PR 99402] >> >>    libstdc++-v3/ChangeLog: >> >>            PR libstdc++/99402 >>            * include/debug/helper_functions.h >>(__can_advance(_InputIterator, >>            const std::pair<_Diff, _Distance_precision>&, int)): New. >>            (__can_advance(const _Safe_iterator<>&, >>            const std::pair<_Diff, _Distance_precision>&, int)): New. >>            * include/debug/macros.h >>(__glibcxx_check_can_increment_dist): New, >>            use latter. >>            (__glibcxx_check_can_increment_range): Adapt to use latter. >>            (__glibcxx_check_can_decrement_range): Likewise. >>            * include/debug/safe_iterator.h >>            (_Safe_iterator<>::_M_can_advance(const std::pair<_Diff, >>_Distance_precision>&, >>            int)): New. >>            (__can_advance(const _Safe_iterator<>&, >>            const std::pair<_Diff, _Distance_precision>&, int)): New. >>            * include/debug/safe_iterator.tcc >>            (_Safe_iterator<>::_M_can_advance(const std::pair<_Diff, >>_Distance_precision>&, >>            int)): New. >>            (_Safe_iterator<>::_M_valid_range(const _Safe_iterator<>&, >>            std::pair&, >>bool)): Adapt for >>            __dp_sign_max_size. >>            (__copy_move_a): Adapt to use >>__glibcxx_check_can_increment_dist. >>            (__copy_move_backward_a): Likewise. >>            (__equal_aux): Likewise. >>            * include/debug/stl_iterator.h (__can_advance(const >>std::reverse_iterator<>&, >>            const std::pair<_Diff, _Distance_precision>&, int)): New. >>            (__can_advance(const std::move_iterator<>&, >>            const std::pair<_Diff, _Distance_precision>&, int)): New. >>            * testsuite/25_algorithms/copy/debug/99402.cc: New test. >> >>Tested under Linux x86_64. >> >>Ok to commit ? >> >>François >> > >diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h >index 513e5460eba..c0144ced979 100644 >--- a/libstdc++-v3/include/debug/helper_functions.h >+++ b/libstdc++-v3/include/debug/helper_functions.h >@@ -291,6 +291,18 @@ namespace __gnu_debug > __can_advance(const _Safe_iterator<_Iterator, _Sequence, _Category>&, > _Size); > >+ template >+ _GLIBCXX_CONSTEXPR >+ inline bool >+ __can_advance(_InputIterator, const std::pair<_Diff, _Distance_precision>&, int) >+ { return true; } >+ >+ template+ typename _Diff> >+ bool >+ __can_advance(const _Safe_iterator<_Iterator, _Sequence, _Category>&, >+ const std::pair<_Diff, _Distance_precision>&, int); >+ > /** Helper function to extract base iterator of random access safe iterator > * in order to reduce performance impact of debug mode. Limited to random > * access iterator because it is the only category for which it is possible >diff --git a/libstdc++-v3/include/debug/macros.h b/libstdc++-v3/include/debug/macros.h >index 0988437046f..9ac52ebd09d 100644 >--- a/libstdc++-v3/include/debug/macros.h >+++ b/libstdc++-v3/include/debug/macros.h >@@ -94,6 +94,12 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Size), \ > ._M_iterator(_First, #_First) \ > ._M_integer(_Size, #_Size)) > >+#define __glibcxx_check_can_increment_dist(_First,_Dist,_Way) \ >+ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Dist, _Way), \ >+ _M_message(__gnu_debug::__msg_iter_subscript_oob) \ >+ ._M_iterator(_First, #_First) \ >+ ._M_integer(_Way * _Dist.first, #_Dist)) >+ > #define __glibcxx_check_can_increment_range(_First1,_Last1,_First2) \ > do \ > { \ >@@ -105,7 +111,7 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Size), \ > ._M_iterator(_Last1, #_Last1), \ > __FILE__,__LINE__,__PRETTY_FUNCTION__); \ > _GLIBCXX_DEBUG_VERIFY_AT_F( \ >- __gnu_debug::__can_advance(_First2, __dist.first),\ >+ __gnu_debug::__can_advance(_First2, __dist, 1), \ > _M_message(__gnu_debug::__msg_iter_subscript_oob)\ > ._M_iterator(_First2, #_First2) \ > ._M_integer(__dist.first), \ >@@ -123,7 +129,7 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Size), \ > ._M_iterator(_Last1, #_Last1), \ > __FILE__,__LINE__,__PRETTY_FUNCTION__); \ > _GLIBCXX_DEBUG_VERIFY_AT_F( \ >- __gnu_debug::__can_advance(_First2, -__dist.first),\ >+ __gnu_debug::__can_advance(_First2, __dist, -1), \ > _M_message(__gnu_debug::__msg_iter_subscript_oob)\ > ._M_iterator(_First2, #_First2) \ > ._M_integer(-__dist.first), \ >diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h >index a10df190969..8e138fd32e5 100644 >--- a/libstdc++-v3/include/debug/safe_iterator.h >+++ b/libstdc++-v3/include/debug/safe_iterator.h >@@ -407,6 +407,12 @@ namespace __gnu_debug > bool > _M_can_advance(difference_type __n, bool __strict = false) const; > >+ // Can we advance the iterator using @p __dist in @p __way direction. >+ template >+ bool >+ _M_can_advance(const std::pair<_Diff, _Distance_precision>& __dist, >+ int __way) const; >+ > // Is the iterator range [*this, __rhs) valid? > bool > _M_valid_range(const _Safe_iterator& __rhs, >@@ -958,6 +964,14 @@ namespace __gnu_debug > _Size __n) > { return __it._M_can_advance(__n); } > >+ template+ typename _Diff> >+ inline bool >+ __can_advance(const _Safe_iterator<_Iterator, _Sequence, _Category>& __it, >+ const std::pair<_Diff, _Distance_precision>& __dist, >+ int __way) >+ { return __it._M_can_advance(__dist, __way); } >+ > template > _Iterator > __base(const _Safe_iterator<_Iterator, _Sequence, >diff --git a/libstdc++-v3/include/debug/safe_iterator.tcc b/libstdc++-v3/include/debug/safe_iterator.tcc >index 81deb10125b..6f14c40bf41 100644 >--- a/libstdc++-v3/include/debug/safe_iterator.tcc >+++ b/libstdc++-v3/include/debug/safe_iterator.tcc >@@ -92,22 +92,30 @@ namespace __gnu_debug > if (__n == 0) > return true; > >+ std::pair __dist = __n < 0 >+ ? _M_get_distance_from_begin() >+ : _M_get_distance_to_end(); >+ > if (__n < 0) >- { >- std::pair __dist = >- _M_get_distance_from_begin(); >- return __dist.second == __dp_exact >- ? __dist.first >= -__n >+ __n = -__n; >+ >+ return __dist.second > __dp_sign >+ ? __dist.first >= __n > : !__strict && __dist.first > 0; > } >- else >+ >+ template >+ template >+ bool >+ _Safe_iterator<_Iterator, _Sequence, _Category>:: >+ _M_can_advance(const std::pair<_Diff, _Distance_precision>& __dist, >+ int __way) const > { >- std::pair __dist = >- _M_get_distance_to_end(); > return __dist.second == __dp_exact >- ? __dist.first >= __n >- : !__strict && __dist.first > 0; >- } >+ ? _M_can_advance(__way * __dist.first) >+ : _M_can_advance(__way * (__dist.first == 0 >+ ? 0 >+ : __dist.first < 0 ? -1 : 1)); > } > > template >@@ -194,20 +202,15 @@ namespace __gnu_debug > switch (__dist.second) > { > case __dp_equality: >- if (__dist.first == 0) >+ // Assume that this is a valid range; we can't check anything else. > return true; >- break; > >- case __dp_sign: >- case __dp_exact: >+ default: > // If range is not empty first iterator must be dereferenceable. >- if (__dist.first > 0) >- return !__check_dereferenceable || _M_dereferenceable(); >- return __dist.first == 0; >+ return __dist.first == 0 >+ || (__dist.first > 0 >+ && (!__check_dereferenceable || _M_dereferenceable())); > } This doesn't really make sense to use a switch now, it could be simply: if (__dist.second != __dp_equality) { // If range is not empty first iterator must be dereferenceable. return __dist.first == 0 || (__dist.first > 0 && (!__check_dereferenceable || _M_dereferenceable())); } // Assume that this is a valid range; we can't check anything else. return true; I don't understand any of this code, but it's currently broken so OK for trunk.