Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c115b14
[libc++][hardening] Always enable all checks during constant evaluation
MitalAshok Sep 7, 2024
817d019
Cut out function call
MitalAshok Sep 7, 2024
9320bb2
Fix test failures
MitalAshok Sep 7, 2024
c1c4505
Fix gcc -Werror=type-limits
MitalAshok Sep 7, 2024
006e98d
Workaround for GCC bug for __builtin_is_constant_evaluated when evalu…
MitalAshok Sep 8, 2024
c9ac5fe
Disable tests related to #107747
MitalAshok Sep 8, 2024
a05ff10
Address some of the comments
MitalAshok Sep 8, 2024
d80a2ae
Merge branch 'main' into libcxx-constant-eval-assume
MitalAshok Sep 13, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
-------------------------
Expand Down
23 changes: 19 additions & 4 deletions libcxx/include/__assert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,38 @@
# 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 \
(__builtin_expect(static_cast<bool>(expression), 1) ? (void)0 \
: _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.

: _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)
# 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
10 changes: 7 additions & 3 deletions libcxx/include/experimental/__simd/vec_ext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,18 @@ 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 __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?


_Tp __data __attribute__((__vector_size__(__vector_size)));

_LIBCPP_HIDE_FROM_ABI _Tp __get(size_t __idx) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__idx >= 0 && __idx < _Np, "Index is out of bounds");
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__idx < _Np, "Index is out of bounds");
return __data[__idx];
}
_LIBCPP_HIDE_FROM_ABI void __set(size_t __idx, _Tp __v) noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__idx >= 0 && __idx < _Np, "Index is out of bounds");
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__idx < _Np, "Index is out of bounds");
__data[__idx] = __v;
}
};
Expand Down
61 changes: 61 additions & 0 deletions libcxx/test/libcxx/assertions/constant_expression.verify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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}}
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.


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 int f(int x) {
return (_LIBCPP_ASSERT(x != 6, "message"), x);
// expected-note@-1 {{subexpression not valid in a constant expression}}
}
constexpr int g(int x) {
return (_LIBCPP_ASSUME(x != 6), x);
// expected-note@-1 {{subexpression not valid in a constant expression}}
}
static_assert(f(6) == 6, "");
// expected-error@-1 {{static assertion expression is not an integral constant expression}}
static_assert(g(6) == 6, "");
// expected-error@-1 {{static assertion expression is not an integral constant expression}}
#endif

#if TEST_STD_VER >= 14
constexpr int ff(int x) {
_LIBCPP_ASSERT(x != 6, "message");
// expected-note@-1 {{subexpression not valid in a constant expression}}
return x;
}
constexpr int gg(int x) {
_LIBCPP_ASSUME(x != 6);
// expected-note@-1 {{subexpression not valid in a constant expression}}
return x;
}
static_assert(ff(6) == 6, "");
// expected-error@-1 {{static assertion expression is not an integral constant expression}}
static_assert(gg(6) == 6, "");
// expected-error@-1 {{static assertion expression is not an integral constant expression}}
#endif
4 changes: 3 additions & 1 deletion libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions libcxx/test/support/nasty_string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@
# define TEST_HAS_NO_NASTY_STRING
#endif

// 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.


#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 {
Expand Down
Loading