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?
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
>
