From f2cfe68708c3be80e52221961155c91600cd1877 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 10 May 2024 16:07:57 -0400 Subject: [PATCH 1/2] 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 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(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 From a01582aff2f7916d0ddc6a05a4baa9f0e8e5784f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 17 Sep 2024 11:12:55 -0400 Subject: [PATCH 2/2] Remove comment --- libcxx/include/__assert | 3 --- 1 file changed, 3 deletions(-) diff --git a/libcxx/include/__assert b/libcxx/include/__assert index 3847a47558a72..90eaa6023587b 100644 --- a/libcxx/include/__assert +++ b/libcxx/include/__assert @@ -34,9 +34,6 @@ # 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.