[libcxx-commits] [libcxx] [libc++][hardening] Check bounds on arithmetic in __bounded_iter (PR #78876)
Konstantin Varlamov via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Mar 11 18:40:31 PDT 2024
https://github.com/var-const updated https://github.com/llvm/llvm-project/pull/78876
>From 49d916898166842c0091cc20edabb571262d2026 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben at google.com>
Date: Sat, 20 Jan 2024 17:32:14 -0500
Subject: [PATCH 1/6] [libc++][hardening] Check bounds on arithmetic in
__bounded_iter
Previously, __bounded_iter only checked operator*. It allowed the
pointer to go out of bounds with operator++, etc., and relied on
operator* (which checked begin <= current < end) to handle everything.
This has several unfortunate consequences:
First, pointer arithmetic is UB if it goes out of bounds. So by the time
operator* checks, it may be too late and the optimizer may have done
something bad. Checking both operations is safer.
Second, std::copy and friends currently bypass bounded iterator checks.
I think the only hope we have to fix this is to key on iter + n doing a
check. See #78771 for further discussion. Note this PR is not sufficient
to fix this. It adds the output bounds check, but ends up doing it after
the memmove, which is too late.
Finally, doing these checks is actually *more* optimizable. See #78829,
which is fixed by this PR. Keeping the iterator always in bounds means
operator* can rely on some invariants and only needs to check
current != end. This aligns better with the ways that callers tend to
condition iterator operations. At least for the common case of iterating
over the entire
See https://godbolt.org/z/vEWrWEf8h for how this new __bounded_iter
impacts compiler output. The old __bounded_iter injected checks inside
the loops for all the sum() functions, which not only added a check
inside a loop, but also impeded Clang's vectorization. The new
__bounded_iter allows all the checks to be optimized out and we emit the
same code as if it wasn't here.
Not everything is ideal however. add_and_deref ends up emitting two
comparisons now instead of one. This is because a missed optimization in
Clang. I've filed #78875 for that. I suspect (with no data) that this
PR is still a net performance win because impeding ranged-for loops is
particularly egregious. But ideally we'd fix the optimizer and make
add_and_deref fine too.
There's also something funny going on with std::ranges::find which I
have not yet figured out yet, but I suspect there are some further
missed optimization opportunities.
Fixes #78829.
---
libcxx/include/__iterator/bounded_iter.h | 52 +++--
.../debug.iterator-indexing.pass.cpp | 175 +++++++++++-----
.../bounded_iter/dereference.pass.cpp | 14 +-
.../debug.iterator-indexing.pass.cpp | 188 ++++++++++++------
4 files changed, 292 insertions(+), 137 deletions(-)
diff --git a/libcxx/include/__iterator/bounded_iter.h b/libcxx/include/__iterator/bounded_iter.h
index 2a667648871c4c..2ffb6d86cfd9a4 100644
--- a/libcxx/include/__iterator/bounded_iter.h
+++ b/libcxx/include/__iterator/bounded_iter.h
@@ -31,13 +31,10 @@ _LIBCPP_BEGIN_NAMESPACE_STD
// Iterator wrapper that carries the valid range it is allowed to access.
//
// This is a simple iterator wrapper for contiguous iterators that points
-// within a [begin, end) range and carries these bounds with it. The iterator
-// ensures that it is pointing within that [begin, end) range when it is
-// dereferenced.
-//
-// Arithmetic operations are allowed and the bounds of the resulting iterator
-// are not checked. Hence, it is possible to create an iterator pointing outside
-// its range, but it is not possible to dereference it.
+// within a [begin, end] range and carries these bounds with it. The iterator
+// ensures that it is pointing within [begin, end) range when it is
+// dereferenced. It also ensures that it is never iterated outside of
+// [begin, end].
template <class _Iterator, class = __enable_if_t< __libcpp_is_contiguous_iterator<_Iterator>::value > >
struct __bounded_iter {
using value_type = typename iterator_traits<_Iterator>::value_type;
@@ -70,18 +67,20 @@ struct __bounded_iter {
private:
// Create an iterator wrapping the given iterator, and whose bounds are described
- // by the provided [begin, end) range.
+ // by the provided [begin, end] range.
//
- // This constructor does not check whether the resulting iterator is within its bounds.
- // However, it does check that the provided [begin, end) range is a valid range (that
- // is, begin <= end).
+ // Except in debug builds, the constructor does not check whether the resulting iterator
+ // is within its bounds. The container is assumed to be self-consistent.
//
// Since it is non-standard for iterators to have this constructor, __bounded_iter must
// be created via `std::__make_bounded_iter`.
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 explicit __bounded_iter(
_Iterator __current, _Iterator __begin, _Iterator __end)
: __current_(__current), __begin_(__begin), __end_(__end) {
- _LIBCPP_ASSERT_INTERNAL(__begin <= __end, "__bounded_iter(current, begin, end): [begin, end) is not a valid range");
+ _LIBCPP_ASSERT_INTERNAL(
+ __begin <= __current, "__bounded_iter(current, begin, end): current and begin are inconsistent");
+ _LIBCPP_ASSERT_INTERNAL(
+ __current <= __end, "__bounded_iter(current, begin, end): current and end are inconsistent");
}
template <class _It>
@@ -91,21 +90,25 @@ struct __bounded_iter {
// Dereference and indexing operations.
//
// These operations check that the iterator is dereferenceable, that is within [begin, end).
+ // The iterator is always within [begin, end], so we only need to check for end. This is
+ // easier to optimize out. See https://github.com/llvm/llvm-project/issues/78829
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference operator*() const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
- __in_bounds(__current_), "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
+ __current_ != __end_, "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
return *__current_;
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pointer operator->() const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
- __in_bounds(__current_), "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
+ __current_ != __end_, "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
return std::__to_address(__current_);
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference operator[](difference_type __n) const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
- __in_bounds(__current_ + __n), "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
+ __n >= __begin_ - __current_, "__bounded_iter::operator[]: Attempt to index an iterator past the start");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __n < __end_ - __current_, "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
return __current_[__n];
}
@@ -114,6 +117,8 @@ struct __bounded_iter {
// These operations do not check that the resulting iterator is within the bounds, since that
// would make it impossible to create a past-the-end iterator.
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator++() _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __current_ != __end_, "__bounded_iter::operator++: Attempt to advance an iterator past the end");
++__current_;
return *this;
}
@@ -124,6 +129,8 @@ struct __bounded_iter {
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator--() _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __current_ != __begin_, "__bounded_iter::operator--: Attempt to rewind an iterator past the start");
--__current_;
return *this;
}
@@ -134,6 +141,10 @@ struct __bounded_iter {
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator+=(difference_type __n) _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __n >= __begin_ - __current_, "__bounded_iter::operator+=: Attempt to rewind an iterator past the start");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __n <= __end_ - __current_, "__bounded_iter::operator+=: Attempt to advance an iterator past the end");
__current_ += __n;
return *this;
}
@@ -151,6 +162,10 @@ struct __bounded_iter {
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator-=(difference_type __n) _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __n <= __current_ - __begin_, "__bounded_iter::operator-=: Attempt to rewind an iterator past the start");
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __n >= __current_ - __end_, "__bounded_iter::operator-=: Attempt to advance an iterator past the end");
__current_ -= __n;
return *this;
}
@@ -197,15 +212,10 @@ struct __bounded_iter {
}
private:
- // Return whether the given iterator is in the bounds of this __bounded_iter.
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool __in_bounds(_Iterator const& __iter) const {
- return __iter >= __begin_ && __iter < __end_;
- }
-
template <class>
friend struct pointer_traits;
_Iterator __current_; // current iterator
- _Iterator __begin_, __end_; // valid range represented as [begin, end)
+ _Iterator __begin_, __end_; // valid range represented as [begin, end]
};
template <class _It>
diff --git a/libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp b/libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp
index 360e7a981a0dfe..f829412679150b 100644
--- a/libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp
+++ b/libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp
@@ -20,77 +20,156 @@ struct Foo {
int x;
};
+template <typename Iter>
+void test_iterator(Iter begin, Iter end, bool reverse) {
+ ptrdiff_t distance = std::distance(begin, end);
+
+ // Dereferencing an iterator at the end.
+ {
+ TEST_LIBCPP_ASSERT_FAILURE(
+ *end,
+ reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
+#if _LIBCPP_STD_VER >= 20
+ // In C++20 mode, std::reverse_iterator implements operator->, but not operator*, with
+ // std::prev instead of operator--. std::prev ultimately calls operator+
+ TEST_LIBCPP_ASSERT_FAILURE(
+ end->x,
+ reverse ? "__bounded_iter::operator+=: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
+#else
+ TEST_LIBCPP_ASSERT_FAILURE(
+ end->x,
+ reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
+#endif
+ }
+
+ // Incrementing an iterator past the end.
+ {
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator++: Attempt to advance an iterator past the end";
+ auto it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it++, msg);
+ it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(++it, msg);
+ }
+
+ // Decrementing an iterator past the start.
+ {
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator++: Attempt to advance an iterator past the end"
+ : "__bounded_iter::operator--: Attempt to rewind an iterator past the start";
+ auto it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it--, msg);
+ it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(--it, msg);
+ }
+
+ // Advancing past the end with operator+= and operator+.
+ {
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator-=: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator+=: Attempt to advance an iterator past the end";
+ auto it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it += 1, msg);
+ TEST_LIBCPP_ASSERT_FAILURE(end + 1, msg);
+ it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it += (distance + 1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin + (distance + 1), msg);
+ }
+
+ // Advancing past the end with operator-= and operator-.
+ {
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator+=: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator-=: Attempt to advance an iterator past the end";
+ auto it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it -= (-1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(end - (-1), msg);
+ it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it -= (-distance - 1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin - (-distance - 1), msg);
+ }
+
+ // Rewinding past the start with operator+= and operator+.
+ {
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator-=: Attempt to advance an iterator past the end"
+ : "__bounded_iter::operator+=: Attempt to rewind an iterator past the start";
+ auto it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it += (-1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin + (-1), msg);
+ it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it += (-distance - 1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(end + (-distance - 1), msg);
+ }
+
+ // Rewinding past the start with operator-= and operator-.
+ {
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator+=: Attempt to advance an iterator past the end"
+ : "__bounded_iter::operator-=: Attempt to rewind an iterator past the start";
+ auto it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it -= 1, msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin - 1, msg);
+ it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it -= (distance + 1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(end - (distance + 1), msg);
+ }
+
+ // Out-of-bounds operator[].
+ {
+ [[maybe_unused]] const char* end_msg =
+ reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator[]: Attempt to index an iterator at or past the end";
+ [[maybe_unused]] const char* past_end_msg =
+ reverse ? "__bounded_iter::operator-=: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator[]: Attempt to index an iterator at or past the end";
+ [[maybe_unused]] const char* past_start_msg =
+ reverse ? "__bounded_iter::operator-=: Attempt to advance an iterator past the end"
+ : "__bounded_iter::operator[]: Attempt to index an iterator past the start";
+ TEST_LIBCPP_ASSERT_FAILURE(begin[distance], end_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin[distance + 1], past_end_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin[-1], past_start_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin[-99], past_start_msg);
+
+ auto it = begin + 1;
+ TEST_LIBCPP_ASSERT_FAILURE(it[distance - 1], end_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(it[distance], past_end_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(it[-2], past_start_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(it[-99], past_start_msg);
+ }
+}
+
int main(int, char**) {
// span<T>::iterator
{
Foo array[] = {{0}, {1}, {2}};
std::span<Foo> const span(array, 3);
- {
- auto it = span.end();
- TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = span.end();
- TEST_LIBCPP_ASSERT_FAILURE(it->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = span.begin();
- TEST_LIBCPP_ASSERT_FAILURE(it[3], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
- }
+ test_iterator(span.begin(), span.end(), /*reverse=*/false);
}
// span<T, N>::iterator
{
Foo array[] = {{0}, {1}, {2}};
std::span<Foo, 3> const span(array, 3);
- {
- auto it = span.end();
- TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = span.end();
- TEST_LIBCPP_ASSERT_FAILURE(it->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = span.begin();
- TEST_LIBCPP_ASSERT_FAILURE(it[3], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
- }
+ test_iterator(span.begin(), span.end(), /*reverse=*/false);
}
// span<T>::reverse_iterator
{
Foo array[] = {{0}, {1}, {2}};
std::span<Foo> const span(array, 3);
- {
- auto it = span.rend();
- TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = span.rend();
- TEST_LIBCPP_ASSERT_FAILURE(it->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = span.rbegin();
- TEST_LIBCPP_ASSERT_FAILURE(it[3], "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
+ test_iterator(span.rbegin(), span.rend(), /*reverse=*/true);
}
// span<T, N>::reverse_iterator
{
Foo array[] = {{0}, {1}, {2}};
std::span<Foo, 3> const span(array, 3);
- {
- auto it = span.rend();
- TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = span.rend();
- TEST_LIBCPP_ASSERT_FAILURE(it->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = span.rbegin();
- TEST_LIBCPP_ASSERT_FAILURE(it[3], "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
+ test_iterator(span.rbegin(), span.rend(), /*reverse=*/true);
}
return 0;
diff --git a/libcxx/test/libcxx/iterators/bounded_iter/dereference.pass.cpp b/libcxx/test/libcxx/iterators/bounded_iter/dereference.pass.cpp
index 8eee4ad2f319a2..bf723f14e80a94 100644
--- a/libcxx/test/libcxx/iterators/bounded_iter/dereference.pass.cpp
+++ b/libcxx/test/libcxx/iterators/bounded_iter/dereference.pass.cpp
@@ -58,15 +58,15 @@ void test_death() {
std::__bounded_iter<Iter> const oob = std::__make_bounded_iter(Iter(e), Iter(b), Iter(e));
// operator*
- TEST_LIBCPP_ASSERT_FAILURE(*oob, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
+ TEST_LIBCPP_ASSERT_FAILURE(*oob, "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
// operator->
- TEST_LIBCPP_ASSERT_FAILURE(oob->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
+ TEST_LIBCPP_ASSERT_FAILURE(oob->x, "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
// operator[]
- TEST_LIBCPP_ASSERT_FAILURE(iter[-1], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
- TEST_LIBCPP_ASSERT_FAILURE(iter[5], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
- TEST_LIBCPP_ASSERT_FAILURE(oob[0], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
- TEST_LIBCPP_ASSERT_FAILURE(oob[1], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
- TEST_LIBCPP_ASSERT_FAILURE(oob[-6], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
+ TEST_LIBCPP_ASSERT_FAILURE(iter[-1], "__bounded_iter::operator[]: Attempt to index an iterator past the start");
+ TEST_LIBCPP_ASSERT_FAILURE(iter[5], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
+ TEST_LIBCPP_ASSERT_FAILURE(oob[0], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
+ TEST_LIBCPP_ASSERT_FAILURE(oob[1], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
+ TEST_LIBCPP_ASSERT_FAILURE(oob[-6], "__bounded_iter::operator[]: Attempt to index an iterator past the start");
}
int main(int, char**) {
diff --git a/libcxx/test/libcxx/strings/string.view/string.view.iterators/debug.iterator-indexing.pass.cpp b/libcxx/test/libcxx/strings/string.view/string.view.iterators/debug.iterator-indexing.pass.cpp
index 5064319a0aee1b..5043a88cbc3daa 100644
--- a/libcxx/test/libcxx/strings/string.view/string.view.iterators/debug.iterator-indexing.pass.cpp
+++ b/libcxx/test/libcxx/strings/string.view/string.view.iterators/debug.iterator-indexing.pass.cpp
@@ -11,82 +11,148 @@
// REQUIRES: has-unix-headers, libcpp-has-abi-bounded-iterators
// UNSUPPORTED: libcpp-hardening-mode=none
+#include <iterator>
#include <string_view>
#include "check_assertion.h"
-int main(int, char**) {
- // string_view::iterator
+template <typename Iter>
+void test_iterator(Iter begin, Iter end, bool reverse) {
+ ptrdiff_t distance = std::distance(begin, end);
+
+ // Dereferencing an iterator at the end.
{
- std::string_view const str("hello world");
- {
- auto it = str.end();
- TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = str.end();
- TEST_LIBCPP_ASSERT_FAILURE(
- it.operator->(), "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = str.begin();
- TEST_LIBCPP_ASSERT_FAILURE(it[99], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
- }
+ TEST_LIBCPP_ASSERT_FAILURE(
+ *end,
+ reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
+#if _LIBCPP_STD_VER >= 20
+ // In C++20 mode, std::reverse_iterator implements operator->, but not operator*, with
+ // std::prev instead of operator--. std::prev ultimately calls operator+
+ TEST_LIBCPP_ASSERT_FAILURE(
+ end.operator->(),
+ reverse ? "__bounded_iter::operator+=: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
+#else
+ TEST_LIBCPP_ASSERT_FAILURE(
+ end.operator->(),
+ reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
+#endif
}
- // string_view::const_iterator
+ // Incrementing an iterator past the end.
{
- std::string_view const str("hello world");
- {
- auto it = str.cend();
- TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = str.cend();
- TEST_LIBCPP_ASSERT_FAILURE(
- it.operator->(), "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = str.cbegin();
- TEST_LIBCPP_ASSERT_FAILURE(it[99], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
- }
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator++: Attempt to advance an iterator past the end";
+ auto it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it++, msg);
+ it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(++it, msg);
}
- // string_view::reverse_iterator
+ // Decrementing an iterator past the start.
{
- std::string_view const str("hello world");
- {
- auto it = str.rend();
- TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = str.rend();
- TEST_LIBCPP_ASSERT_FAILURE(
- it.operator->(), "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = str.rbegin();
- TEST_LIBCPP_ASSERT_FAILURE(it[99], "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator++: Attempt to advance an iterator past the end"
+ : "__bounded_iter::operator--: Attempt to rewind an iterator past the start";
+ auto it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it--, msg);
+ it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(--it, msg);
}
- // string_view::const_reverse_iterator
+ // Advancing past the end with operator+= and operator+.
+ {
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator-=: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator+=: Attempt to advance an iterator past the end";
+ auto it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it += 1, msg);
+ TEST_LIBCPP_ASSERT_FAILURE(end + 1, msg);
+ it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it += (distance + 1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin + (distance + 1), msg);
+ }
+
+ // Advancing past the end with operator-= and operator-.
+ {
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator+=: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator-=: Attempt to advance an iterator past the end";
+ auto it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it -= (-1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(end - (-1), msg);
+ it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it -= (-distance - 1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin - (-distance - 1), msg);
+ }
+
+ // Rewinding past the start with operator+= and operator+.
+ {
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator-=: Attempt to advance an iterator past the end"
+ : "__bounded_iter::operator+=: Attempt to rewind an iterator past the start";
+ auto it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it += (-1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin + (-1), msg);
+ it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it += (-distance - 1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(end + (-distance - 1), msg);
+ }
+
+ // Rewinding past the start with operator-= and operator-.
{
- std::string_view const str("hello world");
- {
- auto it = str.crend();
- TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = str.crend();
- TEST_LIBCPP_ASSERT_FAILURE(
- it.operator->(), "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
- }
- {
- auto it = str.crbegin();
- TEST_LIBCPP_ASSERT_FAILURE(it[99], "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
- }
+ [[maybe_unused]] const char* msg =
+ reverse ? "__bounded_iter::operator+=: Attempt to advance an iterator past the end"
+ : "__bounded_iter::operator-=: Attempt to rewind an iterator past the start";
+ auto it = begin;
+ TEST_LIBCPP_ASSERT_FAILURE(it -= 1, msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin - 1, msg);
+ it = end;
+ TEST_LIBCPP_ASSERT_FAILURE(it -= (distance + 1), msg);
+ TEST_LIBCPP_ASSERT_FAILURE(end - (distance + 1), msg);
}
+ // Out-of-bounds operator[].
+ {
+ [[maybe_unused]] const char* end_msg =
+ reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator[]: Attempt to index an iterator at or past the end";
+ [[maybe_unused]] const char* past_end_msg =
+ reverse ? "__bounded_iter::operator-=: Attempt to rewind an iterator past the start"
+ : "__bounded_iter::operator[]: Attempt to index an iterator at or past the end";
+ [[maybe_unused]] const char* past_start_msg =
+ reverse ? "__bounded_iter::operator-=: Attempt to advance an iterator past the end"
+ : "__bounded_iter::operator[]: Attempt to index an iterator past the start";
+ TEST_LIBCPP_ASSERT_FAILURE(begin[distance], end_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin[distance + 1], past_end_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin[-1], past_start_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(begin[-99], past_start_msg);
+
+ auto it = begin + 1;
+ TEST_LIBCPP_ASSERT_FAILURE(it[distance - 1], end_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(it[distance], past_end_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(it[-2], past_start_msg);
+ TEST_LIBCPP_ASSERT_FAILURE(it[-99], past_start_msg);
+ }
+}
+
+int main(int, char**) {
+ std::string_view const str("hello world");
+
+ // string_view::iterator
+ test_iterator(str.begin(), str.end(), /*reverse=*/false);
+
+ // string_view::const_iterator
+ test_iterator(str.cbegin(), str.cend(), /*reverse=*/false);
+
+ // string_view::reverse_iterator
+ test_iterator(str.rbegin(), str.rend(), /*reverse=*/true);
+
+ // string_view::const_reverse_iterator
+ test_iterator(str.crbegin(), str.crend(), /*reverse=*/true);
+
return 0;
}
>From 89e4ffc7d5c60256020b45c9f68c7c314248ddd7 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben at davidben.net>
Date: Fri, 9 Feb 2024 15:39:44 -0500
Subject: [PATCH 2/6] Update
libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp
Co-authored-by: Louis Dionne <ldionne.2 at gmail.com>
---
.../views/views.span/debug.iterator-indexing.pass.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp b/libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp
index f829412679150b..462355794f5bc9 100644
--- a/libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp
+++ b/libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp
@@ -22,7 +22,7 @@ struct Foo {
template <typename Iter>
void test_iterator(Iter begin, Iter end, bool reverse) {
- ptrdiff_t distance = std::distance(begin, end);
+ std::ptrdiff_t distance = std::distance(begin, end);
// Dereferencing an iterator at the end.
{
>From 007ff76245131cfedbf2d7fc0e8eb4056f962d08 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben at google.com>
Date: Fri, 9 Feb 2024 15:40:38 -0500
Subject: [PATCH 3/6] Rename debug.* to assert.*
---
...erator-indexing.pass.cpp => assert.iterator-indexing.pass.cpp} | 0
...erator-indexing.pass.cpp => assert.iterator-indexing.pass.cpp} | 0
2 files changed, 0 insertions(+), 0 deletions(-)
rename libcxx/test/libcxx/containers/views/views.span/{debug.iterator-indexing.pass.cpp => assert.iterator-indexing.pass.cpp} (100%)
rename libcxx/test/libcxx/strings/string.view/string.view.iterators/{debug.iterator-indexing.pass.cpp => assert.iterator-indexing.pass.cpp} (100%)
diff --git a/libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp b/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
similarity index 100%
rename from libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp
rename to libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
diff --git a/libcxx/test/libcxx/strings/string.view/string.view.iterators/debug.iterator-indexing.pass.cpp b/libcxx/test/libcxx/strings/string.view/string.view.iterators/assert.iterator-indexing.pass.cpp
similarity index 100%
rename from libcxx/test/libcxx/strings/string.view/string.view.iterators/debug.iterator-indexing.pass.cpp
rename to libcxx/test/libcxx/strings/string.view/string.view.iterators/assert.iterator-indexing.pass.cpp
>From d38a00d4ee0191cfa99ed6b0227f2b2cb7034d70 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben at google.com>
Date: Thu, 15 Feb 2024 23:16:21 -0500
Subject: [PATCH 4/6] Address review comments
---
libcxx/include/__iterator/bounded_iter.h | 31 +++++++++++++------
.../assert.iterator-indexing.pass.cpp | 2 --
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/libcxx/include/__iterator/bounded_iter.h b/libcxx/include/__iterator/bounded_iter.h
index 2ffb6d86cfd9a4..558be941cfc0a0 100644
--- a/libcxx/include/__iterator/bounded_iter.h
+++ b/libcxx/include/__iterator/bounded_iter.h
@@ -34,7 +34,17 @@ _LIBCPP_BEGIN_NAMESPACE_STD
// within a [begin, end] range and carries these bounds with it. The iterator
// ensures that it is pointing within [begin, end) range when it is
// dereferenced. It also ensures that it is never iterated outside of
-// [begin, end].
+// [begin, end]. This is important for two reasons:
+//
+// 1. It allows `operator*` and `operator++` bounds checks to be `iter != end`.
+// This is both less for the optimizer to prove, and aligns with how callers
+// typically use iterators.
+//
+// 2. Advancing an iterator out of bounds is undefined behavior (see the table
+// in [input.iterators]). In particular, when the underlying iterator is a
+// pointer, it is undefined at the language level (see [expr.add]). If
+// bounded iterators exhibited this undefined behavior, we risk compiler
+// optimizations deleting non-redundant bounds checks.
template <class _Iterator, class = __enable_if_t< __libcpp_is_contiguous_iterator<_Iterator>::value > >
struct __bounded_iter {
using value_type = typename iterator_traits<_Iterator>::value_type;
@@ -48,8 +58,8 @@ struct __bounded_iter {
// Create a singular iterator.
//
- // Such an iterator does not point to any object and is conceptually out of bounds, so it is
- // not dereferenceable. Observing operations like comparison and assignment are valid.
+ // Such an iterator point past the end of an empty span, so it is not dereferenceable.
+ // Observing operations like comparison and assignment are valid.
_LIBCPP_HIDE_FROM_ABI __bounded_iter() = default;
_LIBCPP_HIDE_FROM_ABI __bounded_iter(__bounded_iter const&) = default;
@@ -69,8 +79,8 @@ struct __bounded_iter {
// Create an iterator wrapping the given iterator, and whose bounds are described
// by the provided [begin, end] range.
//
- // Except in debug builds, the constructor does not check whether the resulting iterator
- // is within its bounds. The container is assumed to be self-consistent.
+ // The constructor does not check whether the resulting iterator is within its bounds. It is a
+ // responsibility of the container to ensure that the given bounds are valid.
//
// Since it is non-standard for iterators to have this constructor, __bounded_iter must
// be created via `std::__make_bounded_iter`.
@@ -89,9 +99,11 @@ struct __bounded_iter {
public:
// Dereference and indexing operations.
//
- // These operations check that the iterator is dereferenceable, that is within [begin, end).
- // The iterator is always within [begin, end], so we only need to check for end. This is
- // easier to optimize out. See https://github.com/llvm/llvm-project/issues/78829
+ // These operations check that the iterator is dereferenceable. Since the class invariant is
+ // that the iterator is always within `[begin, end]`, we only need to check it's not pointing to
+ // `end`. This is easier for the optimizer because it aligns with the `iter != container.end()`
+ // checks that typical callers already use (see
+ // https://github.com/llvm/llvm-project/issues/78829).
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference operator*() const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__current_ != __end_, "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
@@ -114,8 +126,7 @@ struct __bounded_iter {
// Arithmetic operations.
//
- // These operations do not check that the resulting iterator is within the bounds, since that
- // would make it impossible to create a past-the-end iterator.
+ // These operations check the iterator remains within `[begin, end]`.
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator++() _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__current_ != __end_, "__bounded_iter::operator++: Attempt to advance an iterator past the end");
diff --git a/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp b/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
index 462355794f5bc9..ef00e593845888 100644
--- a/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
+++ b/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
@@ -52,7 +52,6 @@ void test_iterator(Iter begin, Iter end, bool reverse) {
: "__bounded_iter::operator++: Attempt to advance an iterator past the end";
auto it = end;
TEST_LIBCPP_ASSERT_FAILURE(it++, msg);
- it = end;
TEST_LIBCPP_ASSERT_FAILURE(++it, msg);
}
@@ -63,7 +62,6 @@ void test_iterator(Iter begin, Iter end, bool reverse) {
: "__bounded_iter::operator--: Attempt to rewind an iterator past the start";
auto it = begin;
TEST_LIBCPP_ASSERT_FAILURE(it--, msg);
- it = begin;
TEST_LIBCPP_ASSERT_FAILURE(--it, msg);
}
>From 17cfcf19cbf9eb370b38e6e8943794fb7666f6f2 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben at google.com>
Date: Fri, 16 Feb 2024 00:04:37 -0500
Subject: [PATCH 5/6] clang-format
A lot of existing files were not formatted correctly...
---
.../assert.iterator-indexing.pass.cpp | 60 +++++++++----------
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp b/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
index ef00e593845888..d4dacb1f2f1c7c 100644
--- a/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
+++ b/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
@@ -17,7 +17,7 @@
#include "check_assertion.h"
struct Foo {
- int x;
+ int x;
};
template <typename Iter>
@@ -142,33 +142,33 @@ void test_iterator(Iter begin, Iter end, bool reverse) {
}
int main(int, char**) {
- // span<T>::iterator
- {
- Foo array[] = {{0}, {1}, {2}};
- std::span<Foo> const span(array, 3);
- test_iterator(span.begin(), span.end(), /*reverse=*/false);
- }
-
- // span<T, N>::iterator
- {
- Foo array[] = {{0}, {1}, {2}};
- std::span<Foo, 3> const span(array, 3);
- test_iterator(span.begin(), span.end(), /*reverse=*/false);
- }
-
- // span<T>::reverse_iterator
- {
- Foo array[] = {{0}, {1}, {2}};
- std::span<Foo> const span(array, 3);
- test_iterator(span.rbegin(), span.rend(), /*reverse=*/true);
- }
-
- // span<T, N>::reverse_iterator
- {
- Foo array[] = {{0}, {1}, {2}};
- std::span<Foo, 3> const span(array, 3);
- test_iterator(span.rbegin(), span.rend(), /*reverse=*/true);
- }
-
- return 0;
+ // span<T>::iterator
+ {
+ Foo array[] = {{0}, {1}, {2}};
+ std::span<Foo> const span(array, 3);
+ test_iterator(span.begin(), span.end(), /*reverse=*/false);
+ }
+
+ // span<T, N>::iterator
+ {
+ Foo array[] = {{0}, {1}, {2}};
+ std::span<Foo, 3> const span(array, 3);
+ test_iterator(span.begin(), span.end(), /*reverse=*/false);
+ }
+
+ // span<T>::reverse_iterator
+ {
+ Foo array[] = {{0}, {1}, {2}};
+ std::span<Foo> const span(array, 3);
+ test_iterator(span.rbegin(), span.rend(), /*reverse=*/true);
+ }
+
+ // span<T, N>::reverse_iterator
+ {
+ Foo array[] = {{0}, {1}, {2}};
+ std::span<Foo, 3> const span(array, 3);
+ test_iterator(span.rbegin(), span.rend(), /*reverse=*/true);
+ }
+
+ return 0;
}
>From c49a028f69ff76d016368d892086e62a488ea176 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben at davidben.net>
Date: Wed, 21 Feb 2024 09:00:22 -0500
Subject: [PATCH 6/6] Apply suggestions from code review
Co-authored-by: Will Hawkins <whh8b at obs.cr>
---
libcxx/include/__iterator/bounded_iter.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libcxx/include/__iterator/bounded_iter.h b/libcxx/include/__iterator/bounded_iter.h
index 558be941cfc0a0..7ac2e25c25d7d7 100644
--- a/libcxx/include/__iterator/bounded_iter.h
+++ b/libcxx/include/__iterator/bounded_iter.h
@@ -58,7 +58,7 @@ struct __bounded_iter {
// Create a singular iterator.
//
- // Such an iterator point past the end of an empty span, so it is not dereferenceable.
+ // Such an iterator points past the end of an empty span, so it is not dereferenceable.
// Observing operations like comparison and assignment are valid.
_LIBCPP_HIDE_FROM_ABI __bounded_iter() = default;
@@ -126,7 +126,7 @@ struct __bounded_iter {
// Arithmetic operations.
//
- // These operations check the iterator remains within `[begin, end]`.
+ // These operations check that the iterator remains within `[begin, end]`.
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator++() _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__current_ != __end_, "__bounded_iter::operator++: Attempt to advance an iterator past the end");
More information about the libcxx-commits
mailing list