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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions libcxx/docs/ReleaseNotes/20.rst
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,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
-------------------------
Expand Down
18 changes: 16 additions & 2 deletions libcxx/include/__assert
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@
# pragma GCC system_header
#endif

#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
Comment on lines +20 to +27
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.


#define _LIBCPP_ASSERT(expression, message) \
(__builtin_expect(static_cast<bool>(expression), 1) \
? (void)0 \
Expand All @@ -27,13 +36,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.

Expand Down
6 changes: 5 additions & 1 deletion libcxx/include/experimental/__simd/vec_ext.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ inline constexpr bool is_abi_tag_v<simd_abi::__vec_ext<_Np>> = _Np > 0 && _Np <=

template <class _Tp, int _Np>
struct __simd_storage<_Tp, simd_abi::__vec_ext<_Np>> {
_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 __n_bytes = std::__bit_ceil(sizeof(_Tp) * _Np);

_Tp __data __attribute__((__vector_size__(__n_bytes)));

_LIBCPP_HIDE_FROM_ABI _Tp __get(size_t __idx) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__idx < _Np, "Index is out of bounds");
Expand Down
24 changes: 24 additions & 0 deletions libcxx/test/libcxx/assertions/constant_expression.verify.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//===----------------------------------------------------------------------===//
//
// 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}}
Comment on lines +17 to +21
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.


static_assert(!__builtin_constant_p(_LIBCPP_ASSERT(false, "message")), "");
static_assert(!__builtin_constant_p(_LIBCPP_ASSUME(false)), "");
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,12 @@ constexpr bool test() {
return value;
};
assert(std::ranges::clamp(3, 2, 4, std::ranges::less{}, projection_function) == 3);
// When assertions are enabled, we call the projection more times
#if defined(_LIBCPP_HARDENING_MODE) && \
_LIBCPP_HARDENING_MODE != _LIBCPP_HARDENING_MODE_EXTENSIVE && \
_LIBCPP_HARDENING_MODE != _LIBCPP_HARDENING_MODE_DEBUG
assert(counter <= 3);
if (!std::__libcpp_is_constant_evaluated())
assert(counter <= 3);
#endif
}

Expand Down
4 changes: 2 additions & 2 deletions libcxx/test/support/nasty_string.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@
#ifndef TEST_HAS_NO_NASTY_STRING
// Make sure the char-like operations in strings do not depend on the char-like type.
struct nasty_char {
template <typename T>
friend auto operator<=>(T, T) = delete;
template <typename T, typename U>
friend auto operator<=>(T, U) = delete;
Comment on lines +43 to +44
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.


template <typename T>
friend void operator+(T&&) = delete;
Expand Down
Loading