On Sun, 3 Aug 2025, 14:54 Jonathan Wakely, <jwakely....@gmail.com> wrote:
> > > 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. > https://wg21.link/p2064 > 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 >> > >> >