[libcxx-commits] [libcxx] 7abbd62 - [libc++] Fix proxy iterator issues that trigger an assertion in Chromium.
Konstantin Varlamov via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jul 20 18:15:59 PDT 2022
Author: Hui Xie
Date: 2022-07-20T18:05:49-07:00
New Revision: 7abbd6224b0b6089e4483a9c939be5d9a16b682b
URL: https://github.com/llvm/llvm-project/commit/7abbd6224b0b6089e4483a9c939be5d9a16b682b
DIFF: https://github.com/llvm/llvm-project/commit/7abbd6224b0b6089e4483a9c939be5d9a16b682b.diff
LOG: [libc++] Fix proxy iterator issues that trigger an assertion in Chromium.
Crash report:
https://bugs.chromium.org/p/chromium/issues/detail?id=1346012
The triggered assertion is related sorting with `v8::internal::AtomicSlot`.
`AtomicSlot` is a proxy iterator with a proxy type `AtomicSlot::Reference`
(see https://chromium.googlesource.com/v8/v8/+/9bcb5eb590643db0c1f688fea316c7f1f4786a3c/src/objects/slots-atomic-inl.h).
https://reviews.llvm.org/D130197 correctly spotted the issue in
`__iter_move` but doesn't actually fix the issue. The reason is that
`AtomicSlot::operator*` returns a prvalue `Reference`. After the fix in
D130197, the return type of `__iter_move` is `Reference&&`. But the
rvalue reference is bound to the temporary value returned by
`operator*`, which will be dangling after `__iter_move` returns.
The idea of the fix in this change is borrowed from C++17's move_iterator
https://timsong-cpp.github.io/cppwp/n4659/move.iterators#move.iterator-1
When the underlying reference is a prvalue, we just return it by value.
Differential Revision: https://reviews.llvm.org/D130212
Added:
Modified:
libcxx/include/__algorithm/iterator_operations.h
libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort_proxy.pass.cpp
Removed:
################################################################################
diff --git a/libcxx/include/__algorithm/iterator_operations.h b/libcxx/include/__algorithm/iterator_operations.h
index a3840594b8ae..b27217d5d868 100644
--- a/libcxx/include/__algorithm/iterator_operations.h
+++ b/libcxx/include/__algorithm/iterator_operations.h
@@ -66,13 +66,24 @@ struct _IterOps<_ClassicAlgPolicy> {
// iter_move
template <class _Iter>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
- // Declaring the return type is necessary for C++03, so we basically mirror what `decltype(auto)` would deduce.
- static typename remove_reference<
- typename iterator_traits<__uncvref_t<_Iter> >::reference
- >::type&& __iter_move(_Iter&& __i) {
+ // Declaring the return type is necessary for C++03, so we basically mirror what `decltype(auto)` would deduce.
+ static __enable_if_t<
+ is_reference<typename iterator_traits<__uncvref_t<_Iter> >::reference>::value,
+ typename remove_reference< typename iterator_traits<__uncvref_t<_Iter> >::reference >::type&&>
+ __iter_move(_Iter&& __i) {
return std::move(*std::forward<_Iter>(__i));
}
+ template <class _Iter>
+ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
+ // Declaring the return type is necessary for C++03, so we basically mirror what `decltype(auto)` would deduce.
+ static __enable_if_t<
+ !is_reference<typename iterator_traits<__uncvref_t<_Iter> >::reference>::value,
+ typename iterator_traits<__uncvref_t<_Iter> >::reference>
+ __iter_move(_Iter&& __i) {
+ return *std::forward<_Iter>(__i);
+ }
+
// iter_swap
template <class _Iter1, class _Iter2>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
diff --git a/libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort_proxy.pass.cpp b/libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort_proxy.pass.cpp
index 4dc872566d92..3ebcb93e59b9 100644
--- a/libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort_proxy.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort_proxy.pass.cpp
@@ -12,16 +12,120 @@
#include <cassert>
#include <vector>
+struct Cpp17ProxyIterator {
+ struct Reference {
+ int* i_;
+ Reference(int& i) : i_(&i) {}
+
+ operator int() const { return *i_; }
+
+ Reference& operator=(int i) {
+ *i_ = i;
+ return *this;
+ }
+
+ friend bool operator<(const Reference& x, const Reference& y) { return *x.i_ < *y.i_; }
+
+ friend bool operator==(const Reference& x, const Reference& y) { return *x.i_ == *y.i_; }
+
+ friend void swap(Reference x, Reference y) { std::swap(*(x.i_), *(y.i_)); }
+ };
+
+ using
diff erence_type = int;
+ using value_type = int;
+ using reference = Reference;
+ using pointer = void*;
+ using iterator_category = std::random_access_iterator_tag;
+
+ int* ptr_;
+
+ Cpp17ProxyIterator(int* ptr) : ptr_(ptr) {}
+
+ Reference operator*() const { return Reference(*ptr_); }
+
+ Cpp17ProxyIterator& operator++() {
+ ++ptr_;
+ return *this;
+ }
+
+ Cpp17ProxyIterator operator++(int) {
+ auto tmp = *this;
+ ++*this;
+ return tmp;
+ }
+
+ friend bool operator==(const Cpp17ProxyIterator& x, const Cpp17ProxyIterator& y) { return x.ptr_ == y.ptr_; }
+ friend bool operator!=(const Cpp17ProxyIterator& x, const Cpp17ProxyIterator& y) { return x.ptr_ != y.ptr_; }
+
+ Cpp17ProxyIterator& operator--() {
+ --ptr_;
+ return *this;
+ }
+
+ Cpp17ProxyIterator operator--(int) {
+ auto tmp = *this;
+ --*this;
+ return tmp;
+ }
+
+ Cpp17ProxyIterator& operator+=(
diff erence_type n) {
+ ptr_ += n;
+ return *this;
+ }
+
+ Cpp17ProxyIterator& operator-=(
diff erence_type n) {
+ ptr_ -= n;
+ return *this;
+ }
+
+ Reference operator[](
diff erence_type i) const { return Reference(*(ptr_ + i)); }
+
+ friend bool operator<(const Cpp17ProxyIterator& x, const Cpp17ProxyIterator& y) { return x.ptr_ < y.ptr_; }
+
+ friend bool operator>(const Cpp17ProxyIterator& x, const Cpp17ProxyIterator& y) { return x.ptr_ > y.ptr_; }
+
+ friend bool operator<=(const Cpp17ProxyIterator& x, const Cpp17ProxyIterator& y) { return x.ptr_ <= y.ptr_; }
+
+ friend bool operator>=(const Cpp17ProxyIterator& x, const Cpp17ProxyIterator& y) { return x.ptr_ >= y.ptr_; }
+
+ friend Cpp17ProxyIterator operator+(const Cpp17ProxyIterator& x,
diff erence_type n) {
+ return Cpp17ProxyIterator(x.ptr_ + n);
+ }
+
+ friend Cpp17ProxyIterator operator+(
diff erence_type n, const Cpp17ProxyIterator& x) {
+ return Cpp17ProxyIterator(n + x.ptr_);
+ }
+
+ friend Cpp17ProxyIterator operator-(const Cpp17ProxyIterator& x,
diff erence_type n) {
+ return Cpp17ProxyIterator(x.ptr_ - n);
+ }
+
+ friend
diff erence_type operator-(Cpp17ProxyIterator x, Cpp17ProxyIterator y) {
+ return static_cast<int>(x.ptr_ - y.ptr_);
+ }
+};
+
void test() {
// TODO: use a custom proxy iterator instead of (or in addition to) `vector<bool>`.
std::vector<bool> v(5, false);
- v[1] = true; v[3] = true;
+ v[1] = true;
+ v[3] = true;
std::sort(v.begin(), v.end());
assert(std::is_sorted(v.begin(), v.end()));
}
+void testCustomProxyIterator() {
+ int a[] = {5, 1, 3, 2, 4};
+ std::sort(Cpp17ProxyIterator(a), Cpp17ProxyIterator(a + 5));
+ assert(a[0] == 1);
+ assert(a[1] == 2);
+ assert(a[2] == 3);
+ assert(a[3] == 4);
+ assert(a[4] == 5);
+}
+
int main(int, char**) {
test();
-
+ testCustomProxyIterator();
return 0;
}
More information about the libcxx-commits
mailing list