[Bug libstdc++/112882] New: [14 Regression] std::clamp no longer usable in header only mode

[Bug libstdc++/112882] New: [14 Regression] std::clamp no longer usable in header only mode

[tnfchris at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

            Bug ID: 112882
           Summary: [14 Regression] std::clamp no longer usable in header
                    only mode
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: tnfchris at gcc dot gnu.org
                CC: redi at gcc dot gnu.org
  Target Milestone: ---

I know this is not a supported scenario, but I'm wondering if it's still easy
to support.

We have some libraries that use C++ mostly as an abstraction layer and try to
ensure that it needs no runtime support from libstdc++.

A recent commit: g:5e8a30d8b8f4d7ea0a8340b46c1e0c865dbde781 changed how
`__glibcxx_assert` is defined and now always calls
`std::__glibcxx_assert_fail`.

This means that now you always need libstdc++ even in contex where things would
have been folded away before.  Similarly we're getting the same thing through
usage of `std::unique_ptr`.

It seems that undefining `_GLIBCXX_VERBOSE_ASSERT` gets it to go to
`__builtin_abort()` which makes it work again.

If this change was intentional, would it be possible to make
`_GLIBCXX_VERBOSE_ASSERT` user configurable?

[Bug libstdc++/112882] [14 Regression] std::clamp no longer usable in header only mode

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-12-06
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Tamar Christina from comment #0)
> I know this is not a supported scenario, but I'm wondering if it's still
> easy to support.
> 
> We have some libraries that use C++ mostly as an abstraction layer and try
> to ensure that it needs no runtime support from libstdc++.
> 
> A recent commit: g:5e8a30d8b8f4d7ea0a8340b46c1e0c865dbde781 changed how
> `__glibcxx_assert` is defined and now always calls
> `std::__glibcxx_assert_fail`.
> 
> This means that now you always need libstdc++ even in contex where things
> would have been folded away before.  Similarly we're getting the same thing
> through usage of `std::unique_ptr`.

Ugh, that's undesirable. It should only depend on that symbol when
_GLIBCXX_ASSERTIONS is defined, but the change means we also use that symbol
for constexpr checks.

I'm surprised it doesn't get folded away though, since the code looks like:

  if (std::__is_constant_evaluated())
    if (__builtin_expect(COND, false))
      __glibcxx_assert_fail(...);

Since __is_constant_evaluated is always false at runtime, that function can
never be called. Either it's needed during compile-time and makes the program
ill-formed, or it's not needed at all.

Ah, __is_constant_evaluated() is not marked always_inline, so at -O0 it just
looks like a normal function call.

> It seems that undefining `_GLIBCXX_VERBOSE_ASSERT` gets it to go to
> `__builtin_abort()` which makes it work again.
> 
> If this change was intentional, would it be possible to make
> `_GLIBCXX_VERBOSE_ASSERT` user configurable?

You can use --disable-libstdcxx-verbose for that, or do you need to control it
during preprocessing?

[Bug libstdc++/112882] [14 Regression] std::clamp no longer usable in header only mode

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
   Target Milestone|---                         |14.0

[Bug libstdc++/112882] [14 Regression] std::clamp no longer usable in header only mode

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
That change was broken anyway: when _GLIBCXX_ASSERTIONS was not defined, the
condition in the assertion is if constexpr (is_constant_evaluated()) which is
always true, even when not actually doing constant eval.

I'll test this:

--- 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
   {
@@ -589,19 +590,28 @@ 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
-
 # 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
+namespace std
+{
+  __attribute__((__always_inline__,visibility("default")))
+  inline void
+  __glibcxx_compile_time_assert_fail()
+  { }
+}
+# define __glibcxx_assert(cond)                                               
\
+  do {                                                                 \
+    if (std::__is_constant_evaluated())                                       
\
+      if (__builtin_expect(!bool(cond), false))                               
\
+       __glibcxx_compile_time_assert_fail();                           \
+  } while (false)
+#else
+# define __glibcxx_assert(cond)
+#endif

 // Macro indicating that TSAN is in use.
 #if __SANITIZE_THREAD__

[Bug libstdc++/112882] [14 Regression] std::clamp no longer usable in header only mode

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug libstdc++/112882] [14 Regression] std::clamp no longer usable in header only mode

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This minimal fix is enough to remove the reference to __glibcxx_assert_fail
when optimization is enabled (at any level):

--- a/libstdc++-v3/include/bits/c++config
+++ b/libstdc++-v3/include/bits/c++config
@@ -598,7 +598,7 @@ namespace std

 # define __glibcxx_assert(cond)                                               
\
   do {                                                                 \
-    if _GLIBCXX17_CONSTEXPR (_GLIBCXX_DO_ASSERT)                       \
+    if (_GLIBCXX_DO_ASSERT)                    \
       if (__builtin_expect(!bool(cond), false))                               
\
        _GLIBCXX_ASSERT_FAIL(cond);                                     \
   } while (false)

But we still need the more complete fix for -O0.

[Bug libstdc++/112882] [14 Regression] std::clamp no longer usable in header only mode

[tnfchris at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

--- Comment #4 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
Awesome! Thanks for taking a look!

> > It seems that undefining `_GLIBCXX_VERBOSE_ASSERT` gets it to go to
> > `__builtin_abort()` which makes it work again.
> > 
> > If this change was intentional, would it be possible to make
> > `_GLIBCXX_VERBOSE_ASSERT` user configurable?
> 
> You can use --disable-libstdcxx-verbose for that, or do you need to control
> it during preprocessing?

Yeah I meant during preprocessing, we distribute the library as source to
client too, so would be harder if they'd also need to recompile the toolchain
:)

Thanks for the fixes!

[Bug libstdc++/112882] [14 Regression] std::clamp no longer usable in header only mode

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The original effect of --disable-libstdcxx-verbose was to change the default
std::terminate handler, which is a configure-time property.

But it's now also used to decide whether to use the verbose assertion or just
abort. That latter property could be flipped at compile-time. Even with this
bug fixed, that might be useful (probably for stage 1 though).

[Bug libstdc++/112882] [14 Regression] std::clamp no longer usable in header only mode

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

--- Comment #6 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:1395c573c523762957bde8c2a08832c5f4350815

commit r14-6291-g1395c573c523762957bde8c2a08832c5f4350815
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Dec 6 17:21:29 2023 +0000

    libstdc++: Fix recent changes to __glibcxx_assert [PR112882]

    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.

    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.

[Bug libstdc++/112882] [14 Regression] std::clamp no longer usable in header only mode

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112882

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed now - thanks for spotting the problem!