[libc++] Make _LIBCPP_ASSUME usable when it is appropriate #91801

davidben · 2024-05-10T20:25:17Z

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 #78929 (comment) 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.

llvmbot · 2024-05-10T20:25:53Z

@llvm/pr-subscribers-libcxx

Author: David Benjamin (davidben)

Changes

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 #78929 (comment) 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.

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

1 Files Affected:

(modified) libcxx/include/__assert (+31-28)

diff --git a/libcxx/include/__assert b/libcxx/include/__assert
index 49769fb4d4497..3847a47558a72 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

Fixes llvm#91634. This could alternatively be done with an _LIBCPP_ASSUME, after llvm#91801 lands, but would also require llvm#91619 be fixed first. Given the dependencies, it seemed simplest to just make a private ctor.

…1804) Fixes #91634. This could alternatively be done with an _LIBCPP_ASSUME, after #91801 lands, but would also require #91619 be fixed first. Given the dependencies, it seemed simplest to just make a private ctor.

ldionne

This makes sense to me. Deferring to @var-const for final approval, but at first glance I like this.

…1804) Summary: Fixes #91634. This could alternatively be done with an _LIBCPP_ASSUME, after #91801 lands, but would also require #91619 be fixed first. Given the dependencies, it seemed simplest to just make a private ctor. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251551

davidben · 2024-09-13T15:24:24Z

Friendly ping on this PR. This is a blocker to fixing #101370 which, in turn, is a blocker to making bounded iterators practical.

var-const

So sorry about the very slow review! LGTM.

var-const · 2024-09-17T05:47:03Z

libcxx/include/__assert

 #  define _LIBCPP_ASSUME(expression)                                                                                   \
    (_LIBCPP_DIAGNOSTIC_PUSH _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wassume")                                              \
         __builtin_assume(static_cast<bool>(expression)) _LIBCPP_DIAGNOSTIC_POP)
 #else
 #  define _LIBCPP_ASSUME(expression) ((void)0)
 #endif

+// Historically, disabled assertions below expanded to `_LIBCPP_ASSUME`, but this both triggers the


Nit: I'd remove this comment -- I understand the desire to make sure somebody doesn't reintroduce the previous state (which seems to make a lot of sense in theory but doesn't work in practice), but IMO the comment we have on _LIBCPP_ASSUME should be enough of a deterrence.

Removed and rebased.

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 llvm#78929 (comment) 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.

libc++ turned off _LIBCPP_ASSUME because turning every debug assert into __builtin_assume tripped [1]. However, this means we can't use _LIBCPP_ASSUME when there is a clear optimization intent. See [2] for discussion of a place where _LIBCPP_ASSUME would be valuable. This patch fixes this by not undefining the definition of _LIBCPP_ASSUME and making sure that we don't attempt to `_LIBCPP_ASSSUME` every assertion in the library. [1]: https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609 [2]: llvm#78929 (comment)

davidben requested a review from a team as a code owner May 10, 2024 20:25

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 10, 2024

This was referenced May 10, 2024

Suppress a redundant hardening check in basic_string_view::substr #91804

Merged

__builtin_assume with embedded function calls are ignored #91612

Closed

davidben mentioned this pull request May 20, 2024

[libc++][hardening] Use bounded iterators in std::vector and std::string #78929

Merged

ldionne approved these changes Jul 24, 2024

View reviewed changes

davidben mentioned this pull request Jul 31, 2024

_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR is not optimized out in range-for loops #101370

Open

ldionne changed the title ~~Make _LIBCPP_ASSUME usable when it is appropriate~~ [libc++] Make _LIBCPP_ASSUME usable when it is appropriate Sep 13, 2024

ldionne force-pushed the libcxx-assume branch from fe88aab to f0eabdc Compare September 13, 2024 13:53

ldionne mentioned this pull request Sep 13, 2024

[libc++][hardening] Always enable all checks during constant evaluation #107713

Open

var-const approved these changes Sep 17, 2024

View reviewed changes

davidben added 2 commits September 17, 2024 10:59

Remove comment

a01582a

davidben force-pushed the libcxx-assume branch from f0eabdc to a01582a Compare September 17, 2024 15:13

ldionne merged commit adeae92 into llvm:main Sep 17, 2024
6 of 12 checks passed

davidben deleted the libcxx-assume branch September 17, 2024 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Make _LIBCPP_ASSUME usable when it is appropriate #91801

[libc++] Make _LIBCPP_ASSUME usable when it is appropriate #91801

davidben commented May 10, 2024

llvmbot commented May 10, 2024

ldionne left a comment

davidben commented Sep 13, 2024

var-const left a comment

var-const Sep 17, 2024

davidben Sep 17, 2024

[libc++] Make _LIBCPP_ASSUME usable when it is appropriate #91801

[libc++] Make _LIBCPP_ASSUME usable when it is appropriate #91801

Conversation

davidben commented May 10, 2024

llvmbot commented May 10, 2024

ldionne left a comment

Choose a reason for hiding this comment

davidben commented Sep 13, 2024

var-const left a comment

Choose a reason for hiding this comment

var-const Sep 17, 2024

Choose a reason for hiding this comment

davidben Sep 17, 2024

Choose a reason for hiding this comment