On Sat, 16 Dec 2023 at 16:26, Patrick Palka <ppa...@redhat.com> wrote: > > On Sat, 16 Dec 2023, Jonathan Wakely wrote: > > > 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. > > Yeah the diagnostic blaming the non-constexpr call to _M_incrementable > was outright wrong and misleading, I filed PR113041 about that. > > > > > > > > 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. > > Would the operator+=, operator-= and the copy assign operator= also need > adjustment?
Maybe ... which suggest we have missing tests for constexpr vector (which is probably the case). > > We could somewhat cleanly encapsulate the lambda workaround with a pair > of macros surrounding the problematic variable, something like: > > diff --git a/libstdc++-v3/include/debug/safe_iterator.h > b/libstdc++-v3/include/debug/safe_iterator.h > index 26f008982f8..df3b4d33f04 100644 > --- a/libstdc++-v3/include/debug/safe_iterator.h > +++ b/libstdc++-v3/include/debug/safe_iterator.h > @@ -360,6 +360,14 @@ namespace __gnu_debug > return base().operator->(); > } > > +#if __cplusplus >= 202002L && __cpp_constexpr < 202110L > +# define _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN [&] { do > +# define _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END while(false); }() > +#else > +# define _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN do > +# define _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END while(false) I think for the limited uses in this file, we don't even need the do-while, as the code we're enclosing is not a single statement anyway. > +#endif > + > // ------ Input iterator requirements ------ > /** > * @brief Iterator preincrement > @@ -378,8 +386,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; > } > Yeah, I'll check if other operators need it, and if it's more than just the two places in my patch I'll go with that. If I don't get around to it (as I'm meant to have stopped work for the year yesterday) then feel free to do that.