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.133.124]) by sourceware.org (Postfix) with ESMTPS id 3961E3858CD1 for ; Thu, 18 Jan 2024 10:30:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3961E3858CD1 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 3961E3858CD1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705573835; cv=none; b=LCRhnWkf7o8imp8UL9MKkFx+88iYtJoeG394HtcBi72ajxwG9ZHCFH7EZ8z+MidVoI6EY9gfPn8S4SXGyncCJfbfPorHWAutaLNNaDwE2iuX8YB9bU88wn7CCamdo19DFniLCFDRYQhSAg4zNoQ3BQUSVexnZB6LbheKMd+BSEA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705573835; c=relaxed/simple; bh=+D/LydDEUpvdHJTPclFzHzk/plS35qxi5PM1dyuMkvM=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=oMZoZx+0fCkqeWYJxzmoxkqx8oUsQGxQPMJB8maqnTSV2XV6cgp+ZIcGuYz2wUM0TP158GW7vGm/39aMNxGTIxLl0NzNOaWhon1PAtUBJ5a2yxb+vsS6jIJEAXBy7dQSMCnrMmbs7tapFStJFNpGkSWu+ucdd+4++TIChypZ1y4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705573823; 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: in-reply-to:in-reply-to:references:references; bh=7ryedD/jqQeMyefsj1ZwkOVbUHsdOH6DtZ5oOLafu60=; b=NwlmtK8gGajuc1tz3KB1dP1ZAlkO2jZzDsFaLSZjmczULzoPIweNOq5VOCTc+RJNbCHRnd z9Avs4VYE8yoq2x9yPrPGW+IHKZ4m8yy6lRYlW1Y7oWJo+yHz3ekJYurOp820hiw7XkrxP SYbt5bJ/hCbu88gd5mMvFQyyJMrm46A= Received: from mail-yb1-f197.google.com (mail-yb1-f197.google.com [209.85.219.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-396-dNEDm7fBPVGhlpsC-9BjYg-1; Thu, 18 Jan 2024 05:30:21 -0500 X-MC-Unique: dNEDm7fBPVGhlpsC-9BjYg-1 Received: by mail-yb1-f197.google.com with SMTP id 3f1490d57ef6-dbcd9f4396eso17524833276.0 for ; Thu, 18 Jan 2024 02:30:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705573821; x=1706178621; h=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=7ryedD/jqQeMyefsj1ZwkOVbUHsdOH6DtZ5oOLafu60=; b=QkMfpzS24TqqNzsTPFb9URJ7jKxHmi1KUq5ePnmPLs2qvfN2ZHGWlTFeTRB4jmkxhz qAurQEi4IZPMnSik+ZDguiyLcB7OPR881phggj2joFBSCrbloZisg3hSlwLSwVKJ6udf SFnm4rukcHL05St09VOv4qQ9nZGuXmeYKNwKa5oUyAnzLcvvVfwqyxI7KqPsAyZX0JH3 SJnXkuUXdj/Ukp9g3S3ZNvyf6ZrN7oHD/BaSIbd0Chwi4I12GiL+/DChDCKxuqzTqAq9 dAifmqKLNUzvgB/fE3KoewF/uy5rlE9+drmI+wnZpdJv86f8BbPShZf8z07EEjq9+uoy 9sKA== X-Gm-Message-State: AOJu0YxbngUKPWAzckrbqVx7wHgqk8TS25bLt23e8U60mTAjdXPNWiZs f4i9DMMq20jsMcD2FI/x35HC/idcZxD5IEQWp7MDQLsOHKd0iXvFE7IbE3hW5Pb3h8HdytuAeka sA0rCZntwIE5QnTHHBKmu5IYMNe/n2GvgV5Kf8ufbMs3HJeppqFdn0t/6HXi5imjBweNqIvEVl9 KTwt6XhCU6rvxPel4vr6PSfaSnWWTM2A== X-Received: by 2002:a25:9007:0:b0:dc2:23f6:15a4 with SMTP id s7-20020a259007000000b00dc223f615a4mr473008ybl.39.1705573821266; Thu, 18 Jan 2024 02:30:21 -0800 (PST) X-Google-Smtp-Source: AGHT+IHrzphI4UXY8KmUbLDZm4YhqoddFnj31k2N29gBGd0ZCfpnFKMXSsaV6i+4q0MUyXQpcKZPDNEDmHqRcMfBp/g= X-Received: by 2002:a25:9007:0:b0:dc2:23f6:15a4 with SMTP id s7-20020a259007000000b00dc223f615a4mr472999ybl.39.1705573820980; Thu, 18 Jan 2024 02:30:20 -0800 (PST) MIME-Version: 1.0 References: <20240118024806.436235-1-ppalka@redhat.com> In-Reply-To: <20240118024806.436235-1-ppalka@redhat.com> From: Jonathan Wakely Date: Thu, 18 Jan 2024 10:30:04 +0000 Message-ID: Subject: Re: [PATCH] libstdc++: Fix constexpr _Safe_iterator in C++20 mode To: Patrick Palka Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-13.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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 List-Id: On Thu, 18 Jan 2024 at 02:48, Patrick Palka wrote: > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk? Please add PR109536 to the commit message. > > -- >8 -- > > Some _Safe_iterator member functions define a variable of non-literal > type __gnu_cxx::__scoped_lock, which automatically disqualifies them from > being constexpr in C++20 mode even if that code path is never constant > evaluated. This restriction was lifted by P2242R3 for C++23, but we > need to work around it in C++20 mode. To that end this patch defines > a pair of macros that encapsulate the lambda-based workaround mentioned > in that paper and uses them to make the functions valid C++20 constexpr > functions. The augmented std::vector test element_access/constexpr.cc > now successfully compiles in C++20 mode with -D_GLIBCXX_DEBUG (and it > tests all modified member functions). > > libstdc++-v3/ChangeLog: > > * include/debug/safe_base.h (_Safe_sequence_base::_M_swap): > Remove _GLIBCXX20_CONSTEXPR. > * include/debug/safe_iterator.h (_GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN): > (_GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END): Define. > (_Safe_iterator::operator=): Use them around the code path that > defines a variable of type __gnu_cxx::__scoped_lock. > (_Safe_iterator::operator++): Likewise. > (_Safe_iterator::operator--): Likewise. > (_Safe_iterator::operator+=): Likewise. > (_Safe_iterator::operator-=): Likewise. > * testsuite/23_containers/vector/element_access/constexpr.cc > (test_iterators): Also test copy and move assignment. > * testsuite/std/ranges/adaptors/all.cc (test08) [_GLIBCXX_DEBUG]: > Use std::vector unconditionally. > --- > libstdc++-v3/include/debug/safe_base.h | 1 - > libstdc++-v3/include/debug/safe_iterator.h | 48 ++++++++++++++----- > .../vector/element_access/constexpr.cc | 2 + > .../testsuite/std/ranges/adaptors/all.cc | 4 -- > 4 files changed, 38 insertions(+), 17 deletions(-) > > diff --git a/libstdc++-v3/include/debug/safe_base.h b/libstdc++-v3/include/debug/safe_base.h > index 107fef3cb02..d5fbe4b1320 100644 > --- a/libstdc++-v3/include/debug/safe_base.h > +++ b/libstdc++-v3/include/debug/safe_base.h > @@ -268,7 +268,6 @@ namespace __gnu_debug > * operation is complete all iterators that originally referenced > * one container now reference the other container. > */ > - _GLIBCXX20_CONSTEXPR > void > _M_swap(_Safe_sequence_base& __x) _GLIBCXX_USE_NOEXCEPT; > > diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h > index 1bc7c904ee0..929fd9b0ade 100644 > --- a/libstdc++-v3/include/debug/safe_iterator.h > +++ b/libstdc++-v3/include/debug/safe_iterator.h > @@ -65,6 +65,20 @@ > _GLIBCXX_DEBUG_VERIFY_OPERANDS(_Lhs, _Rhs, __msg_distance_bad, \ > __msg_distance_different) > > +// This pair of macros helps with writing valid C++20 constexpr functions that > +// contain a non-constexpr code path that defines a non-literal variable, which > +// was otherwise disallowed until P2242R3 for C++23. We use them below for > +// __gnu_cxx::__scoped_lock so that the containing functions are still > +// considered valid C++20 constexpr functions. > + > +#if __cplusplus >= 202002L && __cpp_constexpr < 202110L > +# define _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN [&]() -> void { do > +# define _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END while(false); }(); Do we need the do-while to create a single statement from the block? Isn't the lambda body enough to create a single statement from it, which can't be broken by a dangling else or anything like that? > +#else > +# define _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN > +# define _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END > +#endif > + > namespace __gnu_debug > { > /** Helper struct to deal with sequence offering a before_begin > @@ -266,11 +280,11 @@ namespace __gnu_debug > ._M_iterator(__x, "other")); > > if (this->_M_sequence && this->_M_sequence == __x._M_sequence) > - { > + _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN { > __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > base() = __x.base(); > _M_version = __x._M_sequence->_M_version; > - } > + } _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END > else > { > _M_detach(); > @@ -306,11 +320,11 @@ namespace __gnu_debug > return *this; > > if (this->_M_sequence && this->_M_sequence == __x._M_sequence) > - { > + _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN { > __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > base() = __x.base(); > _M_version = __x._M_sequence->_M_version; > - } > + } _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END > else > { > _M_detach(); > @@ -378,8 +392,10 @@ namespace __gnu_debug > _GLIBCXX_DEBUG_VERIFY(this->_M_incrementable(), > _M_message(__msg_bad_inc) > ._M_iterator(*this, "this")); > - __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > - ++base(); > + _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN { > + __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > + ++base(); > + } _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END > return *this; > } > > @@ -697,8 +713,10 @@ namespace __gnu_debug > _GLIBCXX_DEBUG_VERIFY(this->_M_decrementable(), > _M_message(__msg_bad_dec) > ._M_iterator(*this, "this")); > - __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > - --this->base(); > + _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN { > + __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > + --this->base(); > + } _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END > return *this; > } > > @@ -912,8 +930,10 @@ namespace __gnu_debug > _GLIBCXX_DEBUG_VERIFY(this->_M_can_advance(__n), > _M_message(__msg_advance_oob) > ._M_iterator(*this)._M_integer(__n)); > - __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > - this->base() += __n; > + _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN { > + __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > + this->base() += __n; > + } _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END > return *this; > } > > @@ -930,8 +950,10 @@ namespace __gnu_debug > _GLIBCXX_DEBUG_VERIFY(this->_M_can_advance(-__n), > _M_message(__msg_retreat_oob) > ._M_iterator(*this)._M_integer(__n)); > - __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > - this->base() -= __n; > + _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN { > + __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > + this->base() -= __n; > + } _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END > return *this; > } > > @@ -1156,6 +1178,8 @@ _GLIBCXX_END_NAMESPACE_VERSION > } > #endif > > +#undef _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END > +#undef _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN > #undef _GLIBCXX_DEBUG_VERIFY_DIST_OPERANDS > #undef _GLIBCXX_DEBUG_VERIFY_REL_OPERANDS > #undef _GLIBCXX_DEBUG_VERIFY_EQ_OPERANDS > diff --git a/libstdc++-v3/testsuite/23_containers/vector/element_access/constexpr.cc b/libstdc++-v3/testsuite/23_containers/vector/element_access/constexpr.cc > index ee93d2fd95e..ab1e7f1bb70 100644 > --- a/libstdc++-v3/testsuite/23_containers/vector/element_access/constexpr.cc > +++ b/libstdc++-v3/testsuite/23_containers/vector/element_access/constexpr.cc > @@ -25,6 +25,8 @@ test_iterators() > it -= 2; > it += 1; > VERIFY( (it + 1) == v.end() ); > + it = it + 1; > + it = it; I think we also need to test these operators here: it[n] n + it it - it And also for the reverse iterator. I think that invokes all the operators. For vector, none of those operators do anything different for positive or negative arguments, so we don't need to test cases like it[-1], it+-1, -1+it etc. > > auto rit = v.rbegin(); > VERIFY( &*rit == &v.back() ); > diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/all.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/all.cc > index e7010f80e18..5f7206dc8c3 100644 > --- a/libstdc++-v3/testsuite/std/ranges/adaptors/all.cc > +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/all.cc > @@ -156,11 +156,7 @@ test07() > constexpr bool > test08() > { > -#ifdef _GLIBCXX_DEBUG > - using std::_GLIBCXX_STD_C::vector; > -#else > using std::vector; > -#endif Oh that's nice to remove. > > // Verify P2415R2 "What is a view?" changes. > // In particular, rvalue non-view non-borrowed ranges are now viewable. > -- > 2.43.0.367.g186b115d30 >