Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

override VecDeque::try_rfold, also update iterator #58064

Merged
merged 1 commit into from
Feb 23, 2019

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Feb 2, 2019

This keeps the slice based iteration and updates the iterator state after each slice. It also uses a loop to reduce the amount of code.

This uses unsafe code, so some thorough review would be appreciated. Cc @RalfJung

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 2, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:041ab0fa:start=1549068308862332203,finish=1549068430091833741,duration=121229501538
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:06:33]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:06:33] error[E0407]: method `try_rfold` is not a member of trait `Iterator`
[00:06:33]     --> src/liballoc/collections/vec_deque.rs:2190:5
[00:06:33]      |
[00:06:33] 2190 | /     fn try_rfold<B, F, R>(&mut self, init: B, mut f: F) -> R where
[00:06:33] 2191 | |         Self: Sized, F: FnMut(B, Self::Item) -> R, R: Try<Ok=B>
[00:06:33] 2192 | |     {
[00:06:33] 2193 | |         let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail);
[00:06:33] 2201 | |         accum
[00:06:33] 2202 | |     }
[00:06:33]      | |_____^ not a member of trait `Iterator`
[00:06:33] 
[00:06:33] 
[00:06:34] error[E0599]: no method named `offset` found for type `&'a [T]` in the current scope
[00:06:34]      |
[00:06:34]      |
[00:06:34] 2184 |             self.tail = unsafe { self.ring.offset(iter.as_slice().as_ptr()) };
[00:06:34] 
[00:06:34] error[E0308]: mismatched types
[00:06:34]     --> src/liballoc/collections/vec_deque.rs:2187:9
[00:06:34]      |
[00:06:34]      |
[00:06:34] 2176 |     fn try_fold<B, F, R>(&mut self, init: B, mut f: F) -> R where
[00:06:34]      |                                                           - expected `R` because of return type
[00:06:34] ...
[00:06:34] 2185 |             accum = res?;
[00:06:34] 2186 |         }
[00:06:34] 2187 |         accum
[00:06:34]      |         ^^^^^ expected type parameter, found a different type parameter
[00:06:34]      |
[00:06:34]      |
[00:06:34]      = note: expected type `R`
[00:06:34]                 found type `B`
[00:06:34] 
[00:06:34] error[E0599]: no method named `offset` found for type `&'a [T]` in the current scope
[00:06:34]      |
[00:06:34]      |
[00:06:34] 2198 |             self.tail = unsafe { self.ring.offset(iter.as_slice().as_ptr()) };
[00:06:34] 
[00:06:34] error[E0308]: mismatched types
[00:06:34]     --> src/liballoc/collections/vec_deque.rs:2201:9
[00:06:34]      |
[00:06:34]      |
[00:06:34] 2190 |     fn try_rfold<B, F, R>(&mut self, init: B, mut f: F) -> R where
[00:06:34]      |                                                            - expected `R` because of return type
[00:06:34] ...
[00:06:34] 2199 |             accum = res?;
[00:06:34] 2200 |         }
[00:06:34] 2201 |         accum
[00:06:34]      |         ^^^^^ expected type parameter, found a different type parameter
[00:06:34]      |
---
[00:06:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:06:36] expected success, got: exit code: 101
[00:06:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:06:36] Build completed unsuccessfully in 0:00:38
[00:06:36] make: *** [all] Error 1
[00:06:36] Makefile:18: recipe for target 'all' failed
Sat, 02 Feb 2019 00:53:56 GMT
The command "date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
" exited with 0.
travis_fold:start:after_failure.1
travis_time:start:085f7fb0
---
travis_time:end:06364963:start=1549068837530177196,finish=1549068837535200287,duration=5023091
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:11da2258
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:004cd3f2
travis_time:start:004cd3f2
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:07abceab
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril
Copy link
Contributor

Centril commented Feb 2, 2019

r? @scottmcm

@rust-highfive rust-highfive assigned scottmcm and unassigned sfackler Feb 2, 2019
@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2019
src/liballoc/collections/vec_deque.rs Outdated Show resolved Hide resolved
src/liballoc/collections/vec_deque.rs Outdated Show resolved Hide resolved
@llogiq
Copy link
Contributor Author

llogiq commented Feb 4, 2019

This should now work. I've cross-checked that it behaves like the old implemenation in all tests I wrote.

@llogiq
Copy link
Contributor Author

llogiq commented Feb 4, 2019

I think there is a difference to the old version if f panics. I might be able to correct that using a drop guard. @scottmcm do we need this? Or should we merge & add later?

I note that we make no guarantees for the panic case, so technically the current version should be correct.

@llogiq
Copy link
Contributor Author

llogiq commented Feb 4, 2019

The idea is to have a TryFoldGuard with a pointer to both the iterator and the value to update (*mut self.tail or *mut self.head, respectively, as well as the value of self.ring.as_ptr(). On drop, it writes the offset back into the value to update.

This has the benefit of potentially needing less arithmetic and being panic-safe, at the cost of more unsafe code and bookkeeping. It should be safe to alias, because the original mutable borrow of the iterator has gone out of scope when the guard drops, right?

@scottmcm
Copy link
Member

scottmcm commented Feb 4, 2019

I don't know the answer to panic-safety; opened #58170 to ask libs.

let res = front_iter.try_fold(init, &mut f);
// the slice's `try_fold` leaves the iterator where we want our `tail` to be
self.tail = unsafe { front_iter.as_slice().as_ptr()
.offset_from(self.ring.as_ptr()) as usize };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be format like this (by default rustfmt config):

        self.tail = unsafe {
            front_iter
                .as_slice()
                .as_ptr()
                .offset_from(self.ring.as_ptr()) as usize
        };

// same here
self.tail = unsafe { back_iter.as_slice().as_ptr()
.offset_from(self.ring.as_ptr()) as usize };
res

This comment was marked as outdated.

let res = back_iter.try_rfold(init, &mut f);
// the slice's `try_rfold` leaves the iterator where we want our `head` to be
self.head = unsafe { back_iter.as_slice().as_ptr()
.offset_from(self.ring.as_ptr()) as usize };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here

let res = front_iter.try_rfold(res?, f);
// same here
self.head = unsafe { front_iter.as_slice().as_ptr()
.offset_from(self.ring.as_ptr()) as usize };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

Copy link
Contributor

@tesuji tesuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits about formatting problems

@llogiq llogiq force-pushed the vec-deque-try-rfold branch 2 times, most recently from a184f53 to eade0b7 Compare February 6, 2019 06:18
@llogiq
Copy link
Contributor Author

llogiq commented Feb 6, 2019

rustfmt'd and rebased

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0fae29fc:start=1549433966052672717,finish=1549433967098480868,duration=1045808151
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:04:45]    |
[00:04:45] 23 | use collections::CollectionAllocErr;
[00:04:45]    |     ^^^^^^^^^^^ did you mean `crate::collections`?
[00:04:45]    |
[00:04:45]    = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>
[00:04:45] error[E0432]: unresolved import `raw_vec`
[00:04:45]   --> src/liballoc/collections/vec_deque.rs:24:5
[00:04:45]    |
[00:04:45] 24 | use raw_vec::RawVec;
[00:04:45] 24 | use raw_vec::RawVec;
[00:04:45]    |     ^^^^^^^ did you mean `crate::raw_vec`?
[00:04:45]    |
[00:04:45]    = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>
[00:04:45] error[E0432]: unresolved import `vec`
[00:04:45]   --> src/liballoc/collections/vec_deque.rs:25:5
[00:04:45]    |
[00:04:45] 25 | use vec::Vec;
[00:04:45] 25 | use vec::Vec;
[00:04:45]    |     ^^^ did you mean `crate::vec`?
[00:04:45]    |
[00:04:45]    = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>
[00:04:45] error: hidden lifetime parameters in types are deprecated
[00:04:45]    --> src/liballoc/collections/vec_deque.rs:971:45
[00:04:45]     |
[00:04:45]     |
[00:04:45] 971 |     pub fn drain<R>(&mut self, range: R) -> Drain<T>
[00:04:45]     |                                             ^^^^^^^^ help: indicate the anonymous lifetime: `Drain<'_, T>`
[00:04:45] 
[00:04:45] error[E0207]: the type parameter `B` is not constrained by the impl trait, self type, or predicates
[00:04:45]      |
[00:04:45]      |
[00:04:45] 2584 |         impl<A: $Bound, B> PartialEq<$Rhs> for $Lhs
[00:04:45]      |                         ^ unconstrained type parameter
[00:04:45] ...
[00:04:45] 2600 | __impl_slice_eq1! { VecDeque<A>, Vec<B> }
[00:04:45]      | ----------------------------------------- in this macro invocation
[00:04:45] error: aborting due to 5 previous errors
[00:04:45] 
[00:04:45] Some errors occurred: E0207, E0432.
[00:04:45] For more information about an error, try `rustc --explain E0207`.
[00:04:45] For more information about an error, try `rustc --explain E0207`.
[00:04:45] error: Could not compile `alloc`.
[00:04:45] warning: build failed, waiting for other jobs to finish...
[00:04:45] error: build failed
[00:04:45] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:04:45] expected success, got: exit code: 101
[00:04:45] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:45] Build completed unsuccessfully in 0:00:36
[00:04:45] Makefile:18: recipe for target 'all' failed
[00:04:45] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:13faa6a0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Feb  6 06:24:22 UTC 2019
---
travis_time:end:0a376961:start=1549434263675296141,finish=1549434263681419512,duration=6123371
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0daf3112
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0b2e3ac8
travis_time:start:0b2e3ac8
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0a85b4df
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@llogiq
Copy link
Contributor Author

llogiq commented Feb 6, 2019

Oops, it appears I made an error during the merge.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:09301e51:start=1549437069516193859,finish=1549437070536253633,duration=1020059774
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:04:52]    |
[00:04:52] 23 | use collections::CollectionAllocErr;
[00:04:52]    |     ^^^^^^^^^^^ did you mean `crate::collections`?
[00:04:52]    |
[00:04:52]    = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>
[00:04:52] error[E0432]: unresolved import `raw_vec`
[00:04:52]   --> src/liballoc/collections/vec_deque.rs:24:5
[00:04:52]    |
[00:04:52] 24 | use raw_vec::RawVec;
[00:04:52] 24 | use raw_vec::RawVec;
[00:04:52]    |     ^^^^^^^ did you mean `crate::raw_vec`?
[00:04:52]    |
[00:04:52]    = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>
[00:04:52] error[E0432]: unresolved import `vec`
[00:04:52]   --> src/liballoc/collections/vec_deque.rs:25:5
[00:04:52]    |
[00:04:52] 25 | use vec::Vec;
[00:04:52] 25 | use vec::Vec;
[00:04:52]    |     ^^^ did you mean `crate::vec`?
[00:04:52]    |
[00:04:52]    = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>
[00:04:52] error: hidden lifetime parameters in types are deprecated
[00:04:52]    --> src/liballoc/collections/vec_deque.rs:971:45
[00:04:52]     |
[00:04:52]     |
[00:04:52] 971 |     pub fn drain<R>(&mut self, range: R) -> Drain<T>
[00:04:52]     |                                             ^^^^^^^^ help: indicate the anonymous lifetime: `Drain<'_, T>`
[00:04:52] 
[00:04:52] error[E0207]: the type parameter `B` is not constrained by the impl trait, self type, or predicates
[00:04:52]      |
[00:04:52]      |
[00:04:52] 2584 |         impl<A: $Bound, B> PartialEq<$Rhs> for $Lhs
[00:04:52]      |                         ^ unconstrained type parameter
[00:04:52] ...
[00:04:52] 2600 | __impl_slice_eq1! { VecDeque<A>, Vec<B> }
[00:04:52]      | ----------------------------------------- in this macro invocation
[00:04:52] error: aborting due to 5 previous errors
[00:04:52] 
[00:04:52] Some errors occurred: E0207, E0432.
[00:04:52] For more information about an error, try `rustc --explain E0207`.
[00:04:52] For more information about an error, try `rustc --explain E0207`.
[00:04:52] error: Could not compile `alloc`.
[00:04:52] warning: build failed, waiting for other jobs to finish...
[00:04:53] error: build failed
[00:04:53] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:04:53] expected success, got: exit code: 101
[00:04:53] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:53] Build completed unsuccessfully in 0:00:37
[00:04:53] Makefile:18: recipe for target 'all' failed
[00:04:53] make: *** [all] Error 1
46352 ./src/llvm-emscripten/test/MC
45344 ./src/test/ui
43744 ./src/llvm-project/compiler-rt
42332 ./src/llvm-project/llvm/lib/Target
---
travis_time:end:10d3b9c2:start=1549437375320322081,finish=1549437375326563135,duration=6241054
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:02a61740
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:01453848
travis_time:start:01453848
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@llogiq
Copy link
Contributor Author

llogiq commented Feb 10, 2019

I think I adressed all comments now. r? @scottmcm

Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is the bottom layer of iteration, so there's actually no need to delegate to another try_fold implementation here. Maybe it'd be best to just implement it manually? It seems the delegating is perhaps too nuanced to be worth it...

self.tail = self.ring.len() - front_iter.len();
let mut back_iter = back.iter();
let res = back_iter.try_rfold(res?, f);
self.tail = back.len() - back_iter.len();
Copy link
Member

@scottmcm scottmcm Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned here again. Suppose we did

let mut v = VecDeque::with_capacity(7);
v.push_back(1);
v.push_back(1);
v.push_back(1);
v.pop_front();
v.pop_front();

Then the backing store is

_ _ 1 _ _ _ _ _
head = 2
tail = 3

so (front, back) == ([1], []), and iterating will result in setting tail = 0 - 0, leaving an iterator that thinks it contains 6 elements, and safe code would then be exposed to uninitialized values. (Not to mention that the iter implements FusedIterator, so it's not supposed to return non-None after returning None once.)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:151be792:start=1550339882495244432,finish=1550339958707367624,duration=76212123192
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:10:51] 
[01:10:51] running 119 tests
[01:11:15] .iiiii...i.....i..i...i..i.i..i.ii...i.....i..i....i..........iiii..........i...ii...i.......ii.i.i. 100/119
[01:11:19] i......iii.i.....ii
[01:11:19] 
[01:11:19]  finished in 28.336
[01:11:19] travis_fold:end:test_debuginfo

---
[01:18:11]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[01:18:12] error[E0428]: the name `test_try_fold_unit` is defined multiple times
[01:18:12]     --> src/liballoc/../liballoc/tests/vec_deque.rs:1504:1
[01:18:12]      |
[01:18:12] 1464 | fn test_try_fold_unit() {
[01:18:12]      | ----------------------- previous definition of the value `test_try_fold_unit` here
[01:18:12] 1504 | fn test_try_fold_unit() {
[01:18:12] 1504 | fn test_try_fold_unit() {
[01:18:12]      | ^^^^^^^^^^^^^^^^^^^^^^^ `test_try_fold_unit` redefined here
[01:18:12]      |
[01:18:12]      = note: `test_try_fold_unit` must be defined only once in the value namespace of this module
[01:18:17] error: aborting due to previous error
[01:18:17] 
[01:18:17] For more information about this error, try `rustc --explain E0428`.
[01:18:17] error: Could not compile `alloc`.
[01:18:17] error: Could not compile `alloc`.
[01:18:17] warning: build failed, waiting for other jobs to finish...
[01:18:25] error: build failed
[01:18:25] 
[01:18:25] 
[01:18:25] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "alloc" "--" "--quiet"
[01:18:25] 
[01:18:25] 
[01:18:25] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:18:25] Build completed unsuccessfully in 0:18:57
[01:18:25] Build completed unsuccessfully in 0:18:57
[01:18:25] make: *** [check] Error 1
[01:18:25] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:05de4540
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sat Feb 16 19:17:53 UTC 2019

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:174e70cc:start=1550400773701676845,finish=1550400901763699795,duration=128062022950
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:03:40]    Compiling rustc_msan v0.0.0 (/checkout/src/librustc_msan)
[00:04:02]    Compiling rustc-std-workspace-core v1.0.0 (/checkout/src/tools/rustc-std-workspace-core)
[00:04:04]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[00:04:04]    Compiling rustc-demangle v0.1.10
[00:04:04] error: unexpected `,` in pattern
[00:04:04]      |
[00:04:04] 2179 |         let mut iter, final_res;
[00:04:04] 2179 |         let mut iter, final_res;
[00:04:04]      |             --------^---------- help: try adding parentheses: `(mut iter, final_res)`
[00:04:04] 
[00:04:04] error: unexpected `,` in pattern
[00:04:04]      |
[00:04:04] 2223 |         let mut iter, final_res;
[00:04:04] 2223 |         let mut iter, final_res;
[00:04:04]      |             --------^---------- help: try adding parentheses: `(mut iter, final_res)`
[00:04:04]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:04:07] error: aborting due to 2 previous errors
[00:04:07] 
[00:04:07] error: Could not compile `alloc`.
[00:04:07] error: Could not compile `alloc`.
[00:04:07] 
[00:04:07] To learn more, run the command again with --verbose.
[00:04:07] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:04:07] expected success, got: exit code: 101
[00:04:07] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:07] Build completed unsuccessfully in 0:00:39
[00:04:07] Makefile:18: recipe for target 'all' failed
[00:04:07] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:004dfb48
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Feb 17 10:59:18 UTC 2019
---
travis_time:end:014b84bb:start=1550401159794792019,finish=1550401159799654957,duration=4862938
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:10b5b0c0
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:04af4e19
travis_time:start:04af4e19
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:07cc803d
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:2552edef:start=1550402196864076492,finish=1550402267931572170,duration=71067495678
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:03:46]    Compiling rustc-demangle v0.1.10
[00:03:46] error: expected one of `.`, `;`, `?`, or an operator, found `let`
[00:03:46]     --> src/liballoc/collections/vec_deque.rs:2187:13
[00:03:46]      |
[00:03:46] 2186 |             let (front, back) = self.ring.split_at(self.tail)
[00:03:46]      |                                                              - expected one of `.`, `;`, `?`, or an operator here
[00:03:46] 2187 |             let mut back_iter = back.iter();
[00:03:46] 
[00:03:46] error: expected one of `.`, `;`, `?`, or an operator, found `let`
[00:03:46]     --> src/liballoc/collections/vec_deque.rs:2231:13
[00:03:46]      |
[00:03:46]      |
[00:03:46] 2230 |             let (front, back) = self.ring.split_at(self.tail)
[00:03:46]      |                                                              - expected one of `.`, `;`, `?`, or an operator here
[00:03:46] 2231 |             let mut front_iter = front[..self.head].iter();
[00:03:46] 
[00:03:46]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:03:49] error: aborting due to 2 previous errors
[00:03:49] 
[00:03:49] 
[00:03:49] error: Could not compile `alloc`.
[00:03:49] 
[00:03:49] To learn more, run the command again with --verbose.
[00:03:49] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:49] expected success, got: exit code: 101
[00:03:49] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:03:49] Build completed unsuccessfully in 0:00:38
[00:03:49] make: *** [all] Error 1
[00:03:49] Makefile:18: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0062b25d
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Feb 17 11:21:47 UTC 2019

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0189e62d:start=1550414341396125647,finish=1550414415105717292,duration=73709591645
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:03:55]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:03:56] error[E0615]: attempted to take value of method `iter` on type `&[T]`
[00:03:56]     --> src/liballoc/collections/vec_deque.rs:2188:28
[00:03:56]      |
[00:03:56] 2188 |             let res = back.iter.try_fold(init, &mut f);
[00:03:56]      |                            ^^^^ help: use parentheses to call the method: `iter()`
[00:03:58] error: aborting due to previous error
[00:03:58] 
[00:03:58] For more information about this error, try `rustc --explain E0615`.
[00:03:58] error: Could not compile `alloc`.
[00:03:58] error: Could not compile `alloc`.
[00:03:58] 
[00:03:58] To learn more, run the command again with --verbose.
[00:03:58] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:58] expected success, got: exit code: 101
[00:03:58] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:03:58] Build completed unsuccessfully in 0:00:39
[00:03:58] make: *** [all] Error 1
[00:03:58] Makefile:18: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:160f7b64
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Feb 17 14:44:22 UTC 2019
---
travis_time:end:12f8b540:start=1550414663475853408,finish=1550414663481594249,duration=5740841
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:03de22b0
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:044f51fe
travis_time:start:044f51fe
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:026175f0
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@llogiq
Copy link
Contributor Author

llogiq commented Feb 17, 2019

I inlined the ring_slices and made the code more obvious. It seems to pass all tests.

Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really close, but I'm worried there might still be one edge case.

let (front, back) = self.ring.split_at(self.tail);
let mut back_iter = back.iter();
let res = back_iter.try_fold(init, &mut f);
self.tail = self.ring.len() - back_iter.len();
Copy link
Member

@scottmcm scottmcm Feb 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next will unchecked read from tail, so I wanted to see if I could convince myself that a back-iter of zero length wouldn't ever be seen here, but I don't think that's guaranteed. Take this test:

    use std::collections::VecDeque;

    let mut v = VecDeque::with_capacity(8);
    v.push_back(7);
    v.push_back(8);
    v.push_back(9);
    v.push_front(2);
    v.push_front(1);
    assert_eq!(v.as_slices(), (&[1, 2][..], &[7, 8, 9][..]));
    let mut it = v.iter();
    it.find(|&&x| x == 2);
    assert_eq!(it.next(), Some(&7));

That sets up the storage as

7 8 9 _ _ _ 1 2
    h       t

then the find call returns an break from the first try_fold, but after consuming all the items. So tail gets set to ring.len(), and then the function returns early from the ? on line 2191. Then the call to next reads uninitialized memory.

Does that sound right, or is there something I missed? (I didn't run this iteration locally.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I need a wrapping operation on the fold (though not on the rfold, because in that case the iterator is already placed at the correct element, right?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, since neither slice can be as big as the underlying capacity.

This keeps the slice based iteration and updates the iterator
state after each slice. It also uses a loop to reduce the amount
of code.

This uses unsafe code, so some thorough review would be
appreciated.
@scottmcm
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2019

📌 Commit 64c915e has been approved by scottmcm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 22, 2019
Centril added a commit to Centril/rust that referenced this pull request Feb 22, 2019
override `VecDeque::try_rfold`, also update iterator

This keeps the slice based iteration and updates the iterator state after each slice. It also uses a loop to reduce the amount of code.

This uses unsafe code, so some thorough review would be appreciated. Cc @RalfJung
Centril added a commit to Centril/rust that referenced this pull request Feb 22, 2019
override `VecDeque::try_rfold`, also update iterator

This keeps the slice based iteration and updates the iterator state after each slice. It also uses a loop to reduce the amount of code.

This uses unsafe code, so some thorough review would be appreciated. Cc @RalfJung
bors added a commit that referenced this pull request Feb 22, 2019
Rollup of 17 pull requests

Successful merges:

 - #57656 (Deprecate the unstable Vec::resize_default)
 - #58059 (deprecate before_exec in favor of unsafe pre_exec)
 - #58064 (override `VecDeque::try_rfold`, also update iterator)
 - #58198 (Suggest removing parentheses surrounding lifetimes)
 - #58431 (fix overlapping references in BTree)
 - #58555 (Add a note about 2018e if someone uses `try {` in 2015e)
 - #58588 (remove a bit of dead code)
 - #58589 (cleanup macro after 2018 transition)
 - #58591 (Dedup a rustdoc diagnostic construction)
 - #58600 (fix small documentation typo)
 - #58601 (Search for target_triple.json only if builtin target not found)
 - #58606 (Docs: put Future trait into spotlight)
 - #58607 (Fixes #58586: Make E0505 erronous example fail for the 2018 edition)
 - #58615 (miri: explain why we use static alignment in ref-to-place conversion)
 - #58620 (introduce benchmarks of BTreeSet.intersection)
 - #58621 (Update miri links)
 - #58632 (Make std feature list sorted)

Failed merges:

r? @ghost
@bors bors merged commit 64c915e into rust-lang:master Feb 23, 2019
@llogiq llogiq deleted the vec-deque-try-rfold branch February 24, 2019 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants