-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Fix some aliasing issues in Vec #70558
Conversation
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
self.set_len(len + slice.len()); | ||
self.get_unchecked_mut(len..).copy_from_slice(slice); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately there is no good way to grep for all places where the implicit DerefMut
coercion is used, like here. I grepped for get_unchecked_mut
and get_unchecked
and got rid of those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could maybe print the backtrace in the DerefMut
implementation and then run the tests and it will print what functions call DerefMut
?(locally obviously hehe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since DerefMut
is called for every mutable indexing operation (&mut v[i]
), I expect that to show tons of stacktraces and not be very useful.
a5aea87
to
d959640
Compare
d959640
to
3411ade
Compare
also add smoke test to detect relocation even in rustc runs
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 4eacf45 with merge fd566c8d8da22c80354a2256fdce4fa836e1f906... |
f853bca
to
deaa348
Compare
deaa348
to
5bbaac3
Compare
Let's re-try that with the latest changes. I think now I have all @bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 5bbaac3 with merge c5fdda00c8e50fa1bacea8d98dd211fcac521372... |
☀️ Try build successful - checks-azure |
Queued c5fdda00c8e50fa1bacea8d98dd211fcac521372 with parent 8926bb4, future comparison URL. |
Would a lint for implicit |
Finished benchmarking try commit c5fdda00c8e50fa1bacea8d98dd211fcac521372, comparison URL. |
Perf looks green, except for "syn-opt" where some runs get faster by 3% and some slower by 2% -- but they have a @llogiq hm, good idea. I am not sure, there might be tons of harmless implicit derefs in that file. The problematic ones are the ones for But it might be worth a shot. |
src/liballoc/vec.rs
Outdated
@@ -962,13 +963,15 @@ impl<T> Vec<T> { | |||
#[inline] | |||
#[stable(feature = "rust1", since = "1.0.0")] | |||
pub fn swap_remove(&mut self, index: usize) -> T { | |||
let len = self.len(); | |||
assert!(index < len, "index out of bounds: the len is {} but the index is {}", len, index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using formatting in the error seems to cause performance issues; let me know if you'd prefer an unformatted message.
The changes look fine, and there doesn't seem to be any perf impact. @bors r+ |
📌 Commit 3e2ac9c000cefd624d0324f7ffb16457d6246b72 has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
⌛ Testing commit 3e2ac9c000cefd624d0324f7ffb16457d6246b72 with merge e094a03952477c60b97b8fc3535872714749bec2... |
This comment has been minimized.
This comment has been minimized.
@bors retry |
That perf measurement was from before I added the panic message. |
3e2ac9c
to
7e81c11
Compare
📌 Commit 7e81c11 has been approved by |
Rollup of 5 pull requests Successful merges: - rust-lang#70558 (Fix some aliasing issues in Vec) - rust-lang#70760 (docs: make the description of Result::map_or more clear) - rust-lang#70769 (Miri: remove an outdated FIXME) - rust-lang#70776 (clarify comment in RawVec::into_box) - rust-lang#70806 (fix Miri assignment sanity check) Failed merges: r? @ghost
Vec::extend
andVec::truncate
invalidated references into the vector even without reallocation, because they (implicitly) created a mutable reference covering the entire initialized part of the vector.Fixes #70301
I verified the fix by adding some new tests here that I ran in Miri.