[libcxx-commits] [libcxx] [libc++] Make _LIBCPP_ASSUME usable when it is appropriate (PR #91801)

[Louis Dionne via libcxx-commits]

https://github.com/ldionne updated https://github.com/llvm/llvm-project/pull/91801

>From f0eabdcb1c861123996108f87d2ed81656d0ec21 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben at google.com>
Date: Fri, 10 May 2024 16:07:57 -0400
Subject: [PATCH] Make _LIBCPP_ASSUME usable when it is appropriate

libc++ turned off _LIBCPP_ASSUME because turning every debug assert into
__builtin_assume tripped
https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609

However, this means we can't use _LIBCPP_ASSUME when there is a clear
optimization intent. See
https://github.com/llvm/llvm-project/pull/78929#issuecomment-1936582711
for discussion of a place where _LIBCPP_ASSUME would be valuable.

Go ahead and fix this now, and adjust the comments. I don't think we
want _LIBCPP_ASSUME in disabled asserts anyway, at least not in any of
the hardened modes. This PR should have no impact on libc++ right
now, since _LIBCPP_ASSUME is currently never called standalone, but I
figure I can do this as a standalone PR since it's pretty
straightforward.
---
 libcxx/include/__assert | 59 ++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/libcxx/include/__assert b/libcxx/include/__assert
index 49769fb4d44978..3847a47558a726 100644
--- a/libcxx/include/__assert
+++ b/libcxx/include/__assert
@@ -23,10 +23,10 @@
        : _LIBCPP_ASSERTION_HANDLER(__FILE__ ":" _LIBCPP_TOSTRING(__LINE__) ": assertion " _LIBCPP_TOSTRING(            \
              expression) " failed: " message "\n"))
 
-// TODO: __builtin_assume can currently inhibit optimizations. Until this has been fixed and we can add
-//       assumptions without a clear optimization intent, disable that to avoid worsening the code generation.
-//       See https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609 for a discussion.
-#if 0 && __has_builtin(__builtin_assume)
+// WARNING: __builtin_assume can currently inhibit optimizations. Only add assumptions with a clear
+// optimization intent. See https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609 for a
+// discussion.
+#if __has_builtin(__builtin_assume)
 #  define _LIBCPP_ASSUME(expression)                                                                                   \
     (_LIBCPP_DIAGNOSTIC_PUSH _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wassume")                                              \
          __builtin_assume(static_cast<bool>(expression)) _LIBCPP_DIAGNOSTIC_POP)
@@ -34,6 +34,9 @@
 #  define _LIBCPP_ASSUME(expression) ((void)0)
 #endif
 
+// Historically, disabled assertions below expanded to `_LIBCPP_ASSUME`, but this both triggers the
+// issue described above, and also causes every debug assertion to be a safety risk.
+
 // clang-format off
 // Fast hardening mode checks.
 
@@ -44,18 +47,18 @@
 #  define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message)    _LIBCPP_ASSERT(expression, message)
 // Disabled checks.
 // On most modern platforms, dereferencing a null pointer does not lead to an actual memory access.
-#  define _LIBCPP_ASSERT_NON_NULL(expression, message)                _LIBCPP_ASSUME(expression)
+#  define _LIBCPP_ASSERT_NON_NULL(expression, message)                ((void)0)
 // Overlapping ranges will make algorithms produce incorrect results but don't directly lead to a security
 // vulnerability.
-#  define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message)  _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_VALID_DEALLOCATION(expression, message)      _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(expression, message) _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message)    _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message)  _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_PEDANTIC(expression, message)                _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message)    _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_INTERNAL(expression, message)                _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message)           _LIBCPP_ASSUME(expression)
+#  define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message)  ((void)0)
+#  define _LIBCPP_ASSERT_VALID_DEALLOCATION(expression, message)      ((void)0)
+#  define _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(expression, message) ((void)0)
+#  define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message)    ((void)0)
+#  define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message)  ((void)0)
+#  define _LIBCPP_ASSERT_PEDANTIC(expression, message)                ((void)0)
+#  define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message)    ((void)0)
+#  define _LIBCPP_ASSERT_INTERNAL(expression, message)                ((void)0)
+#  define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message)           ((void)0)
 
 // Extensive hardening mode checks.
 
@@ -73,8 +76,8 @@
 #  define _LIBCPP_ASSERT_PEDANTIC(expression, message)                _LIBCPP_ASSERT(expression, message)
 #  define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message)           _LIBCPP_ASSERT(expression, message)
 // Disabled checks.
-#  define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message)    _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_INTERNAL(expression, message)                _LIBCPP_ASSUME(expression)
+#  define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message)    ((void)0)
+#  define _LIBCPP_ASSERT_INTERNAL(expression, message)                ((void)0)
 
 // Debug hardening mode checks.
 
@@ -99,18 +102,18 @@
 #else
 
 // All checks disabled.
-#  define _LIBCPP_ASSERT_VALID_INPUT_RANGE(expression, message)       _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message)    _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_NON_NULL(expression, message)                _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message)  _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_VALID_DEALLOCATION(expression, message)      _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(expression, message) _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message)    _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message)  _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_PEDANTIC(expression, message)                _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message)    _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_INTERNAL(expression, message)                _LIBCPP_ASSUME(expression)
-#  define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message)           _LIBCPP_ASSUME(expression)
+#  define _LIBCPP_ASSERT_VALID_INPUT_RANGE(expression, message)       ((void)0)
+#  define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message)    ((void)0)
+#  define _LIBCPP_ASSERT_NON_NULL(expression, message)                ((void)0)
+#  define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message)  ((void)0)
+#  define _LIBCPP_ASSERT_VALID_DEALLOCATION(expression, message)      ((void)0)
+#  define _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(expression, message) ((void)0)
+#  define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message)    ((void)0)
+#  define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message)  ((void)0)
+#  define _LIBCPP_ASSERT_PEDANTIC(expression, message)                ((void)0)
+#  define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message)    ((void)0)
+#  define _LIBCPP_ASSERT_INTERNAL(expression, message)                ((void)0)
+#  define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message)           ((void)0)
 
 #endif // _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_FAST
 // clang-format on