[PATCH] libstdc++: Make __gnu_debug::vector usable in constant expressions [PR109536]

[Jonathan Wakely]

On Sat, 16 Dec 2023 at 09:14, Jonathan Wakely wrote:
>
> On Sat, 16 Dec 2023 at 00:27, Patrick Palka wrote:
> >
> > On Wed, 6 Dec 2023, Jonathan Wakely wrote:
> >
> > > Any comments on this approach?
> > >
> > > -- >8 --
> > >
> > > This makes constexpr std::vector (mostly) work in Debug Mode. All safe
> > > iterator instrumentation and checking is disabled during constant
> > > evaluation, because it requires mutex locks and calls to non-inline
> > > functions defined in libstdc++.so. It should be OK to disable the safety
> > > checks, because most UB should be detected during constant evaluation
> > > anyway.
> > >
> > > We could try to enable the full checking in constexpr, but it would mean
> > > wrapping all the non-inline functions like _M_attach with an inline
> > > _M_constexpr_attach that does the iterator housekeeping inline without
> > > mutex locks when calling for constant evaluation, and calls the
> > > non-inline function at runtime. That could be done in future if we find
> > > that we've lost safety or useful checking by disabling the safe
> > > iterators.
> > >
> > > There are a few test failures in C++20 mode, which I'm unable to
> > > explain. The _Safe_iterator::operator++() member gives errors for using
> > > non-constexpr functions during constant evaluation, even though those
> > > functions are guarded by std::is_constant_evaluated() checks. The same
> > > code works fine for C++23 and up.
> >
> > AFAICT these C++20 test failures are really due to the variable
> > definition of non-literal type
> >
> > 381    __gnu_cxx::__scoped_lock __l(this->_M_get_mutex());
> >
> > which were prohibited in a constexpr function (even if that code was
> > never executed) until C++23's P2242R3.
>
> Ah, I figured it was a core change but I couldn't recall which one. Thanks.
>
> > We can use an immediately invoked lambda to work around this:
> >
> > 381    [this] {
> > 382      __gnu_cxx::__scoped_lock __l(this->_M_get_mutex());
> > 383      ++base();
> > 384    }();
> > 385    return *this;
>
> We'd need some #if as this code has to work for C++98. But that's doable.

The attached patch seems simpler, I'm testing it now.
-------------- next part --------------
commit 5d70c6c2965647077749a869e9cdbf7e91dba4c7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Sat Dec 16 09:31:40 2023

    libstdc++: Fix errors for constexpr __gnu_debug::vector in C++23 [PR109536]
    
    In the commit log for r14-6553-g7d00a59229ee17 I noted some tests FAIL
    in C++20 mode. Patrick identified that they were due to the variable
    definitions of non-literal type __scoped_lock, which were prohibited in
    a constexpr function (even if that code was never executed) until
    C++23's P2242R3.
    
    We can move the problematic code into new non-constexpr functions that
    are not called during constant evaluation.
    
    There's also a warning about a constexpr _M_swap function which is never
    defined. That's simply because I added the _GLIBCXX20_CONSTEXPR macro on
    a member that doesn't need it.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/109536
            * include/debug/safe_base.h (_Safe_sequence_base::_M_swap):
            Remove _GLIBCXX20_CONSTEXPR from non-inline member function.
            * include/debug/safe_iterator.h (_Safe_iterator::_M_move_assign)

diff --git a/libstdc++-v3/include/debug/safe_base.h b/libstdc++-v3/include/debug/safe_base.h
index d9c17b52b48..1519ad809a4 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 26f008982f8..bde34e1f99c 100644
--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -295,7 +295,13 @@ namespace __gnu_debug
 	    base() = __x.base();
 	    return *this;
 	  }
+	_M_move_assign(std::move(__x));
+	return *this;
+      }
 
+      void
+      _M_move_assign(_Safe_iterator&& __x) noexcept
+      {
 	_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
 			      || __x._M_value_initialized(),
 			      _M_message(__msg_copy_singular)
@@ -303,7 +309,7 @@ namespace __gnu_debug
 			      ._M_iterator(__x, "other"));
 
 	if (std::__addressof(__x) == this)
-	  return *this;
+	  return;
 
 	if (this->_M_sequence && this->_M_sequence == __x._M_sequence)
 	  {
@@ -320,7 +326,6 @@ namespace __gnu_debug
 
 	__x._M_detach();
 	__x.base() = _Iterator();
-	return *this;
       }
 #endif
 
@@ -370,17 +375,20 @@ namespace __gnu_debug
       operator++() _GLIBCXX_NOEXCEPT
       {
 	if (std::__is_constant_evaluated())
-	  {
-	    ++base();
-	    return *this;
-	  }
+	  ++base();
+	else
+	  _M_increment();
+	return *this;
+      }
 
+      void
+      _M_increment() _GLIBCXX_NOEXCEPT
+      {
 	_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();
-	return *this;
       }
 
       /**
@@ -689,17 +697,20 @@ namespace __gnu_debug
       operator--() _GLIBCXX_NOEXCEPT
       {
 	if (std::__is_constant_evaluated())
-	  {
-	    --this->base();
-	    return *this;
-	  }
+	  --this->base();
+	else
+	  _M_decrement();
+	return *this;
+      }
 
+      void
+      _M_decrement() _GLIBCXX_NOEXCEPT
+      {
 	_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();
-	return *this;
       }
 
       /**