On Sun, 3 Aug 2025, 01:55 Andrew Pinski, <pins...@gmail.com> wrote:
> On Thu, Dec 7, 2023 at 2:07 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> >
> > Tested x86_64-linux. Pushed to trunk.
> >
> > -- >8 --
> >
> > The changes in r14-6198-g5e8a30d8b8f4d7 were broken, as I used
> > _GLIBCXX17_CONSTEXPR for the 'if _GLIBCXX17_CONSTEXPR (true)' condition,
> > forgetting that it would also be used for the is_constant_evaluated()
> > check. Using 'if constexpr (std::is_constant_evaluated())' is a bug.
> >
> > Additionally, relying on __glibcxx_assert_fail to give a "not a constant
> > expression" error is a problem because at -O0 an undefined reference to
> > __glibcxx_assert_fail is present in the compiled code. This means you
> > can't use libstdc++ headers without also linking to libstdc++ for the
> > symbol definition.
> >
> > This fix rewrites the __glibcxx_assert macro again. This still avoids
> > doing the duplicate checks, once for constexpr and once at runtime (if
> > _GLIBCXX_ASSERTIONS is defined). When _GLIBCXX_ASSERTIONS is defined we
> > still rely on __glibcxx_assert_fail to give a "not a constant
> > expression" error during constant evaluation (because when assertions
> > are defined it's not a problem to emit a reference to the symbol). But
> > when that macro is not defined, we use a new inline (but not constexpr)
> > overload of __glibcxx_assert_fail to cause compilation to fail. That
> > inline function doesn't cause an undefined reference to a symbol in the
> > library (and will be optimized away anyway).
> >
> > We can also add always_inline to the __is_constant_evaluated function,
> > although this doesn't actually matter for -O0 and it's always inlined
> > with any optimization enabled.
>
> I know this is an old patch but I am curious if we should include
> __builtin_unreachable() in __glibcxx_assert_fail for the current case
> where it is empty?
>
No. Assertions are not assumptions.
Any check which is worth asserting means it's something that users might
get wrong. If we mark it unreachable, we risk misoptimizing user code in
ways that are hard to debug and hard to understand. It might also cause the
compiler to remove other checks that users put into their own code, using
defensive programming techniques.
I was looking into PR 121280 where it uses front/back without checking
> if the vector is empy and GCC is producing a warning which is not easy
> to understand but seems correct due to this undefinedness (well a
> missed jump threading). Do we think the warning should stay or adding
> the unreachable in __glibcxx_assert_fail is a good idea?
>
> Note I suspect we don't want to add the unreachable. But maybe it is
> better to aways enable the assert instead?
>
> Thanks,
> Andrew
>
> >
> > libstdc++-v3/ChangeLog:
> >
> > PR libstdc++/112882
> > * include/bits/c++config (__is_constant_evaluated): Add
> > always_inline attribute.
> > (_GLIBCXX_DO_ASSERT): Remove macro.
> > (__glibcxx_assert): Define separately for assertions-enabled and
> > constexpr-only cases.
> > ---
> > libstdc++-v3/include/bits/c++config | 33 ++++++++++++++++++++---------
> > 1 file changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/bits/c++config
> b/libstdc++-v3/include/bits/c++config
> > index 284d24d933f..25d37428fc1 100644
> > --- a/libstdc++-v3/include/bits/c++config
> > +++ b/libstdc++-v3/include/bits/c++config
> > @@ -538,6 +538,7 @@ namespace std
> > // This can be used without checking if the compiler supports the
> feature.
> > // The macro _GLIBCXX_HAVE_IS_CONSTANT_EVALUATED can be used to check
> if
> > // the compiler support is present to make this function work as
> expected.
> > + __attribute__((__always_inline__))
> > _GLIBCXX_CONSTEXPR inline bool
> > __is_constant_evaluated() _GLIBCXX_NOEXCEPT
> > {
> > @@ -598,19 +599,31 @@ namespace std
> > #endif
> >
> > #if defined(_GLIBCXX_ASSERTIONS)
> > -# define _GLIBCXX_DO_ASSERT true
> > -#elif _GLIBCXX_HAVE_IS_CONSTANT_EVALUATED
> > -# define _GLIBCXX_DO_ASSERT std::__is_constant_evaluated()
> > -#else
> > -# define _GLIBCXX_DO_ASSERT false
> > -#endif
> > -
> > +// Enable runtime assertion checks, and also check in constant
> expressions.
> > # define __glibcxx_assert(cond)
> \
> > do { \
> > - if _GLIBCXX17_CONSTEXPR (_GLIBCXX_DO_ASSERT) \
> > - if (__builtin_expect(!bool(cond), false))
> \
> > - _GLIBCXX_ASSERT_FAIL(cond); \
> > + if (__builtin_expect(!bool(cond), false)) \
> > + _GLIBCXX_ASSERT_FAIL(cond); \
> > } while (false)
> > +#elif _GLIBCXX_HAVE_IS_CONSTANT_EVALUATED
> > +// Only check assertions during constant evaluation.
> > +namespace std
> > +{
> > + __attribute__((__always_inline__,__visibility__("default")))
> > + inline void
> > + __glibcxx_assert_fail()
> > + { }
> > +}
> > +# define __glibcxx_assert(cond)
> \
> > + do { \
> > + if (std::__is_constant_evaluated())
> \
> > + if (__builtin_expect(!bool(cond), false))
> \
> > + std::__glibcxx_assert_fail(); \
> > + } while (false)
> > +#else
> > +// Don't check any assertions.
> > +# define __glibcxx_assert(cond)
> > +#endif
> >
> > // Macro indicating that TSAN is in use.
> > #if __SANITIZE_THREAD__
> > --
> > 2.43.0
> >
>
