From 905818f9609d0759b47de530da6a9e41b50b68a3 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 4 Jul 2019 12:30:56 -0700 Subject: [PATCH 01/13] Add getter for `Place` which ignores `Deref`s --- src/librustc/mir/mod.rs | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 1e2ec08301cf9..d92faeb580bbc 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -1889,6 +1889,30 @@ impl<'tcx> Place<'tcx> { } } + /// Finds the innermost `PlaceBase` for this `Place`. + pub fn base(&self) -> &PlaceBase<'tcx> { + &self.base + } + + /// Finds the innermost `PlaceBase` for this `Place`, or `None` if the target of this `Place` + /// is behind an indirection. + pub fn base_direct(&self) -> Option<&PlaceBase<'tcx>> { + self.iterate(|base, projections| { + for proj in projections { + if let PlaceElem::Deref = proj.elem { + return None; + } + } + + Some(base) + }) + } + + /// Finds the innermost `Local` from this `Place`. + pub fn base_local(&self) -> Option { + self.base().local() + } + /// Recursively "iterates" over place components, generating a `PlaceBase` and /// `Projections` list and invoking `op` with a `ProjectionsIter`. pub fn iterate( @@ -1984,6 +2008,15 @@ impl<'a, 'tcx> PlaceRef<'a, 'tcx> { } } +impl PlaceBase<'_> { + pub fn local(&self) -> Option { + match *self { + PlaceBase::Local(local) => Some(local), + PlaceBase::Static(_) => None, + } + } +} + /// A linked list of projections running up the stack; begins with the /// innermost projection and extends to the outermost (e.g., `a.b.c` /// would have the place `b` with a "next" pointer to `b.c`). From 0764641e14023a62dd52c99a95f4b12145368b62 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 4 Jul 2019 12:31:26 -0700 Subject: [PATCH 02/13] Use new `Place::base_direct` API when possible Its functionality is duplicated in `borrowed_locals.rs` and `path_utils.rs` --- src/librustc_mir/borrow_check/path_utils.rs | 24 ++++--------------- .../dataflow/impls/borrowed_locals.rs | 20 ++-------------- 2 files changed, 7 insertions(+), 37 deletions(-) diff --git a/src/librustc_mir/borrow_check/path_utils.rs b/src/librustc_mir/borrow_check/path_utils.rs index 43a012e1494f8..bcf3b551e6463 100644 --- a/src/librustc_mir/borrow_check/path_utils.rs +++ b/src/librustc_mir/borrow_check/path_utils.rs @@ -2,8 +2,7 @@ use crate::borrow_check::borrow_set::{BorrowSet, BorrowData, TwoPhaseActivation} use crate::borrow_check::places_conflict; use crate::borrow_check::AccessDepth; use crate::dataflow::indexes::BorrowIndex; -use rustc::mir::{BasicBlock, Location, Body, Place, PlaceBase}; -use rustc::mir::{ProjectionElem, BorrowKind}; +use rustc::mir::{BasicBlock, BorrowKind, Location, Body, Place, PlaceBase}; use rustc::ty::{self, TyCtxt}; use rustc_data_structures::graph::dominators::Dominators; @@ -132,21 +131,8 @@ pub(super) fn is_active<'tcx>( /// Determines if a given borrow is borrowing local data /// This is called for all Yield statements on movable generators -pub(super) fn borrow_of_local_data(place: &Place<'_>) -> bool { - place.iterate(|place_base, place_projection| { - match place_base { - PlaceBase::Static(..) => return false, - PlaceBase::Local(..) => {}, - } - - for proj in place_projection { - // Reborrow of already borrowed data is ignored - // Any errors will be caught on the initial borrow - if proj.elem == ProjectionElem::Deref { - return false; - } - } - - true - }) +pub(super) fn borrow_of_local_data<'tcx>(place: &Place<'tcx>) -> bool { + place.base_direct() + .and_then(PlaceBase::local) + .is_some() } diff --git a/src/librustc_mir/dataflow/impls/borrowed_locals.rs b/src/librustc_mir/dataflow/impls/borrowed_locals.rs index d94ebdbae24ae..617bbef2cfe2a 100644 --- a/src/librustc_mir/dataflow/impls/borrowed_locals.rs +++ b/src/librustc_mir/dataflow/impls/borrowed_locals.rs @@ -64,7 +64,7 @@ impl<'a, 'tcx> BitDenotation<'tcx> for HaveBeenBorrowedLocals<'a, 'tcx> { // Drop terminators borrows the location TerminatorKind::Drop { location, .. } | TerminatorKind::DropAndReplace { location, .. } => { - if let Some(local) = find_local(location) { + if let Some(local) = location.base_direct().and_then(PlaceBase::local) { trans.gen(local); } } @@ -92,28 +92,12 @@ struct BorrowedLocalsVisitor<'gk> { trans: &'gk mut GenKillSet, } -fn find_local(place: &Place<'_>) -> Option { - place.iterate(|place_base, place_projection| { - for proj in place_projection { - if proj.elem == ProjectionElem::Deref { - return None; - } - } - - if let PlaceBase::Local(local) = place_base { - Some(*local) - } else { - None - } - }) -} - 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) { + if let Some(local) = place.base_direct().and_then(PlaceBase::local) { self.trans.gen(local); } } From 6359de10405408e61db7a27c79a321eb4c3e6ec9 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Mon, 8 Jul 2019 14:28:08 -0700 Subject: [PATCH 03/13] Link lifetime of closure param to that of `self` in `Place::iterate` This informs callers that the lifetime of the reference to the underlying `PlaceBase` is the same as that of the `Place` being iterated over. This lets callers return a reference to the underlying `PlaceBase` from the closure passed to `Place::iterate`. --- src/librustc/mir/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index d92faeb580bbc..f2f726a1f4d25 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -1915,17 +1915,17 @@ impl<'tcx> Place<'tcx> { /// Recursively "iterates" over place components, generating a `PlaceBase` and /// `Projections` list and invoking `op` with a `ProjectionsIter`. - pub fn iterate( - &self, - op: impl FnOnce(&PlaceBase<'tcx>, ProjectionsIter<'_, 'tcx>) -> R, + pub fn iterate<'a, R: 'a>( + &'a self, + op: impl FnOnce(&'a PlaceBase<'tcx>, ProjectionsIter<'_, 'tcx>) -> R, ) -> R { Place::iterate_over(&self.base, &self.projection, op) } - pub fn iterate_over( - place_base: &PlaceBase<'tcx>, + pub fn iterate_over<'a, R: 'a>( + place_base: &'a PlaceBase<'tcx>, place_projection: &Option>>, - op: impl FnOnce(&PlaceBase<'tcx>, ProjectionsIter<'_, 'tcx>) -> R, + op: impl FnOnce(&'a PlaceBase<'tcx>, ProjectionsIter<'_, 'tcx>) -> R, ) -> R { fn iterate_over2<'tcx, R>( place_base: &PlaceBase<'tcx>, From dcc4b9f33f6289c8ec6fd77c58ebf4cbc946b0fc Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 14 Jul 2019 14:40:43 -0700 Subject: [PATCH 04/13] Add getter for current dataflow state to `DataflowResultsCursor` --- src/librustc_mir/dataflow/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/librustc_mir/dataflow/mod.rs b/src/librustc_mir/dataflow/mod.rs index 3bdd3e3da048e..0a34c5f20ab22 100644 --- a/src/librustc_mir/dataflow/mod.rs +++ b/src/librustc_mir/dataflow/mod.rs @@ -441,6 +441,10 @@ where self.curr_loc = Some(loc); } + pub fn get(&self) -> &BitSet { + self.flow_state.as_dense() + } + /// Return whether the current state contains bit `x`. pub fn contains(&self, x: BD::Idx) -> bool { self.flow_state.contains(x) From dee972a64e7d5c0a889b350bacf117199dca658c Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 5 Jul 2019 18:18:38 -0700 Subject: [PATCH 05/13] Clean up `rustc_peek` We now use `dataflow::state_for_location` to get the dataflow state before calls to `rustc_peek`. The original version used a custom implementation that only looked at assignment statements. --- src/librustc_mir/transform/rustc_peek.rs | 229 ++++++++++------------- 1 file changed, 98 insertions(+), 131 deletions(-) diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index 7fe8480c819e6..4fde7d67681b0 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -5,11 +5,11 @@ use syntax_pos::Span; use rustc::ty::{self, TyCtxt}; use rustc::hir::def_id::DefId; -use rustc::mir::{self, Body, Location}; +use rustc::mir::{self, Body, Location, Local}; use rustc_data_structures::bit_set::BitSet; use crate::transform::{MirPass, MirSource}; -use crate::dataflow::{do_dataflow, DebugFormatted}; +use crate::dataflow::{self, do_dataflow, DebugFormatted}; use crate::dataflow::MoveDataParamEnv; use crate::dataflow::BitDenotation; use crate::dataflow::DataflowResults; @@ -92,147 +92,114 @@ pub fn sanity_check_via_rustc_peek<'tcx, O>( O: BitDenotation<'tcx, Idx = MovePathIndex> + HasMoveData<'tcx>, { debug!("sanity_check_via_rustc_peek def_id: {:?}", def_id); - // FIXME: this is not DRY. Figure out way to abstract this and - // `dataflow::build_sets`. (But note it is doing non-standard - // stuff, so such generalization may not be realistic.) - - for bb in body.basic_blocks().indices() { - each_block(tcx, body, results, bb); - } -} - -fn each_block<'tcx, O>( - tcx: TyCtxt<'tcx>, - body: &Body<'tcx>, - results: &DataflowResults<'tcx, O>, - bb: mir::BasicBlock, -) where - O: BitDenotation<'tcx, Idx = MovePathIndex> + HasMoveData<'tcx>, -{ - let move_data = results.0.operator.move_data(); - let mir::BasicBlockData { ref statements, ref terminator, is_cleanup: _ } = body[bb]; - - let (args, span) = match is_rustc_peek(tcx, terminator) { - Some(args_and_span) => args_and_span, - None => return, - }; - assert!(args.len() == 1); - let peek_arg_place = match args[0] { - mir::Operand::Copy(ref place @ mir::Place { - base: mir::PlaceBase::Local(_), - projection: None, - }) | - mir::Operand::Move(ref place @ mir::Place { - base: mir::PlaceBase::Local(_), - projection: None, - }) => Some(place), - _ => None, - }; - - let peek_arg_place = match peek_arg_place { - Some(arg) => arg, - None => { - tcx.sess.diagnostic().span_err( - span, "dataflow::sanity_check cannot feed a non-temp to rustc_peek."); - return; - } - }; - - let mut on_entry = results.0.sets.entry_set_for(bb.index()).to_owned(); - let mut trans = results.0.sets.trans_for(bb.index()).clone(); - - // Emulate effect of all statements in the block up to (but not - // including) the borrow within `peek_arg_place`. Do *not* include - // call to `peek_arg_place` itself (since we are peeking the state - // of the argument at time immediate preceding Call to - // `rustc_peek`). - - for (j, stmt) in statements.iter().enumerate() { - debug!("rustc_peek: ({:?},{}) {:?}", bb, j, stmt); - let (place, rvalue) = match stmt.kind { - mir::StatementKind::Assign(ref place, ref rvalue) => { - (place, rvalue) - } - mir::StatementKind::FakeRead(..) | - mir::StatementKind::StorageLive(_) | - mir::StatementKind::StorageDead(_) | - mir::StatementKind::InlineAsm { .. } | - mir::StatementKind::Retag { .. } | - mir::StatementKind::AscribeUserType(..) | - mir::StatementKind::Nop => continue, - mir::StatementKind::SetDiscriminant{ .. } => - span_bug!(stmt.source_info.span, - "sanity_check should run before Deaggregator inserts SetDiscriminant"), - }; - - if place == peek_arg_place { - if let mir::Rvalue::Ref(_, mir::BorrowKind::Shared, ref peeking_at_place) = **rvalue { - // Okay, our search is over. - match move_data.rev_lookup.find(peeking_at_place.as_ref()) { - LookupResult::Exact(peek_mpi) => { - let bit_state = on_entry.contains(peek_mpi); - debug!("rustc_peek({:?} = &{:?}) bit_state: {}", - place, peeking_at_place, bit_state); - if !bit_state { - tcx.sess.span_err(span, "rustc_peek: bit not set"); - } - } - LookupResult::Parent(..) => { - tcx.sess.span_err(span, "rustc_peek: argument untracked"); + let move_data = results.operator().move_data(); + + let peek_calls = body + .basic_blocks() + .iter_enumerated() + .filter_map(|(bb, data)| { + data.terminator + .as_ref() + .and_then(|term| PeekCall::from_terminator(tcx, term)) + .map(|call| (bb, data, call)) + }); + + for (bb, data, call) in peek_calls { + // Look for a sequence like the following to indicate that we should be peeking at `_1`: + // _2 = &_1 + // rustc_peek(_2) + let (statement_index, peek_rval) = data.statements + .iter() + .map(|stmt| value_assigned_to_local(stmt, call.arg)) + .enumerate() + .filter_map(|(idx, rval)| rval.map(|r| (idx, r))) + .next() + .expect("call to rustc_peek should be preceded by \ + assignment to temporary holding its argument"); + + if let mir::Rvalue::Ref(_, mir::BorrowKind::Shared, peeking_at_place) = peek_rval { + let loc = Location { block: bb, statement_index }; + let flow_state = dataflow::state_for_location(loc, results.operator(), results, body); + + match move_data.rev_lookup.find(peeking_at_place) { + LookupResult::Exact(peek_mpi) => { + let bit_state = flow_state.contains(peek_mpi); + debug!("rustc_peek({:?} = &{:?}) bit_state: {}", + call.arg, peeking_at_place, bit_state); + if !bit_state { + tcx.sess.span_err(call.span, "rustc_peek: bit not set"); } } - return; - } else { - // Our search should have been over, but the input - // does not match expectations of `rustc_peek` for - // this sanity_check. - let msg = "rustc_peek: argument expression \ - must be immediate borrow of form `&expr`"; - tcx.sess.span_err(span, msg); + LookupResult::Parent(..) => { + tcx.sess.span_err(call.span, "rustc_peek: argument untracked"); + } } + } else { + let msg = "rustc_peek: argument expression \ + must be immediate borrow of form `&expr`"; + tcx.sess.span_err(call.span, msg); } + } +} - let lhs_mpi = move_data.rev_lookup.find(place.as_ref()); - - debug!("rustc_peek: computing effect on place: {:?} ({:?}) in stmt: {:?}", - place, lhs_mpi, stmt); - // reset GEN and KILL sets before emulating their effect. - trans.clear(); - results.0.operator.before_statement_effect( - &mut trans, - Location { block: bb, statement_index: j }); - results.0.operator.statement_effect( - &mut trans, - Location { block: bb, statement_index: j }); - trans.apply(&mut on_entry); +/// If `stmt` is an assignment where the LHS is the given local (with no projections), returns the +/// RHS of the assignment. +fn value_assigned_to_local<'a, 'tcx>( + stmt: &'a mir::Statement<'tcx>, + local: Local, +) -> Option<&'a mir::Rvalue<'tcx>> { + if let mir::StatementKind::Assign(place, rvalue) = &stmt.kind { + if let mir::Place::Base(mir::PlaceBase::Local(l)) = place { + if local == *l { + return Some(&*rvalue); + } + } } - results.0.operator.before_terminator_effect( - &mut trans, - Location { block: bb, statement_index: statements.len() }); + None +} - tcx.sess.span_err(span, &format!("rustc_peek: MIR did not match \ - anticipated pattern; note that \ - rustc_peek expects input of \ - form `&expr`")); +struct PeekCall { + arg: Local, + span: Span, } -fn is_rustc_peek<'a, 'tcx>( - tcx: TyCtxt<'tcx>, - terminator: &'a Option>, -) -> Option<(&'a [mir::Operand<'tcx>], Span)> { - if let Some(mir::Terminator { ref kind, source_info, .. }) = *terminator { - if let mir::TerminatorKind::Call { func: ref oper, ref args, .. } = *kind { - if let mir::Operand::Constant(ref func) = *oper { - if let ty::FnDef(def_id, _) = func.ty.sty { - let abi = tcx.fn_sig(def_id).abi(); - let name = tcx.item_name(def_id); - if abi == Abi::RustIntrinsic && name == sym::rustc_peek { - return Some((args, source_info.span)); - } +impl PeekCall { + fn from_terminator<'tcx>( + tcx: TyCtxt<'tcx>, + terminator: &mir::Terminator<'tcx>, + ) -> Option { + let span = terminator.source_info.span; + if let mir::TerminatorKind::Call { func: mir::Operand::Constant(func), args, .. } = + &terminator.kind + { + if let ty::FnDef(def_id, _) = func.ty.sty { + let abi = tcx.fn_sig(def_id).abi(); + let name = tcx.item_name(def_id); + if abi != Abi::RustIntrinsic || name != sym::rustc_peek { + return None; } + + assert_eq!(args.len(), 1); + let arg = match args[0] { + | mir::Operand::Copy(mir::Place::Base(mir::PlaceBase::Local(local))) + | mir::Operand::Move(mir::Place::Base(mir::PlaceBase::Local(local))) + => local, + + _ => { + tcx.sess.diagnostic().span_err( + span, "dataflow::sanity_check cannot feed a non-temp to rustc_peek."); + return None; + } + }; + + return Some(PeekCall { + arg, + span, + }); } } + + None } - return None; } From 3594ff16b27100dfb6e047f7bceac4ef028537d0 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 5 Jul 2019 19:39:32 -0700 Subject: [PATCH 06/13] Enable custom `rustc_peek` behavior --- src/librustc_mir/transform/rustc_peek.rs | 58 ++++++++++++++++-------- 1 file changed, 40 insertions(+), 18 deletions(-) diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index 4fde7d67681b0..596ab558c8f6c 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -88,11 +88,8 @@ pub fn sanity_check_via_rustc_peek<'tcx, O>( def_id: DefId, _attributes: &[ast::Attribute], results: &DataflowResults<'tcx, O>, -) where - O: BitDenotation<'tcx, Idx = MovePathIndex> + HasMoveData<'tcx>, -{ +) where O: RustcPeekAt<'tcx> { debug!("sanity_check_via_rustc_peek def_id: {:?}", def_id); - let move_data = results.operator().move_data(); let peek_calls = body .basic_blocks() @@ -121,19 +118,7 @@ pub fn sanity_check_via_rustc_peek<'tcx, O>( let loc = Location { block: bb, statement_index }; let flow_state = dataflow::state_for_location(loc, results.operator(), results, body); - match move_data.rev_lookup.find(peeking_at_place) { - LookupResult::Exact(peek_mpi) => { - let bit_state = flow_state.contains(peek_mpi); - debug!("rustc_peek({:?} = &{:?}) bit_state: {}", - call.arg, peeking_at_place, bit_state); - if !bit_state { - tcx.sess.span_err(call.span, "rustc_peek: bit not set"); - } - } - LookupResult::Parent(..) => { - tcx.sess.span_err(call.span, "rustc_peek: argument untracked"); - } - } + results.operator().peek_at(tcx, peeking_at_place, &flow_state, call); } else { let msg = "rustc_peek: argument expression \ must be immediate borrow of form `&expr`"; @@ -159,7 +144,8 @@ fn value_assigned_to_local<'a, 'tcx>( None } -struct PeekCall { +#[derive(Clone, Copy, Debug)] +pub struct PeekCall { arg: Local, span: Span, } @@ -203,3 +189,39 @@ impl PeekCall { None } } + +pub trait RustcPeekAt<'tcx>: BitDenotation<'tcx> { + fn peek_at( + &self, + tcx: TyCtxt<'tcx>, + place: &mir::Place<'tcx>, + flow_state: &BitSet, + call: PeekCall, + ); +} + +impl<'tcx, O> RustcPeekAt<'tcx> for O + where O: BitDenotation<'tcx, Idx = MovePathIndex> + HasMoveData<'tcx>, +{ + fn peek_at( + &self, + tcx: TyCtxt<'tcx>, + place: &mir::Place<'tcx>, + flow_state: &BitSet, + call: PeekCall, + ) { + match self.move_data().rev_lookup.find(place) { + LookupResult::Exact(peek_mpi) => { + let bit_state = flow_state.contains(peek_mpi); + debug!("rustc_peek({:?} = &{:?}) bit_state: {}", + call.arg, place, bit_state); + if !bit_state { + tcx.sess.span_err(call.span, "rustc_peek: bit not set"); + } + } + LookupResult::Parent(..) => { + tcx.sess.span_err(call.span, "rustc_peek: argument untracked"); + } + } + } +} From 9d144112891d7befcab340c2d3d2e0c2854f963d Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 14 Jul 2019 14:12:15 -0700 Subject: [PATCH 07/13] Allow `rustc_peek` to take arguments by value --- src/librustc_mir/transform/rustc_peek.rs | 48 ++++++++++++++++++------ 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index 596ab558c8f6c..e0808b6531ed1 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -3,7 +3,7 @@ use syntax::ast; use syntax::symbol::sym; use syntax_pos::Span; -use rustc::ty::{self, TyCtxt}; +use rustc::ty::{self, TyCtxt, Ty}; use rustc::hir::def_id::DefId; use rustc::mir::{self, Body, Location, Local}; use rustc_data_structures::bit_set::BitSet; @@ -114,15 +114,20 @@ pub fn sanity_check_via_rustc_peek<'tcx, O>( .expect("call to rustc_peek should be preceded by \ assignment to temporary holding its argument"); - if let mir::Rvalue::Ref(_, mir::BorrowKind::Shared, peeking_at_place) = peek_rval { - let loc = Location { block: bb, statement_index }; - let flow_state = dataflow::state_for_location(loc, results.operator(), results, body); + match (call.kind, peek_rval) { + | (PeekCallKind::ByRef, mir::Rvalue::Ref(_, _, peek_at)) + | (PeekCallKind::ByVal, mir::Rvalue::Use(mir::Operand::Move(peek_at))) + | (PeekCallKind::ByVal, mir::Rvalue::Use(mir::Operand::Copy(peek_at))) + => { + let loc = Location { block: bb, statement_index }; + results.peek_at(tcx, body, peek_at, loc, call); + } - results.operator().peek_at(tcx, peeking_at_place, &flow_state, call); - } else { - let msg = "rustc_peek: argument expression \ - must be immediate borrow of form `&expr`"; - tcx.sess.span_err(call.span, msg); + _ => { + let msg = "rustc_peek: argument expression \ + must be either `place` or `&place`"; + tcx.sess.span_err(call.span, msg); + } } } } @@ -144,9 +149,26 @@ fn value_assigned_to_local<'a, 'tcx>( None } +#[derive(Clone, Copy, Debug)] +enum PeekCallKind { + ByVal, + ByRef, +} + +impl PeekCallKind { + fn from_arg_ty(arg: Ty<'_>) -> Self { + if let ty::Ref(_, _, _) = arg.sty { + PeekCallKind::ByRef + } else { + PeekCallKind::ByVal + } + } +} + #[derive(Clone, Copy, Debug)] pub struct PeekCall { arg: Local, + kind: PeekCallKind, span: Span, } @@ -159,14 +181,15 @@ impl PeekCall { if let mir::TerminatorKind::Call { func: mir::Operand::Constant(func), args, .. } = &terminator.kind { - if let ty::FnDef(def_id, _) = func.ty.sty { - let abi = tcx.fn_sig(def_id).abi(); + if let ty::FnDef(def_id, substs) = func.ty.sty { + let sig = tcx.fn_sig(def_id); let name = tcx.item_name(def_id); - if abi != Abi::RustIntrinsic || name != sym::rustc_peek { + if sig.abi() != Abi::RustIntrinsic || name != sym::rustc_peek { return None; } assert_eq!(args.len(), 1); + let kind = PeekCallKind::from_arg_ty(substs.type_at(0)); let arg = match args[0] { | mir::Operand::Copy(mir::Place::Base(mir::PlaceBase::Local(local))) | mir::Operand::Move(mir::Place::Base(mir::PlaceBase::Local(local))) @@ -181,6 +204,7 @@ impl PeekCall { return Some(PeekCall { arg, + kind, span, }); } From 53fa5a441ea14ae0a692f269f40d9b2225e48586 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 14 Jul 2019 14:41:32 -0700 Subject: [PATCH 08/13] Add reaching definitions analysis and use-def chain --- src/librustc_mir/dataflow/impls/mod.rs | 6 + .../dataflow/impls/reaching_defs.rs | 414 ++++++++++++++++++ .../dataflow/impls/use_def_chain.rs | 302 +++++++++++++ src/librustc_mir/dataflow/mod.rs | 1 + 4 files changed, 723 insertions(+) create mode 100644 src/librustc_mir/dataflow/impls/reaching_defs.rs create mode 100644 src/librustc_mir/dataflow/impls/use_def_chain.rs diff --git a/src/librustc_mir/dataflow/impls/mod.rs b/src/librustc_mir/dataflow/impls/mod.rs index 69bbe08792140..7296e758b7897 100644 --- a/src/librustc_mir/dataflow/impls/mod.rs +++ b/src/librustc_mir/dataflow/impls/mod.rs @@ -18,6 +18,12 @@ use super::drop_flag_effects_for_function_entry; use super::drop_flag_effects_for_location; use super::on_lookup_result_bits; +mod reaching_defs; +mod use_def_chain; + +pub use self::reaching_defs::ReachingDefinitions; +pub use self::use_def_chain::UseDefChain; + mod storage_liveness; pub use self::storage_liveness::*; diff --git a/src/librustc_mir/dataflow/impls/reaching_defs.rs b/src/librustc_mir/dataflow/impls/reaching_defs.rs new file mode 100644 index 0000000000000..f75ef3db40c1f --- /dev/null +++ b/src/librustc_mir/dataflow/impls/reaching_defs.rs @@ -0,0 +1,414 @@ +use rustc::mir::visit::{PlaceContext, Visitor}; +use rustc::mir::visit::{MutatingUseContext, NonMutatingUseContext, NonUseContext}; +use rustc::mir::{self, Local, Location, Place, PlaceBase}; +use rustc_data_structures::bit_set::BitSet; +use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::indexed_vec::{Idx, IndexVec}; +use smallvec::SmallVec; + +use super::{BitDenotation, GenKillSet, BottomValue}; + +newtype_index! { + /// A linear index for each definition in the program. + /// + /// This must be linear since it will be used to index a bit vector containing the set of + /// reaching definitions during dataflow analysis. + pub struct DefIndex { + DEBUG_FORMAT = "def{}", + } +} + +/// A dataflow analysis which tracks whether an assignment to a variable might still constitute the +/// value of that variable at a given point in the program. +/// +/// Reaching definitions uses the term "definition" to refer to places in a program where a +/// variable's value may change. Most definitions are writes to a variable (e.g. via +/// `StatementKind::Assign`). However, a variable can also be implicitly defined by calling code if +/// it is a function parameter or defined to be uninitialized via a call to +/// `MaybeUninit::uninitialized`. +/// +/// An example, using parentheses to denote each definition. +/// +/// ```rust,no_run +/// fn collatz_step(mut n: i32) -> i32 { // (1) Implicit definiton of function param +/// if n % 2 == 0 { +/// n /= 2 // (2) +/// } else { +/// n = 3*n + 1 // (3) +/// } +/// +/// // Only definitions (2) and (3) could possibly constitute the value of `n` at function exit. +/// // (1), the `n` which was passed into the function, is killed by either (2) or (3) +/// // along all paths to this point. +/// n +/// } +/// ``` +/// +/// Not all programs are so simple; some care is required to handle projections. For field and +/// array index projections, we cannot kill all previous definitions of the same variable, since +/// only part of the variable was defined. If we encounter a deref projection on the left-hand side +/// of an assignment, we don't even know *which* variable is being defined, since the pointer being +/// dereferenced may be pointing anywhere. Such definitions are never killed. Finally, we must +/// treat function calls as opaque: We can't know (without MIR inlining or explicit annotation) +/// whether a callee mutates data behind a pointer. If the address of one of our locals is +/// observable, it may be the target of such a mutation. A type-based analysis (e.g. does this +/// function take any type containing a mutable reference as a parameter?) is insufficient, since +/// raw pointers can be laundered through any integral type. +/// +/// At the moment, the possible targets of a definition are tracked at the granularity of a +/// `Local`, not a `MovePath` as is done in the initialized places analysis. That means that a +/// definition like `x.y = 1` does not kill a previous assignment of `x.y`. This could be made more +/// precise with more work; it is not a fundamental limitation of the analysis. +/// +/// Most passes will want to know which definitions of a **specific** local reach a given point in +/// the program. `ReachingDefinitions` does not provide this information; it can only provide all +/// definitions of **any** local. However, a `ReachingDefinitions` analysis can be used to build a +/// `UseDefChain`, which does provide this information. +pub struct ReachingDefinitions { + all: IndexVec, + + /// All (direct) definitions of a local in the program. + for_local_direct: IndexVec>, + + /// All definitions at the given location in the program. + at_location: FxHashMap>, +} + +fn is_call_with_side_effects(terminator: &mir::Terminator<'tcx>) -> bool { + if let mir::TerminatorKind::Call { .. } = terminator.kind { + true + } else { + false + } +} + +impl ReachingDefinitions { + pub fn new(body: &mir::Body<'_>) -> Self { + let mut ret = ReachingDefinitions { + all: IndexVec::new(), + for_local_direct: IndexVec::from_elem(Vec::new(), &body.local_decls), + at_location: Default::default(), + }; + + let mut builder = DefBuilder { defs: &mut ret }; + for arg in body.args_iter() { + builder.add_arg_def(arg); + } + + DefVisitor(|place: &Place<'_>, loc| builder.visit_def(place, loc)) + .visit_body(body); + + // Add all function calls as indirect definitions. + let blocks_with_side_effects = body + .basic_blocks() + .iter_enumerated() + .filter(|(_, data)| is_call_with_side_effects(data.terminator())); + + for (block, data) in blocks_with_side_effects { + let term_loc = Location { block, statement_index: data.statements.len() }; + builder.add_def(term_loc, DefKind::Indirect); + } + + ret + } + + pub fn len(&self) -> usize { + self.all.len() + } + + pub fn get(&self, idx: DefIndex) -> &Definition { + &self.all[idx] + } + + /// Iterates over all definitions which go through a layer of indirection (e.g. `(*_1) = ...`). + pub fn indirect(&self) -> impl Iterator + '_ { + self.all + .iter_enumerated() + .filter(|(_, def)| def.is_indirect()) + .map(|(id, _)| id) + } + + pub fn for_local_direct(&self, local: Local) -> impl Iterator + '_ { + self.for_local_direct[local].iter().cloned() + } + + pub fn at_location(&self, location: Location) -> impl Iterator + '_ { + self.at_location + .get(&location) + .into_iter() + .flat_map(|v| v.iter().cloned()) + } + + pub fn for_args(&self) -> impl Iterator + '_ { + self.all + .iter_enumerated() + .take_while(|(_, def)| def.location.is_none()) + .map(|(def, _)| def) + } + + fn update_trans(&self, trans: &mut GenKillSet, location: Location) { + for def in self.at_location(location) { + trans.gen(def); + + // If we assign directly to a local (e.g. `_1 = ...`), we can kill all other + // direct definitions of that local. We must not kill indirect definitions, since + // they may define locals other than the one currently being assigned to. + // + // FIXME: If we assign to a field of a local `_1.x = ...`, we could kill all other + // definitions of any part of that field (e.g. `_1.x.y = ...`). This would require + // tracking `MovePath`s instead of `Local`s. + if let DefKind::DirectWhole(local) = self.get(def).kind { + let other_direct_defs = self + .for_local_direct(local) + .filter(|&d| d != def); + + trans.kill_all(other_direct_defs); + } + } + } +} + +impl<'tcx> BitDenotation<'tcx> for ReachingDefinitions { + type Idx = DefIndex; + fn name() -> &'static str { "reaching_definitions" } + + fn bits_per_block(&self) -> usize { + self.len() + } + + fn start_block_effect(&self, entry_set: &mut BitSet) { + // Our parameters were defined by the caller at function entry. + for def in self.for_args() { + entry_set.insert(def); + } + } + + fn statement_effect( + &self, + trans: &mut GenKillSet, + location: Location, + ) { + self.update_trans(trans, location); + } + + fn terminator_effect( + &self, + trans: &mut GenKillSet, + location: Location, + ) { + self.update_trans(trans, location); + } + + fn propagate_call_return( + &self, + _in_out: &mut BitSet, + _call_bb: mir::BasicBlock, + _dest_bb: mir::BasicBlock, + _dest_place: &mir::Place<'tcx>, + ) { + // FIXME: RETURN_PLACE should only be defined when a call returns successfully. + } +} + +impl BottomValue for ReachingDefinitions { + /// bottom = definition not reachable + const BOTTOM_VALUE: bool = false; +} + +struct DefBuilder<'a> { + defs: &'a mut ReachingDefinitions, +} + +impl DefBuilder<'_> { + fn add_arg_def(&mut self, arg: Local) -> DefIndex { + let def = Definition { kind: DefKind::DirectWhole(arg), location: None }; + let idx = self.defs.all.push(def); + + self.defs.for_local_direct[arg].push(idx); + idx + } + + fn add_def(&mut self, location: Location, kind: DefKind) -> DefIndex { + let def = Definition { kind, location: Some(location) }; + let idx = self.defs.all.push(def); + + if let Some(local) = kind.direct() { + self.defs.for_local_direct[local].push(idx); + } + + self.defs.at_location.entry(location).or_default().push(idx); + idx + } + + fn visit_def(&mut self, place: &mir::Place<'tcx>, location: Location) { + // We don't track reaching defs for statics. + let def_for_local = DefKind::of_place(place) + .map(PlaceBase::local) + .transpose(); + + if let Some(def) = def_for_local { + self.add_def(location, def); + } + } +} + +#[derive(Debug, Clone, Copy)] +pub struct Definition { + pub kind: DefKind, + + /// The `Location` at which this definition occurs, or `None` if it occurs before the + /// `mir::Body` is entered (e.g. the value of an argument). + pub location: Option, +} + +impl Definition { + fn is_indirect(&self) -> bool { + self.kind == DefKind::Indirect + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DefKind { + /// An entire variable is defined directly, e.g. `_1 = ...`. + DirectWhole(T), + + /// Some part of a variable (a field or an array offset) is defined directly, e.g. `_1.field = + /// ...` or `_1[2] = ...`. + DirectPart(T), + + /// An unknown variable is defined through a pointer, e.g. `(*_1) = ...`. + /// + /// In general, we cannot know where `_1` is pointing: it may reference any local in the + /// current `Body` (whose address has been observed) or somewhere else entirely (e.g. a static, + /// the heap, or a local further down the stack). + Indirect, +} + +impl DefKind { + /// Returns `Some(T)` if the `DefKind` is direct or `None` if it is indirect. + pub fn direct(self) -> Option { + match self { + DefKind::DirectWhole(x) | DefKind::DirectPart(x) => Some(x), + DefKind::Indirect => None, + } + } + + /// Converts a `DefKind` to a `DefKind` by applying a function to the value contained in + /// either `DirectWhole` or `DirectPart`. + fn map(self, f: impl FnOnce(T) -> U) -> DefKind { + match self { + DefKind::DirectWhole(x) => DefKind::DirectWhole(f(x)), + DefKind::DirectPart(x) => DefKind::DirectPart(f(x)), + DefKind::Indirect => DefKind::Indirect, + } + } +} + +impl DefKind> { + /// Converts a `DefKind` of an `Option` into an `Option` of a `DefKind`. + /// + /// The result is `None` if either direct variant contains `None`. Otherwise, the result is + /// `Some(self.map(Option::unwrap))`. + fn transpose(self) -> Option> { + match self { + DefKind::DirectWhole(Some(x)) => Some(DefKind::DirectWhole(x)), + DefKind::DirectPart(Some(x)) => Some(DefKind::DirectPart(x)), + DefKind::Indirect => Some(DefKind::Indirect), + + DefKind::DirectWhole(None) | DefKind::DirectPart(None) => None, + } + } +} + +impl<'a, 'tcx> DefKind<&'a mir::PlaceBase<'tcx>> { + fn of_place(place: &'a mir::Place<'tcx>) -> Self { + place.iterate(|base, projections| { + let mut is_whole = true; + for proj in projections { + is_whole = false; + if proj.elem == mir::ProjectionElem::Deref { + return DefKind::Indirect; + } + } + + if is_whole { + DefKind::DirectWhole(base) + } else { + DefKind::DirectPart(base) + } + }) + } +} + +pub struct DefVisitor(pub F); + +impl<'tcx, F> Visitor<'tcx> for DefVisitor + where F: FnMut(&Place<'tcx>, Location) +{ + fn visit_place(&mut self, + place: &Place<'tcx>, + context: PlaceContext, + location: Location) { + self.super_place(place, context, location); + + match context { + // DEFS + | PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) + | PlaceContext::MutatingUse(MutatingUseContext::Call) + | PlaceContext::MutatingUse(MutatingUseContext::Store) + => self.0(place, location), + + | PlaceContext::MutatingUse(MutatingUseContext::Borrow) + | PlaceContext::MutatingUse(MutatingUseContext::Drop) + | PlaceContext::MutatingUse(MutatingUseContext::Projection) + | PlaceContext::MutatingUse(MutatingUseContext::Retag) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow) + | PlaceContext::NonUse(NonUseContext::AscribeUserTy) + | PlaceContext::NonUse(NonUseContext::StorageDead) + | PlaceContext::NonUse(NonUseContext::StorageLive) + => (), + } + } +} + +pub struct UseVisitor(pub F); + +impl<'tcx, F> Visitor<'tcx> for UseVisitor + where F: FnMut(&Place<'tcx>, Location) +{ + fn visit_place(&mut self, + place: &Place<'tcx>, + context: PlaceContext, + location: Location) { + self.super_place(place, context, location); + + match context { + | PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) + | PlaceContext::MutatingUse(MutatingUseContext::Borrow) + | PlaceContext::MutatingUse(MutatingUseContext::Drop) + | PlaceContext::MutatingUse(MutatingUseContext::Projection) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow) + => self.0(place, location), + + | PlaceContext::MutatingUse(MutatingUseContext::Call) + | PlaceContext::MutatingUse(MutatingUseContext::Retag) + | PlaceContext::MutatingUse(MutatingUseContext::Store) + | PlaceContext::NonUse(NonUseContext::AscribeUserTy) + | PlaceContext::NonUse(NonUseContext::StorageDead) + | PlaceContext::NonUse(NonUseContext::StorageLive) + => (), + } + } +} diff --git a/src/librustc_mir/dataflow/impls/use_def_chain.rs b/src/librustc_mir/dataflow/impls/use_def_chain.rs new file mode 100644 index 0000000000000..6cb5cce6a5b69 --- /dev/null +++ b/src/librustc_mir/dataflow/impls/use_def_chain.rs @@ -0,0 +1,302 @@ +use std::collections::BTreeMap; +use std::ops; + +use rustc::mir::{self, BasicBlock, Local, Location, Place}; +use rustc::mir::visit::{Visitor, MirVisitable}; +use rustc::util::captures::Captures; +use rustc_data_structures::bit_set::BitSet; +use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::indexed_vec::{Idx, IndexVec}; + +use crate::dataflow::{ + DataflowResults, DataflowResultsCursor, + impls::HaveBeenBorrowedLocals, + impls::reaching_defs::{DefIndex, DefKind, Definition, ReachingDefinitions, UseVisitor}, +}; + +type LocationMap = FxHashMap; +type DefsForLocalDense = IndexVec>; +type DefsForLocalSparse = BTreeMap>; +type DefsForUseAtLocation = LocationMap; + +/// A data structure, built using the results of a `ReachingDefinitions` analysis, that maps each +/// use of a variable to the definitions of that variable that could reach it. +/// +/// While `ReachingDefinitions` maps each point in the program to every definition which reaches +/// that point, a `UseDefChain` maps each use of a variable to every definition which could define +/// that variable. See below for an example where this distinction is meaningful. +/// +/// ```rust,no_run +/// fn five() -> i32 { +/// let mut x = 0; // (1) +/// +/// let p = &mut x; // (2) +/// *p = 1; // (3) +/// +/// x = 2; // (4) +/// +/// x + 3 +/// // At this point, a `UseDefChain` knows that (4) is the only possible definition of `x`, +/// // even though (2), (3) and (4) reach here. A reaching definitions analysis can kill (1) +/// // since it is superseded by (4), but it cannot kill (3) since `p` may be pointing +/// // to a local besides `x`. However, a `UseDefChain` is only interested in definitions of +/// // `x`, so it can kill *all* other definitions when (4) is encountered. +/// } +/// ``` +/// +/// This precision comes at the cost of increased memory usage; A `UseDefChain` stores a `BitSet` +/// for every use in the program. This allows consumers to traverse the graph of data dependencies +/// efficiently. For example, a `UseDefChain` could be used for const-propagation by iterating over +/// the definitions which could possibly reach `y` at the exit of the following function and +/// realizing that all of them are the constant `6`. +/// +/// ```rust,no_run +/// fn six(condition: bool) -> i32 { +/// let mut y; +/// let a = 2 * 3; +/// let b = 10 - 4; +/// +/// if condition { +/// y = a; +/// } else { +/// y = b; +/// } +/// +/// y +/// } +/// ``` +pub struct UseDefChain<'me, 'tcx> { + reaching_defs: &'me DataflowResults<'tcx, ReachingDefinitions>, + + /// Maps each local that is used at a given location to the set of definitions which can reach + /// it. + defs_for_use: DefsForUseAtLocation, +} + +impl<'me, 'tcx> UseDefChain<'me, 'tcx> { + pub fn new( + body: &'me mir::Body<'tcx>, + reaching_defs: &'me DataflowResults<'tcx, ReachingDefinitions>, + observed_addresses: &DataflowResults<'tcx, HaveBeenBorrowedLocals<'_, 'tcx>>, + ) -> Self { + let defs = reaching_defs.operator(); + let all_defs_for_local = all_defs_for_local(body, defs, &observed_addresses); + let locals_used_in_block = LocalsUsedInBlock::new(body); + let curr_defs_for_local = + IndexVec::from_elem(BitSet::new_empty(defs.len()), &body.local_decls); + + let mut defs_for_use = DefsForUseAtLocation::default(); + + { + let mut builder = UseDefBuilder { + body, + ud_chain: &mut defs_for_use, + reaching_defs, + locals_used_in_block, + curr_defs_for_local, + all_defs_for_local, + }; + + builder.visit_body(body); + } + + UseDefChain { + defs_for_use, + reaching_defs, + } + } + + pub fn defs_for_use( + &self, + local: Local, + location: Location, + ) -> impl Iterator + Captures<'tcx> + '_ { + let UseDefChain { defs_for_use, reaching_defs, .. } = self; + + defs_for_use + .get(&location) + .and_then(|for_local| for_local.get(&local)) + .into_iter() + .flat_map(|defs| defs.iter()) + .map(move |def| reaching_defs.operator().get(def)) + } +} + +struct UseDefBuilder<'a, 'me, 'tcx> { + ud_chain: &'a mut DefsForUseAtLocation, + + body: &'me mir::Body<'tcx>, + + reaching_defs: &'me DataflowResults<'tcx, ReachingDefinitions>, + + /// It's expensive to clone the entry set at the start of each block for every local and apply + /// the appropriate mask. However, we only need to keep `curr_defs_for_local` up-to-date for + /// the `Local`s which are actually used in a given `BasicBlock`. + locals_used_in_block: LocalsUsedInBlock, + + /// The set of definitions which could have reached a given local at the program point we are + /// currently processing. + curr_defs_for_local: DefsForLocalDense, + + /// The set of all definitions which *might* define the given local. These definitions may or + /// may not be live at the current program point. + all_defs_for_local: DefsForLocalDense, +} + +impl<'me, 'tcx> UseDefBuilder<'_, 'me, 'tcx> { + fn visit_location(&mut self, location: Location, stmt_or_term: &impl MirVisitable<'tcx>) { + let UseDefBuilder { + all_defs_for_local, + curr_defs_for_local, + locals_used_in_block, + reaching_defs, + ud_chain, + .. + } = self; + + // If this is the start of a `BasicBlock`, reset `curr_defs_for_local` for each (tracked) + // local to the set of definitions which reach the start of this block and can modify that + // local. + if location.statement_index == 0 { + for local in locals_used_in_block[location.block].iter() { + let entry_set = &reaching_defs.sets().entry_set_for(location.block.index()); + curr_defs_for_local[local].overwrite(entry_set); + curr_defs_for_local[local].intersect(&all_defs_for_local[local]); + } + } + + // Save the current set of reaching definitions for any local which is used at this + // location. + let record_use = |place: &Place<'tcx>, location| { + if let Some(local) = place.base_local() { + ud_chain.entry(location) + .or_default() + .entry(local) + .or_insert_with(|| { + debug_assert!(locals_used_in_block[location.block].contains(local)); + curr_defs_for_local[local].clone() + }); + } + }; + + stmt_or_term.apply(location, &mut UseVisitor(record_use)); + + // Update `curr_defs_for_local` with any new definitions from this location. + // + // This needs to take place after processing uses, otherwise a def like `x = + // x+1` will be marked as reaching its right hand side. + let reaching_defs = reaching_defs.operator(); + let tracked_in_block = &locals_used_in_block[location.block]; + for def in reaching_defs.at_location(location) { + match reaching_defs.get(def).kind { + // No need to process defs for a local that is untracked + DefKind::DirectWhole(local) | DefKind::DirectPart(local) + if !tracked_in_block.contains(local) + => continue, + + DefKind::DirectWhole(local) => { + let defs = &mut curr_defs_for_local[local]; + defs.clear(); + defs.insert(def); + } + + DefKind::DirectPart(local) => { + curr_defs_for_local[local].insert(def); + } + + DefKind::Indirect => { + self.body + .local_decls + .indices() + .filter(|&local| all_defs_for_local[local].contains(def)) + .for_each(|local| { curr_defs_for_local[local].insert(def); }); + } + } + } + } +} + +impl<'tcx> Visitor<'tcx> for UseDefBuilder<'_, '_, 'tcx> { + fn visit_statement(&mut self, statement: &mir::Statement<'tcx>, location: Location) { + // Don't recurse: We only care about statements and terminators + // self.super_statement(statement, location); + + self.visit_location(location, statement) + } + + fn visit_terminator(&mut self, + terminator: &mir::Terminator<'tcx>, + location: Location) { + // Don't recurse: We only care about statements and terminators + // self.super_terminator(terminator, location); + + self.visit_location(location, terminator) + } +} + +/// The set of `Local`s which are used at least once in each basic block. +struct LocalsUsedInBlock(IndexVec>); + +impl LocalsUsedInBlock { + fn new(body: &mir::Body<'_>) -> Self { + let each_block = + IndexVec::from_elem(BitSet::new_empty(body.local_decls.len()), body.basic_blocks()); + let mut ret = LocalsUsedInBlock(each_block); + + UseVisitor(|place: &Place<'_>, loc| ret.visit_use(place, loc)) + .visit_body(body); + + ret + } + + fn visit_use(&mut self, place: &mir::Place<'tcx>, loc: Location) { + if let Some(local) = place.base_local() { + self.0[loc.block].insert(local); + } + } +} + +impl ops::Deref for LocalsUsedInBlock { + type Target = IndexVec>; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +/// `BitSet` cannot efficiently `FromIterator` since it needs to know its size ahead of time. +fn bitset_from_iter(size: usize, it: impl IntoIterator) -> BitSet { + let mut ret = BitSet::new_empty(size); + for item in it { + ret.insert(item); + } + + ret +} + +fn all_defs_for_local<'tcx>( + body: &mir::Body<'tcx>, + defs: &ReachingDefinitions, + observed_addresses: &DataflowResults<'tcx, HaveBeenBorrowedLocals<'_, 'tcx>>, +) -> DefsForLocalDense { + // Initialize the return value with the direct definitions of each local. + let mut defs_for_local: DefsForLocalDense = body + .local_decls + .iter_enumerated() + .map(|(local, _)| bitset_from_iter(defs.len(), defs.for_local_direct(local))) + .collect(); + + // Add each indirect definition to the set of definitions for each local whose address has been + // observed at that point. + let mut observed_addresses = DataflowResultsCursor::new(observed_addresses, body); + for def_id in defs.indirect() { + let def = defs.get(def_id); + observed_addresses.seek(def.location.expect("Indirect def without location")); + + for local in observed_addresses.get().iter() { + defs_for_local[local].insert(def_id); + } + } + + defs_for_local +} diff --git a/src/librustc_mir/dataflow/mod.rs b/src/librustc_mir/dataflow/mod.rs index 0a34c5f20ab22..387ddee515ffa 100644 --- a/src/librustc_mir/dataflow/mod.rs +++ b/src/librustc_mir/dataflow/mod.rs @@ -19,6 +19,7 @@ use std::usize; pub use self::impls::{MaybeStorageLive, RequiresStorage}; pub use self::impls::{MaybeInitializedPlaces, MaybeUninitializedPlaces}; +pub use self::impls::{ReachingDefinitions, UseDefChain}; pub use self::impls::DefinitelyInitializedPlaces; pub use self::impls::EverInitializedPlaces; pub use self::impls::borrows::Borrows; From 65c8fda718a986505a633f9cc0445d8c13176c32 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 9 Jul 2019 14:29:42 -0700 Subject: [PATCH 09/13] Add support for `UseDefChain` in `rustc_peek` --- src/librustc_mir/transform/rustc_peek.rs | 87 ++++++++++++++++++++++-- src/libsyntax_pos/symbol.rs | 1 + 2 files changed, 82 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index e0808b6531ed1..be8e54e4efa97 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -13,6 +13,8 @@ use crate::dataflow::{self, do_dataflow, DebugFormatted}; use crate::dataflow::MoveDataParamEnv; use crate::dataflow::BitDenotation; use crate::dataflow::DataflowResults; +use crate::dataflow::HaveBeenBorrowedLocals; +use crate::dataflow::{ReachingDefinitions, UseDefChain}; use crate::dataflow::{ DefinitelyInitializedPlaces, MaybeInitializedPlaces, MaybeUninitializedPlaces }; @@ -51,6 +53,17 @@ impl MirPass for SanityCheck { DefinitelyInitializedPlaces::new(tcx, body, &mdpe), |bd, i| DebugFormatted::new(&bd.move_data().move_paths[i])); + let flow_borrowed_locals = + do_dataflow(tcx, body, def_id, &attributes, &dead_unwinds, + HaveBeenBorrowedLocals::new(body), + |_bd, i| DebugFormatted::new(&i)); + let flow_reaching_defs = + do_dataflow(tcx, body, def_id, &attributes, &dead_unwinds, + ReachingDefinitions::new(body), + |bd, i| DebugFormatted::new(&bd.get(i).location)); + let flow_use_def_chain = + UseDefChain::new(body, &flow_reaching_defs, &flow_borrowed_locals); + 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); } @@ -60,6 +73,9 @@ impl MirPass 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); } + if has_rustc_mir_with(&attributes, sym::rustc_peek_use_def_chain).is_some() { + sanity_check_via_rustc_peek(tcx, body, def_id, &attributes, &flow_use_def_chain); + } if has_rustc_mir_with(&attributes, sym::stop_after_dataflow).is_some() { tcx.sess.fatal("stop_after_dataflow ended compilation"); } @@ -87,7 +103,7 @@ pub fn sanity_check_via_rustc_peek<'tcx, O>( body: &Body<'tcx>, def_id: DefId, _attributes: &[ast::Attribute], - results: &DataflowResults<'tcx, O>, + results: &O, ) where O: RustcPeekAt<'tcx> { debug!("sanity_check_via_rustc_peek def_id: {:?}", def_id); @@ -214,27 +230,32 @@ impl PeekCall { } } -pub trait RustcPeekAt<'tcx>: BitDenotation<'tcx> { +pub trait RustcPeekAt<'tcx> { fn peek_at( &self, tcx: TyCtxt<'tcx>, + body: &mir::Body<'tcx>, place: &mir::Place<'tcx>, - flow_state: &BitSet, + location: Location, call: PeekCall, ); } -impl<'tcx, O> RustcPeekAt<'tcx> for O +impl<'tcx, O> RustcPeekAt<'tcx> for DataflowResults<'tcx, O> where O: BitDenotation<'tcx, Idx = MovePathIndex> + HasMoveData<'tcx>, { fn peek_at( &self, tcx: TyCtxt<'tcx>, + body: &mir::Body<'tcx>, place: &mir::Place<'tcx>, - flow_state: &BitSet, + location: Location, call: PeekCall, ) { - match self.move_data().rev_lookup.find(place) { + let operator = self.operator(); + let flow_state = dataflow::state_for_location(location, operator, self, body); + + match operator.move_data().rev_lookup.find(place) { LookupResult::Exact(peek_mpi) => { let bit_state = flow_state.contains(peek_mpi); debug!("rustc_peek({:?} = &{:?}) bit_state: {}", @@ -249,3 +270,57 @@ impl<'tcx, O> RustcPeekAt<'tcx> for O } } } + +impl<'tcx> RustcPeekAt<'tcx> for UseDefChain<'_, 'tcx> { + fn peek_at( + &self, + tcx: TyCtxt<'tcx>, + body: &mir::Body<'tcx>, + place: &mir::Place<'tcx>, + location: Location, + call: PeekCall, + ) { + + let base_local = place + .base_direct() + .expect("Deref in argument to `rustc_peek`") + .local() + .expect("Argument to `rustc_peek` must be a local variable"); + + let mut defs: Vec<_> = self + .defs_for_use(base_local, location) + .map(|def| { + let span = def + .location + .map(|loc| { + let block = &body.basic_blocks()[loc.block]; + block.statements + .get(loc.statement_index) + .map(|stmt| stmt.source_info) + .unwrap_or(block.terminator().source_info) + .span + }) + .unwrap_or_else(|| { + // `def` represents the value of a parameter on function entry. + let local = def.kind.direct().unwrap(); + body.local_decls[local].source_info.span + }); + + let src = tcx.sess.source_map(); + let snippet = src.span_to_snippet(span).unwrap(); + let line_index = src.span_to_lines(span).unwrap().lines[0].line_index; + let line_no = line_index + 1; + + (line_no, snippet) + }) + .collect(); + + defs.sort_by_key(|(line_no, _)| *line_no); + let defs: Vec<_> = defs.into_iter() + .map(|(line_no, snippet)| format!("{}: \"{}\"", line_no, snippet)) + .collect(); + + let msg = format!("rustc_peek: [{}]", defs.join(", ")); + tcx.sess.span_err(call.span, &msg); + } +} diff --git a/src/libsyntax_pos/symbol.rs b/src/libsyntax_pos/symbol.rs index f7e1b983e5446..968576a65ecbe 100644 --- a/src/libsyntax_pos/symbol.rs +++ b/src/libsyntax_pos/symbol.rs @@ -593,6 +593,7 @@ symbols! { rustc_peek_definite_init, rustc_peek_maybe_init, rustc_peek_maybe_uninit, + rustc_peek_use_def_chain, rustc_private, rustc_proc_macro_decls, rustc_promotable, From bdaa90a0c16a528bcacb0e9c24cfe24e7a761cb8 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 14 Jul 2019 14:24:02 -0700 Subject: [PATCH 10/13] Add reaching definitions tests --- src/test/ui/mir-dataflow/reaching-defs-1.rs | 36 +++++++++++++++++++ .../ui/mir-dataflow/reaching-defs-1.stderr | 22 ++++++++++++ src/test/ui/mir-dataflow/reaching-defs-2.rs | 15 ++++++++ .../ui/mir-dataflow/reaching-defs-2.stderr | 10 ++++++ src/test/ui/mir-dataflow/reaching-defs-arg.rs | 19 ++++++++++ .../ui/mir-dataflow/reaching-defs-arg.stderr | 10 ++++++ .../mir-dataflow/reaching-defs-indirect-1.rs | 33 +++++++++++++++++ .../reaching-defs-indirect-1.stderr | 16 +++++++++ 8 files changed, 161 insertions(+) create mode 100644 src/test/ui/mir-dataflow/reaching-defs-1.rs create mode 100644 src/test/ui/mir-dataflow/reaching-defs-1.stderr create mode 100644 src/test/ui/mir-dataflow/reaching-defs-2.rs create mode 100644 src/test/ui/mir-dataflow/reaching-defs-2.stderr create mode 100644 src/test/ui/mir-dataflow/reaching-defs-arg.rs create mode 100644 src/test/ui/mir-dataflow/reaching-defs-arg.stderr create mode 100644 src/test/ui/mir-dataflow/reaching-defs-indirect-1.rs create mode 100644 src/test/ui/mir-dataflow/reaching-defs-indirect-1.stderr diff --git a/src/test/ui/mir-dataflow/reaching-defs-1.rs b/src/test/ui/mir-dataflow/reaching-defs-1.rs new file mode 100644 index 0000000000000..25dd7ae32f22e --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-1.rs @@ -0,0 +1,36 @@ +// General test of reaching_defs state computed by MIR dataflow. + +#![feature(core_intrinsics, rustc_attrs)] +use std::intrinsics::rustc_peek; + +#[rustc_mir(rustc_peek_use_def_chain, stop_after_dataflow, borrowck_graphviz_postflow="flow.dot")] +fn foo(test: bool) -> (i32, i32) { + // Splitting declarations and assignment gives us nicer spans + let mut x; + let mut y; + + x=0; + y=1; + + if test { + x=2; + unsafe { rustc_peek(&x); } + //~^ ERROR rustc_peek: [16: "x=2"] + } else { + x=3; + y=4; + } + + unsafe { rustc_peek(&x); } + //~^ ERROR rustc_peek: [16: "x=2", 17: "rustc_peek(&x)", 20: "x=3"] + + unsafe { rustc_peek(&y); } + //~^ ERROR rustc_peek: [13: "y=1", 21: "y=4"] + + (x, y) +} + +fn main() { + foo(true); + foo(false); +} diff --git a/src/test/ui/mir-dataflow/reaching-defs-1.stderr b/src/test/ui/mir-dataflow/reaching-defs-1.stderr new file mode 100644 index 0000000000000..a5e8fe5464125 --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-1.stderr @@ -0,0 +1,22 @@ +error: rustc_peek: [16: "x=2"] + --> $DIR/reaching-defs-1.rs:17:18 + | +LL | unsafe { rustc_peek(&x); } + | ^^^^^^^^^^^^^^ + +error: rustc_peek: [16: "x=2", 17: "rustc_peek(&x)", 20: "x=3"] + --> $DIR/reaching-defs-1.rs:24:14 + | +LL | unsafe { rustc_peek(&x); } + | ^^^^^^^^^^^^^^ + +error: rustc_peek: [13: "y=1", 21: "y=4"] + --> $DIR/reaching-defs-1.rs:27:14 + | +LL | unsafe { rustc_peek(&y); } + | ^^^^^^^^^^^^^^ + +error: stop_after_dataflow ended compilation + +error: aborting due to 4 previous errors + diff --git a/src/test/ui/mir-dataflow/reaching-defs-2.rs b/src/test/ui/mir-dataflow/reaching-defs-2.rs new file mode 100644 index 0000000000000..39f6047f4cb97 --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-2.rs @@ -0,0 +1,15 @@ +#![feature(core_intrinsics, rustc_attrs)] +use std::intrinsics::rustc_peek; + +#[rustc_mir(rustc_peek_use_def_chain, stop_after_dataflow, borrowck_graphviz_postflow="flow.dot")] +fn main() { + let mut x; + x=0; + + while x != 10 { + x+=1; + } + + unsafe { rustc_peek(x); } + //~^ ERROR rustc_peek: [7: "x=0", 10: "x+=1"] +} diff --git a/src/test/ui/mir-dataflow/reaching-defs-2.stderr b/src/test/ui/mir-dataflow/reaching-defs-2.stderr new file mode 100644 index 0000000000000..1c79796976ab2 --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-2.stderr @@ -0,0 +1,10 @@ +error: rustc_peek: [7: "x=0", 10: "x+=1"] + --> $DIR/reaching-defs-2.rs:13:14 + | +LL | unsafe { rustc_peek(x); } + | ^^^^^^^^^^^^^ + +error: stop_after_dataflow ended compilation + +error: aborting due to 2 previous errors + diff --git a/src/test/ui/mir-dataflow/reaching-defs-arg.rs b/src/test/ui/mir-dataflow/reaching-defs-arg.rs new file mode 100644 index 0000000000000..5139032cfbfd1 --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-arg.rs @@ -0,0 +1,19 @@ +#![feature(core_intrinsics, rustc_attrs)] +use std::intrinsics::rustc_peek; + +#[rustc_mir(rustc_peek_use_def_chain, stop_after_dataflow, borrowck_graphviz_postflow="flow.dot")] +fn foo(test: bool, mut x: i32) -> i32 { + if test { + x=42; + } + + unsafe { rustc_peek(&x); } + //~^ ERROR rustc_peek: [5: "mut x", 7: "x=42"] + + x +} + +fn main() { + foo(true, 32); + foo(false, 56); +} diff --git a/src/test/ui/mir-dataflow/reaching-defs-arg.stderr b/src/test/ui/mir-dataflow/reaching-defs-arg.stderr new file mode 100644 index 0000000000000..8ba01de50fd3f --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-arg.stderr @@ -0,0 +1,10 @@ +error: rustc_peek: [5: "mut x", 7: "x=42"] + --> $DIR/reaching-defs-arg.rs:10:14 + | +LL | unsafe { rustc_peek(&x); } + | ^^^^^^^^^^^^^^ + +error: stop_after_dataflow ended compilation + +error: aborting due to 2 previous errors + diff --git a/src/test/ui/mir-dataflow/reaching-defs-indirect-1.rs b/src/test/ui/mir-dataflow/reaching-defs-indirect-1.rs new file mode 100644 index 0000000000000..588bca53d509a --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-indirect-1.rs @@ -0,0 +1,33 @@ +#![feature(core_intrinsics, rustc_attrs)] +use std::intrinsics::rustc_peek; + +#[rustc_mir(rustc_peek_use_def_chain, stop_after_dataflow, borrowck_graphviz_postflow="flow.dot")] +fn foo(test: bool) -> (i32, i32) { + let mut x; + let mut y; + let mut p; + + x=0; + y=1; + + unsafe { rustc_peek(x); } + //~^ ERROR rustc_peek: [10: "x=0"] + + if test { + p = &mut x; + } else { + p = &mut y; + } + + *p=2; + + unsafe { rustc_peek(x); } + //~^ ERROR rustc_peek: [10: "x=0", 22: "*p=2"] + + (x, y) +} + +fn main() { + foo(true); + foo(false); +} diff --git a/src/test/ui/mir-dataflow/reaching-defs-indirect-1.stderr b/src/test/ui/mir-dataflow/reaching-defs-indirect-1.stderr new file mode 100644 index 0000000000000..0d781927e95b9 --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-indirect-1.stderr @@ -0,0 +1,16 @@ +error: rustc_peek: [10: "x=0"] + --> $DIR/reaching-defs-indirect-1.rs:13:14 + | +LL | unsafe { rustc_peek(x); } + | ^^^^^^^^^^^^^ + +error: rustc_peek: [10: "x=0", 22: "*p=2"] + --> $DIR/reaching-defs-indirect-1.rs:24:14 + | +LL | unsafe { rustc_peek(x); } + | ^^^^^^^^^^^^^ + +error: stop_after_dataflow ended compilation + +error: aborting due to 3 previous errors + From cfd9320c7a37073009d20cd36b7f6be2c7604b8c Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sat, 27 Jul 2019 12:50:27 -0700 Subject: [PATCH 11/13] Update to use new `Place` --- src/librustc/mir/mod.rs | 13 +++++++--- src/librustc_mir/transform/rustc_peek.rs | 33 ++++++++++++------------ 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index f2f726a1f4d25..7cca5cb764fc5 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -1927,11 +1927,11 @@ impl<'tcx> Place<'tcx> { place_projection: &Option>>, op: impl FnOnce(&'a PlaceBase<'tcx>, ProjectionsIter<'_, 'tcx>) -> R, ) -> R { - fn iterate_over2<'tcx, R>( - place_base: &PlaceBase<'tcx>, + fn iterate_over2<'a, 'tcx, R: 'a>( + place_base: &'a PlaceBase<'tcx>, place_projection: &Option>>, next: &Projections<'_, 'tcx>, - op: impl FnOnce(&PlaceBase<'tcx>, ProjectionsIter<'_, 'tcx>) -> R, + op: impl FnOnce(&'a PlaceBase<'tcx>, ProjectionsIter<'_, 'tcx>) -> R, ) -> R { match place_projection { None => { @@ -2242,6 +2242,13 @@ impl<'tcx> Operand<'tcx> { Operand::Move(ref place) => Operand::Copy(place.clone()), } } + + pub fn place(&self) -> Option<&Place<'tcx>> { + match self { + Operand::Copy(place) | Operand::Move(place) => Some(place), + Operand::Constant(_) => None, + } + } } /////////////////////////////////////////////////////////////////////////// diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index be8e54e4efa97..187c2e65488c7 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -155,7 +155,7 @@ fn value_assigned_to_local<'a, 'tcx>( local: Local, ) -> Option<&'a mir::Rvalue<'tcx>> { if let mir::StatementKind::Assign(place, rvalue) = &stmt.kind { - if let mir::Place::Base(mir::PlaceBase::Local(l)) = place { + if let mir::Place { base: mir::PlaceBase::Local(l), projection: None } = place { if local == *l { return Some(&*rvalue); } @@ -206,23 +206,22 @@ impl PeekCall { assert_eq!(args.len(), 1); let kind = PeekCallKind::from_arg_ty(substs.type_at(0)); - let arg = match args[0] { - | mir::Operand::Copy(mir::Place::Base(mir::PlaceBase::Local(local))) - | mir::Operand::Move(mir::Place::Base(mir::PlaceBase::Local(local))) - => local, - - _ => { - tcx.sess.diagnostic().span_err( - span, "dataflow::sanity_check cannot feed a non-temp to rustc_peek."); - return None; + if let mir::Operand::Copy(place) | mir::Operand::Move(place) = &args[0] { + if let mir::Place { + base: mir::PlaceBase::Local(local), + projection: None + } = *place { + return Some(PeekCall { + arg: local, + kind, + span, + }); } - }; + } - return Some(PeekCall { - arg, - kind, - span, - }); + tcx.sess.diagnostic().span_err( + span, "dataflow::sanity_check cannot feed a non-temp to rustc_peek."); + return None; } } @@ -255,7 +254,7 @@ impl<'tcx, O> RustcPeekAt<'tcx> for DataflowResults<'tcx, O> let operator = self.operator(); let flow_state = dataflow::state_for_location(location, operator, self, body); - match operator.move_data().rev_lookup.find(place) { + match operator.move_data().rev_lookup.find(place.as_ref()) { LookupResult::Exact(peek_mpi) => { let bit_state = flow_state.contains(peek_mpi); debug!("rustc_peek({:?} = &{:?}) bit_state: {}", From 047e7caebcb2d6d6606b20c76c440ef775b05189 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 1 Aug 2019 13:33:25 -0700 Subject: [PATCH 12/13] Correctly determine which terminators can have side effects The previous code ignored that `Drop` terminators could execute custom `Drop` implementations which might mutate the environment. Now we correctly implement `has_side_effects` and tests that custom `Drop` impls are recorded as an indirect definition. --- .../dataflow/impls/reaching_defs.rs | 43 ++++++++++++++++--- src/librustc_mir/transform/rustc_peek.rs | 2 +- .../ui/mir-dataflow/reaching-defs-drop.rs | 26 +++++++++++ .../ui/mir-dataflow/reaching-defs-drop.stderr | 10 +++++ 4 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 src/test/ui/mir-dataflow/reaching-defs-drop.rs create mode 100644 src/test/ui/mir-dataflow/reaching-defs-drop.stderr diff --git a/src/librustc_mir/dataflow/impls/reaching_defs.rs b/src/librustc_mir/dataflow/impls/reaching_defs.rs index f75ef3db40c1f..f52895ef1c1ee 100644 --- a/src/librustc_mir/dataflow/impls/reaching_defs.rs +++ b/src/librustc_mir/dataflow/impls/reaching_defs.rs @@ -1,10 +1,14 @@ use rustc::mir::visit::{PlaceContext, Visitor}; use rustc::mir::visit::{MutatingUseContext, NonMutatingUseContext, NonUseContext}; use rustc::mir::{self, Local, Location, Place, PlaceBase}; +use rustc::ty::{self, TyCtxt, ParamEnv}; use rustc_data_structures::bit_set::BitSet; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::indexed_vec::{Idx, IndexVec}; +use rustc_target::spec::abi::Abi; + use smallvec::SmallVec; +use syntax::symbol::sym; use super::{BitDenotation, GenKillSet, BottomValue}; @@ -74,16 +78,41 @@ pub struct ReachingDefinitions { at_location: FxHashMap>, } -fn is_call_with_side_effects(terminator: &mir::Terminator<'tcx>) -> bool { - if let mir::TerminatorKind::Call { .. } = terminator.kind { - true - } else { - false +fn has_side_effects( + tcx: TyCtxt<'tcx>, + body: &mir::Body<'tcx>, + param_env: ParamEnv<'tcx>, + terminator: &mir::Terminator<'tcx>, +) -> bool { + match &terminator.kind { + mir::TerminatorKind::Call { .. } => true, + + // Types with special drop glue may mutate their environment. + | mir::TerminatorKind::Drop { location: place, .. } + | mir::TerminatorKind::DropAndReplace { location: place, .. } + => place.ty(body, tcx).ty.needs_drop(tcx, param_env), + + | mir::TerminatorKind::Goto { .. } + | mir::TerminatorKind::SwitchInt { .. } + | mir::TerminatorKind::Resume + | mir::TerminatorKind::Abort + | mir::TerminatorKind::Return + | mir::TerminatorKind::Unreachable + | mir::TerminatorKind::Assert { .. } + | mir::TerminatorKind::FalseEdges { .. } + | mir::TerminatorKind::FalseUnwind { .. } + => false, + + // FIXME: I don't know the semantics around these so assume that they may mutate their + // environment. + | mir::TerminatorKind::Yield { .. } + | mir::TerminatorKind::GeneratorDrop + => true, } } impl ReachingDefinitions { - pub fn new(body: &mir::Body<'_>) -> Self { + pub fn new<'tcx>(tcx: TyCtxt<'tcx>, body: &mir::Body<'tcx>, param_env: ParamEnv<'tcx>) -> Self { let mut ret = ReachingDefinitions { all: IndexVec::new(), for_local_direct: IndexVec::from_elem(Vec::new(), &body.local_decls), @@ -102,7 +131,7 @@ impl ReachingDefinitions { let blocks_with_side_effects = body .basic_blocks() .iter_enumerated() - .filter(|(_, data)| is_call_with_side_effects(data.terminator())); + .filter(|(_, data)| has_side_effects(tcx, body, param_env, data.terminator())); for (block, data) in blocks_with_side_effects { let term_loc = Location { block, statement_index: data.statements.len() }; diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index 187c2e65488c7..9a16f20f85def 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -59,7 +59,7 @@ impl MirPass for SanityCheck { |_bd, i| DebugFormatted::new(&i)); let flow_reaching_defs = do_dataflow(tcx, body, def_id, &attributes, &dead_unwinds, - ReachingDefinitions::new(body), + ReachingDefinitions::new(tcx, body, tcx.param_env(def_id)), |bd, i| DebugFormatted::new(&bd.get(i).location)); let flow_use_def_chain = UseDefChain::new(body, &flow_reaching_defs, &flow_borrowed_locals); diff --git a/src/test/ui/mir-dataflow/reaching-defs-drop.rs b/src/test/ui/mir-dataflow/reaching-defs-drop.rs new file mode 100644 index 0000000000000..2a0f69ee14881 --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-drop.rs @@ -0,0 +1,26 @@ +#![feature(core_intrinsics, rustc_attrs)] +use std::intrinsics::rustc_peek; + +struct NoSideEffectsInDrop<'a>(&'a mut u32); +struct SideEffectsInDrop<'a>(&'a mut u32); + +impl Drop for SideEffectsInDrop<'_> { + fn drop(&mut self) { + *self.0 = 42 + } +} + +#[rustc_mir(rustc_peek_use_def_chain, stop_after_dataflow, borrowck_graphviz_postflow="flow.dot")] +fn main() { + let mut x; + x=0; + + NoSideEffectsInDrop(&mut x); + SideEffectsInDrop(&mut x); + + // The ";" on line 19 is the point at which `SideEffectsInDrop` is dropped. + unsafe { rustc_peek(x); } + //~^ ERROR rustc_peek: [16: "x=0", 19: ";"] + + assert_eq!(x, 42); +} diff --git a/src/test/ui/mir-dataflow/reaching-defs-drop.stderr b/src/test/ui/mir-dataflow/reaching-defs-drop.stderr new file mode 100644 index 0000000000000..88e310744b8f0 --- /dev/null +++ b/src/test/ui/mir-dataflow/reaching-defs-drop.stderr @@ -0,0 +1,10 @@ +error: rustc_peek: [16: "x=0", 19: ";"] + --> $DIR/reaching-defs-drop.rs:22:14 + | +LL | unsafe { rustc_peek(x); } + | ^^^^^^^^^^^^^ + +error: stop_after_dataflow ended compilation + +error: aborting due to 2 previous errors + From 976ba13e076aaee480e99efb1e8f71e5634faf2d Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 1 Aug 2019 13:33:58 -0700 Subject: [PATCH 13/13] Record the fact that `rustc_peek` has no side effects This means that calls to `rustc_peek` are no longer indirect definitions of any locals in the body. Other pure intrinsics (e.g. `transmute`) could also be added to the list of exceptions. --- src/librustc_mir/dataflow/impls/reaching_defs.rs | 16 +++++++++++++++- src/test/ui/mir-dataflow/reaching-defs-1.rs | 2 +- src/test/ui/mir-dataflow/reaching-defs-1.stderr | 2 +- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/reaching_defs.rs b/src/librustc_mir/dataflow/impls/reaching_defs.rs index f52895ef1c1ee..017121784e704 100644 --- a/src/librustc_mir/dataflow/impls/reaching_defs.rs +++ b/src/librustc_mir/dataflow/impls/reaching_defs.rs @@ -85,7 +85,21 @@ fn has_side_effects( terminator: &mir::Terminator<'tcx>, ) -> bool { match &terminator.kind { - mir::TerminatorKind::Call { .. } => true, + mir::TerminatorKind::Call { func, .. } => { + // Special-case some intrinsics that do not have side effects. + if let mir::Operand::Constant(func) = func { + if let ty::FnDef(def_id, _) = func.ty.sty { + if let Abi::RustIntrinsic = tcx.fn_sig(def_id).abi() { + match tcx.item_name(def_id) { + sym::rustc_peek => return false, + _ => (), + } + } + } + } + + true + } // Types with special drop glue may mutate their environment. | mir::TerminatorKind::Drop { location: place, .. } diff --git a/src/test/ui/mir-dataflow/reaching-defs-1.rs b/src/test/ui/mir-dataflow/reaching-defs-1.rs index 25dd7ae32f22e..054fe232230e3 100644 --- a/src/test/ui/mir-dataflow/reaching-defs-1.rs +++ b/src/test/ui/mir-dataflow/reaching-defs-1.rs @@ -22,7 +22,7 @@ fn foo(test: bool) -> (i32, i32) { } unsafe { rustc_peek(&x); } - //~^ ERROR rustc_peek: [16: "x=2", 17: "rustc_peek(&x)", 20: "x=3"] + //~^ ERROR rustc_peek: [16: "x=2", 20: "x=3"] unsafe { rustc_peek(&y); } //~^ ERROR rustc_peek: [13: "y=1", 21: "y=4"] diff --git a/src/test/ui/mir-dataflow/reaching-defs-1.stderr b/src/test/ui/mir-dataflow/reaching-defs-1.stderr index a5e8fe5464125..4c99a47571dd0 100644 --- a/src/test/ui/mir-dataflow/reaching-defs-1.stderr +++ b/src/test/ui/mir-dataflow/reaching-defs-1.stderr @@ -4,7 +4,7 @@ error: rustc_peek: [16: "x=2"] LL | unsafe { rustc_peek(&x); } | ^^^^^^^^^^^^^^ -error: rustc_peek: [16: "x=2", 17: "rustc_peek(&x)", 20: "x=3"] +error: rustc_peek: [16: "x=2", 20: "x=3"] --> $DIR/reaching-defs-1.rs:24:14 | LL | unsafe { rustc_peek(&x); }