From: Hui Xie Date: Thu, 21 Jul 2022 00:03:29 +0000 (-0700) Subject: [libc++] Fix proxy iterator issues that trigger an assertion in Chromium. X-Git-Tag: upstream/15.0.7~1027 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=7abbd6224b0b6089e4483a9c939be5d9a16b682b;p=platform%2Fupstream%2Fllvm.git [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 --- diff --git a/libcxx/include/__algorithm/iterator_operations.h b/libcxx/include/__algorithm/iterator_operations.h index a384059..b27217d 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 _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 >::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 + _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 >::reference>::value, + typename iterator_traits<__uncvref_t<_Iter> >::reference> + __iter_move(_Iter&& __i) { + return *std::forward<_Iter>(__i); + } + // iter_swap template _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 4dc8725..3ebcb93 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 #include +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(x.ptr_ - y.ptr_); + } +}; + void test() { // TODO: use a custom proxy iterator instead of (or in addition to) `vector`. std::vector 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; }