libstdc++: Fix constexpr _Safe_iterator in C++20 mode

Message ID 20240118024806.436235-1-ppalka@redhat.com
State Accepted
Headers
Series libstdc++: Fix constexpr _Safe_iterator in C++20 mode |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Patrick Palka Jan. 18, 2024, 2:48 a.m. UTC
  Tested on x86_64-pc-linux-gnu, does this look OK for trunk?

-- >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(-)
  

Comments

Jonathan Wakely Jan. 18, 2024, 10:30 a.m. UTC | #1
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
>
  
Patrick Palka Jan. 18, 2024, 1:51 p.m. UTC | #2
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.
  
Jonathan Wakely Jan. 18, 2024, 2:17 p.m. UTC | #3
On Thu, 18 Jan 2024 at 13:51, Patrick Palka <ppalka@redhat.com> wrote:
>
> 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?

Looks great, thanks for fixing this.

OK for trunk.


>
> -- >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
>
  

Patch

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); }();
+#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;
 
   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
 
   // Verify P2415R2 "What is a view?" changes.
   // In particular, rvalue non-view non-borrowed ranges are now viewable.