Consider indirect mutation during const qualification dataflow #90214
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,39 +5,44 @@ | |
| use rustc_index::bit_set::BitSet; | ||
| use rustc_middle::mir::visit::Visitor; | ||
| use rustc_middle::mir::{self, BasicBlock, Local, Location, Statement, StatementKind}; | ||
| use rustc_mir_dataflow::fmt::DebugWithContext; | ||
| use rustc_mir_dataflow::JoinSemiLattice; | ||
| use rustc_span::DUMMY_SP; | ||
|
|
||
| use std::fmt; | ||
| use std::marker::PhantomData; | ||
|
|
||
| use super::{qualifs, ConstCx, Qualif}; | ||
|
|
||
| /// A `Visitor` that propagates qualifs between locals. This defines the transfer function of | ||
| /// `FlowSensitiveAnalysis`. | ||
| /// | ||
| /// To account for indirect assignments, data flow conservatively assumes that local becomes | ||
| /// qualified immediately after it is borrowed or its address escapes. The borrow must allow for | ||
| /// mutation, which includes shared borrows of places with interior mutability. The type of | ||
| /// borrowed place must contain the qualif. | ||
| struct TransferFunction<'a, 'mir, 'tcx, Q> { | ||
| ccx: &'a ConstCx<'mir, 'tcx>, | ||
| state: &'a mut State, | ||
| _qualif: PhantomData<Q>, | ||
| } | ||
|
|
||
| impl<Q> TransferFunction<'a, 'mir, 'tcx, Q> | ||
| where | ||
| Q: Qualif, | ||
| { | ||
| fn new(ccx: &'a ConstCx<'mir, 'tcx>, state: &'a mut State) -> Self { | ||
| TransferFunction { ccx, state, _qualif: PhantomData } | ||
| } | ||
|
|
||
| fn initialize_state(&mut self) { | ||
| self.state.qualif.clear(); | ||
| self.state.borrow.clear(); | ||
|
|
||
| for arg in self.ccx.body.args_iter() { | ||
| let arg_ty = self.ccx.body.local_decls[arg].ty; | ||
| if Q::in_any_value_of_ty(self.ccx, arg_ty) { | ||
| self.state.qualif.insert(arg); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -47,15 +52,15 @@ where | |
|
|
||
| match (value, place.as_ref()) { | ||
| (true, mir::PlaceRef { local, .. }) => { | ||
| self.state.qualif.insert(local); | ||
| } | ||
|
|
||
| // For now, we do not clear the qualif if a local is overwritten in full by | ||
| // an unqualified rvalue (e.g. `y = 5`). This is to be consistent | ||
| // with aggregates where we overwrite all fields with assignments, which would not | ||
| // get this feature. | ||
| (false, mir::PlaceRef { local: _, projection: &[] }) => { | ||
| // self.state.qualif.remove(*local); | ||
| } | ||
|
|
||
| _ => {} | ||
|
|
@@ -78,6 +83,29 @@ where | |
| self.assign_qualif_direct(&return_place, qualif); | ||
| } | ||
| } | ||
|
|
||
| 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), | ||
| } | ||
| } | ||
|
|
||
| fn ref_allows_mutation(&self, kind: mir::BorrowKind, place: mir::Place<'tcx>) -> bool { | ||
| match kind { | ||
| mir::BorrowKind::Mut { .. } => true, | ||
| mir::BorrowKind::Shared | mir::BorrowKind::Shallow | mir::BorrowKind::Unique => { | ||
| self.shared_borrow_allows_mutation(place) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn shared_borrow_allows_mutation(&self, place: mir::Place<'tcx>) -> bool { | ||
| !place | ||
| .ty(self.ccx.body, self.ccx.tcx) | ||
| .ty | ||
| .is_freeze(self.ccx.tcx.at(DUMMY_SP), self.ccx.param_env) | ||
| } | ||
| } | ||
|
|
||
| impl<Q> Visitor<'tcx> for TransferFunction<'_, '_, 'tcx, Q> | ||
|
|
@@ -95,7 +123,12 @@ where | |
| // it no longer needs to be dropped. | ||
| if let mir::Operand::Move(place) = operand { | ||
| if let Some(local) = place.as_local() { | ||
| // For backward compatibility with the MaybeMutBorrowedLocals used in an earlier | ||
| // implementation we retain qualif if a local had been borrowed before. This might | ||
| // not be strictly necessary since the local is no longer initialized. | ||
| if !self.state.borrow.contains(local) { | ||
| self.state.qualif.remove(local); | ||
| } | ||
|
Comment on lines
+129
to
+131
There was a problem hiding this comment. This is pretty conservative. 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 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
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 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? |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -106,11 +139,8 @@ where | |
| rvalue: &mir::Rvalue<'tcx>, | ||
| location: Location, | ||
| ) { | ||
| let qualif = | ||
| qualifs::in_rvalue::<Q, _>(self.ccx, &mut |l| self.state.qualif.contains(l), rvalue); | ||
| if !place.is_indirect() { | ||
| self.assign_qualif_direct(place, qualif); | ||
| } | ||
|
|
@@ -120,10 +150,53 @@ where | |
| self.super_assign(place, rvalue, location); | ||
| } | ||
|
|
||
| fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { | ||
| self.super_rvalue(rvalue, location); | ||
|
|
||
| match rvalue { | ||
| mir::Rvalue::AddressOf(mt, borrowed_place) => { | ||
| if !borrowed_place.is_indirect() | ||
| && self.address_of_allows_mutation(*mt, *borrowed_place) | ||
| { | ||
| let place_ty = borrowed_place.ty(self.ccx.body, self.ccx.tcx).ty; | ||
| if Q::in_any_value_of_ty(self.ccx, place_ty) { | ||
| self.state.qualif.insert(borrowed_place.local); | ||
| self.state.borrow.insert(borrowed_place.local); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| mir::Rvalue::Ref(_, kind, borrowed_place) => { | ||
| if !borrowed_place.is_indirect() && self.ref_allows_mutation(*kind, *borrowed_place) | ||
| { | ||
| let place_ty = borrowed_place.ty(self.ccx.body, self.ccx.tcx).ty; | ||
| if Q::in_any_value_of_ty(self.ccx, place_ty) { | ||
| self.state.qualif.insert(borrowed_place.local); | ||
| self.state.borrow.insert(borrowed_place.local); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| mir::Rvalue::Cast(..) | ||
| | mir::Rvalue::ShallowInitBox(..) | ||
| | mir::Rvalue::Use(..) | ||
| | mir::Rvalue::ThreadLocalRef(..) | ||
| | mir::Rvalue::Repeat(..) | ||
| | mir::Rvalue::Len(..) | ||
| | mir::Rvalue::BinaryOp(..) | ||
| | mir::Rvalue::CheckedBinaryOp(..) | ||
| | mir::Rvalue::NullaryOp(..) | ||
| | mir::Rvalue::UnaryOp(..) | ||
| | mir::Rvalue::Discriminant(..) | ||
| | mir::Rvalue::Aggregate(..) => {} | ||
| } | ||
| } | ||
|
|
||
| fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { | ||
| match statement.kind { | ||
| StatementKind::StorageDead(local) => { | ||
| self.state.qualif.remove(local); | ||
| self.state.borrow.remove(local); | ||
| } | ||
| _ => self.super_statement(statement, location), | ||
| } | ||
|
|
@@ -136,7 +209,7 @@ where | |
| if let mir::TerminatorKind::DropAndReplace { value, place, .. } = &terminator.kind { | ||
| let qualif = qualifs::in_operand::<Q, _>( | ||
| self.ccx, | ||
| &mut |l| self.state.qualif.contains(l), | ||
| value, | ||
| ); | ||
|
|
||
|
|
@@ -145,6 +218,9 @@ where | |
| } | ||
| } | ||
|
|
||
| // We ignore borrow on drop because custom drop impls are not allowed in consts. | ||
| // FIXME: Reconsider if accounting for borrows in drops is necessary for const drop. | ||
|
|
||
| // We need to assign qualifs to the dropped location before visiting the operand that | ||
| // replaces it since qualifs can be cleared on move. | ||
| self.super_terminator(terminator, location); | ||
|
|
@@ -165,24 +241,76 @@ where | |
| FlowSensitiveAnalysis { ccx, _qualif: PhantomData } | ||
| } | ||
|
|
||
| fn transfer_function(&self, state: &'a mut State) -> TransferFunction<'a, 'mir, 'tcx, Q> { | ||
| TransferFunction::<Q>::new(self.ccx, state) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub(super) struct State { | ||
| /// Describes whether a local contains qualif. | ||
| pub qualif: BitSet<Local>, | ||
| /// Describes whether a local's address escaped and it might become qualified as a result an | ||
| /// indirect mutation. | ||
| pub borrow: BitSet<Local>, | ||
| } | ||
|
|
||
| impl State { | ||
| #[inline] | ||
| pub(super) fn contains(&self, local: Local) -> bool { | ||
| self.qualif.contains(local) | ||
| } | ||
| } | ||
|
|
||
| impl<C> DebugWithContext<C> for State { | ||
| fn fmt_with(&self, ctxt: &C, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.write_str("qualif: ")?; | ||
| self.qualif.fmt_with(ctxt, f)?; | ||
| f.write_str(" borrow: ")?; | ||
| self.borrow.fmt_with(ctxt, f)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn fmt_diff_with(&self, old: &Self, ctxt: &C, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| if self == old { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if self.qualif != old.qualif { | ||
| f.write_str("qualif: ")?; | ||
| self.qualif.fmt_diff_with(&old.qualif, ctxt, f)?; | ||
| f.write_str("\n")?; | ||
| } | ||
|
|
||
| if self.borrow != old.borrow { | ||
| f.write_str("borrow: ")?; | ||
| self.qualif.fmt_diff_with(&old.borrow, ctxt, f)?; | ||
| f.write_str("\n")?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| impl JoinSemiLattice for State { | ||
| fn join(&mut self, other: &Self) -> bool { | ||
| self.qualif.join(&other.qualif) || self.borrow.join(&other.borrow) | ||
| } | ||
| } | ||
|
|
||
| impl<Q> rustc_mir_dataflow::AnalysisDomain<'tcx> for FlowSensitiveAnalysis<'_, '_, 'tcx, Q> | ||
| where | ||
| Q: Qualif, | ||
| { | ||
| type Domain = State; | ||
|
|
||
| const NAME: &'static str = Q::ANALYSIS_NAME; | ||
|
|
||
| fn bottom_value(&self, body: &mir::Body<'tcx>) -> Self::Domain { | ||
| State { | ||
| qualif: BitSet::new_empty(body.local_decls.len()), | ||
| borrow: BitSet::new_empty(body.local_decls.len()), | ||
| } | ||
| } | ||
|
|
||
| fn initialize_start_block(&self, _body: &mir::Body<'tcx>, state: &mut Self::Domain) { | ||
|
|
||
Oops, something went wrong.
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.
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.
This seems to presuppose a particular resolution of #56604, where casting an
&mutto*constwill 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.