On Thu, 18 Jan 2024, Jonathan Wakely wrote:
> 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.
Done.
>
>
>
> >
> > -- >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?
I was thinking that the do-while gives compile-time assurance that the
macros are used properly and in particular every ..._BEGIN is matched
with an ..._END, so that e.g.
_GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN {
do_stuff();
} // omitted ..._END
doesn't parse. But it turns out that won't parse even without the
do-while, due to a missing semicolon. And the parse error is much more
readable when the do-while isn't used.
One risk without the do-while is that the seemingly innocent
_GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN {
do_stuff();
};
will parse, and (in C++20 mode) define a lambda that's never invoked,
and thus do_stuff() is never invoked. But tests should catch that,
so consider the do-while removed.
>
>
> > +#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.
Done. I also adjusted the vector/bool test to match. How does the
following look?
-- >8 --
Subject: [PATCH] libstdc++/debug: Fix constexpr _Safe_iterator in C++20 mode
[PR109536]
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
should test all member functions modified by this patch).
PR libstdc++/109536
libstdc++-v3/ChangeLog:
* include/debug/safe_base.h (_Safe_sequence_base::_M_swap):
Remove _GLIBCXX20_CONSTEXPR from non-inline member function.
* 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): Test more iterator operations.
* testsuite/23_containers/vector/bool/element_access/constexpr.cc
(test_iterators): Likewise.
* 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/bool/element_access/constexpr.cc | 18 +++++++
.../vector/element_access/constexpr.cc | 18 +++++++
.../testsuite/std/ranges/adaptors/all.cc | 4 --
5 files changed, 72 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..d3e959b8fa7 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 around
+// __gnu_cxx::__scoped_lock variables 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
+# define _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END ();
+#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/bool/element_access/constexpr.cc
b/libstdc++-v3/testsuite/23_containers/vector/bool/element_access/constexpr.cc
index d6b657e0161..bff9f7b4e0f 100644
---
a/libstdc++-v3/testsuite/23_containers/vector/bool/element_access/constexpr.cc
+++
b/libstdc++-v3/testsuite/23_containers/vector/bool/element_access/constexpr.cc
@@ -18,22 +18,40 @@ test_iterators()
VERIFY( v.crend() == v.rend() );
auto it = v.begin();
+ VERIFY( it[0] == 0 );
VERIFY( *it == v.front() );
+ VERIFY( it[1] == v[1] );
VERIFY( it++ == v.begin() );
VERIFY( ++it == v.end() );
VERIFY( (it - 2) == v.begin() );
+ VERIFY( (it - v.begin()) == 2 );
it -= 2;
it += 1;
VERIFY( (it + 1) == v.end() );
+ VERIFY( (1 + it) == v.end() );
+ it = it + 1;
+ auto it2 = v.begin();
+ std::swap(it, it2);
+ VERIFY( it == v.begin() );
+ VERIFY( it2 == v.end() );
auto rit = v.rbegin();
+ VERIFY( rit[0] == 0 );
VERIFY( *rit == v.back() );
+ VERIFY( rit[1] == v[0] );
VERIFY( rit++ == v.rbegin() );
VERIFY( ++rit == v.rend() );
VERIFY( (rit - 2) == v.rbegin() );
+ VERIFY( (rit - v.rbegin()) == 2 );
rit -= 2;
rit += 1;
VERIFY( (rit + 1) == v.rend() );
+ VERIFY( (1 + rit) == v.rend() );
+ rit = rit + 1;
+ auto rit2 = v.rbegin();
+ std::swap(rit, rit2);
+ VERIFY( rit == v.rbegin() );
+ VERIFY( rit2 == v.rend() );
return true;
}
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..19c91d28cd6 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/element_access/constexpr.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/element_access/constexpr.cc
@@ -18,22 +18,40 @@ test_iterators()
VERIFY( v.crend() == v.rend() );
auto it = v.begin();
+ VERIFY( it[0] == 0 );
VERIFY( &*it == &v.front() );
+ VERIFY( &it[1] == &v[1] );
VERIFY( it++ == v.begin() );
VERIFY( ++it == v.end() );
VERIFY( (it - 2) == v.begin() );
+ VERIFY( (it - v.begin()) == 2 );
it -= 2;
it += 1;
VERIFY( (it + 1) == v.end() );
+ VERIFY( (1 + it) == v.end() );
+ it = it + 1;
+ auto it2 = v.begin();
+ std::swap(it, it2);
+ VERIFY( it == v.begin() );
+ VERIFY( it2 == v.end() );
auto rit = v.rbegin();
+ VERIFY( rit[0] == 0 );
VERIFY( &*rit == &v.back() );
+ VERIFY( &rit[1] == &v[0] );
VERIFY( rit++ == v.rbegin() );
VERIFY( ++rit == v.rend() );
VERIFY( (rit - 2) == v.rbegin() );
+ VERIFY( (rit - v.rbegin()) == 2 );
rit -= 2;
rit += 1;
VERIFY( (rit + 1) == v.rend() );
+ VERIFY( (1 + rit) == v.rend() );
+ rit = rit + 1;
+ auto rit2 = v.rbegin();
+ std::swap(rit, rit2);
+ VERIFY( rit == v.rbegin() );
+ VERIFY( rit2 == v.rend() );
return true;
}
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
// Verify P2415R2 "What is a view?" changes.
// In particular, rvalue non-view non-borrowed ranges are now viewable.
--
2.43.0.367.g186b115d30
