[libcxx-commits] [PATCH] D106735: [libc++] Fix signed overflow inside ranges::advance.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone updated this revision to Diff 361458.
Quuxplusone added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106735/new/

https://reviews.llvm.org/D106735

Files:
  libcxx/include/__iterator/advance.h
  libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp


Index: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
===================================================================
--- libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
+++ libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
@@ -16,6 +16,7 @@
 
 #include <array>
 #include <cassert>
+#include <climits>
 
 #include "test_iterators.h"
 
@@ -100,6 +101,25 @@
   assert(current.stride_count() == -current.stride_displacement());
 }
 
+struct Iota {
+    using difference_type = int;
+    using value_type = int;
+
+    int x;
+
+    constexpr int operator*() const { return x; }
+    constexpr Iota& operator++() { ++x; return *this; }
+    constexpr Iota operator++(int) { ++x; return Iota{x - 1}; }
+    constexpr bool operator==(const Iota&) const = default;
+
+    constexpr int operator-(const Iota& that) const { return x - that.x; }
+
+    constexpr Iota& operator--() { --x; return *this; }
+    constexpr Iota operator--(int) { --x; return Iota{x + 1}; }
+};
+static_assert(std::bidirectional_iterator<Iota>);
+static_assert(std::sized_sentinel_for<Iota, Iota>);
+
 constexpr bool test() {
   auto range = range_t{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
   check_forward_sized_sentinel<cpp17_input_iterator<range_t::const_iterator> >(1, {range.begin() + 1, 0}, range);
@@ -136,6 +156,19 @@
   check_forward<forward_iterator<range_t::const_iterator> >(1000, {range.end(), 990}, range);
   check_backward<bidirectional_iterator<range_t::const_iterator> >(1000, {range.begin(), -990}, range);
 
+  // regression-test that INT_MIN doesn't cause any undefined behavior
+  {
+    auto i = Iota{+1};
+    assert(std::ranges::advance(i, INT_MIN, Iota{-2}) == INT_MIN+3);
+    assert(i == Iota{-2});
+    i = Iota{+1};
+    assert(std::ranges::advance(i, -2, Iota{INT_MIN+1}) == 0);
+    assert(i == Iota{-1});
+    i = Iota{+1};
+    assert(std::ranges::advance(i, INT_MIN, Iota{INT_MIN+1}) == 0);
+    assert(i == Iota{INT_MIN+1});
+  }
+
   return true;
 }
 
Index: libcxx/include/__iterator/advance.h
===================================================================
--- libcxx/include/__iterator/advance.h
+++ libcxx/include/__iterator/advance.h
@@ -76,8 +76,8 @@
 private:
   template <class _Tp>
   _LIBCPP_HIDE_FROM_ABI
-  static constexpr _Tp __abs(_Tp __n) noexcept {
-    return __n < 0 ? -__n : __n;
+  static constexpr _Tp __magnitude_geq(_Tp __a, _Tp __b) noexcept {
+    return __a < 0 ? (__a <= __b) : (__a >= __b);
   }
 
   template <class _Ip>
@@ -158,7 +158,7 @@
     // If `S` and `I` model `sized_sentinel_for<S, I>`:
     if constexpr (sized_sentinel_for<_Sp, _Ip>) {
       // If |n| >= |bound - i|, equivalent to `ranges::advance(i, bound)`.
-      if (const auto __M = __bound - __i; __abs(__n) >= __abs(__M)) {
+      if (const auto __M = __bound - __i; __magnitude_geq(__n, __M)) {
         (*this)(__i, __bound);
         return __n - __M;
       }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D106735.361458.patch
Type: text/x-patch
Size: 3077 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20210724/181912f3/attachment.bin>