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

Vec: avoid creating slices to the elements #61114

Merged
merged 2 commits into from May 26, 2019

Conversation

Projects
None yet
6 participants
@RalfJung
Copy link
Member

commented May 24, 2019

Instead of self.deref_mut().as_mut_ptr() to get a raw pointer to the buffer, use self.buf.ptr_mut(). This (a) avoids creating a unique reference to all existing elements without any need, and (b) creates a pointer that can actually be used for the entire buffer, and not just for the part of it covered by self.deref_mut().

I also got worried about RawVec::ptr returning a *mut T from an &self, so I added both a mutable and an immutable version.

Cc @Gankro in particular for the assume changes -- I don't know why that is not in Unique, but I moved it up from Vec::deref to RawVec::ptr to avoid having to repeat it everywhere.

Fixes #60847

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented May 24, 2019

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:1b2a3436:start=1558682796455696403,finish=1558682797225924393,duration=770227990
$ 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
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---

[00:04:41] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:42] tidy error: /checkout/src/liballoc/tests/vec.rs:1163: line longer than 100 chars
[00:04:46] some tidy checks failed
[00:04:46] 
[00:04:46] 
[00:04:46] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:46] 
[00:04:46] 
[00:04:46] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:46] Build completed unsuccessfully in 0:01:12
[00:04:46] Build completed unsuccessfully in 0:01:12
[00:04:46] make: *** [tidy] Error 1
[00:04:46] Makefile:67: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2d360642
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri May 24 07:31:34 UTC 2019
---
travis_time:end:1f7f86e9:start=1558683095050265808,finish=1558683095054971336,duration=4705528
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:06a5ca68
$ 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:016ff896
travis_time:start:016ff896
$ 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:04e983d2
$ 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)

@RalfJung RalfJung force-pushed the RalfJung:vec branch from 64a0e07 to 98afa8f May 24, 2019

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented May 24, 2019

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:001d36cb:start=1558684546408943655,finish=1558684547220886444,duration=811942789
$ 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
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:07:33]    Compiling arena v0.0.0 (/checkout/src/libarena)
[00:07:33] error[E0308]: mismatched types
[00:07:33]   --> src/libarena/lib.rs:91:9
[00:07:33]    |
[00:07:33] 90 |     fn start(&self) -> *mut T {
[00:07:33]    |                        ------ expected `*mut T` because of return type
[00:07:33]    |         ^^^^^^^^^^^^^^^^^^ types differ in mutability
[00:07:33]    |
[00:07:33]    = note: expected type `*mut T`
[00:07:33]               found type `*const T`
---
travis_time:end:0301103a:start=1558685030399082758,finish=1558685030403830623,duration=4747865
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:187e9707
$ 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:30c7d896
[

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)

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

There's just too many things using RawVec, I am going to give up on that "better mutability tracking". Not sure if y'all would want that anyway.

@RalfJung RalfJung force-pushed the RalfJung:vec branch 2 times, most recently from 684f4a5 to 487a108 May 24, 2019

@@ -194,7 +195,9 @@ impl<T, A: Alloc> RawVec<T, A> {
/// Unique::empty() if `cap = 0` or T is zero-sized. In the former case, you must
/// be careful.
pub fn ptr(&self) -> *mut T {

This comment has been minimized.

Copy link
@sfackler

sfackler May 24, 2019

Member

Should this just return a NonNull<T>?

This comment has been minimized.

Copy link
@RalfJung

RalfJung May 24, 2019

Author Member

Maybe? There might be a reason for why this looks the way it does. Possibly because NonNull is not very ergonomic.

This comment has been minimized.

Copy link
@Gankro

Gankro May 24, 2019

Contributor

yeah, NonNull is only useful as a storage type, it's useless for doing actual work.

@rkruppe

This comment has been minimized.

Copy link
Member

commented May 24, 2019

(b) creates a pointer that can actually be used for the entire buffer, and not just for the part of it covered by self.deref_mut().

This seems like it would also be a problem for anyone outside liballoc who uses as_mut_ptr to e.g.

  • initialize parts of a vector before calling set_len
  • disassemble a vector and later reconstruct it with Vec::from_raw_parts
  • keep a cursor into the Vec as its length changes (similar to the test you added, but moving the pointer around, into portions of the buffer that are added later)

Is that right? And the workaround used here (accessing the internal RawVec) isn't available outside liballoc, right?

So perhaps we should add Vec::as_mut_ptr that returns a pointer covering the vector's entire buffer, not just the 0..len portion? (Plus Vec::as_ptr for symmetry, I guess.)

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

Yes that is correct. Vec::as_mut_ptr (via DerefMut) may only be used to access the elements 0..len with len taken at the time of the invocation.

That's what Stacked Borrows says, anyway. ;) This might also be an indication that the model is too strict? The desired interaction with unsized types is not really clear. One custom DST are a thing, I doubt we want Stacked Borrows to run the arbitrary user code to compute the size...

@rkruppe

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Assuming that this restriction on the use of <[T]>::{as_ptr, as_mut_ptr} prevails in some form, what do you think about (rephrasing the last part of my previous comment for clarity) adding inherent methods as_ptr, as_mut_ptr to Vec that shadow the slice methods and differ only in that they return a pointer that may be used to access elements 0..capacity?

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

That sounds very reasonable!

I didn't do that here because I was not sure how that shadowing would work out.

@Gankro

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

re: assume: I believe historically we found that llvm gets really overexcited when it finds assumes and wastes waaay too much time trying to profitably use them. So we moved to only inserting them in strategic hotspots, instead of everywhere we could.

@Gankro

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Also I agree with @rkruppe. If as_ptr is a semantic footgun for the implementation, it's a footgun for our users too. We should add the shadowing implementation (which works fine, Vec shadows slice methods in a few places already).

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented May 25, 2019

I will have to make them insta-stable though to not regress people doing vec.as_mut_ptr(), right?

@RalfJung RalfJung force-pushed the RalfJung:vec branch from 487a108 to 428ab7e May 25, 2019

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented May 25, 2019

All right, I rewrote this PR to add shadowing methods instead. I also moved the assume there.

@@ -1754,7 +1819,6 @@ impl<T> IntoIterator for Vec<T> {
fn into_iter(mut self) -> IntoIter<T> {
unsafe {
let begin = self.as_mut_ptr();
assume(!begin.is_null());

This comment has been minimized.

Copy link
@RalfJung

RalfJung May 25, 2019

Author Member

as_mut_ptr now contains an assume so I think I can remove this? Unfortunately whoever added this did not leave a comment, because it seemed redundant before already.

@sfackler

This comment has been minimized.

Copy link
Member

commented May 25, 2019

r? @Gankro

@rust-highfive rust-highfive assigned Gankro and unassigned sfackler May 25, 2019

@Gankro

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

@Gankro: 🔑 Insufficient privileges: Not in reviewers

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented May 25, 2019

@bors r=Gankro

@bors

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

📌 Commit 428ab7e has been approved by Gankro

Centril added a commit to Centril/rust that referenced this pull request May 25, 2019

Rollup merge of rust-lang#61114 - RalfJung:vec, r=Gankro
Vec: avoid creating slices to the elements

Instead of `self.deref_mut().as_mut_ptr()` to get a raw pointer to the buffer, use `self.buf.ptr_mut()`. This (a) avoids creating a unique reference to all existing elements without any need, and (b) creates a pointer that can actually be used for the *entire* buffer, and not just for the part of it covered by `self.deref_mut()`.

I also got worried about `RawVec::ptr` returning a `*mut T` from an `&self`, so I added both a mutable and an immutable version.

Cc @Gankro in particular for the `assume` changes -- I don't know why that is not in `Unique`, but I moved it up from `Vec::deref` to `RawVec::ptr` to avoid having to repeat it everywhere.

Fixes rust-lang#60847

Centril added a commit to Centril/rust that referenced this pull request May 26, 2019

Rollup merge of rust-lang#61114 - RalfJung:vec, r=Gankro
Vec: avoid creating slices to the elements

Instead of `self.deref_mut().as_mut_ptr()` to get a raw pointer to the buffer, use `self.buf.ptr_mut()`. This (a) avoids creating a unique reference to all existing elements without any need, and (b) creates a pointer that can actually be used for the *entire* buffer, and not just for the part of it covered by `self.deref_mut()`.

I also got worried about `RawVec::ptr` returning a `*mut T` from an `&self`, so I added both a mutable and an immutable version.

Cc @Gankro in particular for the `assume` changes -- I don't know why that is not in `Unique`, but I moved it up from `Vec::deref` to `RawVec::ptr` to avoid having to repeat it everywhere.

Fixes rust-lang#60847

bors added a commit that referenced this pull request May 26, 2019

Auto merge of #61201 - Centril:rollup-975knrk, r=Centril
Rollup of 9 pull requests

Successful merges:

 - #61087 (Tweak `self` arg not as first argument of a method diagnostic)
 - #61114 (Vec: avoid creating slices to the elements)
 - #61144 (Suggest borrowing for loop head on move error)
 - #61149 (Fix spelling in release notes)
 - #61161 (MaybeUninit doctest: remove unnecessary type ascription)
 - #61173 (Auto-derive Encode and Decode implementations of DefPathTable)
 - #61184 (Add additional trace statements to the const propagator)
 - #61189 (Turn turbo 🐟 🍨 into an error)
 - #61193 (Add comment to explain why we change the layout for Projection)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request May 26, 2019

Auto merge of #61201 - Centril:rollup-975knrk, r=Centril
Rollup of 9 pull requests

Successful merges:

 - #61087 (Tweak `self` arg not as first argument of a method diagnostic)
 - #61114 (Vec: avoid creating slices to the elements)
 - #61144 (Suggest borrowing for loop head on move error)
 - #61149 (Fix spelling in release notes)
 - #61161 (MaybeUninit doctest: remove unnecessary type ascription)
 - #61173 (Auto-derive Encode and Decode implementations of DefPathTable)
 - #61184 (Add additional trace statements to the const propagator)
 - #61189 (Turn turbo 🐟 🍨 into an error)
 - #61193 (Add comment to explain why we change the layout for Projection)

Failed merges:

r? @ghost

@bors bors merged commit 428ab7e into rust-lang:master May 26, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@RalfJung RalfJung deleted the RalfJung:vec branch May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.