[libc] Add __builtin_expect tag on assert conditions; NFC

[goldsteinn]

No description provided.

[llvmbot]

@llvm/pr-subscribers-libc

Author: None (goldsteinn)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/99498.diff

2 Files Affected:

• (modified) libc/src/__support/libc_assert.h (+1-1)
• (modified) libc/src/assert/assert.h (+3-3)

diff --git a/libc/src/__support/libc_assert.h b/libc/src/__support/libc_assert.h
index e3235199780c2..8bbd06c14fb66 100644
--- a/libc/src/__support/libc_assert.h
+++ b/libc/src/__support/libc_assert.h
@@ -71,7 +71,7 @@ LIBC_INLINE void report_assertion_failure(const char *assertion,
 
 #define LIBC_ASSERT(COND)                                                      \
   do {                                                                         \
-    if (!(COND)) {                                                             \
+    if (LIBC_UNLIKELY(!(COND))) {                                              \
       LIBC_NAMESPACE::write_to_stderr(__FILE__ ":" __LIBC_LINE_STR__           \
                                                ": Assertion failed: '" #COND   \
                                                "' in function: '");            \
diff --git a/libc/src/assert/assert.h b/libc/src/assert/assert.h
index 6f352af1988b3..a1104a79bfc3e 100644
--- a/libc/src/assert/assert.h
+++ b/libc/src/assert/assert.h
@@ -19,7 +19,7 @@
 #define assert(e) (void)0
 #else
 #define assert(e)                                                              \
-  ((e) ? (void)0                                                               \
-       : LIBC_NAMESPACE::__assert_fail(#e, __FILE__, __LINE__,                 \
-                                       __PRETTY_FUNCTION__))
+  (LIBC_LIKELY(e) ? (void)0                                                    \
+                  : LIBC_NAMESPACE::__assert_fail(#e, __FILE__, __LINE__,      \
+                                                  __PRETTY_FUNCTION__))
 #endif // NDEBUG

[jhuber6]

I remember discussing something similar with @AaronBallman about adding __builtin_assume to the assert macros. I think the answer then was that the C standard specifies exactly what assert should look like so we can't change it? I think LIBC_ASSERT probably should though.

[goldsteinn]

I remember discussing something similar with @AaronBallman about adding __builtin_assume to the assert macros. I think the answer then was that the C standard specifies exactly what assert should look like so we can't change it? I think LIBC_ASSERT probably should though.

__builtin_assume is a bit different, no? This is properly NFC, it just has an off chance of helping codegen.
I don't think any standard says much about how assert is implemented other than it will abort and maybe write something to stderr (and I guess the linux reference saying __assert_fail is defined) but this doesn't change any of those things.

[jhuber6]

I remember discussing something similar with @AaronBallman about adding __builtin_assume to the assert macros. I think the answer then was that the C standard specifies exactly what assert should look like so we can't change it? I think LIBC_ASSERT probably should though.

__builtin_assume is a bit different, no? This is properly NFC, it just has an off chance of helping codegen. I don't think any standard says much about how assert is implemented other than it will abort and maybe write something to stderr (and I guess the linux reference saying __assert_fail is defined) but this doesn't change any of those things.

Nope -- you cannot expand the argument given to assert if NDEBUG is defined (also, 7.2p1 defines what assert is required to expand to in this case.)

Was the response I got earlier, but I guess that only applies to making #define assert(cond) __builtin_assume(!cond); so this might be fine.

[AaronBallman]

I remember discussing something similar with @AaronBallman about adding __builtin_assume to the assert macros. I think the answer then was that the C standard specifies exactly what assert should look like so we can't change it? I think LIBC_ASSERT probably should though.

__builtin_assume is a bit different, no? This is properly NFC, it just has an off chance of helping codegen. I don't think any standard says much about how assert is implemented other than it will abort and maybe write something to stderr (and I guess the linux reference saying __assert_fail is defined) but this doesn't change any of those things.

Nope -- you cannot expand the argument given to assert if NDEBUG is defined (also, 7.2p1 defines what assert is required to expand to in this case.)

Was the response I got earlier, but I guess that only applies to making #define assert(cond) __builtin_assume(!cond); so this might be fine.

Yeah, I think this is okay. One thing I did note when I reviewed the code is that the definition of assert changed in C23, it's now defined as assert(...) rather than assert(e), see https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2829.htm for details.

[nickdesaulniers]

Whats the status of this PR? Looks like it's good to go?

[goldsteinn]

Whats the status of this PR? Looks like it's good to go?

Still waiting on approval.

[llvm-ci]

LLVM Buildbot has detected a new failure on builder libc-riscv32-qemu-yocto-fullbuild-dbg running on rv32gc-qemu-system while building libc at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/196/builds/119

Here is the relevant piece of the build log for the reference

Step 4 (annotate) failure: 'python ../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py ...' (failure) (timed out)
...
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcDifftime.SmokeTest
[       OK ] LlvmLibcDifftime.SmokeTest (2 ms)
Ran 1 tests.  PASS: 1  FAIL: 0
[2884/2888] Running unit test libc.test.src.time.nanosleep_test.__unit__
sh: line 1: /timer.17550: Permission denied
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcNanosleep.SmokeTest
[       OK ] LlvmLibcNanosleep.SmokeTest (847 us)
Ran 1 tests.  PASS: 1  FAIL: 0
command timed out: 1200 seconds without output running [b'python', b'../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py', b'--debug'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=2060.851458
Step 8 (libc-unit-tests) failure: libc-unit-tests (failure)
...
[ RUN      ] LlvmLibcSignalTest.SigaltstackInvalidStack
[       OK ] LlvmLibcSignalTest.SigaltstackInvalidStack (266 us)
Ran 2 tests.  PASS: 2  FAIL: 0
[2880/2888] Running unit test libc.test.src.time.time_test
sh: line 1: /timer.17574: Permission denied
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcTimeTest.SmokeTest
[       OK ] LlvmLibcTimeTest.SmokeTest (1 ms)
Ran 1 tests.  PASS: 1  FAIL: 0
[2881/2888] Running unit test libc.test.src.time.ctime_r_test
sh: line 1: /timer.17568: Permission denied
[==========] Running 4 tests from 1 test suite.
[ RUN      ] LlvmLibcCtimeR.Nullptr
[       OK ] LlvmLibcCtimeR.Nullptr (527 us)
[ RUN      ] LlvmLibcCtimeR.ValidUnixTimestamp0
[       OK ] LlvmLibcCtimeR.ValidUnixTimestamp0 (1 ms)
[ RUN      ] LlvmLibcCtime.ValidUnixTimestamp32Int
[       OK ] LlvmLibcCtime.ValidUnixTimestamp32Int (68 us)
[ RUN      ] LlvmLibcCtimeR.InvalidArgument
[       OK ] LlvmLibcCtimeR.InvalidArgument (46 us)
Ran 4 tests.  PASS: 4  FAIL: 0
[2882/2888] Running unit test libc.test.src.pthread.pthread_condattr_test
sh: line 1: /timer.17575: Permission denied
[==========] Running 4 tests from 1 test suite.
[ RUN      ] LlvmLibcPThreadCondAttrTest.InitAndDestroy
[       OK ] LlvmLibcPThreadCondAttrTest.InitAndDestroy (284 us)
[ RUN      ] LlvmLibcPThreadCondAttrTest.GetDefaultValues
[       OK ] LlvmLibcPThreadCondAttrTest.GetDefaultValues (202 us)
[ RUN      ] LlvmLibcPThreadCondAttrTest.SetGoodValues
[       OK ] LlvmLibcPThreadCondAttrTest.SetGoodValues (425 us)
[ RUN      ] LlvmLibcPThreadCondAttrTest.SetBadValues
[       OK ] LlvmLibcPThreadCondAttrTest.SetBadValues (209 us)
Ran 4 tests.  PASS: 4  FAIL: 0
[2883/2888] Running unit test libc.test.src.time.difftime_test
sh: line 1: /timer.17579: Permission denied
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcDifftime.SmokeTest
[       OK ] LlvmLibcDifftime.SmokeTest (2 ms)
Ran 1 tests.  PASS: 1  FAIL: 0
[2884/2888] Running unit test libc.test.src.time.nanosleep_test.__unit__
sh: line 1: /timer.17550: Permission denied
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcNanosleep.SmokeTest
[       OK ] LlvmLibcNanosleep.SmokeTest (847 us)
Ran 1 tests.  PASS: 1  FAIL: 0

command timed out: 1200 seconds without output running [b'python', b'../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py', b'--debug'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=2060.851458