Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MitalAshok
Copy link
Contributor

Change _LIBCPP_ASSUME to test the expression when __builtin_is_constant_evaluated(), calling __builtin_unreachable() when it is false. This will cause it to not be a constant expression.

Also changes _LIBCPP_ASSERT to do the same when __builtin_is_constant_evaluated(). This is so the diagnostic will be consistent.

Fixes #107453

@MitalAshok MitalAshok requested a review from a team as a code owner September 7, 2024 19:11
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 7, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 7, 2024

@llvm/pr-subscribers-libcxx

Author: Mital Ashok (MitalAshok)

Changes

Change _LIBCPP_ASSUME to test the expression when __builtin_is_constant_evaluated(), calling __builtin_unreachable() when it is false. This will cause it to not be a constant expression.

Also changes _LIBCPP_ASSERT to do the same when __builtin_is_constant_evaluated(). This is so the diagnostic will be consistent.

Fixes #107453


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

3 Files Affected:

  • (modified) libcxx/docs/ReleaseNotes/20.rst (+2)
  • (modified) libcxx/include/__assert (+14-4)
  • (added) libcxx/test/libcxx/assertions/constant_expression.verify.cpp (+61)
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

@MitalAshok
Copy link
Contributor Author

You can view the diagnostic messages online: https://godbolt.org/z/GeeM1GcqM

@MitalAshok MitalAshok marked this pull request as draft September 7, 2024 21:35
@MitalAshok MitalAshok marked this pull request as ready for review September 8, 2024 06:51
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
Comment on lines +20 to +27
#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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly are you guarding against here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant where this is diagnosed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 31 to 32
: _LIBCPP_ASSERT_IS_CONSTANT_EVALUATED \
? __builtin_unreachable() \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this better than padding through to the assertion handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's very confusing to call a variable __vector_size to then pass it to an attribute __vector_size__.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

libcxx/include/experimental/__simd/vec_ext.h Outdated Show resolved Hide resolved
Comment on lines +17 to +21
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}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 40 to 43
// TODO re-enable after #107747 is fixed
#ifndef TEST_HAS_NO_NASTY_STRING
# define TEST_HAS_NO_NASTY_STRING
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reverted, see the bug.

@var-const
Copy link
Member

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?

@philnik777
Copy link
Contributor

@var-const I think char_traits::copy is a great example. It's UB to pass overlapping ranges to it, but we currently happen to define the behaviour because we use memmove for its implementation. With this change such UB will be diagnosed during constant evaluation.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +43 to +44
template <typename T, typename U>
friend auto operator<=>(T, U) = delete;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks a bit like something that should be in a separate patch. Is this change required as part of this patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@philnik777
Copy link
Contributor

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.

@var-const var-const added the hardening Issues related to the hardening effort label Sep 24, 2024
@var-const
Copy link
Member

var-const commented Oct 6, 2024

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 constexpr regardless of the runtime setting (honestly, I don't think it's very valuable but I don't have a strong objection to that if others find it useful), but I don't think we should enable this unconditionally or even by default.

@ldionne
Copy link
Member

ldionne commented Oct 22, 2024

@MitalAshok In light of the latest comments, do you still want to pursue this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hardening Issues related to the hardening effort libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libc++] Enable assertions unconditionally during constant evaluation
5 participants