[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 9bcb5eb590/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
This commit is contained in:
Hui Xie 2022-07-20 17:03:29 -07:00 committed by Konstantin Varlamov
parent f6b5f24c19
commit 7abbd6224b
2 changed files with 121 additions and 6 deletions

View File

@ -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

View File

@ -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 difference_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+=(difference_type n) {
ptr_ += n;
return *this;
}
Cpp17ProxyIterator& operator-=(difference_type n) {
ptr_ -= n;
return *this;
}
Reference operator[](difference_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, difference_type n) {
return Cpp17ProxyIterator(x.ptr_ + n);
}
friend Cpp17ProxyIterator operator+(difference_type n, const Cpp17ProxyIterator& x) {
return Cpp17ProxyIterator(n + x.ptr_);
}
friend Cpp17ProxyIterator operator-(const Cpp17ProxyIterator& x, difference_type n) {
return Cpp17ProxyIterator(x.ptr_ - n);
}
friend difference_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;
}