[libcxx-commits] [libcxx] [libc++] Add some _LIBCPP_ASSUMEs for bounded iterators (PR #109033)

[David Benjamin via libcxx-commits]

https://github.com/davidben updated https://github.com/llvm/llvm-project/pull/109033

>From 3666789b571b21432901778f10b5a8ceed069cec Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben at google.com>
Date: Tue, 17 Sep 2024 15:18:45 -0400
Subject: [PATCH 1/2] [libc++] Add some _LIBCPP_ASSUMEs for bounded iterators

Playing around, this seems to address #101370 for `std::vector<char>`,
but not `std::vector<int>`. `std::vector<int>` I believe also needs a
solution to #101372, which is an alignment issue.

The root problem is that vector uses end_cap instead of end as the
hardening fencepost. But user code (be it an actual `iter != vec.end()`
check, or one synthesized by the language in a range-for loop) uses the
container end as the fencepost.

We would like the user fencepost to delete the hardening fencepost. For
that to happen, the compiler must know that if you take your iterator
and then steadily `++iter`, stopping at `iter == end`, you won't hit
`iter == end_cap` along the way. To fgire this out, the compiler needs
to know a few things:

1. `iter <= end <= end_cap` at the start

2. `iter`, `end`, and `end_cap` are all compatibly aligned, such that
   `++iter` cannot skip over `end` and then get to `end_cap`.

The first of these is not obvious in `std::vector` for because
`std::vector` stores three pointers, rather than one pointer and then
sizes. That means the compiler never sees `end` (or `end_cap`) computed
as `begin + size` (or `begin + capacity`). Without type invariants, the
compiler does not know that the three pointers have any relation at all.

This PR addresses it by putting assumes in `__bounded_iter` itself. We
could also place it in `std::vector::__make_iter`, but this invariant is
important enough for reasoning about bounds that it seemed worth
establishing it across the board. (Note this means we trust container
implementations to use the bounded iterators correctly, which we already
do. We're interested in catching bugs in user code, not the STL itself.)

That alone is actually enough to handle this because constructing
`vector::end()` is enough to tell the compiler that `begin <= end`, and
loops usually start at `begin`. But since `__make_iter` is sometimes
called on non-endpoint iterators, I added one extra invariant to
`__make_iter`.

The second issue is #101372. This PR does not address it but will
(hopefully) take advantage of it once available.

In working on this, I noticed that _LIBCPP_ASSUME silences -Wassume.
Without that warning, I ended up spending a lot of time debugging
silently no-op assumes. This seems to be a remnant of when
_LIBCPP_ASSUME was part of _LIBCPP_ASSERT. Now that it's standalone, I
think we shouldn't disable the warning by default. If we ever need to
silence the warning, let's do it explicitly.
---
 libcxx/include/__assert                  |  4 +---
 libcxx/include/__iterator/bounded_iter.h | 12 ++++++++++++
 libcxx/include/__vector/vector.h         | 10 +++++++++-
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/libcxx/include/__assert b/libcxx/include/__assert
index 90eaa6023587b9..5a5cad472425c5 100644
--- a/libcxx/include/__assert
+++ b/libcxx/include/__assert
@@ -27,9 +27,7 @@
 // optimization intent. See https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609 for a
 // discussion.
 #if __has_builtin(__builtin_assume)
-#  define _LIBCPP_ASSUME(expression)                                                                                   \
-    (_LIBCPP_DIAGNOSTIC_PUSH _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wassume")                                              \
-         __builtin_assume(static_cast<bool>(expression)) _LIBCPP_DIAGNOSTIC_POP)
+#  define _LIBCPP_ASSUME(expression) __builtin_assume(static_cast<bool>(expression))
 #else
 #  define _LIBCPP_ASSUME(expression) ((void)0)
 #endif
diff --git a/libcxx/include/__iterator/bounded_iter.h b/libcxx/include/__iterator/bounded_iter.h
index d12750d1f81ac7..d05abce76fe50c 100644
--- a/libcxx/include/__iterator/bounded_iter.h
+++ b/libcxx/include/__iterator/bounded_iter.h
@@ -101,10 +101,22 @@ struct __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) {
+    // These are internal checks rather than hardening checks because the STL container is expected to ensure they are
+    // in order.
     _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");
+
+    // However, this order is important to help the compiler reason about bounds checks. For example, `std::vector` sets
+    // `__end_ptr` to the capacity, not the true container end. To translate container-end fenceposts into hardening-end
+    // fenceposts, we must know that container-end <= hardening-end. `std::__to_address` is needed because `_Iterator`
+    // may be wrapped type, such that `operator<=` has side effects.
+    pointer __begin_ptr   = std::__to_address(__begin);
+    pointer __current_ptr = std::__to_address(__current);
+    pointer __end_ptr     = std::__to_address(__end);
+    _LIBCPP_ASSUME(__begin_ptr <= __current_ptr);
+    _LIBCPP_ASSUME(__current_ptr <= __end_ptr);
   }
 
   template <class _It>
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index ae3ea1de61de01..503d231324a717 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -611,6 +611,10 @@ class _LIBCPP_TEMPLATE_VIS vector {
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator __make_iter(pointer __p) _NOEXCEPT {
 #ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
+    // `__bounded_iter` will tell the compiler that `__p` is bounded by `__begin_` and `__end_cap`, but nothing a priori
+    // relates `__p` to `__end_`.
+    _LIBCPP_ASSUME(__p <= this->__end_);
+
     // Bound the iterator according to the capacity, rather than the size.
     //
     // Vector guarantees that iterators stay valid as long as no reallocation occurs even if new elements are inserted
@@ -631,7 +635,11 @@ class _LIBCPP_TEMPLATE_VIS vector {
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_iterator __make_iter(const_pointer __p) const _NOEXCEPT {
 #ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
-    // Bound the iterator according to the capacity, rather than the size.
+    // `__bounded_iter` will tell the compiler that `__p` is bounded by `__begin_` and `__end_cap`, but nothing a priori
+    // relates `__p` to `__end_`.
+    _LIBCPP_ASSUME(__p <= this->__end_);
+
+    // Bound the iterator according to the capacity, rather than the size. See above.
     return std::__make_bounded_iter(
         std::__wrap_iter<const_pointer>(__p),
         std::__wrap_iter<const_pointer>(this->__begin_),

>From bc319a50cedd6c531f6354c49a58ef1461788cb5 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben at google.com>
Date: Wed, 25 Sep 2024 14:01:09 -0400
Subject: [PATCH 2/2] Silence GCC warnings

---
 libcxx/include/__iterator/bounded_iter.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libcxx/include/__iterator/bounded_iter.h b/libcxx/include/__iterator/bounded_iter.h
index d05abce76fe50c..642307ce9f2f77 100644
--- a/libcxx/include/__iterator/bounded_iter.h
+++ b/libcxx/include/__iterator/bounded_iter.h
@@ -117,6 +117,10 @@ struct __bounded_iter {
     pointer __end_ptr     = std::__to_address(__end);
     _LIBCPP_ASSUME(__begin_ptr <= __current_ptr);
     _LIBCPP_ASSUME(__current_ptr <= __end_ptr);
+    // Silence warnings when assumptions are disabled.
+    (void)__begin_ptr;
+    (void)__current_ptr;
+    (void)__end_ptr;
   }
 
   template <class _It>