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

[Jonathan Wakely <jwakely.gcc@gmail.com>]

On Sat, 16 Dec 2023 at 16:26, Patrick Palka <ppalka@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.