[libc++][hardening] Categorize more assertions. #75918
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Konstantin Varlamov (var-const) ChangesAlso introduce Patch is 89.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75918.diff 54 Files Affected:
diff --git a/libcxx/include/__algorithm/pop_heap.h b/libcxx/include/__algorithm/pop_heap.h
index a93a9875f70581..798a1d09934bc3 100644
--- a/libcxx/include/__algorithm/pop_heap.h
+++ b/libcxx/include/__algorithm/pop_heap.h
@@ -36,7 +36,8 @@ __pop_heap(_RandomAccessIterator __first,
_RandomAccessIterator __last,
_Compare& __comp,
typename iterator_traits<_RandomAccessIterator>::difference_type __len) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__len > 0, "The heap given to pop_heap must be non-empty");
+ // Calling `pop_heap` on an empty range is undefined behavior, but in practice it will be a no-op.
+ _LIBCPP_ASSERT_PEDANTIC(__len > 0, "The heap given to pop_heap must be non-empty");
__comp_ref_type<_Compare> __comp_ref = __comp;
diff --git a/libcxx/include/__algorithm/ranges_max.h b/libcxx/include/__algorithm/ranges_max.h
index 782ce2670f0553..0f89cb2ff5bf22 100644
--- a/libcxx/include/__algorithm/ranges_max.h
+++ b/libcxx/include/__algorithm/ranges_max.h
@@ -54,7 +54,8 @@ struct __fn {
indirect_strict_weak_order<projected<const _Tp*, _Proj>> _Comp = ranges::less>
_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr _Tp
operator()(initializer_list<_Tp> __il, _Comp __comp = {}, _Proj __proj = {}) const {
- _LIBCPP_ASSERT_UNCATEGORIZED(__il.begin() != __il.end(), "initializer_list must contain at least one element");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __il.begin() != __il.end(), "initializer_list must contain at least one element");
auto __comp_lhs_rhs_swapped = [&](auto&& __lhs, auto&& __rhs) -> bool { return std::invoke(__comp, __rhs, __lhs); };
return *ranges::__min_element_impl(__il.begin(), __il.end(), __comp_lhs_rhs_swapped, __proj);
@@ -69,7 +70,7 @@ struct __fn {
auto __first = ranges::begin(__r);
auto __last = ranges::end(__r);
- _LIBCPP_ASSERT_UNCATEGORIZED(__first != __last, "range must contain at least one element");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__first != __last, "range must contain at least one element");
if constexpr (forward_range<_Rp> && !__is_cheap_to_copy<range_value_t<_Rp>>) {
auto __comp_lhs_rhs_swapped = [&](auto&& __lhs, auto&& __rhs) -> bool {
diff --git a/libcxx/include/__algorithm/ranges_min.h b/libcxx/include/__algorithm/ranges_min.h
index be15b4536734df..8757358cdf37d9 100644
--- a/libcxx/include/__algorithm/ranges_min.h
+++ b/libcxx/include/__algorithm/ranges_min.h
@@ -53,7 +53,8 @@ struct __fn {
indirect_strict_weak_order<projected<const _Tp*, _Proj>> _Comp = ranges::less>
_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr _Tp
operator()(initializer_list<_Tp> __il, _Comp __comp = {}, _Proj __proj = {}) const {
- _LIBCPP_ASSERT_UNCATEGORIZED(__il.begin() != __il.end(), "initializer_list must contain at least one element");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __il.begin() != __il.end(), "initializer_list must contain at least one element");
return *ranges::__min_element_impl(__il.begin(), __il.end(), __comp, __proj);
}
@@ -65,7 +66,7 @@ struct __fn {
operator()(_Rp&& __r, _Comp __comp = {}, _Proj __proj = {}) const {
auto __first = ranges::begin(__r);
auto __last = ranges::end(__r);
- _LIBCPP_ASSERT_UNCATEGORIZED(__first != __last, "range must contain at least one element");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__first != __last, "range must contain at least one element");
if constexpr (forward_range<_Rp> && !__is_cheap_to_copy<range_value_t<_Rp>>) {
return *ranges::__min_element_impl(__first, __last, __comp, __proj);
} else {
diff --git a/libcxx/include/__algorithm/ranges_minmax.h b/libcxx/include/__algorithm/ranges_minmax.h
index a5b5cf9bd0ab98..22a62b620c936f 100644
--- a/libcxx/include/__algorithm/ranges_minmax.h
+++ b/libcxx/include/__algorithm/ranges_minmax.h
@@ -65,7 +65,8 @@ struct __fn {
indirect_strict_weak_order<projected<const _Type*, _Proj>> _Comp = ranges::less>
_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr ranges::minmax_result<_Type>
operator()(initializer_list<_Type> __il, _Comp __comp = {}, _Proj __proj = {}) const {
- _LIBCPP_ASSERT_UNCATEGORIZED(__il.begin() != __il.end(), "initializer_list has to contain at least one element");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __il.begin() != __il.end(), "initializer_list has to contain at least one element");
auto __iters = std::__minmax_element_impl(__il.begin(), __il.end(), __comp, __proj);
return ranges::minmax_result<_Type>{*__iters.first, *__iters.second};
}
@@ -80,7 +81,7 @@ struct __fn {
auto __last = ranges::end(__r);
using _ValueT = range_value_t<_Range>;
- _LIBCPP_ASSERT_UNCATEGORIZED(__first != __last, "range has to contain at least one element");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__first != __last, "range has to contain at least one element");
if constexpr (forward_range<_Range>) {
// Special-case the one element case. Avoid repeatedly initializing objects from the result of an iterator
diff --git a/libcxx/include/__algorithm/sample.h b/libcxx/include/__algorithm/sample.h
index cc29dd686f6be8..ebe5180b7eeca6 100644
--- a/libcxx/include/__algorithm/sample.h
+++ b/libcxx/include/__algorithm/sample.h
@@ -89,7 +89,7 @@ _LIBCPP_HIDE_FROM_ABI _SampleIterator __sample(
_SampleIterator __output_iter,
_Distance __n,
_UniformRandomNumberGenerator& __g) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__n >= 0, "N must be a positive number.");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__n >= 0, "N must be a positive number.");
using _PopIterCategory = typename _IterOps<_AlgPolicy>::template __iterator_category<_PopulationIterator>;
using _Difference = typename _IterOps<_AlgPolicy>::template __difference_type<_PopulationIterator>;
diff --git a/libcxx/include/__algorithm/sift_down.h b/libcxx/include/__algorithm/sift_down.h
index 7f152e4dbd7f34..42803e30631fb1 100644
--- a/libcxx/include/__algorithm/sift_down.h
+++ b/libcxx/include/__algorithm/sift_down.h
@@ -85,7 +85,7 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _RandomAccessIterator __floy
_Compare&& __comp,
typename iterator_traits<_RandomAccessIterator>::difference_type __len) {
using difference_type = typename iterator_traits<_RandomAccessIterator>::difference_type;
- _LIBCPP_ASSERT_UNCATEGORIZED(__len >= 2, "shouldn't be called unless __len >= 2");
+ _LIBCPP_ASSERT_INTERNAL(__len >= 2, "shouldn't be called unless __len >= 2");
_RandomAccessIterator __hole = __first;
_RandomAccessIterator __child_i = __first;
diff --git a/libcxx/include/__algorithm/sort.h b/libcxx/include/__algorithm/sort.h
index 1b878c33c7a16f..ac47489af0aac9 100644
--- a/libcxx/include/__algorithm/sort.h
+++ b/libcxx/include/__algorithm/sort.h
@@ -533,7 +533,7 @@ __bitset_partition(_RandomAccessIterator __first, _RandomAccessIterator __last,
using _Ops = _IterOps<_AlgPolicy>;
typedef typename std::iterator_traits<_RandomAccessIterator>::value_type value_type;
typedef typename std::iterator_traits<_RandomAccessIterator>::difference_type difference_type;
- _LIBCPP_ASSERT_UNCATEGORIZED(__last - __first >= difference_type(3), "");
+ _LIBCPP_ASSERT_INTERNAL(__last - __first >= difference_type(3), "");
const _RandomAccessIterator __begin = __first; // used for bounds checking, those are not moved around
const _RandomAccessIterator __end = __last;
(void)__end; //
@@ -625,7 +625,7 @@ __partition_with_equals_on_right(_RandomAccessIterator __first, _RandomAccessIte
using _Ops = _IterOps<_AlgPolicy>;
typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
typedef typename std::iterator_traits<_RandomAccessIterator>::value_type value_type;
- _LIBCPP_ASSERT_UNCATEGORIZED(__last - __first >= difference_type(3), "");
+ _LIBCPP_ASSERT_INTERNAL(__last - __first >= difference_type(3), "");
const _RandomAccessIterator __begin = __first; // used for bounds checking, those are not moved around
const _RandomAccessIterator __end = __last;
(void)__end; //
diff --git a/libcxx/include/__charconv/to_chars_base_10.h b/libcxx/include/__charconv/to_chars_base_10.h
index 33c512e20f04c2..c1a5d4f9d5d52e 100644
--- a/libcxx/include/__charconv/to_chars_base_10.h
+++ b/libcxx/include/__charconv/to_chars_base_10.h
@@ -132,13 +132,13 @@ __base_10_u64(char* __buffer, uint64_t __value) noexcept {
/// range that can be used. However the range is sufficient for
/// \ref __base_10_u128.
_LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI inline __uint128_t __pow_10(int __exp) noexcept {
- _LIBCPP_ASSERT_UNCATEGORIZED(__exp >= __pow10_128_offset, "Index out of bounds");
+ _LIBCPP_ASSERT_INTERNAL(__exp >= __pow10_128_offset, "Index out of bounds");
return __pow10_128[__exp - __pow10_128_offset];
}
_LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI inline char*
__base_10_u128(char* __buffer, __uint128_t __value) noexcept {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_INTERNAL(
__value > numeric_limits<uint64_t>::max(), "The optimizations for this algorithm fail when this isn't true.");
// Unlike the 64 to 32 bit case the 128 bit case the "upper half" can't be
diff --git a/libcxx/include/__charconv/to_chars_integral.h b/libcxx/include/__charconv/to_chars_integral.h
index f50cc55a4c6d90..40fbe334d8d54c 100644
--- a/libcxx/include/__charconv/to_chars_integral.h
+++ b/libcxx/include/__charconv/to_chars_integral.h
@@ -246,7 +246,7 @@ __to_chars_integral(char* __first, char* __last, _Tp __value) {
template <typename _Tp>
_LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI int __to_chars_integral_width(_Tp __value, unsigned __base) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__value >= 0, "The function requires a non-negative value.");
+ _LIBCPP_ASSERT_INTERNAL(__value >= 0, "The function requires a non-negative value.");
unsigned __base_2 = __base * __base;
unsigned __base_3 = __base_2 * __base;
diff --git a/libcxx/include/__charconv/traits.h b/libcxx/include/__charconv/traits.h
index d3884b560dfd7f..b4907c3f775715 100644
--- a/libcxx/include/__charconv/traits.h
+++ b/libcxx/include/__charconv/traits.h
@@ -101,11 +101,11 @@ struct _LIBCPP_HIDDEN __traits_base<_Tp, __enable_if_t<sizeof(_Tp) == sizeof(__u
/// zero is set to one. This means the first element of the lookup table is
/// zero.
static _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI int __width(_Tp __v) {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_INTERNAL(
__v > numeric_limits<uint64_t>::max(), "The optimizations for this algorithm fail when this isn't true.");
// There's always a bit set in the upper 64-bits.
auto __t = (128 - std::__libcpp_clz(static_cast<uint64_t>(__v >> 64))) * 1233 >> 12;
- _LIBCPP_ASSERT_UNCATEGORIZED(__t >= __itoa::__pow10_128_offset, "Index out of bounds");
+ _LIBCPP_ASSERT_INTERNAL(__t >= __itoa::__pow10_128_offset, "Index out of bounds");
// __t is adjusted since the lookup table misses the lower entries.
return __t - (__v < __itoa::__pow10_128[__t - __itoa::__pow10_128_offset]) + 1;
}
diff --git a/libcxx/include/__chrono/parser_std_format_spec.h b/libcxx/include/__chrono/parser_std_format_spec.h
index 296be8794ec59d..86c9712ba487da 100644
--- a/libcxx/include/__chrono/parser_std_format_spec.h
+++ b/libcxx/include/__chrono/parser_std_format_spec.h
@@ -160,7 +160,7 @@ class _LIBCPP_TEMPLATE_VIS __parser_chrono {
private:
_LIBCPP_HIDE_FROM_ABI constexpr _ConstIterator
__parse_chrono_specs(_ConstIterator __begin, _ConstIterator __end, __flags __flags) {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__begin != __end,
"When called with an empty input the function will cause "
"undefined behavior by evaluating data not in the input");
diff --git a/libcxx/include/__config b/libcxx/include/__config
index adff13e714cb64..9d3192453b701d 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -273,6 +273,9 @@
// - `_LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR` -- checks any operations that exchange nodes between containers to make sure
// the containers have compatible allocators.
//
+// - `_LIBCPP_ASSERT_PEDANTIC` -- checks prerequisites which are imposed by the Standard, but violating which happens to
+// be benign in our implementation.
+//
// - `_LIBCPP_ASSERT_INTERNAL` -- checks that internal invariants of the library hold. These assertions don't depend on
// user input.
//
@@ -315,6 +318,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
// vulnerability.
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
+# 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)
@@ -330,6 +334,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
// Disabled checks.
+# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
// Debug hardening mode checks.
@@ -342,6 +347,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
+# 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)
diff --git a/libcxx/include/__filesystem/path_iterator.h b/libcxx/include/__filesystem/path_iterator.h
index 1a9aaf0e7d99e6..d2d65cd122cab8 100644
--- a/libcxx/include/__filesystem/path_iterator.h
+++ b/libcxx/include/__filesystem/path_iterator.h
@@ -61,7 +61,7 @@ class _LIBCPP_EXPORTED_FROM_ABI path::iterator {
_LIBCPP_HIDE_FROM_ABI pointer operator->() const { return &__stashed_elem_; }
_LIBCPP_HIDE_FROM_ABI iterator& operator++() {
- _LIBCPP_ASSERT_UNCATEGORIZED(__state_ != _Singular, "attempting to increment a singular iterator");
+ _LIBCPP_ASSERT_NON_NULL(__state_ != _Singular, "attempting to increment a singular iterator");
_LIBCPP_ASSERT_UNCATEGORIZED(__state_ != _AtEnd, "attempting to increment the end iterator");
return __increment();
}
@@ -73,7 +73,7 @@ class _LIBCPP_EXPORTED_FROM_ABI path::iterator {
}
_LIBCPP_HIDE_FROM_ABI iterator& operator--() {
- _LIBCPP_ASSERT_UNCATEGORIZED(__state_ != _Singular, "attempting to decrement a singular iterator");
+ _LIBCPP_ASSERT_NON_NULL(__state_ != _Singular, "attempting to decrement a singular iterator");
_LIBCPP_ASSERT_UNCATEGORIZED(
__entry_.data() != __path_ptr_->native().data(), "attempting to decrement the begin iterator");
return __decrement();
diff --git a/libcxx/include/__format/buffer.h b/libcxx/include/__format/buffer.h
index 7ee583d8139455..8598f0a1c03957 100644
--- a/libcxx/include/__format/buffer.h
+++ b/libcxx/include/__format/buffer.h
@@ -115,7 +115,7 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
// The output doesn't fit in the internal buffer.
// Copy the data in "__capacity_" sized chunks.
- _LIBCPP_ASSERT_UNCATEGORIZED(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
+ _LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
const _InCharT* __first = __str.data();
do {
size_t __chunk = std::min(__n, __capacity_);
@@ -134,7 +134,7 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
class _UnaryOperation,
__fmt_char_type _InCharT = typename iterator_traits<_Iterator>::value_type>
_LIBCPP_HIDE_FROM_ABI void __transform(_Iterator __first, _Iterator __last, _UnaryOperation __operation) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__first <= __last, "not a valid range");
+ _LIBCPP_ASSERT_INTERNAL(__first <= __last, "not a valid range");
size_t __n = static_cast<size_t>(__last - __first);
__flush_on_overflow(__n);
@@ -146,7 +146,7 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
// The output doesn't fit in the internal buffer.
// Transform the data in "__capacity_" sized chunks.
- _LIBCPP_ASSERT_UNCATEGORIZED(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
+ _LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
do {
size_t __chunk = std::min(__n, __capacity_);
std::transform(__first, __first + __chunk, std::addressof(__ptr_[__size_]), __operation);
@@ -168,7 +168,7 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
// The output doesn't fit in the internal buffer.
// Fill the buffer in "__capacity_" sized chunks.
- _LIBCPP_ASSERT_UNCATEGORIZED(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
+ _LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
do {
size_t __chunk = std::min(__n, __capacity_);
std::fill_n(std::addressof(__ptr_[__size_]), __chunk, __value);
@@ -596,7 +596,7 @@ class _LIBCPP_TEMPLATE_VIS __retarget_buffer {
class _UnaryOperation,
__fmt_char_type _InCharT = typename iterator_traits<_Iterator>::value_type>
_LIBCPP_HIDE_FROM_ABI void __transform(_Iterator __first, _Iterator __last, _UnaryOperation __operation) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__first <= __last, "not a valid range");
+ _LIBCPP_ASSERT_INTERNAL(__first <= __last, "not a valid range");
size_t __n = static_cast<size_t>(__last - __first);
if (__size_ + __n >= __capacity_)
@@ -623,7 +623,7 @@ class _LIBCPP_TEMPLATE_VIS __retarget_buffer {
_LIBCPP_HIDE_FROM_ABI void __grow_buffer() { __grow_buffer(__capacity_ * 1.6); }
_LIBCPP_HIDE_FROM_ABI void __grow_buffer(size_t __capacity) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__capacity > __capacity_, "the buffer must grow");
+ _LIBCPP_ASSERT_INTERNAL(__capacity > __capacity_, "the buffer must grow");
auto __result = std::__allocate_at_least(__alloc_, __capacity);
auto __guard = std::__make_exception_guard([&] {
allocator_traits<_Alloc>::deallocate(__alloc_, __result.ptr, __result.count);
diff --git a/libcxx/include/__format/format_arg.h b/libcxx/include/__format/format_arg.h
index 280c9108241754..10fca15d5a7a94 100644
--- a/libcxx/include/__format/format_arg.h
+++ b/libcxx/include/__format/format_arg.h
@@ -83,7 +83,7 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool __use_packed_format_arg_store(size_t __size
}
_LIBCPP_HIDE_FROM_ABI constexpr __arg_t __get_packed_type(uint64_t __types, size_t __id) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__id <= __packed_types_max, "");
+ _LIBCPP_ASSERT_INTERNAL(__id <= __packed_types_max, "");
if (__id > 0)
__types >>= __id * __packed_arg_t_bits;
diff --git a/libcxx/include/__format/formatter_bool.h b/libcxx/include/__format/formatter_bool.h
index 3c8ae95f55fa1c..1c479501b675f8 100644
--- a/libcxx/include/__format/formatter_bool.h
+++ b/libcxx/include/__format/formatter_bool.h
@@ -62,7 +62,7 @@ struct _LIBCPP_TEMPLATE_VIS formatter<bool, _CharT> {
static_cast<unsigned>(__value), __ctx, __parser_.__get_parsed_std_specifications(__ctx));
default:
- _LIBCPP_ASSERT_UNCATEGORIZED(false, "The parse function should have validated the type");
+ _LIBCPP_ASSERT_INTERNAL(false, "The parse function should have validated the type");
__libcpp_unreachable();
}
}
diff --git a/libcxx/include/__format/formatter_floating_point.h b/libcxx/include/__format/formatter_floating_point.h
index 33cc2a4ed66122..6802a8b7bd4...
[truncated]
|
|
This will be ready for review once #71620 is merged. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Are |
Yes, exactly. |
| @@ -36,7 +36,8 @@ __pop_heap(_RandomAccessIterator __first, | |||
| _RandomAccessIterator __last, | |||
| _Compare& __comp, | |||
| typename iterator_traits<_RandomAccessIterator>::difference_type __len) { | |||
| _LIBCPP_ASSERT_UNCATEGORIZED(__len > 0, "The heap given to pop_heap must be non-empty"); | |||
| // Calling `pop_heap` on an empty range is undefined behavior, but in practice it will be a no-op. | |||
There was a problem hiding this comment.
I would consider taking advantage of this precondition and removing if (__len > 1), but then bumping this assertion to a valid-range (or similar) assertion. Thoughts? If we opt to do this, it would be as a separate change.
There was a problem hiding this comment.
I like the idea, but would prefer to do this in a separate PR and merge after the next release is branched.
var-const
force-pushed
the
varconst/hardening-categorization-3b
branch
from
30b70b2 to
d797ab9
Compare
| @@ -115,7 +115,7 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer { | |||
|
|
|||
| // The output doesn't fit in the internal buffer. | |||
| // Copy the data in "__capacity_" sized chunks. | |||
| _LIBCPP_ASSERT_UNCATEGORIZED(__size_ == 0, "the buffer should be flushed by __flush_on_overflow"); | |||
| _LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow"); | |||
There was a problem hiding this comment.
@mordante If you have the time, it would be great if you could see if the classification in <format> and <charconv> looks reasonable to you. I classified many of these as "internal" because, if I'm reading the code correctly, these check for internal variants that should always hold unless there's a bug in our implementation. But I'm not very familiar with this part of the code and would really appreciate your feedback. Thanks!
| @@ -73,7 +73,8 @@ class directory_iterator { | |||
| _LIBCPP_HIDE_FROM_ABI ~directory_iterator() = default; | |||
|
|
|||
| _LIBCPP_HIDE_FROM_ABI const directory_entry& operator*() const { | |||
| _LIBCPP_ASSERT_UNCATEGORIZED(__imp_, "The end iterator cannot be dereferenced"); | |||
| // Note: this check duplicates a check in `__dereference()`. | |||
There was a problem hiding this comment.
Per our discussion just now, these are the different ways we can think of handling the situation of "redundant" checks:
// Option #1: leave it as-is
void f(std::optional<T> foo) {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(foo.has_value(), "oops");
use(*foo);
}
// Option #2: remove it, it's implicitly checked, we "know" it
void f(std::optional<T> foo) {
use(*foo);
}
// Option #3: Use a comment
void f(std::optional<T> foo) {
// implicit precondition: foo.has_value(), already checked in operator* below
use(*foo);
}
// Option #4: Macro orthogonal to the assertion category
void f(std::optional<T> foo) {
_LIBCPP_REDUNDANT_ASSERTION(_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(foo.has_value(), "oops"));
use(*foo);
}
// Option #5: Add a new category
void f(std::optional<T> foo) {
// REDUNDANT|EARLY|EXTRA|...
_LIBCPP_ASSERT_EARLY(foo.has_value(), "oops");
use(*foo);
}There was a problem hiding this comment.
Discussion just now, we discussed this by elimination:
- We think we should not do
#1because the fast mode should be fast, so we should remove duplicate checks #2is just going to make us scratch our head in the future, it seems that#3is strictly better in that case#3is OK, but we lose the property of having early diagnostics with more meaningful error messages in the case where the extensive mode is enabled. So it is worse than#4and#5.#4has the benefit over#5that we can retain the real category of the assertion, so it's kinda cleaner in that way. But either would work.
It seems we have a preference for #4 over #5, although not an extremely strong one.
Thanks for separating these out. How often do they fire? Also this change LGTM. Thanks (once fixing the bots of course). |
| return __pow10_128[__exp - __pow10_128_offset]; | ||
| } | ||
|
|
||
| _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI inline char* | ||
| __base_10_u128(char* __buffer, __uint128_t __value) noexcept { | ||
| _LIBCPP_ASSERT_UNCATEGORIZED( | ||
| _LIBCPP_ASSERT_INTERNAL( | ||
| __value > numeric_limits<uint64_t>::max(), "The optimizations for this algorithm fail when this isn't true."); |
There was a problem hiding this comment.
| __value > numeric_limits<uint64_t>::max(), "The optimizations for this algorithm fail when this isn't true."); | |
| __value > numeric_limits<uint64_t>::max(), "The optimizations for this algorithm fails when this isn't true."); |
Can you do this as a drive-by?
There was a problem hiding this comment.
Hmm, I think the current state is right (The optimizations... fail) since optimizations is plural. Perhaps I'm misreading it?
| @@ -160,7 +160,7 @@ class _LIBCPP_TEMPLATE_VIS __parser_chrono { | |||
| private: | |||
| _LIBCPP_HIDE_FROM_ABI constexpr _ConstIterator | |||
| __parse_chrono_specs(_ConstIterator __begin, _ConstIterator __end, __flags __flags) { | |||
| _LIBCPP_ASSERT_UNCATEGORIZED( | |||
| _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( | |||
There was a problem hiding this comment.
This should be internal. Using an empty format-spec is valid.
| _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( | |
| _LIBCPP_ASSERT_INTERNAL( |
| // While passing a non-positive load factor is undefined behavior, in practice the result will be benign (the | ||
| // call will be equivalent to `max_load_factor(load_factor())`, which is also the case for passing a valid value | ||
| // less than the current `load_factor`). | ||
| _LIBCPP_ASSERT_PEDANTIC(__mlf > 0, "unordered container::max_load_factor(lf) called with lf <= 0"); | ||
| max_load_factor() = std::max(__mlf, load_factor()); |
There was a problem hiding this comment.
Not really for this patch, but should we add an assert the value is not INF or NAN?
There was a problem hiding this comment.
That's actually a very interesting question. So far, our non-internal assertions always (AFAIK) directly correspond to a prerequisite in the Standard. In this case, the only prerequisite is that the given argument is greater than 0. Should we start extending our (non-internal) validation beyond what the Standard requires directly? Tagging @ldionne as well.
There was a problem hiding this comment.
Typically they indeed are. But I wouldn't mind to test for other issues too with asserts.
| _LIBCPP_ASSERT_UNCATEGORIZED( | ||
| __current != ranges::begin(__base_), "Trying to call __find_prev() on a begin iterator."); | ||
| _LIBCPP_ASSERT_UNCATEGORIZED( | ||
| // Attempting to increment an end iterator is a no-op (`__find_next` would return the same argument given to it). |
There was a problem hiding this comment.
Is this comment copy pasted? If not, it's very confusion.
There was a problem hiding this comment.
Thanks for catching this! It is a copy-paste error.
libcxx/include/print
Outdated
| @@ -122,6 +122,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr void __encode(_OutIt&, char32_t) = delete; | |||
| template <class _OutIt> | |||
| requires __utf16_code_unit<iter_value_t<_OutIt>> | |||
| _LIBCPP_HIDE_FROM_ABI constexpr void __encode(_OutIt& __out_it, char32_t __value) { | |||
| // From the Standard: "if `out` contains invalid code units, the behavior is undefined and implementations are | |||
There was a problem hiding this comment.
I prefer to have a reference to the Standard's stable name and paragraph so people can look it up again.
Due to the impact of these asserts I would suggest to make this a pedantic assert.
| @@ -4587,28 +4587,36 @@ public: | |||
|
|
|||
| // element access: | |||
| _LIBCPP_HIDE_FROM_ABI difference_type length(size_type __sub = 0) const { | |||
| _LIBCPP_ASSERT_UNCATEGORIZED(ready(), "match_results::length() called when not ready"); | |||
| // If the match results are not ready, this will return `0`. | |||
| _LIBCPP_ASSERT_PEDANTIC(ready(), "match_results::length() called when not ready"); | |||
There was a problem hiding this comment.
Since this likely gives the wrong result and is explicitly done by the developer I would make this a stronger assert.
There was a problem hiding this comment.
Hmm, this is exactly the sort of situation for which I intended the new "pedantic" category (perhaps the name could be improved?). These are always triggered by user input (otherwise it would have to be classified as internal). It returns a "reasonable" default value (by reasonable, I mean that if, for the sake of the argument, we wanted to make this situation well-defined, that's the value we would probably return in this situation -- not saying it would be a good idea, just hypothetically); we cannot say for sure whether that value would cause any problems.
I suppose it could be argued that pedantic could only be used for no-ops (and thus not for any function returning a value). We don't currently have a category to capture "returns an incorrect value but we don't know if it leads to any problems since that's purely on the user's side". We would probably need something like that, but IMO "pedantic" is reasonable here since this is as close to a no-op as possible for a function returning a value. Perhaps renaming "pedantic" would improve things?
There was a problem hiding this comment.
if we do pedantic only for no-ops I would rename it. I have a different view what the word pedantic means.
| @@ -4587,28 +4587,36 @@ public: | |||
|
|
|||
| // element access: | |||
| _LIBCPP_HIDE_FROM_ABI difference_type length(size_type __sub = 0) const { | |||
| _LIBCPP_ASSERT_UNCATEGORIZED(ready(), "match_results::length() called when not ready"); | |||
| // If the match results are not ready, this will return `0`. | |||
| _LIBCPP_ASSERT_PEDANTIC(ready(), "match_results::length() called when not ready"); | |||
| return (*this)[__sub].length(); | |||
There was a problem hiding this comment.
Suggestion for a followup: add a range validation on __sub.
There was a problem hiding this comment.
Surprisingly enough, if I'm reading the preconditions correctly, all possible values are valid:
If
n == 0, the length of the entire matched expression is returned.
Ifn > 0 && n < size(), the length of nth sub-match is returned.
ifn >= size(), a length of the unmatched match is returned.
There was a problem hiding this comment.
interesting, then it seems there is a bug in our code.
@EricWF Unfortunately, I don't think we really have the data to answer that. I'm not aware of any bug reports (@ldionne Louis, do you know of any?), but I think we've seen cases when even pretty significant bugs were not reported for a long time despite people hitting them, so I don't think we can be confident these are never triggered just because we don't hear from users. Intuitively though, it does seem like it should be rare. I'm personally somewhat in favor of keeping them (or at least most of them), not so much for actually catching bugs in the wild (although if that happens, it's awesome) but more as a useful form of documentation and an aid in refactoring. I think there is some value in a function stating its invariants and preconditions explicitly in the form of assertions, and I definitely hit some internal assertions when doing |
Also introduce `_LIBCPP_ASSERT_PEDANTIC` for assertions violating which results in a no-op or other benign behavior, but which may nevertheless indicate a bug in the invoking code.
There was a problem hiding this comment.
@mordante Thank you for your review! I have addressed all your comments except the ones about classifying some assertions as pedantic. I'd like to discuss this further, perhaps on Discord. Would you mind if I merge the patch as is (that is, with everything addressed except those comments) and apply the outcome of our discussion in a follow-up? I have another patch with more classifications coming up, and it would be much easier for me if I could just rebase it on top of main.
| @@ -4587,28 +4587,36 @@ public: | |||
|
|
|||
| // element access: | |||
| _LIBCPP_HIDE_FROM_ABI difference_type length(size_type __sub = 0) const { | |||
| _LIBCPP_ASSERT_UNCATEGORIZED(ready(), "match_results::length() called when not ready"); | |||
| // If the match results are not ready, this will return `0`. | |||
| _LIBCPP_ASSERT_PEDANTIC(ready(), "match_results::length() called when not ready"); | |||
| return (*this)[__sub].length(); | |||
There was a problem hiding this comment.
Surprisingly enough, if I'm reading the preconditions correctly, all possible values are valid:
If
n == 0, the length of the entire matched expression is returned.
Ifn > 0 && n < size(), the length of nth sub-match is returned.
ifn >= size(), a length of the unmatched match is returned.
| @@ -4587,28 +4587,36 @@ public: | |||
|
|
|||
| // element access: | |||
| _LIBCPP_HIDE_FROM_ABI difference_type length(size_type __sub = 0) const { | |||
| _LIBCPP_ASSERT_UNCATEGORIZED(ready(), "match_results::length() called when not ready"); | |||
| // If the match results are not ready, this will return `0`. | |||
| _LIBCPP_ASSERT_PEDANTIC(ready(), "match_results::length() called when not ready"); | |||
There was a problem hiding this comment.
Hmm, this is exactly the sort of situation for which I intended the new "pedantic" category (perhaps the name could be improved?). These are always triggered by user input (otherwise it would have to be classified as internal). It returns a "reasonable" default value (by reasonable, I mean that if, for the sake of the argument, we wanted to make this situation well-defined, that's the value we would probably return in this situation -- not saying it would be a good idea, just hypothetically); we cannot say for sure whether that value would cause any problems.
I suppose it could be argued that pedantic could only be used for no-ops (and thus not for any function returning a value). We don't currently have a category to capture "returns an incorrect value but we don't know if it leads to any problems since that's purely on the user's side". We would probably need something like that, but IMO "pedantic" is reasonable here since this is as close to a no-op as possible for a function returning a value. Perhaps renaming "pedantic" would improve things?
libcxx/include/print
Outdated
| @@ -122,6 +122,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr void __encode(_OutIt&, char32_t) = delete; | |||
| template <class _OutIt> | |||
| requires __utf16_code_unit<iter_value_t<_OutIt>> | |||
| _LIBCPP_HIDE_FROM_ABI constexpr void __encode(_OutIt& __out_it, char32_t __value) { | |||
| // From the Standard: "if `out` contains invalid code units, the behavior is undefined and implementations are | |||
| return __pow10_128[__exp - __pow10_128_offset]; | ||
| } | ||
|
|
||
| _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI inline char* | ||
| __base_10_u128(char* __buffer, __uint128_t __value) noexcept { | ||
| _LIBCPP_ASSERT_UNCATEGORIZED( | ||
| _LIBCPP_ASSERT_INTERNAL( | ||
| __value > numeric_limits<uint64_t>::max(), "The optimizations for this algorithm fail when this isn't true."); |
There was a problem hiding this comment.
Hmm, I think the current state is right (The optimizations... fail) since optimizations is plural. Perhaps I'm misreading it?
| @@ -160,7 +160,7 @@ class _LIBCPP_TEMPLATE_VIS __parser_chrono { | |||
| private: | |||
| _LIBCPP_HIDE_FROM_ABI constexpr _ConstIterator | |||
| __parse_chrono_specs(_ConstIterator __begin, _ConstIterator __end, __flags __flags) { | |||
| _LIBCPP_ASSERT_UNCATEGORIZED( | |||
| _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS( | |||
| // While passing a non-positive load factor is undefined behavior, in practice the result will be benign (the | ||
| // call will be equivalent to `max_load_factor(load_factor())`, which is also the case for passing a valid value | ||
| // less than the current `load_factor`). | ||
| _LIBCPP_ASSERT_PEDANTIC(__mlf > 0, "unordered container::max_load_factor(lf) called with lf <= 0"); | ||
| max_load_factor() = std::max(__mlf, load_factor()); |
There was a problem hiding this comment.
That's actually a very interesting question. So far, our non-internal assertions always (AFAIK) directly correspond to a prerequisite in the Standard. In this case, the only prerequisite is that the given argument is greater than 0. Should we start extending our (non-internal) validation beyond what the Standard requires directly? Tagging @ldionne as well.
| _LIBCPP_ASSERT_UNCATEGORIZED( | ||
| __current != ranges::begin(__base_), "Trying to call __find_prev() on a begin iterator."); | ||
| _LIBCPP_ASSERT_UNCATEGORIZED( | ||
| // Attempting to increment an end iterator is a no-op (`__find_next` would return the same argument given to it). |
There was a problem hiding this comment.
Thanks for catching this! It is a copy-paste error.
var-const
force-pushed
the
varconst/hardening-categorization-3b
branch
from
2d219a5 to
8405233
Compare
Sometimes we essentially check the same condition twice -- for example, a class might check that an index into its vector member variable is valid before accessing it, but `vector::operator[]` contains the same check. These "redundant" checks allow catching errors closer to the source and providing a better error message, but they also impose additional overhead. Marking the "early" checks as redundant allows ignoring them in the fast mode (while still getting a guaranteed trap) while still getting better error messages in the extensive mode and above. Introducing a separate wrapper macro allows making the concept of redundant assertions orthogonal to assertion categories and retaining the actual category of a check. This is a follow-up to llvm#75918, specifically to [this discussion](llvm#75918 (comment)).
Also introduce `_LIBCPP_ASSERT_PEDANTIC` for assertions violating which results in a no-op or other benign behavior, but which may nevertheless indicate a bug in the invoking code.
Also introduce
_LIBCPP_ASSERT_PEDANTICfor assertions violating whichresults in a no-op or other benign behavior, but which may nevertheless
indicate a bug in the invoking code.