Consider indirect mutation during const qualification dataflow #90214
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
How much of MaybeMutBorrowedLocals is duplicated now? Should we try to deduplicate or at least keep them in sync? |
There was a problem hiding this comment.
I would have taken a more minimal approach, although this is fine as well and might be more robust. Thanks for writing those tests by the way! Clearly, our coverage is not great at the moment.
My thought was to check MaybeMutBorrowedLocals whenever an assignment occurs in the Qualif transfer function in addition to checking it when inspecting the qualifs for a type. This is maybe a bit more convoluted, but it allows us to share more code. Did you consider this approach? Do you prefer the one in this PR?
I think giving mutable borrows a bit of field sensitivity is fine BTW. #73255 is another step in that direction (since drop elaboration is field sensitive), and I suspect all of Qualif will be similarly updated at some point for consistency's sake.
Comment on lines
+126
to
+131
| if !self.state.borrow.contains(local) { | ||
| self.state.qualif.remove(local); | ||
| } |
There was a problem hiding this comment.
This is pretty conservative. IS_CLEARED_ON_MOVE is only used for the Drop qualifs. Wouldn't this prevent code like the following, which I believe retains a frivolous Drop terminator for the argument, from compiling?
fn foo<T>(opt: Option<T>) -> Option<T> {
let _ = &opt; // `T` may have interior mutability
opt
}#73255 would reduce the impact by removing the frivolous drop altogether, although I believe there are still edge cases where a borrowed variable is moved out of in one CFG path and assigned an unqualified value in the other. I don't think we care about those very much, however.
Perhaps you chose to be conservative here because valid raw pointers can point to moved-from variables. I don't think it's guaranteed that the second Box in the following example will be leaked, but that's what we do currently do AFAIK:
let mut x = Box::new(42);
let px = std::ptr::addr_of_mut!(x);
let y = x; // Move `x`
unsafe { *px = Box::new(12); } // This `Box` never gets dropped.There was a problem hiding this comment.
In terms of semantics, I extrapolated the existing logic of MaybeMutBorrowedLocals and consider its effects on overall computation. For this decision it followed that:
- The analysis is field sensitive, because
MaybeMutBorrowedLocalsis field sensitive when looking for the interior mutability. - The qualifs are not cleared after move, because neither are borrows in
MaybeMutBorrowedLocals.
I think it would be fine to clear a qualif after move, under the condition that the local becomes qualified on the assignment if it was borrowed before. Though, this is something I would consider independently of this changes.
The specific example given above is already rejected though, because a local which is borrowed is the same which is dropped, so indirectly_mutable works as intended (note the missing const fn).
There was a problem hiding this comment.
True. No need to change behavior as part of this PR.
Would you mind leaving a note that this isn't fixing a known issue (assuming the raw pointers to moved-from variables doesn't count), but is there to preserve backwards compatibility?
I didn't consider this specific approach. It would indeed allow us to reuse |
|
I think I like your approach rather than the "quick fix" I described above. Yours seems easier to extend going forward, and including r=me on the dataflow stuff and overall approach. Maybe @oli-obk wants to weigh in on potential code sharing. Personally I think this version is okay, since your helper functions are nicely encapsulated. Another question for @oli-obk, it looks like const trait and thus const Drop is making progress. Was there any thought given to the implicit mutable borrow that occurs when a |
|
r? @oli-obk (see above) |
Previously a local would be qualified if either one of two separate data flow computations indicated so. First determined if a local could contain the qualif, but ignored any forms of indirect mutation. Second determined if a local could be mutably borrowed (and so indirectly mutated), but which in turn ignored the qualif. The end result was incorrect because the effect of indirect mutation was effectivelly ignored in the all but the final stage of computation. In the new implementation the indirect mutation is directly incorporated into the qualif data flow. The local variable becomes immediately qualified once it is mutably borrowed and borrowed place type can contain the qualif. In general we will now reject additional programs, program that were prevously unintentionally accepted. There are also some cases which are now accepted but were previously rejected, because previous implementation didn't consider whether borrowed place could have the qualif under the consideration.
tmiasko
force-pushed
the
indirect-mutation-qualif
branch
from
0df4b06 to
93f85f5
Compare
apiraino
added
the
T-compiler
No. I wasn't aware there was a concern here (due to ignorance, not because anything is "obvious to me"). But I believe your reasoning is correct. Whatever we called drop on may not be accessed again (beyond deallocating its memory). @bors r=ecstatic-morse,oli-obk |
|
📌 Commit 93f85f5 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 93f85f5 with merge 5ceac2fdf966a0f93b678c185849f19caae75063... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (37f70a0): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
RalfJung
reviewed
Oct 29, 2021
| fn address_of_allows_mutation(&self, mt: mir::Mutability, place: mir::Place<'tcx>) -> bool { | ||
| match mt { | ||
| mir::Mutability::Mut => true, | ||
| mir::Mutability::Not => self.shared_borrow_allows_mutation(place), |
There was a problem hiding this comment.
This seems to presuppose a particular resolution of #56604, where casting an &mut to *const will always produce an immutable pointer (except for UnsafeCell). I don't think we have officially committed to this yet.
There was a problem hiding this comment.
Do you think we should change this to be more conservative here? The semantics is inherited from previously used MaybeMutBorrowedLocals.
There was a problem hiding this comment.
Yeah I think it might be good not to assume that addr_of!(x.field) is read-only, for now. I personally think it should be read-only but I am not sure if there is consensus for that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously a local would be qualified if either one of two separate data
flow computations indicated so. First determined if a local could
contain the qualif, but ignored any forms of indirect mutation. Second
determined if a local could be mutably borrowed (and so indirectly
mutated), but which in turn ignored the qualif.
The end result was incorrect because the effect of indirect mutation was
effectivelly ignored in the all but the final stage of computation.
In the new implementation the indirect mutation is directly incorporated
into the qualif data flow. The local variable becomes immediately
qualified once it is mutably borrowed and borrowed place type can
contain the qualif.
In general we will now reject additional programs, program that were
prevously unintentionally accepted.
There are also some cases which are now accepted but were previously
rejected, because previous implementation didn't consider whether
borrowed place could have the qualif under the consideration.
Fixes #90124.
r? @ecstatic-morse