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

Use pointers in cell::{Ref,RefMut} to avoid noalias #97027

Merged
merged 6 commits into from
May 20, 2022

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented May 13, 2022

When Ref and RefMut were based on references, they would get LLVM noalias attributes that were incorrect, because that alias guarantee is only true until the guard drops. A &RefCell on the same value can get a new borrow that aliases the previous guard, possibly leading to miscompilation. Using NonNull pointers in Ref and RefCell avoids noalias.

Fixes the library side of #63787, but we still might want to explore language solutions there.

@rust-highfive

This comment was marked as resolved.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 13, 2022
@rust-highfive
Copy link
Collaborator

r? @thomcc

(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 May 13, 2022
Copy link
Contributor

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

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

Ref doesn't need a PhantomData<&T> for correctness, but it might make sense to include it for explicitness / symmetry with RefMut?

Also, I forget how NonNull acts w.r.t. UnwindSafe; this is a reminder to someone to check that it's not changed by this PR.

@@ -1548,23 +1557,20 @@ impl<'b, T: ?Sized> RefMut<'b, T> {
/// ```
#[unstable(feature = "cell_filter_map", reason = "recently added", issue = "81061")]
#[inline]
pub fn filter_map<U: ?Sized, F>(orig: RefMut<'b, T>, f: F) -> Result<RefMut<'b, U>, Self>
pub fn filter_map<U: ?Sized, F>(mut orig: RefMut<'b, T>, f: F) -> Result<RefMut<'b, U>, Self>
where
F: FnOnce(&mut T) -> Option<&mut U>,
{
// FIXME(nll-rfc#40): fix borrow-check
Copy link
Member

Choose a reason for hiding this comment

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

I assume this no longer applies now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? I'm not really sure what needed fixing -- was it that orig had to be destructured, then rebuilt in the Err case?

@cuviper
Copy link
Member Author

cuviper commented May 13, 2022

@CAD97 :

Ref doesn't need a PhantomData<&T> for correctness, but it might make sense to include it for explicitness / symmetry with RefMut?

I could just add a comment addressing covariance, but are there more effects you want from PhantomData?

Also, I forget how NonNull acts w.r.t. UnwindSafe; this is a reminder to someone to check that it's not changed by this PR.

All of the auto traits are unchanged, at least according to rustdoc. NonNull does implement UnwindSafe for T: RefUnwindSafe, but Ref and RefMut do not.

@thomcc
Copy link
Member

thomcc commented May 14, 2022

I think this is fine as-is, no need for additional PhantomData.

@bors r+

@bors
Copy link
Contributor

bors commented May 14, 2022

📌 Commit 15d8c00 has been approved by thomcc

@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 May 14, 2022
@givebk-bot

This comment was marked as spam.

@antl3x

This comment was marked as spam.

@givebk-bot

This comment was marked as spam.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 14, 2022
Use pointers in `cell::{Ref,RefMut}` to avoid `noalias`

When `Ref` and `RefMut` were based on references, they would get LLVM `noalias` attributes that were incorrect, because that alias guarantee is only true until the guard drops. A `&RefCell` on the same value can get a new borrow that aliases the previous guard, possibly leading to miscompilation. Using `NonNull` pointers in `Ref` and `RefCell` avoids `noalias`.

Fixes the library side of rust-lang#63787, but we still might want to explore language solutions there.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 15, 2022
Use pointers in `cell::{Ref,RefMut}` to avoid `noalias`

When `Ref` and `RefMut` were based on references, they would get LLVM `noalias` attributes that were incorrect, because that alias guarantee is only true until the guard drops. A `&RefCell` on the same value can get a new borrow that aliases the previous guard, possibly leading to miscompilation. Using `NonNull` pointers in `Ref` and `RefCell` avoids `noalias`.

Fixes the library side of rust-lang#63787, but we still might want to explore language solutions there.
@bors
Copy link
Contributor

bors commented May 15, 2022

⌛ Testing commit 15d8c00 with merge 0b2b6a751bfaaef94c621925159a481da0356b9d...

@bors
Copy link
Contributor

bors commented May 15, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 15, 2022
@cuviper
Copy link
Member Author

cuviper commented May 17, 2022

I took a stab at fixing natvis, but I'm just guessing...

@thomcc
Copy link
Member

thomcc commented May 19, 2022

(I'm assuming I need to r+ this again)

@bors r+

@bors
Copy link
Contributor

bors commented May 19, 2022

📌 Commit 1c3921f has been approved by thomcc

@bors
Copy link
Contributor

bors commented May 19, 2022

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@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 May 19, 2022
@bors
Copy link
Contributor

bors commented May 20, 2022

⌛ Testing commit 1c3921f with merge 4d6992b...

@bors
Copy link
Contributor

bors commented May 20, 2022

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 4d6992b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 20, 2022
@bors bors merged commit 4d6992b into rust-lang:master May 20, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 20, 2022
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #97027!

Tested on commit 4d6992b.
Direct link to PR: #97027

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request May 20, 2022
Tested on commit rust-lang/rust@4d6992b.
Direct link to PR: <rust-lang/rust#97027>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4d6992b): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 1 1 0 2 1
mean2 0.1% 3.3% N/A -3.1% 0.1%
max 0.1% 3.3% N/A -3.2% 0.1%

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 1 0 2 0
mean2 N/A 1.7% N/A -2.4% N/A
max N/A 1.7% N/A -3.0% N/A

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

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes 2

  2. the arithmetic mean of the percent change 2

@RalfJung
Copy link
Member

The Miri tests fail because this affects the Display output of Ref and RefMut: they used to print the value, now they print the address of the RefCell. That is probably a bug?

@RalfJung RalfJung mentioned this pull request May 20, 2022
bors added a commit to rust-lang/miri that referenced this pull request May 20, 2022
rustup

`Display` of `Ref`/`RefMut` was broken by rust-lang/rust#97027, let's deref them to use the underlying reference `Display`.

Cc rust-lang/rust#97204
@cuviper
Copy link
Member Author

cuviper commented May 20, 2022

Ah, yes, I'll fix that. The Debug impls use Deref, but Display is directly reaching for self.value (now a pointer).

cuviper added a commit to cuviper/rust that referenced this pull request May 20, 2022
These guards changed to pointers in rust-lang#97027, but their `Display` was
formatting that field directly, which made it show the raw pointer
value. Now we go through `Deref` to display the real value again.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 22, 2022
Fix `Display` for `cell::{Ref,RefMut}`

These guards changed to pointers in rust-lang#97027, but their `Display` was
formatting that field directly, which made it show the raw pointer
value. Now we go through `Deref` to display the real value again.

Miri noticed this change, rust-lang#97204, so hopefully that will be fixed.
@cuviper cuviper deleted the yesalias-refcell branch October 15, 2022 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.