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.
> >
> > libstdc++-v3/ChangeLog:
> >
> > PR libstdc++/109536
> > * include/bits/c++config (__glibcxx_constexpr_assert): Remove
> > macro.
> > * include/bits/stl_algobase.h (__niter_base, __copy_move_a)
> > (__copy_move_backward_a, __fill_a, __fill_n_a, __equal_aux)
> > (__lexicographical_compare_aux): Add constexpr to overloads for
> > debug mode iterators.
> > * include/debug/helper_functions.h (__unsafe): Add constexpr.
> > * include/debug/macros.h (_GLIBCXX_DEBUG_VERIFY_COND_AT): Remove
> > macro, folding it into ...
> > (_GLIBCXX_DEBUG_VERIFY_AT_F): ... here. Do not use
> > __glibcxx_constexpr_assert.
> > * include/debug/safe_base.h (_Safe_iterator_base): Add constexpr
> > to some member functions. Omit attaching, detaching and checking
> > operations during constant evaluation.
> > * include/debug/safe_container.h (_Safe_container): Likewise.
> > * include/debug/safe_iterator.h (_Safe_iterator): Likewise.
> > * include/debug/safe_iterator.tcc (__niter_base, __copy_move_a)
> > (__copy_move_backward_a, __fill_a, __fill_n_a, __equal_aux)
> > (__lexicographical_compare_aux): Add constexpr.
> > * include/debug/vector (_Safe_vector, vector): Add constexpr.
> > Omit safe iterator operations during constant evaluation.
> > * testsuite/23_containers/vector/bool/capacity/constexpr.cc:
> > Remove dg-xfail-if for debug mode.
> > * testsuite/23_containers/vector/bool/cmp_c++20.cc: Likewise.
> > * testsuite/23_containers/vector/bool/cons/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/bool/element_access/1.cc:
> > Likewise.
> > * testsuite/23_containers/vector/bool/element_access/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/bool/modifiers/assign/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/bool/modifiers/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/bool/modifiers/swap/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/capacity/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/cmp_c++20.cc: Likewise.
> > * testsuite/23_containers/vector/cons/constexpr.cc: Likewise.
> > * testsuite/23_containers/vector/data_access/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/element_access/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/modifiers/assign/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/modifiers/constexpr.cc:
> > Likewise.
> > * testsuite/23_containers/vector/modifiers/swap/constexpr.cc:
> > Likewise.
> > ---
> > libstdc++-v3/include/bits/c++config | 9 -
> > libstdc++-v3/include/bits/stl_algobase.h | 15 ++
> > libstdc++-v3/include/debug/helper_functions.h | 1 +
> > libstdc++-v3/include/debug/macros.h | 9 +-
> > libstdc++-v3/include/debug/safe_base.h | 35 +++-
> > libstdc++-v3/include/debug/safe_container.h | 15 +-
> > libstdc++-v3/include/debug/safe_iterator.h | 186 +++++++++++++++---
> > libstdc++-v3/include/debug/safe_iterator.tcc | 15 ++
> > libstdc++-v3/include/debug/vector | 146 ++++++++++++--
> > .../vector/bool/capacity/constexpr.cc | 1 -
> > .../23_containers/vector/bool/cmp_c++20.cc | 1 -
> > .../vector/bool/cons/constexpr.cc | 1 -
> > .../vector/bool/element_access/1.cc | 1 -
> > .../vector/bool/element_access/constexpr.cc | 1 -
> > .../vector/bool/modifiers/assign/constexpr.cc | 1 -
> > .../vector/bool/modifiers/constexpr.cc | 1 -
> > .../vector/bool/modifiers/swap/constexpr.cc | 3 +-
> > .../vector/capacity/constexpr.cc | 1 -
> > .../23_containers/vector/cmp_c++20.cc | 1 -
> > .../23_containers/vector/cons/constexpr.cc | 1 -
> > .../vector/data_access/constexpr.cc | 1 -
> > .../vector/element_access/constexpr.cc | 1 -
> > .../vector/modifiers/assign/constexpr.cc | 1 -
> > .../vector/modifiers/constexpr.cc | 1 -
> > .../vector/modifiers/swap/constexpr.cc | 1 -
> > 25 files changed, 369 insertions(+), 80 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/bits/c++config
> > b/libstdc++-v3/include/bits/c++config
> > index 284d24d933f..13d416845c3 100644
> > --- a/libstdc++-v3/include/bits/c++config
> > +++ b/libstdc++-v3/include/bits/c++config
> > @@ -565,15 +565,6 @@ namespace std
> > # define _GLIBCXX_EXTERN_TEMPLATE -1
> > #endif
> >
> > -
> > -#if _GLIBCXX_HAVE_IS_CONSTANT_EVALUATED
> > -# define __glibcxx_constexpr_assert(cond) \
> > - if (std::__is_constant_evaluated() && !bool(cond)) \
> > - __builtin_unreachable() /* precondition violation detected! */
> > -#else
> > -# define __glibcxx_constexpr_assert(unevaluated)
> > -#endif
> > -
> > #undef _GLIBCXX_VERBOSE_ASSERT
> >
> > // Assert.
> > diff --git a/libstdc++-v3/include/bits/stl_algobase.h
> > b/libstdc++-v3/include/bits/stl_algobase.h
> > index 01ca4496dfd..77d0ee7bcf5 100644
> > --- a/libstdc++-v3/include/bits/stl_algobase.h
> > +++ b/libstdc++-v3/include/bits/stl_algobase.h
> > @@ -318,6 +318,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > { return __it; }
> >
> > template<typename _Ite, typename _Seq>
> > + _GLIBCXX20_CONSTEXPR
> > _Ite
> > __niter_base(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq,
> > std::random_access_iterator_tag>&);
> > @@ -545,6 +546,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> >
> > template<bool _IsMove,
> > typename _Ite, typename _Seq, typename _Cat, typename _OI>
> > + _GLIBCXX20_CONSTEXPR
> > _OI
> > __copy_move_a(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&,
> > const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&,
> > @@ -552,6 +554,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> >
> > template<bool _IsMove,
> > typename _II, typename _Ite, typename _Seq, typename _Cat>
> > + _GLIBCXX20_CONSTEXPR
> > __gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>
> > __copy_move_a(_II, _II,
> > const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&);
> > @@ -559,6 +562,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> > template<bool _IsMove,
> > typename _IIte, typename _ISeq, typename _ICat,
> > typename _OIte, typename _OSeq, typename _OCat>
> > + _GLIBCXX20_CONSTEXPR
> > ::__gnu_debug::_Safe_iterator<_OIte, _OSeq, _OCat>
> > __copy_move_a(const ::__gnu_debug::_Safe_iterator<_IIte, _ISeq,
> > _ICat>&,
> > const ::__gnu_debug::_Safe_iterator<_IIte, _ISeq, _ICat>&,
> > @@ -812,6 +816,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> >
> > template<bool _IsMove,
> > typename _Ite, typename _Seq, typename _Cat, typename _OI>
> > + _GLIBCXX20_CONSTEXPR
> > _OI
> > __copy_move_backward_a(
> > const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&,
> > @@ -820,6 +825,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> >
> > template<bool _IsMove,
> > typename _II, typename _Ite, typename _Seq, typename _Cat>
> > + _GLIBCXX20_CONSTEXPR
> > __gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>
> > __copy_move_backward_a(_II, _II,
> > const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&);
> > @@ -827,6 +833,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> > template<bool _IsMove,
> > typename _IIte, typename _ISeq, typename _ICat,
> > typename _OIte, typename _OSeq, typename _OCat>
> > + _GLIBCXX20_CONSTEXPR
> > ::__gnu_debug::_Safe_iterator<_OIte, _OSeq, _OCat>
> > __copy_move_backward_a(
> > const ::__gnu_debug::_Safe_iterator<_IIte, _ISeq, _ICat>&,
> > @@ -977,6 +984,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> > { std::__fill_a1(__first, __last, __value); }
> >
> > template<typename _Ite, typename _Seq, typename _Cat, typename _Tp>
> > + _GLIBCXX20_CONSTEXPR
> > void
> > __fill_a(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&,
> > const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&,
> > @@ -1082,6 +1090,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> >
> > template<typename _Ite, typename _Seq, typename _Cat, typename _Size,
> > typename _Tp>
> > + _GLIBCXX20_CONSTEXPR
> > ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>
> > __fill_n_a(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&
> > __first,
> > _Size __n, const _Tp& __value,
> > @@ -1230,18 +1239,21 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> > }
> >
> > template<typename _II1, typename _Seq1, typename _Cat1, typename _II2>
> > + _GLIBCXX20_CONSTEXPR
> > bool
> > __equal_aux(const ::__gnu_debug::_Safe_iterator<_II1, _Seq1, _Cat1>&,
> > const ::__gnu_debug::_Safe_iterator<_II1, _Seq1, _Cat1>&,
> > _II2);
> >
> > template<typename _II1, typename _II2, typename _Seq2, typename _Cat2>
> > + _GLIBCXX20_CONSTEXPR
> > bool
> > __equal_aux(_II1, _II1,
> > const ::__gnu_debug::_Safe_iterator<_II2, _Seq2, _Cat2>&);
> >
> > template<typename _II1, typename _Seq1, typename _Cat1,
> > typename _II2, typename _Seq2, typename _Cat2>
> > + _GLIBCXX20_CONSTEXPR
> > bool
> > __equal_aux(const ::__gnu_debug::_Safe_iterator<_II1, _Seq1, _Cat1>&,
> > const ::__gnu_debug::_Safe_iterator<_II1, _Seq1, _Cat1>&,
> > @@ -1430,6 +1442,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> >
> > template<typename _Iter1, typename _Seq1, typename _Cat1,
> > typename _II2>
> > + _GLIBCXX20_CONSTEXPR
> > bool
> > __lexicographical_compare_aux(
> > const ::__gnu_debug::_Safe_iterator<_Iter1, _Seq1, _Cat1>&,
> > @@ -1438,6 +1451,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> >
> > template<typename _II1,
> > typename _Iter2, typename _Seq2, typename _Cat2>
> > + _GLIBCXX20_CONSTEXPR
> > bool
> > __lexicographical_compare_aux(
> > _II1, _II1,
> > @@ -1446,6 +1460,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
> >
> > template<typename _Iter1, typename _Seq1, typename _Cat1,
> > typename _Iter2, typename _Seq2, typename _Cat2>
> > + _GLIBCXX20_CONSTEXPR
> > bool
> > __lexicographical_compare_aux(
> > const ::__gnu_debug::_Safe_iterator<_Iter1, _Seq1, _Cat1>&,
> > diff --git a/libstdc++-v3/include/debug/helper_functions.h
> > b/libstdc++-v3/include/debug/helper_functions.h
> > index 052b36b484c..4b76cb00f9a 100644
> > --- a/libstdc++-v3/include/debug/helper_functions.h
> > +++ b/libstdc++-v3/include/debug/helper_functions.h
> > @@ -324,6 +324,7 @@ namespace __gnu_debug
> >
> > /* Remove debug mode safe iterator layer, if any. */
> > template<typename _Iterator>
> > + _GLIBCXX_CONSTEXPR
> > inline _Iterator
> > __unsafe(_Iterator __it)
> > { return __it; }
> > diff --git a/libstdc++-v3/include/debug/macros.h
> > b/libstdc++-v3/include/debug/macros.h
> > index 0fef0a006fc..4a3d0f2ea84 100644
> > --- a/libstdc++-v3/include/debug/macros.h
> > +++ b/libstdc++-v3/include/debug/macros.h
> > @@ -38,15 +38,12 @@
> > * the user error and where the error is reported.
> > *
> > */
> > -#define _GLIBCXX_DEBUG_VERIFY_COND_AT(_Cond,_ErrMsg,_File,_Line,_Func)
> > \
> > - if (__builtin_expect(!bool(_Cond), false)) \
> > - __gnu_debug::_Error_formatter::_S_at(_File, _Line, _Func)
> > \
> > - ._ErrMsg._M_error()
> >
> > #define _GLIBCXX_DEBUG_VERIFY_AT_F(_Cond,_ErrMsg,_File,_Line,_Func) \
> > do {
> > \
> > - __glibcxx_constexpr_assert(_Cond);
> > \
> > - _GLIBCXX_DEBUG_VERIFY_COND_AT(_Cond,_ErrMsg,_File,_Line,_Func); \
> > + if (__builtin_expect(!bool(_Cond), false))
> > \
> > + __gnu_debug::_Error_formatter::_S_at(_File, _Line, _Func)
> > \
> > + ._ErrMsg._M_error(); \
> > } while (false)
> >
> > #define _GLIBCXX_DEBUG_VERIFY_AT(_Cond,_ErrMsg,_File,_Line) \
> > diff --git a/libstdc++-v3/include/debug/safe_base.h
> > b/libstdc++-v3/include/debug/safe_base.h
> > index 1dfa9f68b65..d9c17b52b48 100644
> > --- a/libstdc++-v3/include/debug/safe_base.h
> > +++ b/libstdc++-v3/include/debug/safe_base.h
> > @@ -75,6 +75,7 @@ namespace __gnu_debug
> >
> > protected:
> > /** Initializes the iterator and makes it singular. */
> > + _GLIBCXX20_CONSTEXPR
> > _Safe_iterator_base()
> > : _M_sequence(0), _M_version(0), _M_prior(0), _M_next(0)
> > { }
> > @@ -86,18 +87,31 @@ namespace __gnu_debug
> > * singular. Otherwise, the iterator will reference @p __seq and
> > * be nonsingular.
> > */
> > + _GLIBCXX20_CONSTEXPR
> > _Safe_iterator_base(const _Safe_sequence_base* __seq, bool __constant)
> > : _M_sequence(0), _M_version(0), _M_prior(0), _M_next(0)
> > - { this->_M_attach(const_cast<_Safe_sequence_base*>(__seq),
> > __constant); }
> > + {
> > + if (!std::__is_constant_evaluated())
> > + this->_M_attach(const_cast<_Safe_sequence_base*>(__seq), __constant);
> > + }
> >
> > /** Initializes the iterator to reference the same sequence that
> > @p __x does. @p __constant is true if this is a constant
> > iterator, and false if it is mutable. */
> > + _GLIBCXX20_CONSTEXPR
> > _Safe_iterator_base(const _Safe_iterator_base& __x, bool __constant)
> > : _M_sequence(0), _M_version(0), _M_prior(0), _M_next(0)
> > - { this->_M_attach(__x._M_sequence, __constant); }
> > + {
> > + if (!std::__is_constant_evaluated())
> > + this->_M_attach(__x._M_sequence, __constant);
> > + }
> >
> > - ~_Safe_iterator_base() { this->_M_detach(); }
> > + _GLIBCXX20_CONSTEXPR
> > + ~_Safe_iterator_base()
> > + {
> > + if (!std::__is_constant_evaluated())
> > + this->_M_detach();
> > + }
> >
> > /** For use in _Safe_iterator. */
> > __gnu_cxx::__mutex&
> > @@ -201,24 +215,34 @@ namespace __gnu_debug
> >
> > protected:
> > // Initialize with a version number of 1 and no iterators
> > + _GLIBCXX20_CONSTEXPR
> > _Safe_sequence_base() _GLIBCXX_NOEXCEPT
> > : _M_iterators(0), _M_const_iterators(0), _M_version(1)
> > { }
> >
> > #if __cplusplus >= 201103L
> > + _GLIBCXX20_CONSTEXPR
> > _Safe_sequence_base(const _Safe_sequence_base&) noexcept
> > : _Safe_sequence_base() { }
> >
> > // Move constructor swap iterators.
> > + _GLIBCXX20_CONSTEXPR
> > _Safe_sequence_base(_Safe_sequence_base&& __seq) noexcept
> > : _Safe_sequence_base()
> > - { _M_swap(__seq); }
> > + {
> > + if (!std::__is_constant_evaluated())
> > + _M_swap(__seq);
> > + }
> > #endif
> >
> > /** Notify all iterators that reference this sequence that the
> > sequence is being destroyed. */
> > + _GLIBCXX20_CONSTEXPR
> > ~_Safe_sequence_base()
> > - { this->_M_detach_all(); }
> > + {
> > + if (!std::__is_constant_evaluated())
> > + this->_M_detach_all();
> > + }
> >
> > /** Detach all iterators, leaving them singular. */
> > void
> > @@ -244,6 +268,7 @@ 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;
>
> With -Wsystem-headers on some ranges tests I get:
>
> /gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_base.h:273:5:
> warning: inline function ‘constexpr void
> __gnu_debug::_Safe_sequence_base::_M_swap(__gnu_debug::_Safe_sequence_base&)’
> used but never defined
> 273 | _M_swap(_Safe_sequence_base& __x) _GLIBCXX_USE_NOEXCEPT;
> | ^~~~~~~
Oh I think we can remove the CONSTEXPR macro. I added it too
aggressively and forgot to remove it again in some places.
