From fc5c2956b11e29f931cec010e3f38461ec4ac309 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 12 Feb 2020 13:33:43 -0800 Subject: [PATCH 01/12] Impl `GenKill` for old dataflow framework's `GenKillSet` This impl is temporary and will be removed along with the old dataflow framework. It allows us to reuse the transfer function of new dataflow analyses when defining old ones --- src/librustc_mir/dataflow/generic/mod.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/librustc_mir/dataflow/generic/mod.rs b/src/librustc_mir/dataflow/generic/mod.rs index ea643042c5f86..23b22550a3b0e 100644 --- a/src/librustc_mir/dataflow/generic/mod.rs +++ b/src/librustc_mir/dataflow/generic/mod.rs @@ -395,5 +395,16 @@ impl GenKill for BitSet { } } +// For compatibility with old framework +impl GenKill for crate::dataflow::GenKillSet { + fn gen(&mut self, elem: T) { + self.gen(elem); + } + + fn kill(&mut self, elem: T) { + self.kill(elem); + } +} + #[cfg(test)] mod tests; From 7d9dadcccca9efc63f30fa1c9adee00effb860d4 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 12 Feb 2020 13:36:47 -0800 Subject: [PATCH 02/12] Implement `Maybe{Mut,}BorrowedLocals` analyses --- .../dataflow/impls/borrowed_locals.rs | 268 ++++++++++++++---- src/librustc_mir/dataflow/mod.rs | 3 +- 2 files changed, 209 insertions(+), 62 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrowed_locals.rs b/src/librustc_mir/dataflow/impls/borrowed_locals.rs index 63834d0ecda00..519db963d1e7d 100644 --- a/src/librustc_mir/dataflow/impls/borrowed_locals.rs +++ b/src/librustc_mir/dataflow/impls/borrowed_locals.rs @@ -1,102 +1,250 @@ pub use super::*; -use crate::dataflow::{BitDenotation, GenKillSet}; +use crate::dataflow::generic::{AnalysisDomain, GenKill, GenKillAnalysis}; use rustc::mir::visit::Visitor; use rustc::mir::*; +use rustc::ty::{ParamEnv, TyCtxt}; +use rustc_span::DUMMY_SP; + +pub type MaybeMutBorrowedLocals<'mir, 'tcx> = MaybeBorrowedLocals>; + +/// A dataflow analysis that tracks whether a pointer or reference could possibly exist that points +/// to a given local. +/// +/// The `K` parameter determines what kind of borrows are tracked. By default, +/// `MaybeBorrowedLocals` looks for *any* borrow of a local. If you are only interested in borrows +/// that might allow mutation, use the `MaybeMutBorrowedLocals` type alias instead. +/// +/// At present, this is used as a very limited form of alias analysis. For example, +/// `MaybeBorrowedLocals` is used to compute which locals are live during a yield expression for +/// immovable generators. `MaybeMutBorrowedLocals` is used during const checking to prove that a +/// local has not been mutated via indirect assignment (e.g., `*p = 42`), the side-effects of a +/// function call or inline assembly. +pub struct MaybeBorrowedLocals { + kind: K, +} -/// This calculates if any part of a MIR local could have previously been borrowed. -/// This means that once a local has been borrowed, its bit will be set -/// from that point and onwards, until we see a StorageDead statement for the local, -/// at which points there is no memory associated with the local, so it cannot be borrowed. -/// This is used to compute which locals are live during a yield expression for -/// immovable generators. -#[derive(Copy, Clone)] -pub struct HaveBeenBorrowedLocals<'a, 'tcx> { - body: &'a Body<'tcx>, +impl MaybeBorrowedLocals { + /// A dataflow analysis that records whether a pointer or reference exists that may alias the + /// given local. + pub fn new() -> Self { + MaybeBorrowedLocals { kind: AnyBorrow } + } } -impl<'a, 'tcx> HaveBeenBorrowedLocals<'a, 'tcx> { - pub fn new(body: &'a Body<'tcx>) -> Self { - HaveBeenBorrowedLocals { body } +impl MaybeMutBorrowedLocals<'mir, 'tcx> { + /// A dataflow analysis that records whether a pointer or reference exists that may *mutably* + /// alias the given local. + pub fn new_mut_only( + tcx: TyCtxt<'tcx>, + body: &'mir mir::Body<'tcx>, + param_env: ParamEnv<'tcx>, + ) -> Self { + MaybeBorrowedLocals { kind: MutBorrow { body, tcx, param_env } } } +} - pub fn body(&self) -> &Body<'tcx> { - self.body +impl MaybeBorrowedLocals { + fn transfer_function<'a, T>(&'a self, trans: &'a mut T) -> TransferFunction<'a, T, K> { + TransferFunction { kind: &self.kind, trans } } } -impl<'a, 'tcx> BitDenotation<'tcx> for HaveBeenBorrowedLocals<'a, 'tcx> { +impl AnalysisDomain<'tcx> for MaybeBorrowedLocals +where + K: BorrowAnalysisKind<'tcx>, +{ type Idx = Local; - fn name() -> &'static str { - "has_been_borrowed_locals" + + const NAME: &'static str = K::ANALYSIS_NAME; + + fn bits_per_block(&self, body: &mir::Body<'tcx>) -> usize { + body.local_decls().len() + } + + fn initialize_start_block(&self, _: &mir::Body<'tcx>, _: &mut BitSet) { + // No locals are aliased on function entry + } +} + +impl GenKillAnalysis<'tcx> for MaybeBorrowedLocals +where + K: BorrowAnalysisKind<'tcx>, +{ + fn statement_effect( + &self, + trans: &mut impl GenKill, + statement: &mir::Statement<'tcx>, + location: Location, + ) { + self.transfer_function(trans).visit_statement(statement, location); } - fn bits_per_block(&self) -> usize { - self.body.local_decls.len() + + fn terminator_effect( + &self, + trans: &mut impl GenKill, + terminator: &mir::Terminator<'tcx>, + location: Location, + ) { + self.transfer_function(trans).visit_terminator(terminator, location); } - fn start_block_effect(&self, _on_entry: &mut BitSet) { - // Nothing is borrowed on function entry + fn call_return_effect( + &self, + _trans: &mut impl GenKill, + _block: mir::BasicBlock, + _func: &mir::Operand<'tcx>, + _args: &[mir::Operand<'tcx>], + _dest_place: &mir::Place<'tcx>, + ) { } +} - fn statement_effect(&self, trans: &mut GenKillSet, loc: Location) { - let stmt = &self.body[loc.block].statements[loc.statement_index]; +impl BottomValue for MaybeBorrowedLocals { + // bottom = unborrowed + const BOTTOM_VALUE: bool = false; +} - BorrowedLocalsVisitor { trans }.visit_statement(stmt, loc); +/// A `Visitor` that defines the transfer function for `MaybeBorrowedLocals`. +struct TransferFunction<'a, T, K> { + trans: &'a mut T, + kind: &'a K, +} - // StorageDead invalidates all borrows and raw pointers to a local - match stmt.kind { - StatementKind::StorageDead(l) => trans.kill(l), - _ => (), +impl Visitor<'tcx> for TransferFunction<'a, T, K> +where + T: GenKill, + K: BorrowAnalysisKind<'tcx>, +{ + fn visit_statement(&mut self, stmt: &Statement<'tcx>, location: Location) { + self.super_statement(stmt, location); + + // When we reach a `StorageDead` statement, we can assume that any pointers to this memory + // are now invalid. + if let StatementKind::StorageDead(local) = stmt.kind { + self.trans.kill(local); } } - fn terminator_effect(&self, trans: &mut GenKillSet, loc: Location) { - let terminator = self.body[loc.block].terminator(); - BorrowedLocalsVisitor { trans }.visit_terminator(terminator, loc); - match &terminator.kind { - // Drop terminators borrows the location - TerminatorKind::Drop { location, .. } - | TerminatorKind::DropAndReplace { location, .. } => { - if let Some(local) = find_local(location) { - trans.gen(local); + 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.kind.in_address_of(*mt, borrowed_place) { + self.trans.gen(borrowed_place.local); } } - _ => (), + + mir::Rvalue::Ref(_, kind, borrowed_place) => { + if !borrowed_place.is_indirect() && self.kind.in_ref(*kind, borrowed_place) { + self.trans.gen(borrowed_place.local); + } + } + + mir::Rvalue::Cast(..) + | mir::Rvalue::Use(..) + | 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 propagate_call_return( - &self, - _in_out: &mut BitSet, - _call_bb: mir::BasicBlock, - _dest_bb: mir::BasicBlock, - _dest_place: &mir::Place<'tcx>, - ) { - // Nothing to do when a call returns successfully + fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, location: Location) { + self.super_terminator(terminator, location); + + match terminator.kind { + // Drop terminators may call custom drop glue (`Drop::drop`), which takes `&mut self` + // as a parameter. Hypothetically, a drop impl could launder that reference into the + // surrounding environment through a raw pointer, thus creating a valid `*mut` pointing + // to the dropped local. We are not yet willing to declare this particular case UB, so + // we must treat all dropped locals as mutably borrowed for now. See discussion on + // [#61069]. + // + // [#61069]: https://github.com/rust-lang/rust/pull/61069 + mir::TerminatorKind::Drop { location: dropped_place, .. } + | mir::TerminatorKind::DropAndReplace { location: dropped_place, .. } => { + self.trans.gen(dropped_place.local); + } + + TerminatorKind::Abort + | TerminatorKind::Assert { .. } + | TerminatorKind::Call { .. } + | TerminatorKind::FalseEdges { .. } + | TerminatorKind::FalseUnwind { .. } + | TerminatorKind::GeneratorDrop + | TerminatorKind::Goto { .. } + | TerminatorKind::Resume + | TerminatorKind::Return + | TerminatorKind::SwitchInt { .. } + | TerminatorKind::Unreachable + | TerminatorKind::Yield { .. } => {} + } } } -impl<'a, 'tcx> BottomValue for HaveBeenBorrowedLocals<'a, 'tcx> { - // bottom = unborrowed - const BOTTOM_VALUE: bool = false; +pub struct AnyBorrow; + +pub struct MutBorrow<'mir, 'tcx> { + tcx: TyCtxt<'tcx>, + body: &'mir Body<'tcx>, + param_env: ParamEnv<'tcx>, } -struct BorrowedLocalsVisitor<'gk> { - trans: &'gk mut GenKillSet, +impl MutBorrow<'mir, 'tcx> { + // `&` and `&raw` only allow mutation if the borrowed place is `!Freeze`. + // + // This assumes that it is UB to take the address of a struct field whose type is + // `Freeze`, then use pointer arithmetic to derive a pointer to a *different* field of + // that same struct whose type is `!Freeze`. If we decide that this is not UB, we will + // have to check the type of the borrowed **local** instead of the borrowed **place** + // below. See [rust-lang/unsafe-code-guidelines#134]. + // + // [rust-lang/unsafe-code-guidelines#134]: https://github.com/rust-lang/unsafe-code-guidelines/issues/134 + fn shared_borrow_allows_mutation(&self, place: &Place<'tcx>) -> bool { + !place.ty(self.body, self.tcx).ty.is_freeze(self.tcx, self.param_env, DUMMY_SP) + } } -fn find_local(place: &Place<'_>) -> Option { - if !place.is_indirect() { Some(place.local) } else { None } +pub trait BorrowAnalysisKind<'tcx> { + const ANALYSIS_NAME: &'static str; + + fn in_address_of(&self, mt: Mutability, place: &Place<'tcx>) -> bool; + fn in_ref(&self, kind: mir::BorrowKind, place: &Place<'tcx>) -> bool; } -impl<'tcx> Visitor<'tcx> for BorrowedLocalsVisitor<'_> { - fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { - if let Rvalue::Ref(_, _, ref place) = *rvalue { - if let Some(local) = find_local(place) { - self.trans.gen(local); +impl BorrowAnalysisKind<'tcx> for AnyBorrow { + const ANALYSIS_NAME: &'static str = "maybe_borrowed_locals"; + + fn in_ref(&self, _: mir::BorrowKind, _: &Place<'_>) -> bool { + true + } + fn in_address_of(&self, _: Mutability, _: &Place<'_>) -> bool { + true + } +} + +impl BorrowAnalysisKind<'tcx> for MutBorrow<'mir, 'tcx> { + const ANALYSIS_NAME: &'static str = "maybe_mut_borrowed_locals"; + + fn in_ref(&self, kind: mir::BorrowKind, place: &Place<'tcx>) -> bool { + match kind { + mir::BorrowKind::Mut { .. } => true, + mir::BorrowKind::Shared | mir::BorrowKind::Shallow | mir::BorrowKind::Unique => { + self.shared_borrow_allows_mutation(place) } } + } - self.super_rvalue(rvalue, location) + fn in_address_of(&self, mt: Mutability, place: &Place<'tcx>) -> bool { + match mt { + Mutability::Mut => true, + Mutability::Not => self.shared_borrow_allows_mutation(place), + } } } diff --git a/src/librustc_mir/dataflow/mod.rs b/src/librustc_mir/dataflow/mod.rs index 7cd7fc309b6b9..41bac894e48ce 100644 --- a/src/librustc_mir/dataflow/mod.rs +++ b/src/librustc_mir/dataflow/mod.rs @@ -23,8 +23,7 @@ pub(crate) use self::drop_flag_effects::*; pub use self::impls::borrows::Borrows; pub use self::impls::DefinitelyInitializedPlaces; pub use self::impls::EverInitializedPlaces; -pub use self::impls::HaveBeenBorrowedLocals; -pub use self::impls::IndirectlyMutableLocals; +pub use self::impls::{MaybeBorrowedLocals, MaybeMutBorrowedLocals}; pub use self::impls::{MaybeInitializedPlaces, MaybeUninitializedPlaces}; pub use self::impls::{MaybeStorageLive, RequiresStorage}; From 34783b73bd891a66fb2af613fef7492376fc7c8a Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 12 Feb 2020 13:37:19 -0800 Subject: [PATCH 03/12] Remove outdated `IndirectlyMutableLocals` `MaybeMutBorrowedLocals` serves the same purpose and has a better name. --- .../dataflow/impls/indirect_mutation.rs | 136 ------------------ src/librustc_mir/dataflow/impls/mod.rs | 2 - 2 files changed, 138 deletions(-) delete mode 100644 src/librustc_mir/dataflow/impls/indirect_mutation.rs diff --git a/src/librustc_mir/dataflow/impls/indirect_mutation.rs b/src/librustc_mir/dataflow/impls/indirect_mutation.rs deleted file mode 100644 index 85bf342c8a39a..0000000000000 --- a/src/librustc_mir/dataflow/impls/indirect_mutation.rs +++ /dev/null @@ -1,136 +0,0 @@ -use rustc::mir::visit::Visitor; -use rustc::mir::{self, Local, Location}; -use rustc::ty::{self, TyCtxt}; -use rustc_index::bit_set::BitSet; -use rustc_span::DUMMY_SP; - -use crate::dataflow::{self, GenKillSet}; - -/// Whether a borrow to a `Local` has been created that could allow that `Local` to be mutated -/// indirectly. This could either be a mutable reference (`&mut`) or a shared borrow if the type of -/// that `Local` allows interior mutability. Operations that can mutate local's indirectly include: -/// assignments through a pointer (`*p = 42`), function calls, drop terminators and inline assembly. -/// -/// If this returns false for a `Local` at a given statement (or terminator), that `Local` could -/// not possibly have been mutated indirectly prior to that statement. -#[derive(Copy, Clone)] -pub struct IndirectlyMutableLocals<'mir, 'tcx> { - body: &'mir mir::Body<'tcx>, - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, -} - -impl<'mir, 'tcx> IndirectlyMutableLocals<'mir, 'tcx> { - pub fn new( - tcx: TyCtxt<'tcx>, - body: &'mir mir::Body<'tcx>, - param_env: ty::ParamEnv<'tcx>, - ) -> Self { - IndirectlyMutableLocals { body, tcx, param_env } - } - - fn transfer_function<'a>( - &self, - trans: &'a mut GenKillSet, - ) -> TransferFunction<'a, 'mir, 'tcx> { - TransferFunction { body: self.body, tcx: self.tcx, param_env: self.param_env, trans } - } -} - -impl<'mir, 'tcx> dataflow::BitDenotation<'tcx> for IndirectlyMutableLocals<'mir, 'tcx> { - type Idx = Local; - - fn name() -> &'static str { - "mut_borrowed_locals" - } - - fn bits_per_block(&self) -> usize { - self.body.local_decls.len() - } - - fn start_block_effect(&self, _entry_set: &mut BitSet) { - // Nothing is borrowed on function entry - } - - fn statement_effect(&self, trans: &mut GenKillSet, loc: Location) { - let stmt = &self.body[loc.block].statements[loc.statement_index]; - self.transfer_function(trans).visit_statement(stmt, loc); - } - - fn terminator_effect(&self, trans: &mut GenKillSet, loc: Location) { - let terminator = self.body[loc.block].terminator(); - self.transfer_function(trans).visit_terminator(terminator, loc); - } - - fn propagate_call_return( - &self, - _in_out: &mut BitSet, - _call_bb: mir::BasicBlock, - _dest_bb: mir::BasicBlock, - _dest_place: &mir::Place<'tcx>, - ) { - // Nothing to do when a call returns successfully - } -} - -impl<'mir, 'tcx> dataflow::BottomValue for IndirectlyMutableLocals<'mir, 'tcx> { - // bottom = unborrowed - const BOTTOM_VALUE: bool = false; -} - -/// A `Visitor` that defines the transfer function for `IndirectlyMutableLocals`. -struct TransferFunction<'a, 'mir, 'tcx> { - trans: &'a mut GenKillSet, - body: &'mir mir::Body<'tcx>, - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, -} - -impl<'tcx> TransferFunction<'_, '_, 'tcx> { - /// Returns `true` if this borrow would allow mutation of the `borrowed_place`. - fn borrow_allows_mutation( - &self, - kind: mir::BorrowKind, - borrowed_place: &mir::Place<'tcx>, - ) -> bool { - match kind { - mir::BorrowKind::Mut { .. } => true, - - mir::BorrowKind::Shared | mir::BorrowKind::Shallow | mir::BorrowKind::Unique => { - !borrowed_place.ty(self.body, self.tcx).ty.is_freeze( - self.tcx, - self.param_env, - DUMMY_SP, - ) - } - } - } -} - -impl<'tcx> Visitor<'tcx> for TransferFunction<'_, '_, 'tcx> { - fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { - if let mir::Rvalue::Ref(_, kind, ref borrowed_place) = *rvalue { - if self.borrow_allows_mutation(kind, borrowed_place) { - if !borrowed_place.is_indirect() { - self.trans.gen(borrowed_place.local); - } - } - } - - self.super_rvalue(rvalue, location); - } - - fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, location: Location) { - // This method purposely does nothing except call `super_terminator`. It exists solely to - // document the subtleties around drop terminators. - - self.super_terminator(terminator, location); - - if let mir::TerminatorKind::Drop { location: _, .. } - | mir::TerminatorKind::DropAndReplace { location: _, .. } = &terminator.kind - { - // Although drop terminators mutably borrow the location being dropped, that borrow - // cannot live beyond the drop terminator because the dropped location is invalidated. - } - } -} diff --git a/src/librustc_mir/dataflow/impls/mod.rs b/src/librustc_mir/dataflow/impls/mod.rs index 5b2264c2a6526..acea31857818b 100644 --- a/src/librustc_mir/dataflow/impls/mod.rs +++ b/src/librustc_mir/dataflow/impls/mod.rs @@ -20,11 +20,9 @@ use super::drop_flag_effects_for_location; use super::on_lookup_result_bits; mod borrowed_locals; -mod indirect_mutation; mod storage_liveness; pub use self::borrowed_locals::*; -pub use self::indirect_mutation::IndirectlyMutableLocals; pub use self::storage_liveness::*; pub(super) mod borrows; From 9972502bafab062b06ef04c02c653f1b868937bd Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 12 Feb 2020 13:38:11 -0800 Subject: [PATCH 04/12] Reenable peek test for indirect mutation analysis This uses the new `MaybeMutBorrowedLocals` pass but we keep the `rustc_peek_indirectly_mutable` since the two are interchangable except when inspecting a local after it has been marked `StorageDead`. --- src/librustc_mir/transform/rustc_peek.rs | 26 ++++++------------------ 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index 7d8506eb28105..16edb7d552b24 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -12,9 +12,8 @@ use rustc_index::bit_set::BitSet; use crate::dataflow::generic::{Analysis, Results, ResultsCursor}; use crate::dataflow::move_paths::{HasMoveData, MoveData}; use crate::dataflow::move_paths::{LookupResult, MovePathIndex}; -use crate::dataflow::IndirectlyMutableLocals; +use crate::dataflow::MaybeMutBorrowedLocals; use crate::dataflow::MoveDataParamEnv; -use crate::dataflow::{do_dataflow, DebugFormatted}; use crate::dataflow::{ DefinitelyInitializedPlaces, MaybeInitializedPlaces, MaybeUninitializedPlaces, }; @@ -24,7 +23,6 @@ pub struct SanityCheck; impl<'tcx> MirPass<'tcx> for SanityCheck { fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, body: &mut BodyAndCache<'tcx>) { use crate::dataflow::has_rustc_mir_with; - let def_id = src.def_id(); if !tcx.has_attr(def_id, sym::rustc_mir) { debug!("skipping rustc_peek::SanityCheck on {}", tcx.def_path_str(def_id)); @@ -37,7 +35,6 @@ impl<'tcx> MirPass<'tcx> for SanityCheck { let param_env = tcx.param_env(def_id); let move_data = MoveData::gather_moves(body, tcx, param_env).unwrap(); let mdpe = MoveDataParamEnv { move_data: move_data, param_env: param_env }; - let dead_unwinds = BitSet::new_empty(body.basic_blocks().len()); let flow_inits = MaybeInitializedPlaces::new(tcx, body, &mdpe) .into_engine(tcx, body, def_id) @@ -48,15 +45,9 @@ impl<'tcx> MirPass<'tcx> for SanityCheck { let flow_def_inits = DefinitelyInitializedPlaces::new(tcx, body, &mdpe) .into_engine(tcx, body, def_id) .iterate_to_fixpoint(); - let _flow_indirectly_mut = do_dataflow( - tcx, - body, - def_id, - &attributes, - &dead_unwinds, - IndirectlyMutableLocals::new(tcx, body, param_env), - |_, i| DebugFormatted::new(&i), - ); + let flow_mut_borrowed = MaybeMutBorrowedLocals::new_mut_only(tcx, body, param_env) + .into_engine(tcx, body, def_id) + .iterate_to_fixpoint(); if has_rustc_mir_with(&attributes, sym::rustc_peek_maybe_init).is_some() { sanity_check_via_rustc_peek(tcx, body, def_id, &attributes, &flow_inits); @@ -67,12 +58,9 @@ impl<'tcx> MirPass<'tcx> for SanityCheck { if has_rustc_mir_with(&attributes, sym::rustc_peek_definite_init).is_some() { sanity_check_via_rustc_peek(tcx, body, def_id, &attributes, &flow_def_inits); } - // FIXME: Uncomment these as analyses are migrated to the new framework - /* if has_rustc_mir_with(&attributes, sym::rustc_peek_indirectly_mutable).is_some() { - sanity_check_via_rustc_peek(tcx, body, def_id, &attributes, &flow_indirectly_mut); + sanity_check_via_rustc_peek(tcx, body, def_id, &attributes, &flow_mut_borrowed); } - */ if has_rustc_mir_with(&attributes, sym::stop_after_dataflow).is_some() { tcx.sess.fatal("stop_after_dataflow ended compilation"); } @@ -276,8 +264,7 @@ where } } -/* FIXME: Add this back once `IndirectlyMutableLocals` uses the new dataflow framework. -impl<'tcx> RustcPeekAt<'tcx> for IndirectlyMutableLocals<'_, 'tcx> { +impl<'tcx> RustcPeekAt<'tcx> for MaybeMutBorrowedLocals<'_, 'tcx> { fn peek_at( &self, tcx: TyCtxt<'tcx>, @@ -298,4 +285,3 @@ impl<'tcx> RustcPeekAt<'tcx> for IndirectlyMutableLocals<'_, 'tcx> { } } } -*/ From 1d737fb032b762e69e8d809c0e042d45e08b457d Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 12 Feb 2020 13:41:26 -0800 Subject: [PATCH 05/12] Use `MaybeBorrowedLocals` for generator analyses It should have the same semantics as `HaveBeenBorrowedLocals` --- .../dataflow/impls/storage_liveness.rs | 46 ++++++++----------- src/librustc_mir/transform/generator.rs | 20 +++----- 2 files changed, 27 insertions(+), 39 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/storage_liveness.rs b/src/librustc_mir/dataflow/impls/storage_liveness.rs index 040c13e8210ea..7508d71945e5f 100644 --- a/src/librustc_mir/dataflow/impls/storage_liveness.rs +++ b/src/librustc_mir/dataflow/impls/storage_liveness.rs @@ -1,8 +1,8 @@ pub use super::*; +use crate::dataflow::generic::{Results, ResultsRefCursor}; use crate::dataflow::BitDenotation; -use crate::dataflow::HaveBeenBorrowedLocals; -use crate::dataflow::{DataflowResults, DataflowResultsCursor, DataflowResultsRefCursor}; +use crate::dataflow::MaybeBorrowedLocals; use rustc::mir::visit::{NonMutatingUseContext, PlaceContext, Visitor}; use rustc::mir::*; use std::cell::RefCell; @@ -69,22 +69,23 @@ impl<'a, 'tcx> BottomValue for MaybeStorageLive<'a, 'tcx> { const BOTTOM_VALUE: bool = false; } +type BorrowedLocalsResults<'a, 'tcx> = ResultsRefCursor<'a, 'a, 'tcx, MaybeBorrowedLocals>; + /// Dataflow analysis that determines whether each local requires storage at a /// given location; i.e. whether its storage can go away without being observed. pub struct RequiresStorage<'mir, 'tcx> { body: ReadOnlyBodyAndCache<'mir, 'tcx>, - borrowed_locals: - RefCell>>, + borrowed_locals: RefCell>, } impl<'mir, 'tcx: 'mir> RequiresStorage<'mir, 'tcx> { pub fn new( body: ReadOnlyBodyAndCache<'mir, 'tcx>, - borrowed_locals: &'mir DataflowResults<'tcx, HaveBeenBorrowedLocals<'mir, 'tcx>>, + borrowed_locals: &'mir Results<'tcx, MaybeBorrowedLocals>, ) -> Self { RequiresStorage { body, - borrowed_locals: RefCell::new(DataflowResultsCursor::new(borrowed_locals, *body)), + borrowed_locals: RefCell::new(ResultsRefCursor::new(*body, borrowed_locals)), } } @@ -111,11 +112,12 @@ impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> { } fn before_statement_effect(&self, sets: &mut GenKillSet, loc: Location) { - // If we borrow or assign to a place then it needs storage for that - // statement. - self.check_for_borrow(sets, loc); - let stmt = &self.body[loc.block].statements[loc.statement_index]; + + // If a place is borrowed in a statement, it needs storage for that statement. + self.borrowed_locals.borrow().analysis().statement_effect(sets, stmt, loc); + + // If a place is assigned to in a statement, it needs storage for that statement. match stmt.kind { StatementKind::StorageDead(l) => sets.kill(l), StatementKind::Assign(box (ref place, _)) @@ -138,12 +140,13 @@ impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> { } fn before_terminator_effect(&self, sets: &mut GenKillSet, loc: Location) { - self.check_for_borrow(sets, loc); + let terminator = self.body[loc.block].terminator(); - if let TerminatorKind::Call { destination: Some((Place { local, .. }, _)), .. } = - self.body[loc.block].terminator().kind - { - sets.gen(local); + // If a place is borrowed in a terminator, it needs storage for that terminator. + self.borrowed_locals.borrow().analysis().terminator_effect(sets, terminator, loc); + + if let TerminatorKind::Call { destination: Some((place, _)), .. } = terminator.kind { + sets.gen(place.local); } } @@ -179,14 +182,6 @@ impl<'mir, 'tcx> RequiresStorage<'mir, 'tcx> { let mut visitor = MoveVisitor { sets, borrowed_locals: &self.borrowed_locals }; visitor.visit_location(self.body, loc); } - - /// Gen locals that are newly borrowed. This includes borrowing any part of - /// a local (we rely on this behavior of `HaveBeenBorrowedLocals`). - fn check_for_borrow(&self, sets: &mut GenKillSet, loc: Location) { - let mut borrowed_locals = self.borrowed_locals.borrow_mut(); - borrowed_locals.seek(loc); - borrowed_locals.each_gen_bit(|l| sets.gen(l)); - } } impl<'mir, 'tcx> BottomValue for RequiresStorage<'mir, 'tcx> { @@ -195,8 +190,7 @@ impl<'mir, 'tcx> BottomValue for RequiresStorage<'mir, 'tcx> { } struct MoveVisitor<'a, 'mir, 'tcx> { - borrowed_locals: - &'a RefCell>>, + borrowed_locals: &'a RefCell>, sets: &'a mut GenKillSet, } @@ -204,7 +198,7 @@ impl<'a, 'mir: 'a, 'tcx> Visitor<'tcx> for MoveVisitor<'a, 'mir, 'tcx> { fn visit_local(&mut self, local: &Local, context: PlaceContext, loc: Location) { if PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) == context { let mut borrowed_locals = self.borrowed_locals.borrow_mut(); - borrowed_locals.seek(loc); + borrowed_locals.seek_before(loc); if !borrowed_locals.contains(*local) { self.sets.kill(*local); } diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index a6fc65731780a..cd00274afcc00 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -49,9 +49,10 @@ //! For generators with state 1 (returned) and state 2 (poisoned) it does nothing. //! Otherwise it drops all the values in scope at the last suspension point. +use crate::dataflow::generic::{Analysis, ResultsCursor}; use crate::dataflow::{do_dataflow, DataflowResultsCursor, DebugFormatted}; use crate::dataflow::{DataflowResults, DataflowResultsConsumer, FlowAtLocation}; -use crate::dataflow::{HaveBeenBorrowedLocals, MaybeStorageLive, RequiresStorage}; +use crate::dataflow::{MaybeBorrowedLocals, MaybeStorageLive, RequiresStorage}; use crate::transform::no_landing_pads::no_landing_pads; use crate::transform::simplify; use crate::transform::{MirPass, MirSource}; @@ -471,17 +472,10 @@ fn locals_live_across_suspend_points( // Calculate the MIR locals which have been previously // borrowed (even if they are still active). - let borrowed_locals_analysis = HaveBeenBorrowedLocals::new(body_ref); - let borrowed_locals_results = do_dataflow( - tcx, - body_ref, - def_id, - &[], - &dead_unwinds, - borrowed_locals_analysis, - |bd, p| DebugFormatted::new(&bd.body().local_decls[p]), - ); - let mut borrowed_locals_cursor = DataflowResultsCursor::new(&borrowed_locals_results, body_ref); + let borrowed_locals_results = + MaybeBorrowedLocals::new().into_engine(tcx, body_ref, def_id).iterate_to_fixpoint(); + + let mut borrowed_locals_cursor = ResultsCursor::new(body_ref, &borrowed_locals_results); // Calculate the MIR locals that we actually need to keep storage around // for. @@ -521,7 +515,7 @@ fn locals_live_across_suspend_points( // If a borrow is converted to a raw reference, we must also assume that it lives // forever. Note that the final liveness is still bounded by the storage liveness // of the local, which happens using the `intersect` operation below. - borrowed_locals_cursor.seek(loc); + borrowed_locals_cursor.seek_before(loc); liveness.outs[block].union(borrowed_locals_cursor.get()); } From d045a17c4b032a858323f6c9bea698ee2e5920b7 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 12 Feb 2020 13:42:31 -0800 Subject: [PATCH 06/12] Use `MaybeMutBorrowedLocals` for const-checking --- .../transform/check_consts/resolver.rs | 2 +- .../transform/check_consts/validation.rs | 31 ++++++++----------- 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/resolver.rs b/src/librustc_mir/transform/check_consts/resolver.rs index b804dc4b5b678..3e14cc6d32a67 100644 --- a/src/librustc_mir/transform/check_consts/resolver.rs +++ b/src/librustc_mir/transform/check_consts/resolver.rs @@ -15,7 +15,7 @@ use crate::dataflow::{self as old_dataflow, generic as dataflow}; /// `FlowSensitiveAnalysis`. /// /// This transfer does nothing when encountering an indirect assignment. Consumers should rely on -/// the `IndirectlyMutableLocals` dataflow pass to see if a `Local` may have become qualified via +/// the `MaybeMutBorrowedLocals` dataflow pass to see if a `Local` may have become qualified via /// an indirect assignment or function call. struct TransferFunction<'a, 'mir, 'tcx, Q> { item: &'a Item<'mir, 'tcx>, diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index e9715f682b04a..9fef62325f281 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -15,17 +15,19 @@ use rustc_span::Span; use std::borrow::Cow; use std::ops::Deref; -use self::old_dataflow::IndirectlyMutableLocals; use super::ops::{self, NonConstOp}; use super::qualifs::{self, HasMutInterior, NeedsDrop}; use super::resolver::FlowSensitiveAnalysis; use super::{is_lang_panic_fn, ConstKind, Item, Qualif}; use crate::const_eval::{is_const_fn, is_unstable_const_fn}; -use crate::dataflow::{self as old_dataflow, generic as dataflow}; -use dataflow::Analysis; +use crate::dataflow::generic::{self as dataflow, Analysis}; +use crate::dataflow::MaybeMutBorrowedLocals; +// We are using `MaybeMutBorrowedLocals` as a proxy for whether an item may have been mutated +// through a pointer prior to the given point. This is okay even though `MaybeMutBorrowedLocals` +// kills locals upon `StorageDead` because a local will never be used after a `StorageDead`. pub type IndirectlyMutableResults<'mir, 'tcx> = - old_dataflow::DataflowResultsCursor<'mir, 'tcx, IndirectlyMutableLocals<'mir, 'tcx>>; + dataflow::ResultsCursor<'mir, 'tcx, MaybeMutBorrowedLocals<'mir, 'tcx>>; struct QualifCursor<'a, 'mir, 'tcx, Q: Qualif> { cursor: dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q>>, @@ -58,7 +60,7 @@ pub struct Qualifs<'a, 'mir, 'tcx> { impl Qualifs<'a, 'mir, 'tcx> { fn indirectly_mutable(&mut self, local: Local, location: Location) -> bool { - self.indirectly_mutable.seek(location); + self.indirectly_mutable.seek_before(location); self.indirectly_mutable.get().contains(local) } @@ -134,22 +136,15 @@ impl Deref for Validator<'_, 'mir, 'tcx> { impl Validator<'a, 'mir, 'tcx> { pub fn new(item: &'a Item<'mir, 'tcx>) -> Self { + let Item { tcx, body, def_id, param_env, .. } = *item; + let needs_drop = QualifCursor::new(NeedsDrop, item); let has_mut_interior = QualifCursor::new(HasMutInterior, item); - let dead_unwinds = BitSet::new_empty(item.body.basic_blocks().len()); - let indirectly_mutable = old_dataflow::do_dataflow( - item.tcx, - &*item.body, - item.def_id, - &item.tcx.get_attrs(item.def_id), - &dead_unwinds, - old_dataflow::IndirectlyMutableLocals::new(item.tcx, *item.body, item.param_env), - |_, local| old_dataflow::DebugFormatted::new(&local), - ); - - let indirectly_mutable = - old_dataflow::DataflowResultsCursor::new(indirectly_mutable, *item.body); + let indirectly_mutable = MaybeMutBorrowedLocals::new_mut_only(tcx, *body, param_env) + .into_engine(tcx, *body, def_id) + .iterate_to_fixpoint() + .into_results_cursor(*body); let qualifs = Qualifs { needs_drop, has_mut_interior, indirectly_mutable }; From 6f167e9c5f421613ff3de37771b1352cd98dd4f7 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 12 Feb 2020 13:42:56 -0800 Subject: [PATCH 07/12] Remove ignore and add explanation of indirect mutation peek test --- src/test/ui/mir-dataflow/indirect-mutation-offset.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/ui/mir-dataflow/indirect-mutation-offset.rs b/src/test/ui/mir-dataflow/indirect-mutation-offset.rs index 884c83b66163c..caa307e269fe7 100644 --- a/src/test/ui/mir-dataflow/indirect-mutation-offset.rs +++ b/src/test/ui/mir-dataflow/indirect-mutation-offset.rs @@ -1,6 +1,11 @@ // compile-flags: -Zunleash-the-miri-inside-of-you -// ignore-test Temporarily ignored while this analysis is migrated to the new framework. +// This test demonstrates a shortcoming of the `MaybeMutBorrowedLocals` analysis. It does not +// handle code that takes a reference to one field of a struct, then use pointer arithmetic to +// transform it to another field of that same struct that may have interior mutability. For now, +// this is UB, but this may change in the future. See [rust-lang/unsafe-code-guidelines#134]. +// +// [rust-lang/unsafe-code-guidelines#134]: https://github.com/rust-lang/unsafe-code-guidelines/issues/134 #![feature(core_intrinsics, rustc_attrs, const_raw_ptr_deref)] From 15a5382ef1115ff11c1357fd21ab4aa12626efee Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 13 Feb 2020 13:56:19 -0800 Subject: [PATCH 08/12] Rename `MaybeBorrowedLocals` constructors --- src/librustc_mir/dataflow/impls/borrowed_locals.rs | 7 +++++-- src/librustc_mir/transform/check_consts/validation.rs | 2 +- src/librustc_mir/transform/generator.rs | 2 +- src/librustc_mir/transform/rustc_peek.rs | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrowed_locals.rs b/src/librustc_mir/dataflow/impls/borrowed_locals.rs index 519db963d1e7d..20aab8b32a51d 100644 --- a/src/librustc_mir/dataflow/impls/borrowed_locals.rs +++ b/src/librustc_mir/dataflow/impls/borrowed_locals.rs @@ -27,7 +27,7 @@ pub struct MaybeBorrowedLocals { impl MaybeBorrowedLocals { /// A dataflow analysis that records whether a pointer or reference exists that may alias the /// given local. - pub fn new() -> Self { + pub fn all_borrows() -> Self { MaybeBorrowedLocals { kind: AnyBorrow } } } @@ -35,7 +35,10 @@ impl MaybeBorrowedLocals { impl MaybeMutBorrowedLocals<'mir, 'tcx> { /// A dataflow analysis that records whether a pointer or reference exists that may *mutably* /// alias the given local. - pub fn new_mut_only( + /// + /// This includes `&mut` and pointers derived from an `&mut`, as well as shared borrows of + /// types with interior mutability. + pub fn mut_borrows_only( tcx: TyCtxt<'tcx>, body: &'mir mir::Body<'tcx>, param_env: ParamEnv<'tcx>, diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index 9fef62325f281..d9e179ad42a68 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -141,7 +141,7 @@ impl Validator<'a, 'mir, 'tcx> { let needs_drop = QualifCursor::new(NeedsDrop, item); let has_mut_interior = QualifCursor::new(HasMutInterior, item); - let indirectly_mutable = MaybeMutBorrowedLocals::new_mut_only(tcx, *body, param_env) + let indirectly_mutable = MaybeMutBorrowedLocals::mut_borrows_only(tcx, *body, param_env) .into_engine(tcx, *body, def_id) .iterate_to_fixpoint() .into_results_cursor(*body); diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index cd00274afcc00..4b3cd2be66d1d 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -473,7 +473,7 @@ fn locals_live_across_suspend_points( // Calculate the MIR locals which have been previously // borrowed (even if they are still active). let borrowed_locals_results = - MaybeBorrowedLocals::new().into_engine(tcx, body_ref, def_id).iterate_to_fixpoint(); + MaybeBorrowedLocals::all_borrows().into_engine(tcx, body_ref, def_id).iterate_to_fixpoint(); let mut borrowed_locals_cursor = ResultsCursor::new(body_ref, &borrowed_locals_results); diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index 16edb7d552b24..6176cf8bc0fa6 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -45,7 +45,7 @@ impl<'tcx> MirPass<'tcx> for SanityCheck { let flow_def_inits = DefinitelyInitializedPlaces::new(tcx, body, &mdpe) .into_engine(tcx, body, def_id) .iterate_to_fixpoint(); - let flow_mut_borrowed = MaybeMutBorrowedLocals::new_mut_only(tcx, body, param_env) + let flow_mut_borrowed = MaybeMutBorrowedLocals::mut_borrows_only(tcx, body, param_env) .into_engine(tcx, body, def_id) .iterate_to_fixpoint(); From 0984639348c2fc98389746f6815e576cfcaacda8 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 13 Feb 2020 13:57:01 -0800 Subject: [PATCH 09/12] Ignore mut borrow from drop terminator in const-eval --- .../dataflow/impls/borrowed_locals.rs | 45 ++++++++++++++----- .../transform/check_consts/validation.rs | 6 +++ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrowed_locals.rs b/src/librustc_mir/dataflow/impls/borrowed_locals.rs index 20aab8b32a51d..0ce37aea69df3 100644 --- a/src/librustc_mir/dataflow/impls/borrowed_locals.rs +++ b/src/librustc_mir/dataflow/impls/borrowed_locals.rs @@ -22,13 +22,14 @@ pub type MaybeMutBorrowedLocals<'mir, 'tcx> = MaybeBorrowedLocals { kind: K, + ignore_borrow_on_drop: bool, } impl MaybeBorrowedLocals { /// A dataflow analysis that records whether a pointer or reference exists that may alias the /// given local. pub fn all_borrows() -> Self { - MaybeBorrowedLocals { kind: AnyBorrow } + MaybeBorrowedLocals { kind: AnyBorrow, ignore_borrow_on_drop: false } } } @@ -43,13 +44,37 @@ impl MaybeMutBorrowedLocals<'mir, 'tcx> { body: &'mir mir::Body<'tcx>, param_env: ParamEnv<'tcx>, ) -> Self { - MaybeBorrowedLocals { kind: MutBorrow { body, tcx, param_env } } + MaybeBorrowedLocals { + kind: MutBorrow { body, tcx, param_env }, + ignore_borrow_on_drop: false, + } } } impl MaybeBorrowedLocals { + /// During dataflow analysis, ignore the borrow that may occur when a place is dropped. + /// + /// Drop terminators may call custom drop glue (`Drop::drop`), which takes `&mut self` as a + /// parameter. In the general case, a drop impl could launder that reference into the + /// surrounding environment through a raw pointer, thus creating a valid `*mut` pointing to the + /// dropped local. We are not yet willing to declare this particular case UB, so we must treat + /// all dropped locals as mutably borrowed for now. See discussion on [#61069]. + /// + /// In some contexts, we know that this borrow will never occur. For example, during + /// const-eval, custom drop glue cannot be run. Code that calls this should document the + /// assumptions that justify `Drop` terminators in this way. + /// + /// [#61069]: https://github.com/rust-lang/rust/pull/61069 + pub fn unsound_ignore_borrow_on_drop(self) -> Self { + MaybeBorrowedLocals { ignore_borrow_on_drop: true, ..self } + } + fn transfer_function<'a, T>(&'a self, trans: &'a mut T) -> TransferFunction<'a, T, K> { - TransferFunction { kind: &self.kind, trans } + TransferFunction { + kind: &self.kind, + trans, + ignore_borrow_on_drop: self.ignore_borrow_on_drop, + } } } @@ -112,6 +137,7 @@ impl BottomValue for MaybeBorrowedLocals { struct TransferFunction<'a, T, K> { trans: &'a mut T, kind: &'a K, + ignore_borrow_on_drop: bool, } impl Visitor<'tcx> for TransferFunction<'a, T, K> @@ -162,17 +188,12 @@ where self.super_terminator(terminator, location); match terminator.kind { - // Drop terminators may call custom drop glue (`Drop::drop`), which takes `&mut self` - // as a parameter. Hypothetically, a drop impl could launder that reference into the - // surrounding environment through a raw pointer, thus creating a valid `*mut` pointing - // to the dropped local. We are not yet willing to declare this particular case UB, so - // we must treat all dropped locals as mutably borrowed for now. See discussion on - // [#61069]. - // - // [#61069]: https://github.com/rust-lang/rust/pull/61069 mir::TerminatorKind::Drop { location: dropped_place, .. } | mir::TerminatorKind::DropAndReplace { location: dropped_place, .. } => { - self.trans.gen(dropped_place.local); + // See documentation for `unsound_ignore_borrow_on_drop` for an explanation. + if !self.ignore_borrow_on_drop { + self.trans.gen(dropped_place.local); + } } TerminatorKind::Abort diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index d9e179ad42a68..d4fa2b10152fe 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -141,7 +141,13 @@ impl Validator<'a, 'mir, 'tcx> { let needs_drop = QualifCursor::new(NeedsDrop, item); let has_mut_interior = QualifCursor::new(HasMutInterior, item); + // We can use `unsound_ignore_borrow_on_drop` here because custom drop impls are not + // allowed in a const. + // + // FIXME(ecstaticmorse): Someday we want to allow custom drop impls. How do we do this + // without breaking stable code? let indirectly_mutable = MaybeMutBorrowedLocals::mut_borrows_only(tcx, *body, param_env) + .unsound_ignore_borrow_on_drop() .into_engine(tcx, *body, def_id) .iterate_to_fixpoint() .into_results_cursor(*body); From 668d2fe807068073647f9c92d81ff0e7b9ab8486 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 13 Feb 2020 15:58:56 -0800 Subject: [PATCH 10/12] Update line # in test output --- src/test/ui/mir-dataflow/indirect-mutation-offset.stderr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/mir-dataflow/indirect-mutation-offset.stderr b/src/test/ui/mir-dataflow/indirect-mutation-offset.stderr index 0ae9a40c96aaa..8d3548ececdd9 100644 --- a/src/test/ui/mir-dataflow/indirect-mutation-offset.stderr +++ b/src/test/ui/mir-dataflow/indirect-mutation-offset.stderr @@ -1,5 +1,5 @@ error: rustc_peek: bit not set - --> $DIR/indirect-mutation-offset.rs:34:14 + --> $DIR/indirect-mutation-offset.rs:41:14 | LL | unsafe { rustc_peek(x) }; | ^^^^^^^^^^^^^ From 9d423950cc141e110242ff1973f2d7b84c719a86 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Mon, 17 Feb 2020 13:39:50 -0800 Subject: [PATCH 11/12] Use doc comment for explanation of `shared_borrow_allows_mutation` --- .../dataflow/impls/borrowed_locals.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrowed_locals.rs b/src/librustc_mir/dataflow/impls/borrowed_locals.rs index 0ce37aea69df3..44693c50eac9c 100644 --- a/src/librustc_mir/dataflow/impls/borrowed_locals.rs +++ b/src/librustc_mir/dataflow/impls/borrowed_locals.rs @@ -221,15 +221,15 @@ pub struct MutBorrow<'mir, 'tcx> { } impl MutBorrow<'mir, 'tcx> { - // `&` and `&raw` only allow mutation if the borrowed place is `!Freeze`. - // - // This assumes that it is UB to take the address of a struct field whose type is - // `Freeze`, then use pointer arithmetic to derive a pointer to a *different* field of - // that same struct whose type is `!Freeze`. If we decide that this is not UB, we will - // have to check the type of the borrowed **local** instead of the borrowed **place** - // below. See [rust-lang/unsafe-code-guidelines#134]. - // - // [rust-lang/unsafe-code-guidelines#134]: https://github.com/rust-lang/unsafe-code-guidelines/issues/134 + /// `&` and `&raw` only allow mutation if the borrowed place is `!Freeze`. + /// + /// This assumes that it is UB to take the address of a struct field whose type is + /// `Freeze`, then use pointer arithmetic to derive a pointer to a *different* field of + /// that same struct whose type is `!Freeze`. If we decide that this is not UB, we will + /// have to check the type of the borrowed **local** instead of the borrowed **place** + /// below. See [rust-lang/unsafe-code-guidelines#134]. + /// + /// [rust-lang/unsafe-code-guidelines#134]: https://github.com/rust-lang/unsafe-code-guidelines/issues/134 fn shared_borrow_allows_mutation(&self, place: &Place<'tcx>) -> bool { !place.ty(self.body, self.tcx).ty.is_freeze(self.tcx, self.param_env, DUMMY_SP) } From 077a93c6a9b1c8ee0e541ea484f7e13c207d50d0 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Mon, 17 Feb 2020 13:43:13 -0800 Subject: [PATCH 12/12] Fix typo in comment --- src/librustc_mir/dataflow/impls/borrowed_locals.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/dataflow/impls/borrowed_locals.rs b/src/librustc_mir/dataflow/impls/borrowed_locals.rs index 44693c50eac9c..95a676c0892c5 100644 --- a/src/librustc_mir/dataflow/impls/borrowed_locals.rs +++ b/src/librustc_mir/dataflow/impls/borrowed_locals.rs @@ -62,7 +62,7 @@ impl MaybeBorrowedLocals { /// /// In some contexts, we know that this borrow will never occur. For example, during /// const-eval, custom drop glue cannot be run. Code that calls this should document the - /// assumptions that justify `Drop` terminators in this way. + /// assumptions that justify ignoring `Drop` terminators in this way. /// /// [#61069]: https://github.com/rust-lang/rust/pull/61069 pub fn unsound_ignore_borrow_on_drop(self) -> Self {