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

Should a mutable reference which is only used for reads become invalid on reads through other ptrs? #351

Open
RalfJung opened this issue Jul 11, 2022 · 7 comments
Labels
A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows) A-SB-vs-TB Topic: Design questions where SB and TB are opposite sides of the design axis

Comments

@RalfJung
Copy link
Member

Brought up by @saethlin around here: rust-lang/rust#92092 fixes a standard library issue where

there is a mutable reference which is used only for reads becomes invalid as a result of interior mutation of the referent, where a shared reference does not become invalid.

So, this is a case where using a shared ref rather than a mutable ref was more correct, in the face of mutation inside an UnsafeCell.

I have to say I consider this fully intended, the promise of a mutable reference is uniqueness and UnsafeCell deliberately has no effect on &mut. But it is worth tracking whether violating this rule is a common issue, and whether it might be worth somehow allowing this. The most extreme version of this would in general allow reads through other pointers when using an &mut (perhaps making the &mut read-only in the process? that would be a new mechanism in the aliasing model though). But that will severely impact our even entirely remove our ability to reorder writes to mutable references.

@RalfJung RalfJung added the A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows) label Jul 11, 2022
@saethlin
Copy link
Member

saethlin commented Jul 11, 2022

I'm very mixed on whether this is important or not. On the one hand, the code that the linked PR fixes is "obviously" silly. But it was checked in to the standard library, which I would hope gets some of the best code review out there, and is generally exposed to a lot of eyeballs (seems like every day I skim the list of open PRs on rust-lang/rust I see a few fixes for typos). So this is code that sort of obviously could have been cleaned up for reasons other than soundness... but it wasn't.

But on the other hand, writing Rust code which is both generic and uses unsafe is already very difficult because of the complexity of dealing with unwinding panics from any user-provided code such as in safe traits. So now with this situation, I feel like we need to teach library authors to make sure that their tests include both panics from generic code (notably panicking drops) but also tests under Miri where the generic type exercises interior mutability in strange place (like a PartialEq impl).

Ultimately though, I don't know how often this situation comes up. I feel like the code pattern that makes this UB is both bizarre yet easily enough written, so I don't know if my lack of examples is more an indication that it's not a problem or an indication that people are simply not testing for it and generic code is full of land mines.

@digama0
Copy link

digama0 commented Jul 12, 2022

Note that one motivation for making this legal is to allow all &mut borrows to be "two-phase", using a special borrow stack element to encode this (see can &mut just always be two-phase). Under that proposal, a &mut which is only ever used for reading is essentially the same as a shared borrow in terms of the legal programs it permits.

@RalfJung
Copy link
Member Author

Under that proposal, a &mut which is only ever used for reading is essentially the same as a shared borrow in terms of the legal programs it permits.

Note that a &mut you receive as a function argument would be already "activated" though (or else we again lose the mentioned optimizations), so this is only the case within a function.

@comex
Copy link

comex commented Jul 13, 2022

Prior art: LLVM's noalias is documented as follows (emphasis added):

This indicates that memory locations accessed via pointer values based on the argument or return value are not also accessed, during the execution of the function, via pointer values not based on the argument or return value. This guarantee only holds for memory locations that are modified, by any means, during the execution of the function.

@saethlin
Copy link
Member

Isn't LLVM's noalias designed to model C's restrict (which was at one point called noalias)? I am wary of defining too much behavior and ruling out more sophisticated optimizations.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 13, 2022 via email

@RalfJung
Copy link
Member Author

The most extreme version of this would in general allow reads through other pointers when using an &mut (perhaps making the &mut read-only in the process? that would be a new mechanism in the aliasing model though).

FWIW this is exactly what happens in Tree Borrows (for other reasons: to allow reordering reads). A mutable reference that is only read from accepts as much "interference" from other references as a shared reference does.

@RalfJung RalfJung added the A-SB-vs-TB Topic: Design questions where SB and TB are opposite sides of the design axis label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows) A-SB-vs-TB Topic: Design questions where SB and TB are opposite sides of the design axis
Projects
None yet
Development

No branches or pull requests

4 participants