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 [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 7CC7F3858C66 for ; Thu, 12 Jan 2023 21:35:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7CC7F3858C66 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673559358; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GlTuBOdxOEH1A+YcHIgW16pOPmgnsCW2j/nbF0hXzMM=; b=UFpQSxQEneV38Em7RfhW7R4SECqwPqSAm2CGnfKmDGYZ+gz8jWXCOdrkF7U3Wgp3K1vx39 l2iK/v3P8FB1mSN6myPgngGzDsHO/Qf3GsoorHOieZlSB4tlPcF1iAbH3QlYJgaa5BHg4a Cxf3Lx2fBdoUuQ1+JHrerrh/DLvMD+8= Received: from mail-lf1-f70.google.com (mail-lf1-f70.google.com [209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-571-W5z_BIcRM3eKMvViXKPrXw-1; Thu, 12 Jan 2023 16:35:56 -0500 X-MC-Unique: W5z_BIcRM3eKMvViXKPrXw-1 Received: by mail-lf1-f70.google.com with SMTP id v19-20020ac25933000000b004b55ec28779so7371193lfi.8 for ; Thu, 12 Jan 2023 13:35:56 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=GlTuBOdxOEH1A+YcHIgW16pOPmgnsCW2j/nbF0hXzMM=; b=S5cYVbaJbUjJjCe/8pPcpFZctQMCjfWXbMH+3IGs32SBPVpvPn1zxWwE+PbixHxd/F njATcWTg/pKhWquJNY8pLoMJIiYZUVCUSzpi1umILWJANrwGd4XA6X1WDsc9oHE7fBLt 5dKHdxFGHoOsIo9KGXMXG3hOkXmOmGZ+ScSmRuD0jeXJZ1A+03MrCr0+2N9lMFREMI7e u8I6j+8HWkQ9Crbju3WLrfXOQLicbvJ4zXV2A2XTb6hVip15eexwXxydYbA82kNxfMcW q3Ep91iixJB3bOmUxiDtrcV7rGELkGV6qPDhyniSa4sCGrr/skeZUtgJlEHznMDMJnZl ThTQ== X-Gm-Message-State: AFqh2koaDSql//WP/+vvNCdFmK5jxp9wIkf9aiCVVgUg7kNETRxyUKUc w6f4xI9yzDf+8wGSDlpznra4pirjw+xPX+9dNSMftVkD+Ajxc562DxJFal6g+/ZJKFLpll3iHDn uwi1ncPvkqOvwAzGLTxzr8Iwb5SX9GKo= X-Received: by 2002:a2e:900c:0:b0:286:9b01:b036 with SMTP id h12-20020a2e900c000000b002869b01b036mr1052350ljg.193.1673559355541; Thu, 12 Jan 2023 13:35:55 -0800 (PST) X-Google-Smtp-Source: AMrXdXvVL8poKWPdFwHFF6+tSlHUwR7Y2fGjJskZyABrNzEab+mejoXIobt1BnRIYOpwt3zHE3TCaAoVAyWVukOHYtQ= X-Received: by 2002:a2e:900c:0:b0:286:9b01:b036 with SMTP id h12-20020a2e900c000000b002869b01b036mr1052348ljg.193.1673559355275; Thu, 12 Jan 2023 13:35:55 -0800 (PST) MIME-Version: 1.0 References: <20230106115402.178926-1-jwakely@redhat.com> <6f88e8e7-6771-d344-beb4-1ca37d79dd5c@gmail.com> In-Reply-To: <6f88e8e7-6771-d344-beb4-1ca37d79dd5c@gmail.com> From: Jonathan Wakely Date: Thu, 12 Jan 2023 21:35:43 +0000 Message-ID: Subject: Re: libstdc++: Fix deadlock in debug iterator increment [PR108288] To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, 12 Jan 2023 at 18:25, Fran=C3=A7ois Dumont w= rote: > > On 12/01/23 13:00, Jonathan Wakely wrote: > > On Thu, 12 Jan 2023 at 05:52, Fran=C3=A7ois Dumont wrote: > >> Small update for an obvious compilation issue and to review new test > >> case that could have lead to an infinite loop if the increment issue w= as > >> not detected. > >> > >> I also forgot to ask if there is more chance for the instantiation to = be > >> elided when it is implemented like in the _Safe_local_iterator: > >> return { __cur, this->_M_sequence }; > > No, that doesn't make any difference. > > > >> than in the _Safe_iterator: > >> return _Safe_iterator(__cur, this->_M_sequence); > >> > >> In the case where the user code do not use it ? > >> > >> Fully tested now, ok to commit ? > >> > >> Fran=C3=A7ois > >> > >> On 11/01/23 07:03, Fran=C3=A7ois Dumont wrote: > >>> Thanks for fixing this. > >>> > >>> Here is the extension of the fix to all post-increment/decrement > >>> operators we have on _GLIBCXX_DEBUG iterator. > > Thanks, I completely forgot we have other partial specializations, I > > just fixed the one that showed a deadlock in the user's example! > > > >>> I prefer to restore somehow previous implementation to continue to > >>> have _GLIBCXX_DEBUG post operators implemented in terms of normal pos= t > >>> operators. > > Why? > > > > Implementing post-increment as: > > > > auto tmp =3D *this; > > ++*this; > > return tmp; > > > > is the idiomatic way to write it, and it works fine in this case. I > > don't think it performs any more work than your version, does it? > > Why not use the idiomatic form? > > > > Is it just so that post-inc of a debug iterator uses post-inc of the > > underlying iterator? Why does that matter? > > > A little yes, but that's a minor reason that is just making me happy. > > Main reason is that this form could produce a __msg_init_copy_singular > before the __msg_bad_inc. Ah yes, I see. That's a shame. I find the idiomatic form much simpler to read, and it will generate better code (because it just reuses existing functions, instead of adding new ones). We could do this though, right? _GLIBCXX_DEBUG_VERIFY(this->_M_incrementable(), _M_message(__msg_bad_inc) ._M_iterator(*this, "this")); _Safe_iterator __tmp =3D *this; ++*this; return __tmp; That does the VERIFY check twice though. > And moreover I plan to propose a patch later to skip any check in the > call to _Safe_iterator(__cur, _M_sequence) as we already know that __cur > is ok here like anywhere else in the lib. > > There will still be one in the constructor normally elided unless > --no-elide-constructors but there is not much I can do about it. Don't worry about it. Nobody should ever use -fno-elide-constructors in any real cases (except maybe debugging some very strange corner cases, and in that case the extra safe iterator checks are not going to be their biggest problem). The patch is OK for trunk then.