[libc++] Enable assertions unconditionally during constant evaluation

[MitalAshok]

Undefined behaviour when calling a library function is unspecified ([expr.const]p(6.1)). However, it is strange that whether it is detected depends on if libc++ is built with assertions or not.

Consider https://godbolt.org/z/Kx6ez5no5:

#include <vector>

constexpr int f() {
  std::vector<int> v;
  v.reserve(1);
  int& i = v.front();  // Library UB
  v.push_back(4);
  return i;
}

static_assert(f() == 4);

This doesn't compile with assertions:

<source>:11:15: error: static assertion expression is not an integral constant expression
   11 | static_assert(f() == 4);
      |               ^~~~~~~~
/opt/compiler-explorer/clang-assertions-trunk-20240904/bin/../include/c++/v1/vector:652:5: note: subexpression not valid in a constant expression
  652 |     _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "front() called on an empty vector");
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/compiler-explorer/clang-assertions-trunk-20240904/bin/../include/c++/v1/__assert:66:71: note: expanded from macro '_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS'
   66 | #  define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message)    _LIBCPP_ASSERT(expression, message)
      |                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/compiler-explorer/clang-assertions-trunk-20240904/bin/../include/c++/v1/__assert:23:10: note: expanded from macro '_LIBCPP_ASSERT'
   23 |        : _LIBCPP_ASSERTION_HANDLER(__FILE__ ":" _LIBCPP_TOSTRING(__LINE__) ": assertion " _LIBCPP_TOSTRING(            \
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   24 |              expression) " failed: " message "\n"))
      |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/compiler-explorer/clang-assertions-trunk-20240904/bin/../include/c++/v1/__assertion_handler:33:50: note: expanded from macro '_LIBCPP_ASSERTION_HANDLER'
   33 | #      define _LIBCPP_ASSERTION_HANDLER(message) __builtin_verbose_trap("libc++", message)
      |                                                  ^~~~~~~~~~~~~~~~~~~~~~
<source>:6:12: note: in call to 'v.front()'
    6 |   int& i = v.front();  // Library UB
      |            ^~~~~~~~~
<source>:11:15: note: in call to 'f()'
   11 | static_assert(f() == 4);
      |               ^~~

But does compile if assertions are disabled.

I believe libstdc++ does something like this, so this fails even without assertions enabled.

This can be done with __libcpp_is_constant_evaluated/__builtin_is_constant_evaluated. This also removes the need for [[maybe_unused]] for variables used just for assertions.

The downside is the compile time would to increase on non-debug builds, where LIBCPP_ASSERT(x) goes from (void) 0 to is_constant_evaluated() && !(x) ? assert_handler(...) : (void) 0. I haven't measured the actual impact, but I don't expect it to be much.

[philnik777]

I agree it's a good idea to enable the assertions during constant evaluation, but where did you get the idea that libc++ cares about MSVC?

[MitalAshok]

Oh I got confused because the test suite is used by MSVC's STL. Thanks for the clarification. (The macro still can't use if consteval because I would expect this to work in C++11 too and apparently clang/gcc only accept if consteval as an extension in C++20. Probably because it's not reserved in C++17/14/11, so this is moot)

[philnik777]

Right, we care that our test suite is portable across any conforming implementations, but not for the implementation itself. You're also correct that consteval is only backported to C++20 because it's not a keyword before.

[frederick-vs-ja]

There's a drawback - the changes will cause more full-expressions executed, so there will be some formerly successful constant evaluation (which didn't trigger library UB) made failing, see [expr.const]/5.7 and [implimits]/1.39.

libc++'s test suit sometimes tests the maximum ability of constant evaluation for libc++, and the relevant tests fails with MSVC STL because MSVC STL's debugging mechanism is relatively heavier.

[MitalAshok]

the changes will cause more full-expressions executed

@frederick-vs-ja In my proposed implementation of this, _LIBCPP_ASSERT_*(E) goes from (void)0 -> __builtin_is_constant_evaluated() ? (E) ? (void)0 : __builtin_unreachable() : (void)0. If E doesn't contain any function calls, that is one full-expression -> one full expression, not changing the amount. Besides, users would run into this anyway if they enable hardening and they can increase the constexpr steps in clang or gcc to fix it.

Also, once issues with __builtin_assume are fixed (esp. #40658), there could be the same number of steps regardless.

sometimes tests the maximum ability of constant evaluation for libc++, and the relevant tests fails

These tests would have failed if they were run in libc++ hardening modes. Some other tests don't work if hardening is enabled, and they would have to be changed to also not run during constant evaluation (I've only found this test so far: aa603e4#diff-a45ba96855919f6e85ce51ab51eb7a00bc1622e227596959721ceddc499c0218)

[philnik777]

There's a drawback - the changes will cause more full-expressions executed, so there will be some formerly successful constant evaluation (which didn't trigger library UB) made failing, see [expr.const]/5.7 and [implimits]/1.39.

libc++'s test suit sometimes tests the maximum ability of constant evaluation for libc++, and the relevant tests fails with MSVC STL because MSVC STL's debugging mechanism is relatively heavier.

There is a trivial fix: -fconstexpr-steps=<some high number>. The number of steps is completely arbitrary anyways (e.g. using __builtin_memmove is always exactly one step), so I don't think we should consider that at all. Actual compile times are the only relevant measure IMO.

[ldionne]

Closing in light of the discussions in #107713. Please reopen if you still want to pursue this.

[philnik777]

I don't think the arguments in #107713 are particularly compelling TBH. We do catch real UB during constant evaluation by enabling the assertions. I don't see why we should separate that from the tool that exists explicitly to find or eliminate library UB.