[libc++] Use __builtin_expect and __builtin_assume in _LIBCPP_ASSERT

[ldionne]

Summary

Since we expect the condition to be true most of the time, we might
as well tell the compiler. And when assertions are disabled, we
might as well tell the compiler that it's allowed to assume that
the condition holds.

[philnik]

Maybe it would make sense to use __builtin_assume if assertions are disabled?

[ldionne]

In D122397#3405618, @philnik wrote:

Maybe it would make sense to use __builtin_assume if assertions are disabled?

My understanding is that __builtin_assume(x) does not evaluate x ever at runtime, but the documentation is a little bit light on the details. I think this is a fantastic idea, I'll try it out. If __builtin_assume does evaluate x, I added a test that should fail in my assertions handler patch.

[philnik]

In D122397#3406074, @ldionne wrote:

In D122397#3405618, @philnik wrote:

Maybe it would make sense to use __builtin_assume if assertions are disabled?

My understanding is that __builtin_assume(x) does not evaluate x ever at runtime, but the documentation is a little bit light on the details. I think this is a fantastic idea, I'll try it out. If __builtin_assume does evaluate x, I added a test that should fail in my assertions handler patch.

I'm pretty sure it doesn't evaluate at compile-time. https://godbolt.org/z/8d7fh8nc7 clang specifically warns that side-effects will be discarded.

[ldionne]

In D122397#3406135, @philnik wrote:

I'm pretty sure it doesn't evaluate at compile-time. https://godbolt.org/z/8d7fh8nc7 clang specifically warns that side-effects will be discarded.

Yup, I just noticed that while implementing your suggestion too!

[ldionne]

Address review comments.

[Mordante]

In D122397#3406074, @ldionne wrote:

In D122397#3405618, @philnik wrote:

Maybe it would make sense to use __builtin_assume if assertions are disabled?

My understanding is that __builtin_assume(x) does not evaluate x ever at runtime, but the documentation is a little bit light on the details. I think this is a fantastic idea, I'll try it out. If __builtin_assume does evaluate x, I added a test that should fail in my assertions handler patch.

Clang's documentation is truncated. Based on my archeology (D122423) the last sentence should read
The argument itself is never evaluated, so any side effects of the expression will be discarded.

We already claim that the assertions should be fatal and returning from the assertion handler is undefined behaviour. Therefore I think using __builtin_assume isn't wrong. I only wonder whether we should add an opt-out for vendors who don't like possible undefined behaviour of __builtin_assume in case the code hits an assertion that's wrong. (Much like the false positives in the Clang assertions.)

The original patch LGTM, but I would like you to investigate @philnik's suggestion further.

[Mordante]

Note I started writing my previous message before @philnik's second comment was posted.

[philnik]

__builtin_assume seems to ignore most of our asserts, but it definitely doesn't hurt. (https://godbolt.org/z/W7Gb43cx5). LGTM.

[Mordante]

In D122397#3406340, @philnik wrote:

__builtin_assume seems to ignore most of our asserts, but it definitely doesn't hurt. (https://godbolt.org/z/W7Gb43cx5). LGTM.

It might hurt when the assertion is wrong and the user builds without assertions.
https://godbolt.org/z/hsxhr9bWY
test1 validates the pointer and test2 doesn't since we told the compiler the if is always true.

Note that I'm not against doing this. I only wonder whether or not we need an opt-out for vendors/users.

[ldionne]

In D122397#3406376, @Mordante wrote:

In D122397#3406340, @philnik wrote:

__builtin_assume seems to ignore most of our asserts, but it definitely doesn't hurt. (https://godbolt.org/z/W7Gb43cx5). LGTM.

It might hurt when the assertion is wrong and the user builds without assertions.
https://godbolt.org/z/hsxhr9bWY
test1 validates the pointer and test2 doesn't since we told the compiler the if is always true.

Note that I'm not against doing this. I only wonder whether or not we need an opt-out for vendors/users.

IMO our assertions should simply not be wrong, otherwise we have a whole other problem when assertions are enabled. Assertions already encode things that, if they aren't satisfied, then the code exhibits UB. I'm not sure there is a lot of value in dialling how messed up the UB is going to be, so I'd suggest going for simplicity and adding a escape hatch if we have a reason to later on. And to that -- if someone came and asked for a escape hatch, I'd immediately ask them why they are not turning assertions on instead.

[ldionne]

Workaround test failure. Unfortunately I don't know how to fix the test without disabling it and without removing the assertion from vector::swap altogether. IMO keeping the assertion has more value than being robust against ADL in that case.

[Mordante]

In D122397#3407820, @ldionne wrote:

In D122397#3406376, @Mordante wrote:

In D122397#3406340, @philnik wrote:

__builtin_assume seems to ignore most of our asserts, but it definitely doesn't hurt. (https://godbolt.org/z/W7Gb43cx5). LGTM.

It might hurt when the assertion is wrong and the user builds without assertions.
https://godbolt.org/z/hsxhr9bWY
test1 validates the pointer and test2 doesn't since we told the compiler the if is always true.

Note that I'm not against doing this. I only wonder whether or not we need an opt-out for vendors/users.

IMO our assertions should simply not be wrong, otherwise we have a whole other problem when assertions are enabled. Assertions already encode things that, if they aren't satisfied, then the code exhibits UB. I'm not sure there is a lot of value in dialling how messed up the UB is going to be, so I'd suggest going for simplicity and adding a escape hatch if we have a reason to later on. And to that -- if someone came and asked for a escape hatch, I'd immediately ask them why they are not turning assertions on instead.

I agree assertions should always be right. But in my experience, bugs happen, and assertions can be wrong.
However I don't feel very strongly about this, I mainly want to have it considered. So I'm not objecting
against the simple approach without an escape hatch.

[ldionne]

Fix CI failure

[aeubanks]

Just a heads up, we've bisected some leaks to this change. There are some calls to operator new that aren't be optimized away because they're now the subject of various llvm.assume() calls. Still investigating.

[ldionne]

In D122397#3427001, @aeubanks wrote:

Just a heads up, we've bisected some leaks to this change. There are some calls to operator new that aren't be optimized away because they're now the subject of various llvm.assume() calls. Still investigating.

Ugh, thanks for the heads up. Please keep me in the loop (here or ideally in Discord so I don't miss it). I'm open to removing __builtin_assume until the issue is fixed if it gets to that.

[aeubanks]

In D122397#3427029, @ldionne wrote:

In D122397#3427001, @aeubanks wrote:

Just a heads up, we've bisected some leaks to this change. There are some calls to operator new that aren't be optimized away because they're now the subject of various llvm.assume() calls. Still investigating.

Ugh, thanks for the heads up. Please keep me in the loop (here or ideally in Discord so I don't miss it). I'm open to removing __builtin_assume until the issue is fixed if it gets to that.

I can't share the preprocessed source code but I did reduce the issue down to missed DSE (https://github.com/llvm/llvm-project/blob/3b9833597e810d4c485487d2f094a8e223af5548/llvm/lib/Analysis/CaptureTracking.cpp#L375) causing the call to operator new to not get removed

@global = external constant i8

define void @f() {
  %tmp1 = tail call noalias i8* @_Znwm(i64 noundef 32)
  %tmp2 = icmp ugt i8* %tmp1, @global
  ; this assume isn't even necessary to stop DSE, just the icmp
  ;tail call void @llvm.assume(i1 %tmp2)
  store i8 0, i8* %tmp1, align 1
  ret void
}

declare i8* @_Znwm(i64)

declare void @llvm.assume(i1)

So __builtin_assume is causing worse codegen in some cases. I think putting this under a flag would be nice while we figure out remaining regressions caused by more __builtin_assume.

In our case I believe we actually do have a leak in some sense in the original code which was previously optimized away. (although whether that's a real leak depends on your definition of a leak, see https://discourse.llvm.org/t/eliminating-global-memory-roots-or-not-to-help-leak-checkers/58066, anyway that's drifting too far from this issue)

[aeubanks]

https://reviews.llvm.org/D123175 to add a macro that turns off calling __builtin_assume

[aeubanks]

were there any benchmarks showing that this was beneficial? as discussed in https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609, sprinkling assumes everywhere is not necessarily a worthwhile tradeoff and perhaps the default should be to not add assumes everywhere

libcxx/include/__assert

 # define _LIBCPP_ENABLE_ASSERTIONS _LIBCPP_ENABLE_ASSERTIONS_DEFAULT
 #endif
 
 #if _LIBCPP_ENABLE_ASSERTIONS != 0 && _LIBCPP_ENABLE_ASSERTIONS != 1
 # error "_LIBCPP_ENABLE_ASSERTIONS must be set to 0 or 1"
 #endif
 
 #if _LIBCPP_ENABLE_ASSERTIONS
-# define _LIBCPP_ASSERT(expression, message) ((expression) ? (void)0 : ::std::__libcpp_assertion_handler(__FILE__, __LINE__, #expression, message))
+# define _LIBCPP_ASSERT(expression, message)                                        \
+    (__builtin_expect(static_cast<bool>(expression), 1) ?                           \
+      (void)0 :                                                                     \
+      ::std::__libcpp_assertion_handler(__FILE__, __LINE__, #expression, message))
 #else
-# define _LIBCPP_ASSERT(x, m) ((void)0)
+# if __has_builtin(__builtin_assume)
+#   define _LIBCPP_ASSERT(expression, message)                                      \
+      (_LIBCPP_DIAGNOSTIC_PUSH                                                      \
+      _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wassume")                                  \
+      __builtin_assume(static_cast<bool>(expression))                               \
+      _LIBCPP_DIAGNOSTIC_POP)
+# else
+#   define _LIBCPP_ASSERT(expression, message) ((void)0)
+# endif
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 _LIBCPP_OVERRIDABLE_FUNC_VIS _LIBCPP_AVAILABILITY_ASSERTION_HANDLER
 void __libcpp_assertion_handler(char const* __file, int __line, char const* __expression, char const* __message);
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ASSERT

libcxx/test/libcxx/assertions/single_expression.sh.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// Make sure that _LIBCPP_ASSERT is a single expression. This is useful so we can use
+// it in places that require an expression, such as in a constructor initializer list.
+
+// RUN: %{build} -Wno-macro-redefined -D_LIBCPP_ENABLE_ASSERTIONS=1
+// RUN: %{run}
+
+// RUN: %{build} -Wno-macro-redefined -D_LIBCPP_ENABLE_ASSERTIONS=0
+// RUN: %{run}
+
+// We flag uses of the assertion handler in older dylibs at compile-time to avoid runtime
+// failures when back-deploying.
+// UNSUPPORTED: use_system_cxx_lib && target={{.+}}-apple-macosx{{10.9|10.10|10.11|10.12|10.13|10.14|10.15|11|12}}
+
+#include <__assert>
+#include <cassert>
+
+void f() {
+  int i = (_LIBCPP_ASSERT(true, "message"), 3);
+  assert(i == 3);
+  return _LIBCPP_ASSERT(true, "message");
+}
+
+int main(int, char**) {
+  f();
+  return 0;
+}

libcxx/test/libcxx/containers/sequences/vector/robust_against_adl.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
-// UNSUPPORTED: debug_level=0, debug_level=1, libcpp-has-assertions
 
 // <vector>
 
 #include <vector>
 
 #include "test_macros.h"
 
 struct Incomplete;
int main(int, char**)
     std::vector<int, MyAlloc<int>> w(100);
     v.push_back(1);
     v.insert(v.end(), 2);
     v.insert(v.end(), w.begin(), w.end());
     v.pop_back();
     v.erase(v.begin());
     v.erase(v.begin(), v.end());
 #if TEST_STD_VER >= 14
-    v.swap(w);
+    // TODO: vector::swap is not robust against ADL because we compare allocators, and that
+    //       triggers ADL when looking up operator==.
+    // v.swap(w);
 #endif
     return 0;
 }