From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x335.google.com (mail-wm1-x335.google.com [IPv6:2a00:1450:4864:20::335]) by sourceware.org (Postfix) with ESMTPS id 09D743AA9819; Fri, 9 Apr 2021 20:02:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 09D743AA9819 Received: by mail-wm1-x335.google.com with SMTP id j4-20020a05600c4104b029010c62bc1e20so3567092wmi.3; Fri, 09 Apr 2021 13:02:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=qJTPnRt/DQEcUHDoNN6Ak/f2LWIHo9Uw5X1U4CacnCk=; b=kcAstYtv4HYi/0iEGTihxWKyXm2sxNoIiOvQIp7jm2jAQrVMlcQXTk7jfhN9nj8o5B dt2xBnp4L6ldrKQDNPVQZ5JrSR+IiB/6/CrJSnmODNhlgSoS+pz2NkVfTL1NSngl5+nz Sf+4+J3memoj6Iul5guBNJSH06PLY1DEf6+UvIoR0rhn0Zn+sw5TcvRd8w3/l3xIpQHP iEOmoKu2IOvctqxWWjJUypWXBiBF479qTNJZaeW1D0p7uZIKdJrV0H5aJfBpDBRoasH5 hrTmAAUMfcbqQZBPuoQe9q2wsYpcHpRAYVb+gyumxw5K0MWwe7S7Wx3JYxfv+dtbmUC2 AA2g== X-Gm-Message-State: AOAM532qVt8ep7pdSCwGUCTgklfazyBIuA6Qs2CqQ6bGeB6S50/+qbP5 fKT4VjM6MxRwe/5NsQatwYLdgry7WSxdDA== X-Google-Smtp-Source: ABdhPJx5xg+6DcXpkZ+DR76XS5uagnybnJaMaIqPIr0nhFBZjuaZfZQSRrmc5vIN+0FnHwVTzvF8SA== X-Received: by 2002:a1c:2781:: with SMTP id n123mr15634816wmn.64.1617998551245; Fri, 09 Apr 2021 13:02:31 -0700 (PDT) Received: from ?IPv6:2a01:e0a:1dc:b1c0:dd26:a206:88c6:5202? ([2a01:e0a:1dc:b1c0:dd26:a206:88c6:5202]) by smtp.googlemail.com with ESMTPSA id c16sm7003228wrs.81.2021.04.09.13.02.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 09 Apr 2021 13:02:30 -0700 (PDT) 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 To: Jonathan Wakely Cc: "libstdc++@gcc.gnu.org" , gcc-patches References: <5a5ae1ac-f3e3-70a4-c37d-80dd7baf6d05@gmail.com> <20210408130748.GF3008@redhat.com> From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= Message-ID: <209fe25d-4b64-6f5f-cc71-b8d5ee333b5c@gmail.com> Date: Fri, 9 Apr 2021 22:02:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210408130748.GF3008@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: fr X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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: Fri, 09 Apr 2021 20:02:36 -0000 On 08/04/21 3:07 pm, Jonathan Wakely wrote: > 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; > Yes, I missed that. I tested and committed with this simplification. > > I don't understand any of this code, but it's currently broken so OK > for trunk. > Maybe there will be something to do to clarify then.