diff --git a/libcxx/include/__config b/libcxx/include/__config index 33a79b9a79f62..4b39959a07011 100644 --- a/libcxx/include/__config +++ b/libcxx/include/__config @@ -308,6 +308,19 @@ // user input. // // - `_LIBCPP_ASSERT_UNCATEGORIZED` -- for assertions that haven't been properly classified yet. +// +// In addition to these categories, `_LIBCPP_REDUNDANT_ASSERTION` should be used to wrap assertions that duplicate other +// assertions (for example, a range view might check that its `optional` data member holds a value before dereferencing +// it, but this is already checked by `optional` itself). Redundant assertions incur an additional performance overhead +// and don't provide any extra security benefit, but catching an error earlier allows halting the program closer to the +// root cause and giving the user an error message that contains more context. Due to these tradeoffs, redundant +// assertions are disabled in the fast mode but are enabled in the extensive mode and above. Thus, going back to the +// example above, the view should wrap its check for the empty optional member variable in +// `_LIBCPP_REDUNDANT_ASSERTION`. Then: +// - in the fast mode, the program will only perform one check and will trap inside the optional (with an error +// amounting to "Attempting to dereference an empty optional"); +// - in the extensive mode, the program will normally perform two checks (in the non-error case), and if the optional is +// empty, it will trap inside the view (with an error like "`foo_view` doesn't have a valid predicate"). // clang-format off # define _LIBCPP_HARDENING_MODE_NONE (1 << 1) @@ -350,6 +363,9 @@ _LIBCPP_HARDENING_MODE_DEBUG # define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression) # define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression) # define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression) +// TODO(hardening): if `_LIBCPP_ASSUME` becomes functional again (it's currently a no-op), this would essentially +// discard the assumption. +# define _LIBCPP_REDUNDANT_ASSERTION(expression) ((void)0) // Extensive hardening mode checks. @@ -363,6 +379,7 @@ _LIBCPP_HARDENING_MODE_DEBUG # define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message) # define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSERT(expression, message) # define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message) +# define _LIBCPP_REDUNDANT_ASSERTION(expression) expression # define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message) // Disabled checks. # define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression) @@ -381,6 +398,7 @@ _LIBCPP_HARDENING_MODE_DEBUG # define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message) # define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSERT(expression, message) # define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message) +# define _LIBCPP_REDUNDANT_ASSERTION(expression) expression // Disable all checks if hardening is not enabled. @@ -396,6 +414,7 @@ _LIBCPP_HARDENING_MODE_DEBUG # define _LIBCPP_ASSERT_PEDANTIC(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_REDUNDANT_ASSERTION(expression) ((void)0) # endif // _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_FAST // clang-format on diff --git a/libcxx/include/__filesystem/directory_iterator.h b/libcxx/include/__filesystem/directory_iterator.h index 5287a4d8b055f..9b55513c87e50 100644 --- a/libcxx/include/__filesystem/directory_iterator.h +++ b/libcxx/include/__filesystem/directory_iterator.h @@ -74,7 +74,8 @@ class directory_iterator { _LIBCPP_HIDE_FROM_ABI const directory_entry& operator*() const { // Note: this check duplicates a check in `__dereference()`. - _LIBCPP_ASSERT_NON_NULL(__imp_, "The end iterator cannot be dereferenced"); + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_NON_NULL(__imp_, "The end iterator cannot be dereferenced")); return __dereference(); } diff --git a/libcxx/include/__iterator/next.h b/libcxx/include/__iterator/next.h index 21d3688ad9eb6..b08881e1313e5 100644 --- a/libcxx/include/__iterator/next.h +++ b/libcxx/include/__iterator/next.h @@ -29,8 +29,9 @@ inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _InputIter next(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n = 1) { // Calling `advance` with a negative value on a non-bidirectional iterator is a no-op in the current implementation. // Note that this check duplicates the similar check in `std::advance`. - _LIBCPP_ASSERT_PEDANTIC(__n >= 0 || __has_bidirectional_iterator_category<_InputIter>::value, - "Attempt to next(it, n) with negative n on a non-bidirectional iterator"); + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_PEDANTIC(__n >= 0 || __has_bidirectional_iterator_category<_InputIter>::value, + "Attempt to next(it, n) with negative n on a non-bidirectional iterator")); std::advance(__x, __n); return __x; diff --git a/libcxx/include/__iterator/prev.h b/libcxx/include/__iterator/prev.h index 2f0e6a088edb3..2590ea27d2b9b 100644 --- a/libcxx/include/__iterator/prev.h +++ b/libcxx/include/__iterator/prev.h @@ -29,8 +29,9 @@ inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _InputIter prev(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n = 1) { // Calling `advance` with a negative value on a non-bidirectional iterator is a no-op in the current implementation. // Note that this check duplicates the similar check in `std::advance`. - _LIBCPP_ASSERT_PEDANTIC(__n <= 0 || __has_bidirectional_iterator_category<_InputIter>::value, - "Attempt to prev(it, n) with a positive n on a non-bidirectional iterator"); + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_PEDANTIC(__n <= 0 || __has_bidirectional_iterator_category<_InputIter>::value, + "Attempt to prev(it, n) with a positive n on a non-bidirectional iterator")); std::advance(__x, -__n); return __x; } diff --git a/libcxx/include/__mdspan/layout_left.h b/libcxx/include/__mdspan/layout_left.h index fd644fa0c5322..0935e7e8f5688 100644 --- a/libcxx/include/__mdspan/layout_left.h +++ b/libcxx/include/__mdspan/layout_left.h @@ -151,9 +151,10 @@ class layout_left::mapping { // Mappings are generally meant to be used for accessing allocations and are meant to guarantee to never // return a value exceeding required_span_size(), which is used to know how large an allocation one needs // Thus, this is a canonical point in multi-dimensional data structures to make invalid element access checks - // However, mdspan does check this on its own, so for now we avoid double checking in hardened mode - _LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...), - "layout_left::mapping: out of bounds indexing"); + // However, mdspan does check this on its own, so we avoid double checking in hardened mode + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...), + "layout_left::mapping: out of bounds indexing")); array __idx_a{static_cast(__idx)...}; return [&](index_sequence<_Pos...>) { index_type __res = 0; diff --git a/libcxx/include/__mdspan/layout_right.h b/libcxx/include/__mdspan/layout_right.h index 8e64d07dd5230..ac2e38b33a09c 100644 --- a/libcxx/include/__mdspan/layout_right.h +++ b/libcxx/include/__mdspan/layout_right.h @@ -150,9 +150,10 @@ class layout_right::mapping { // Mappings are generally meant to be used for accessing allocations and are meant to guarantee to never // return a value exceeding required_span_size(), which is used to know how large an allocation one needs // Thus, this is a canonical point in multi-dimensional data structures to make invalid element access checks - // However, mdspan does check this on its own, so for now we avoid double checking in hardened mode - _LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...), - "layout_right::mapping: out of bounds indexing"); + // However, mdspan does check this on its own, so we avoid double checking in hardened mode + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...), + "layout_right::mapping: out of bounds indexing")); return [&](index_sequence<_Pos...>) { index_type __res = 0; ((__res = static_cast(__idx) + __extents_.extent(_Pos) * __res), ...); diff --git a/libcxx/include/__mdspan/layout_stride.h b/libcxx/include/__mdspan/layout_stride.h index 77934bfa11d9d..04e7c2a9b0490 100644 --- a/libcxx/include/__mdspan/layout_stride.h +++ b/libcxx/include/__mdspan/layout_stride.h @@ -283,9 +283,10 @@ class layout_stride::mapping { // Mappings are generally meant to be used for accessing allocations and are meant to guarantee to never // return a value exceeding required_span_size(), which is used to know how large an allocation one needs // Thus, this is a canonical point in multi-dimensional data structures to make invalid element access checks - // However, mdspan does check this on its own, so for now we avoid double checking in hardened mode - _LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...), - "layout_stride::mapping: out of bounds indexing"); + // However, mdspan does check this on its own, so we avoid double checking in hardened mode + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...), + "layout_stride::mapping: out of bounds indexing")); return [&](index_sequence<_Pos...>) { return ((static_cast(__idx) * __strides_[_Pos]) + ... + index_type(0)); }(make_index_sequence()); diff --git a/libcxx/include/__ranges/chunk_by_view.h b/libcxx/include/__ranges/chunk_by_view.h index b04a23de99fb2..478497c034cba 100644 --- a/libcxx/include/__ranges/chunk_by_view.h +++ b/libcxx/include/__ranges/chunk_by_view.h @@ -66,8 +66,10 @@ class _LIBCPP_ABI_LLVM18_NO_UNIQUE_ADDRESS chunk_by_view : public view_interface _LIBCPP_HIDE_FROM_ABI constexpr iterator_t<_View> __find_next(iterator_t<_View> __current) { // Note: this duplicates a check in `optional` but provides a better error message. - _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( - __pred_.__has_value(), "Trying to call __find_next() on a chunk_by_view that does not have a valid predicate."); + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( + __pred_.__has_value(), + "Trying to call __find_next() on a chunk_by_view that does not have a valid predicate.")); auto __reversed_pred = [this](_Tp&& __x, _Up&& __y) -> bool { return !std::invoke(*__pred_, std::forward<_Tp>(__x), std::forward<_Up>(__y)); }; @@ -81,8 +83,10 @@ class _LIBCPP_ABI_LLVM18_NO_UNIQUE_ADDRESS chunk_by_view : public view_interface // Attempting to decrement a begin iterator is a no-op (`__find_prev` would return the same argument given to it). _LIBCPP_ASSERT_PEDANTIC(__current != ranges::begin(__base_), "Trying to call __find_prev() on a begin iterator."); // Note: this duplicates a check in `optional` but provides a better error message. - _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( - __pred_.__has_value(), "Trying to call __find_prev() on a chunk_by_view that does not have a valid predicate."); + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( + __pred_.__has_value(), + "Trying to call __find_prev() on a chunk_by_view that does not have a valid predicate.")); auto __first = ranges::begin(__base_); reverse_view __reversed{subrange{__first, __current}}; @@ -112,8 +116,9 @@ class _LIBCPP_ABI_LLVM18_NO_UNIQUE_ADDRESS chunk_by_view : public view_interface _LIBCPP_HIDE_FROM_ABI constexpr __iterator begin() { // Note: this duplicates a check in `optional` but provides a better error message. - _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( - __pred_.__has_value(), "Trying to call begin() on a chunk_by_view that does not have a valid predicate."); + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( + __pred_.__has_value(), "Trying to call begin() on a chunk_by_view that does not have a valid predicate.")); auto __first = ranges::begin(__base_); if (!__cached_begin_.__has_value()) { diff --git a/libcxx/include/__ranges/drop_while_view.h b/libcxx/include/__ranges/drop_while_view.h index 4e3ef61678f4d..52d4aa2a7fd1b 100644 --- a/libcxx/include/__ranges/drop_while_view.h +++ b/libcxx/include/__ranges/drop_while_view.h @@ -66,10 +66,11 @@ class _LIBCPP_ABI_LLVM18_NO_UNIQUE_ADDRESS drop_while_view : public view_interfa _LIBCPP_HIDE_FROM_ABI constexpr auto begin() { // Note: this duplicates a check in `optional` but provides a better error message. - _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( - __pred_.__has_value(), - "drop_while_view needs to have a non-empty predicate before calling begin() -- did a previous " - "assignment to this drop_while_view fail?"); + _LIBCPP_REDUNDANT_ASSERTION( // + _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( + __pred_.__has_value(), + "drop_while_view needs to have a non-empty predicate before calling begin() -- did a previous " + "assignment to this drop_while_view fail?")); if constexpr (_UseCache) { if (!__cached_begin_.__has_value()) { __cached_begin_.__emplace(ranges::find_if_not(__base_, std::cref(*__pred_))); diff --git a/libcxx/include/__ranges/filter_view.h b/libcxx/include/__ranges/filter_view.h index 6e6719c14470d..acba8ad2e4053 100644 --- a/libcxx/include/__ranges/filter_view.h +++ b/libcxx/include/__ranges/filter_view.h @@ -84,8 +84,9 @@ class _LIBCPP_ABI_LLVM18_NO_UNIQUE_ADDRESS filter_view : public view_interface::format( const char_type* __fmt_last, regex_constants::match_flag_type __flags) const { // Note: this duplicates a check in `vector::operator[]` but provides a better error message. - _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(ready(), "match_results::format() called when not ready"); + _LIBCPP_REDUNDANT_ASSERTION( + _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(ready(), "match_results::format() called when not ready")); if (__flags & regex_constants::format_sed) { for (; __fmt_first != __fmt_last; ++__fmt_first) { if (*__fmt_first == '&') diff --git a/libcxx/test/libcxx/assertions/single_expression.pass.cpp b/libcxx/test/libcxx/assertions/single_expression.pass.cpp index 13253e4cb6ef5..15b77f24944cd 100644 --- a/libcxx/test/libcxx/assertions/single_expression.pass.cpp +++ b/libcxx/test/libcxx/assertions/single_expression.pass.cpp @@ -6,27 +6,35 @@ // //===----------------------------------------------------------------------===// -// Make sure that `_LIBCPP_ASSERT` and `_LIBCPP_ASSUME` are each a single expression. -// This is useful so we can use them in places that require an expression, such as -// in a constructor initializer list. +// Make sure that `_LIBCPP_ASSERT` and `_LIBCPP_ASSUME` are each a single expression, and that it still holds when an +// assertion is wrapped in the `_LIBCPP_REDUNDANT_ASSERTION` macro. This is useful so we can use them in places that +// require an expression, such as in a constructor initializer list. #include <__assert> #include -void f() { +void test_assert() { int i = (_LIBCPP_ASSERT(true, "message"), 3); assert(i == 3); return _LIBCPP_ASSERT(true, "message"); } -void g() { +void test_assume() { int i = (_LIBCPP_ASSUME(true), 3); assert(i == 3); return _LIBCPP_ASSUME(true); } +void test_redundant() { + int i = (_LIBCPP_REDUNDANT_ASSERTION(_LIBCPP_ASSERT(true, "message")), 3); + assert(i == 3); + return _LIBCPP_REDUNDANT_ASSERTION(_LIBCPP_ASSERT(true, "message")); +} + int main(int, char**) { - f(); - g(); + test_assert(); + test_assume(); + test_redundant(); + return 0; }