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.

Sign up for GitHub

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

Jump to bottom

Fix #91306 by deriving all access from a single *mut T #91616

Merged
merged 1 commit into from
Dec 16, 2021
Merged

Fix #91306 by deriving all access from a single *mut T #91616

merged 1 commit into from
Dec 16, 2021

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Dec 7, 2021 •
edited by camelid
Loading

Fixes #91306.

The previous code is invalid because the first argument to copy_nonoverlapping is invalidated by the mutable borrow taken out to construct the second argument.

I believe this patch fixes that, and this code should now pass Miri with -Ztag-raw-pointers, but I'm currently stuck trying to run my reproducer with a this patched version of the standard library (alternatively, running Miri on the standard library tests itself would suffice). Ralf walked me through this on Zulip.

I've also added fixes for 7 more problems other than those I reported. Most of them are easy to hit by calling sort_unstable on random arrays. I don't have reproducers for every change, but they seem pretty clear-cut to me. But I did only start learning stacked borrows 2 days ago so that might be a large dash of Dunning-Kruger.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Dec 7, 2021

📌 Commit fd6c78c3112e43e262db2050bc16f5bbc0111362 has been approved by Mark-Simulacrum

@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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2021
@saethlin saethlin force-pushed the sort_unchecked-sb-fix branch 2 times, most recently from ae21d2a to 5b02faf Compare December 9, 2021 02:17
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 9, 2021
@camelid
Copy link
Member

camelid commented Dec 14, 2021

This needs to be re-reviewed since the branch was updated.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 14, 2021
@camelid
Copy link
Member

camelid commented Dec 14, 2021

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 14, 2021
Mark-Simulacrum
Mark-Simulacrum requested changes Dec 15, 2021
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Would be good to squash commits as well.

library/core/src/slice/sort.rs Outdated Show resolved Hide resolved
library/core/src/slice/sort.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum Mark-Simulacrum 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 Dec 15, 2021
@camelid
Copy link
Member

camelid commented Dec 15, 2021

I edited the PR description to mark this as auto-closing #91306 given the PR title.

@saethlin
Copy link
Member Author

saethlin commented Dec 16, 2021 •
edited
Loading

I'm very confused. The diff in GitHub doesn't look anything like what I have locally. Going to squash this down and see if the situation resolves...

@Mark-Simulacrum I think you somehow reviewed an old commit? I'm never clear on how force-pushes interact with the review/comment timeline. In any case, I've squashed down so hopefully things won't be confusing anymore.

@saethlin saethlin force-pushed the sort_unchecked-sb-fix branch from 5b02faf to 35cf0fe Compare December 16, 2021 02:05
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 16, 2021
@saethlin
Copy link
Member Author

@rustbot ready

@saethlin @Mark-Simulacrum
Fix SB problems in slice sorting
3a0fa03 
Most of these problems originate in use of get_unchecked_mut.

When calling ptr::copy_nonoverlapping, using get_unchecked_mut for both
arguments causes the borrow created to make the second pointer to invalid the
first.

The pairs of identical MaybeUninit::slice_as_mut_ptr calls similarly
invalidate each other.

There was also a similar borrow invalidation problem with the use of
slice::get_unchecked_mut to derive the pointer for the CopyOnDrop.
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 16, 2021

📌 Commit 3a0fa03 has been approved by Mark-Simulacrum

@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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2021
@bors
Copy link
Contributor

bors commented Dec 16, 2021

⌛ Testing commit 3a0fa03 with merge 5531927...

@bors
Copy link
Contributor

bors commented Dec 16, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 5531927 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 16, 2021
@bors bors merged commit 5531927 into rust-lang:master Dec 16, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 16, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5531927): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Moderate improvement in instruction counts (up to -1.3% on incr-unchanged builds of regression-31157)
  • Very large regression in instruction counts (up to 7.3% on incr-unchanged builds of ctfe-stress-4)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Dec 17, 2021
@rylev
Copy link
Member

rylev commented Dec 21, 2021

This seems likely to be noise as a result of the issue tracked in rust-lang/rustc-perf#1126 -- marking as triaged, not something we should address directly.

@rustbot label +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 21, 2021
@saethlin saethlin deleted the sort_unchecked-sb-fix branch May 16, 2022 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum requested changes

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.

Miri detects a violation of stacked borrows in slice::sort_unstable with -Zmiri-track-raw-pointers
9 participants
@saethlin @rust-highfive @Mark-Simulacrum @bors @camelid @rust-timer @rylev @apiraino @rustbot