From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com [IPv6:2a00:1450:4864:20::62e]) by sourceware.org (Postfix) with ESMTPS id 85673385703C; Mon, 8 Aug 2022 22:26:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 85673385703C Received: by mail-ej1-x62e.google.com with SMTP id w19so19134225ejc.7; Mon, 08 Aug 2022 15:26:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=uJQ1ZPA+/ryDlr6dxei9smaIJotNUTKtTuTN7l8pix0=; b=eMePm+8jtRpHQyHuuxJh+8lJs+S07hSv3nc0sf+Meb4EDj9TaAn5KyoGM5u6wzvEUi TRojMvkDzZE7SruBpwQ9gY85nzyd5F5o+P9ZB+I/F81lMyeB0GMML9d40JUZm/JdRk2W tLTs2OWisOvbD4QCQo4ZMc7/QalVL+g05BuEjg1yMhJt4XJ4Nm6BOPTxsFSMXa8Hyz3L KWy8M0ZIFHQDupZR1gUEx+6XT/rA9pDl0Cht8H4BooRm7i2eF0A32ghUp04BncqcFMOp QfK5/tWpuzOQo1TLswouI3XgjXtp63AmgWpL84XQ4/R70ra08pDEiksr6GSdo0Nkzv9a jAoA== X-Gm-Message-State: ACgBeo20sqtWPnw+e0Wx+M9kZs54SMnsDhYv041o/WsiDo3YPDRsthHn qu8i+wVWbLulfOGeDug45iOhOSHPpRM0v30FeoE= X-Google-Smtp-Source: AA6agR5IeT7hCM/rrU9QtZej6JYYVYqDfFSRl3/NFbCTlq9fJxbPbVfxq4haAWJIBiC7Oo4Ii0RCkYxsZjotCzPMoe0= X-Received: by 2002:a17:906:dc90:b0:72f:c504:45c with SMTP id cs16-20020a170906dc9000b0072fc504045cmr15125920ejc.386.1659997599130; Mon, 08 Aug 2022 15:26:39 -0700 (PDT) MIME-Version: 1.0 References: <3b6d1c3d-8bc7-e5e5-0a76-58d960f86e3f@gmail.com> <047769c5-7833-4b1f-787d-5de0a2755aa1@gmail.com> In-Reply-To: <047769c5-7833-4b1f-787d-5de0a2755aa1@gmail.com> From: Jonathan Wakely Date: Mon, 8 Aug 2022 23:26:16 +0100 Message-ID: Subject: Re: [PATCH][_GLIBCXX_DEBUG] Refine singular iterator state To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: Jonathan Wakely , "libstdc++" , gcc-patches X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Mon, 08 Aug 2022 22:26:43 -0000 On Mon, 8 Aug 2022, 19:15 Fran=C3=A7ois Dumont via Libstdc++, < libstdc++@gcc.gnu.org> wrote: > On 08/08/22 15:19, Jonathan Wakely wrote: > > On Mon, 8 Aug 2022 at 06:07, Fran=C3=A7ois Dumont via Libstdc++ > > wrote: > >> Another version of this patch with just a new test case showing what > >> wrong code was unnoticed previously by the _GLIBCXX_DEBUG mode. > >> > >> On 04/08/22 22:56, Fran=C3=A7ois Dumont wrote: > >>> This an old patch I had prepared a long time ago, I don't think I eve= r > >>> submitted it. > >>> > >>> libstdc++: [_GLIBCXX_DEBUG] Do not consider detached iterators a= s > >>> value-initialized > >>> > >>> An attach iterator has its _M_version set to something !=3D 0. T= his > >>> value shall be preserved > >>> when detaching it so that the iterator does not look like a valu= e > >>> initialized one. > >>> > >>> libstdc++-v3/ChangeLog: > >>> > >>> * include/debug/formatter.h (__singular_value_init): New > >>> _Iterator_state enum entry. > >>> (_Parameter<>(const _Safe_iterator<>&, const char*, > >>> _Is_iterator)): Check if iterator > >>> parameter is value-initialized. > >>> (_Parameter<>(const _Safe_local_iterator<>&, const char*= , > >>> _Is_iterator)): Likewise. > >>> * include/debug/safe_iterator.h > >>> (_Safe_iterator<>::_M_value_initialized()): New. Adapt > >>> checks. > >>> * include/debug/safe_local_iterator.h > >>> (_Safe_local_iterator<>::_M_value_initialized()): New. > >>> Adapt checks. > >>> * src/c++11/debug.cc (_Safe_iterator_base::_M_reset): Do > >>> not reset _M_version. > >>> (print_field(PrintContext&, const _Parameter&, const > >>> char*)): Adapt state_names. > >>> * testsuite/23_containers/deque/debug/iterator1_neg.cc: > >>> New test. > >>> * testsuite/23_containers/deque/debug/iterator2_neg.cc: > >>> New test. > >>> * > >>> testsuite/23_containers/forward_list/debug/iterator1_neg.cc: New test= . > >>> * > >>> testsuite/23_containers/forward_list/debug/iterator2_neg.cc: New test= . > >>> > >>> Tested under Linux x86_64 _GLIBCXX_DEBUG mode. > >>> > >>> Ok to commit ? > >>> > >>> Fran=C3=A7ois > > > >> diff --git a/libstdc++-v3/src/c++11/debug.cc > b/libstdc++-v3/src/c++11/debug.cc > >> index 4706defedf1..cf8e6f48081 100644 > >> --- a/libstdc++-v3/src/c++11/debug.cc > >> +++ b/libstdc++-v3/src/c++11/debug.cc > >> @@ -426,7 +426,8 @@ namespace __gnu_debug > >> _M_reset() throw () > >> { > >> __atomic_store_n(&_M_sequence, (_Safe_sequence_base*)0, > __ATOMIC_RELEASE); > >> - _M_version =3D 0; > >> + // Detach iterator shall not look like a value-initialized one. > >> + // _M_version =3D 0; > >> _M_prior =3D 0; > >> _M_next =3D 0; > >> } > > I think this would be clearer as "Do not reset version, so that a > > detached iterator does not look like a value-initialized one." > > > >> +// { dg-do run { xfail *-*-* } } > >> +// { dg-require-debug-mode "" } > >> + > >> +#include > >> + > >> +#include > >> + > >> +void test01() > >> +{ > >> + typedef typename std::deque::iterator It; > >> + std::deque dq; > >> + dq.push_back(1); > >> + > >> + It it =3D It(); > >> + VERIFY( dq.begin() !=3D it ); > > Is there any reason to use VERIFY here? > Only make sure the compiler do not optimize this check away. > It can't do that. If the debug check results in output to the terminal and aborting the program then that's an observable side effect that cannot be optimised away. If the compiler was somehow smart enough to know that the comparison is undefined then we wouldn't need debug mode. > > > We're expecting the comparison to abort in the debug mode checks, > > right? Which would happen if we just do: > > > > (void) dq.begin() =3D=3D it; > > I guess this (void) cast is doing the same so adopted. > No, the cast silences a warning. > > Using VERIFY just makes it look like we're expecting the test to be > > XFAIL because the assertion will fail, but that's not what is being > > tested. > > > > OK for trunk with those changes, thanks. > > > Updated committed patch attached. > Thanks. >