[libc++][hardening] Always enable all checks during constant evaluation #107713
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Mital Ashok (MitalAshok) ChangesChange Also changes Fixes #107453 Full diff: https://github.com/llvm/llvm-project/pull/107713.diff 3 Files Affected:
diff --git a/libcxx/docs/ReleaseNotes/20.rst b/libcxx/docs/ReleaseNotes/20.rst
index 93d6027291ad95..875b0a361576a9 100644
--- a/libcxx/docs/ReleaseNotes/20.rst
+++ b/libcxx/docs/ReleaseNotes/20.rst
@@ -51,6 +51,8 @@ Improvements and New Features
- The ``_LIBCPP_ENABLE_CXX20_REMOVED_UNCAUGHT_EXCEPTION`` macro has been added to make ``std::uncaught_exception`` available in C++20 and later modes.
+- ``_LIBCPP_ASSUME(expression)`` now checks the expression is ``true`` during constant evaluation.
+ This means that all assertions are now checked regardless of hardening mode in constant expressions.
Deprecations and Removals
-------------------------
diff --git a/libcxx/include/__assert b/libcxx/include/__assert
index 49769fb4d44978..d422a1ce7ae800 100644
--- a/libcxx/include/__assert
+++ b/libcxx/include/__assert
@@ -17,9 +17,14 @@
# pragma GCC system_header
#endif
+#define _LIBCPP_ASSERT_IS_CONSTANT_EVALUATED \
+ (_LIBCPP_DIAGNOSTIC_PUSH _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wconstant-evaluated") __builtin_is_constant_evaluated() \
+ _LIBCPP_DIAGNOSTIC_POP)
+
#define _LIBCPP_ASSERT(expression, message) \
- (__builtin_expect(static_cast<bool>(expression), 1) \
- ? (void)0 \
+ (__builtin_expect(static_cast<bool>(expression), 1) ? (void)0 \
+ : _LIBCPP_ASSERT_IS_CONSTANT_EVALUATED \
+ ? __builtin_unreachable() \
: _LIBCPP_ASSERTION_HANDLER(__FILE__ ":" _LIBCPP_TOSTRING(__LINE__) ": assertion " _LIBCPP_TOSTRING( \
expression) " failed: " message "\n"))
@@ -27,13 +32,18 @@
// 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)
-# define _LIBCPP_ASSUME(expression) \
+# define _LIBCPP_RUNTIME_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)
+# define _LIBCPP_RUNTIME_ASSUME(expression) ((void)0)
#endif
+#define _LIBCPP_ASSUME(expression) \
+ (_LIBCPP_ASSERT_IS_CONSTANT_EVALUATED \
+ ? static_cast<bool>(expression) ? (void)0 : __builtin_unreachable() \
+ : _LIBCPP_RUNTIME_ASSUME(expression))
+
// clang-format off
// Fast hardening mode checks.
diff --git a/libcxx/test/libcxx/assertions/constant_expression.verify.cpp b/libcxx/test/libcxx/assertions/constant_expression.verify.cpp
new file mode 100644
index 00000000000000..93ed0b595de57f
--- /dev/null
+++ b/libcxx/test/libcxx/assertions/constant_expression.verify.cpp
@@ -0,0 +1,61 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 both `_LIBCPP_ASSERT(false, ...)` and `_LIBCPP_ASSUME(false)`
+// mean that a constant expression cannot be formed.
+
+#include <__assert>
+#include "test_macros.h"
+
+// expected-note@*:* 0+ {{expanded from macro}}
+
+static_assert((_LIBCPP_ASSERT(false, "message"), true), "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+
+static_assert((_LIBCPP_ASSUME(false), true), "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+
+const int i = (_LIBCPP_ASSERT(false, "message"), 1);
+const int j = (_LIBCPP_ASSUME(false), 1);
+
+static_assert(i, "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+static_assert(j, "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+
+#if TEST_STD_VER >= 11
+constexpr bool f() {
+ return (_LIBCPP_ASSERT(false, "message"), true);
+ // expected-note@-1 {{subexpression not valid in a constant expression}}
+}
+constexpr bool g() {
+ return (_LIBCPP_ASSUME(false), true);
+ // expected-note@-1 {{subexpression not valid in a constant expression}}
+}
+static_assert(f(), "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+static_assert(g(), "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+#endif
+
+#if TEST_STD_VER >= 14
+constexpr bool ff() {
+ _LIBCPP_ASSERT(false, "message");
+ // expected-note@-1 {{subexpression not valid in a constant expression}}
+ return true;
+}
+constexpr bool gg() {
+ _LIBCPP_ASSUME(false);
+ // expected-note@-1 {{subexpression not valid in a constant expression}}
+ return true;
+}
+static_assert(ff(), "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+static_assert(gg(), "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+#endif
|
|
You can view the diagnostic messages online: https://godbolt.org/z/GeeM1GcqM |
MitalAshok
force-pushed
the
libcxx-constant-eval-assume
branch
from
cbe2a7c
to
6be148c
Compare
It was originally added so that the assertion message would be in the constexpr call stack but Clang just shows the source location, including the line number, expression and message anyways
…ating attribute argument
MitalAshok
force-pushed
the
libcxx-constant-eval-assume
branch
from
6be148c
to
006e98d
Compare
| #ifdef _LIBCPP_COMPILER_CLANG_BASED | ||
| // TODO: use `_LIBCPP_DIAGNOSTIC_*` macros after #107715 is fixed in all supported clang compilers | ||
| # define _LIBCPP_ASSERT_IS_CONSTANT_EVALUATED \ | ||
| (_Pragma("clang diagnostic push") _Pragma("clang diagnostic ignored \"-Wconstant-evaluated\"") \ | ||
| __builtin_is_constant_evaluated() _Pragma("clang diagnostic pop")) | ||
| #else | ||
| # define _LIBCPP_ASSERT_IS_CONSTANT_EVALUATED (__builtin_is_constant_evaluated()) | ||
| #endif |
There was a problem hiding this comment.
What exactly are you guarding against here?
There was a problem hiding this comment.
#107715 means that an unguarded:
#define _LIBCPP_ASSERT_IS_CONSTANT_EVALUATED \
(_LIBCPP_DIAGNOSTIC_PUSH _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wconstant-evaluated") \
__builtin_is_constant_evaluated() _LIBCPP_DIAGNOSTIC_POP)doesn't actually ignore the diagnostic (The diagnostic being an assertion used in a consteval function or the true branch of a consteval if or some other manifestly constant evaluated context)
There was a problem hiding this comment.
I meant where this is diagnosed.
There was a problem hiding this comment.
This would be diagnosed if used in a manifestly constant evaluated context. This is mainly in an if consteval branch or consteval function. No if consteval currently exists in libc++ and no consteval libc++ functions contain any assertions, but in the future they might.
Also more exotic cases, like when the assertion is directly in a constinit initialiser, array bound or static_assert, like in the added test. But these will never happen in real libc++ code.
libcxx/include/__assert
Outdated
| : _LIBCPP_ASSERT_IS_CONSTANT_EVALUATED \ | ||
| ? __builtin_unreachable() \ |
There was a problem hiding this comment.
Why is this better than padding through to the assertion handler?
There was a problem hiding this comment.
The original idea was the diagnostic would be consistent, but on second thoughts that isn't worth the extra complexity. I'll revert this.
| _Tp __data __attribute__((__vector_size__(std::__bit_ceil((sizeof(_Tp) * _Np))))); | ||
| // This doesn't work in GCC if it is directly inside the __vector_size__ attribute because of a call to | ||
| // __builtin_is_constant_evaluated. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105233 | ||
| static constexpr size_t __vector_size = std::__bit_ceil(sizeof(_Tp) * _Np); |
There was a problem hiding this comment.
I think it's very confusing to call a variable __vector_size to then pass it to an attribute __vector_size__.
There was a problem hiding this comment.
The argument to __attribute__((vector_size(...))) is called the "vector size" (https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html), so this seems like the only natural name. Maybe __n_bytes since it's also the size in bytes?
| static_assert((_LIBCPP_ASSERT(false, "message"), true), ""); | ||
| // expected-error@-1 {{static assertion expression is not an integral constant expression}} | ||
|
|
||
| static_assert((_LIBCPP_ASSUME(false), true), ""); | ||
| // expected-error@-1 {{static assertion expression is not an integral constant expression}} |
There was a problem hiding this comment.
I think it's enough to have a single check that asserts always fire during constant evaluation. There's no way to differentiate between the different ways of getting into constant evaluation AFAIK. We can also use static_assert(!__builtin_constant_p(_LIBCPP_ASSERT(false, "")), "") to check that it in fact fires instead.
libcxx/test/support/nasty_string.h
Outdated
| // TODO re-enable after #107747 is fixed | ||
| #ifndef TEST_HAS_NO_NASTY_STRING | ||
| # define TEST_HAS_NO_NASTY_STRING | ||
| #endif |
There was a problem hiding this comment.
This should be reverted, see the bug.
|
What is the motivation for this change? The main purpose of hardening is to eliminate undefined behavior, especially "harmful" undefined behavior. IIRC, theoretically there is no (language-level) undefined behavior in constant expression. In practice, the compiler might have some omissions in its checks, and it also won't check for certain library undefined behavior, but in general the usefulness of hardening during constant evaluation seems much less compared to runtime. Do you have any particular cases in mind where enabling a hardening check in a constant expression significantly improves the user experience? |
|
@var-const I think |
There was a problem hiding this comment.
I would like us to settle on #91801 first.
My mental model is that hardening is a configuration setting of its own, and I'm not sure that coupling it with something else is a good idea. If hardening is enabled and you run into library-UB inside a constant expression, then you'd get the hardening check. But I'm not certain that forcing the hardening checks on people just because they are in a constant expression makes sense.
This is both philosophical and practical. I wouldn't expect that running code in a constant expression behaves any differently than running it at runtime (except where strictly necessary because we can't actually implement X or Y inside constexpr). Making these two things non-orthogonal (when they are naturally orthogonal) reduces the generality of the library.
On the flipside, it is true that the compiler behaves differently w.r.t. UB inside constant expressions (ignored at runtime, diagnosed at compile-time), so one could argue that this makes the library and the compiler behavior more consistent.
Either way, I feel like piggy-backing on _LIBCPP_ASSUME isn't what we really want to do here if we move forward with the patch, given that #91801 makes a lot of sense but it would conflict with this patch.
| template <typename T, typename U> | ||
| friend auto operator<=>(T, U) = delete; |
There was a problem hiding this comment.
This change looks a bit like something that should be in a separate patch. Is this change required as part of this patch?
There was a problem hiding this comment.
Some tests fail with the unstable ABI (See #107747). This is the easiest remedy to make std::__wrap_iter<nasty_string*> a Cpp17RandomAccessIterator (otherwise this overload is found for it <= it) which some assertions rely on
It would make sense as a separate patch too, since these tests currently fail with unstable ABI + hardening right now.
|
IMO one of the most useful perks of constant evaluating this is that you can be sure there is no UB in your code. Adding compile time only assertions that catch UB in the library seems like a natural extension of this utility. Of course we have a few places where we throw that utility away because we know that we're doing stuff that's technically UB but compilers don't optimize on, but these are relatively rare and quite obvious. |
|
At least in my mind, the primary benefit of hardening is preventing security vulnerabilities, and to a lesser extent logic bugs; I don't see it as a general-purpose bug-finding tool. I think it's useful to distinguish between library-level and language-level undefined behavior, but at the same time a lot, perhaps most, of library-level UB will actually manifest as language-level UB (especially the most harmful cases); thus, in a mode where (at least aspirationally) language-level UB cannot happen, the benefits of checking for library-level UB are significantly reduced. This approach also has a compile-time cost and no way to turn it off. I also think that conceptually, hardening and constant evaluation are completely orthogonal and shouldn't be tied together, especially in an implicit way. I would consider an optional opt-in switch that enables hardening in |
|
@MitalAshok In light of the latest comments, do you still want to pursue this patch? |
Change
_LIBCPP_ASSUMEto test the expression when__builtin_is_constant_evaluated(), calling__builtin_unreachable()when it isfalse. This will cause it to not be a constant expression.Also changes
_LIBCPP_ASSERTto do the same when__builtin_is_constant_evaluated(). This is so the diagnostic will be consistent.Fixes #107453