From: Jonathan Wakely <jwakely.gcc@gmail.com>
To: Patrick Palka <ppalka@redhat.com>
Cc: Jonathan Wakely <jwakely@redhat.com>,
libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] libstdc++: Make __gnu_debug::vector usable in constant expressions [PR109536]
Date: Sat, 16 Dec 2023 09:14:54 +0000 [thread overview]
Message-ID: <CAH6eHdTxiWXNNPFUQ_TQ=Mn0rP7Ok8-z1nC140uvOtSeE0tVtQ@mail.gmail.com> (raw)
In-Reply-To: <44072750-3a93-419e-af86-924fc4f29f02@idea>
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.
next prev parent reply other threads:[~2023-12-16 9:15 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-06 14:18 Jonathan Wakely
2023-12-14 16:34 ` Jonathan Wakely
2023-12-16 0:26 ` Patrick Palka
2023-12-16 9:14 ` Jonathan Wakely [this message]
2023-12-16 9:37 ` Jonathan Wakely
2023-12-16 16:26 ` Patrick Palka
2023-12-16 16:30 ` Jonathan Wakely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAH6eHdTxiWXNNPFUQ_TQ=Mn0rP7Ok8-z1nC140uvOtSeE0tVtQ@mail.gmail.com' \
--to=jwakely.gcc@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jwakely@redhat.com \
--cc=libstdc++@gcc.gnu.org \
--cc=ppalka@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).