From f18535fa46752ac33ceaebe71386cf77a6df8f9a Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 5 Jul 2019 18:18:38 -0700 Subject: [PATCH 1/3] Refactor `rustc_peek` We now use `DataflowResultsCursor` to get the dataflow state before calls to `rustc_peek`. The original version used a custom implementation that only looked at assignment statements. This also extends `rustc_peek` to take arguments by-value as well as by-reference, and allows *all* dataflow analyses, not just those dependent on `MoveData`, to be inspected. --- src/librustc_mir/transform/rustc_peek.rs | 284 ++++++++++++----------- 1 file changed, 153 insertions(+), 131 deletions(-) diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index f509d2d7104eb..d0ffcbbae8855 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -3,9 +3,9 @@ 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}; +use rustc::mir::{self, Body, Location, Local}; use rustc_index::bit_set::BitSet; use crate::transform::{MirPass, MirSource}; @@ -13,6 +13,7 @@ use crate::dataflow::{do_dataflow, DebugFormatted}; use crate::dataflow::MoveDataParamEnv; use crate::dataflow::BitDenotation; use crate::dataflow::DataflowResults; +use crate::dataflow::DataflowResultsCursor; use crate::dataflow::{ DefinitelyInitializedPlaces, MaybeInitializedPlaces, MaybeUninitializedPlaces }; @@ -88,151 +89,172 @@ 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); - // 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); - } -} + let mut cursor = DataflowResultsCursor::new(results, body); -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: box [], - }) | - mir::Operand::Move(ref place @ mir::Place { - base: mir::PlaceBase::Local(_), - projection: box [], - }) => 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(box(ref place, ref rvalue)) => { - (place, rvalue) + let peek_calls = body + .basic_blocks() + .iter_enumerated() + .filter_map(|(bb, block_data)| { + PeekCall::from_terminator(tcx, block_data.terminator()) + .map(|call| (bb, block_data, call)) + }); + + for (bb, block_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); + // + // /* or */ + // + // _2 = _1; + // rustc_peek(_2); + let (statement_index, peek_rval) = block_data + .statements + .iter() + .enumerate() + .filter_map(|(i, stmt)| value_assigned_to_local(stmt, call.arg).map(|rval| (i, rval))) + .next() + .expect("call to rustc_peek should be preceded by \ + assignment to temporary holding its argument"); + + match (call.kind, peek_rval) { + | (PeekCallKind::ByRef, mir::Rvalue::Ref(_, _, place)) + | (PeekCallKind::ByVal, mir::Rvalue::Use(mir::Operand::Move(place))) + | (PeekCallKind::ByVal, mir::Rvalue::Use(mir::Operand::Copy(place))) + => { + let loc = Location { block: bb, statement_index }; + cursor.seek(loc); + let state = cursor.get(); + results.operator().peek_at(tcx, place, state, call); } - 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"); - } - } - 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); + must be either `place` or `&place`"; + 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(box (place, rvalue)) = &stmt.kind { + if let mir::Place { base: mir::PlaceBase::Local(l), projection: box [] } = 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`")); +#[derive(Clone, Copy, Debug)] +enum PeekCallKind { + ByVal, + ByRef, } -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.literal.ty.kind { - 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 PeekCallKind { + fn from_arg_ty(arg: Ty<'_>) -> Self { + match arg.kind { + ty::Ref(_, _, _) => PeekCallKind::ByRef, + _ => PeekCallKind::ByVal, + } + } +} + +#[derive(Clone, Copy, Debug)] +pub struct PeekCall { + arg: Local, + kind: PeekCallKind, + span: Span, +} + +impl PeekCall { + fn from_terminator<'tcx>( + tcx: TyCtxt<'tcx>, + terminator: &mir::Terminator<'tcx>, + ) -> Option { + use mir::{Operand, Place, PlaceBase}; + + let span = terminator.source_info.span; + if let mir::TerminatorKind::Call { func: Operand::Constant(func), args, .. } = + &terminator.kind + { + if let ty::FnDef(def_id, substs) = func.literal.ty.kind { + let sig = tcx.fn_sig(def_id); + let name = tcx.item_name(def_id); + 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] { + | Operand::Copy(Place { base: PlaceBase::Local(local), projection: box [] }) + | Operand::Move(Place { base: PlaceBase::Local(local), projection: box [] }) + => local, + + _ => { + tcx.sess.diagnostic().span_err( + span, "dataflow::sanity_check cannot feed a non-temp to rustc_peek."); + return None; } + }; + + return Some(PeekCall { + arg, + kind, + span, + }); + } + } + + 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.as_ref()) { + 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"); + } } } - return None; } From 767550ef0fc0a01abd8dc50fead0967d215eca41 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 1 Oct 2019 20:14:01 -0700 Subject: [PATCH 2/3] Add `rustc_peek` support for `IndirectlyMutableLocals` --- src/librustc_mir/transform/rustc_peek.rs | 32 ++++++++++++++++++++++++ src/libsyntax_pos/symbol.rs | 1 + 2 files changed, 33 insertions(+) diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index d0ffcbbae8855..6edd28a4259a5 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -17,6 +17,7 @@ use crate::dataflow::DataflowResultsCursor; use crate::dataflow::{ DefinitelyInitializedPlaces, MaybeInitializedPlaces, MaybeUninitializedPlaces }; +use crate::dataflow::IndirectlyMutableLocals; use crate::dataflow::move_paths::{MovePathIndex, LookupResult}; use crate::dataflow::move_paths::{HasMoveData, MoveData}; @@ -51,6 +52,10 @@ impl<'tcx> MirPass<'tcx> for SanityCheck { do_dataflow(tcx, body, def_id, &attributes, &dead_unwinds, DefinitelyInitializedPlaces::new(tcx, body, &mdpe), |bd, i| DebugFormatted::new(&bd.move_data().move_paths[i])); + let flow_indirectly_mut = + do_dataflow(tcx, body, def_id, &attributes, &dead_unwinds, + IndirectlyMutableLocals::new(tcx, body, param_env), + |_, i| DebugFormatted::new(&i)); 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); @@ -61,6 +66,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); } + 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); + } if has_rustc_mir_with(&attributes, sym::stop_after_dataflow).is_some() { tcx.sess.fatal("stop_after_dataflow ended compilation"); } @@ -252,9 +260,33 @@ impl<'tcx, O> RustcPeekAt<'tcx> for O tcx.sess.span_err(call.span, "rustc_peek: bit not set"); } } + LookupResult::Parent(..) => { tcx.sess.span_err(call.span, "rustc_peek: argument untracked"); } } } } + +impl<'tcx> RustcPeekAt<'tcx> for IndirectlyMutableLocals<'_, 'tcx> { + fn peek_at( + &self, + tcx: TyCtxt<'tcx>, + place: &mir::Place<'tcx>, + flow_state: &BitSet, + call: PeekCall, + ) { + warn!("peek_at: place={:?}", place); + let local = match place { + mir::Place { base: mir::PlaceBase::Local(l), projection: box [] } => *l, + _ => { + tcx.sess.span_err(call.span, "rustc_peek: argument was not a local"); + return; + } + }; + + if !flow_state.contains(local) { + tcx.sess.span_err(call.span, "rustc_peek: bit not set"); + } + } +} diff --git a/src/libsyntax_pos/symbol.rs b/src/libsyntax_pos/symbol.rs index 1769135e7f21a..82c47e6dbb758 100644 --- a/src/libsyntax_pos/symbol.rs +++ b/src/libsyntax_pos/symbol.rs @@ -597,6 +597,7 @@ symbols! { rustc_peek_definite_init, rustc_peek_maybe_init, rustc_peek_maybe_uninit, + rustc_peek_indirectly_mutable, rustc_private, rustc_proc_macro_decls, rustc_promotable, From 33aa5e855ed1deec5d086c7919b2a9572128b7ee Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 1 Oct 2019 20:14:27 -0700 Subject: [PATCH 3/3] Add test exposing unsoundness in `IndirectlyMutableLocals` --- .../mir-dataflow/indirect-mutation-offset.rs | 42 +++++++++++++++++++ .../indirect-mutation-offset.stderr | 10 +++++ 2 files changed, 52 insertions(+) create mode 100644 src/test/ui/mir-dataflow/indirect-mutation-offset.rs create mode 100644 src/test/ui/mir-dataflow/indirect-mutation-offset.stderr diff --git a/src/test/ui/mir-dataflow/indirect-mutation-offset.rs b/src/test/ui/mir-dataflow/indirect-mutation-offset.rs new file mode 100644 index 0000000000000..804b70d26527a --- /dev/null +++ b/src/test/ui/mir-dataflow/indirect-mutation-offset.rs @@ -0,0 +1,42 @@ +// compile-flags: -Zunleash-the-miri-inside-of-you + +#![feature(core_intrinsics, rustc_attrs, const_raw_ptr_deref)] + +use std::cell::UnsafeCell; +use std::intrinsics::rustc_peek; + +#[repr(C)] +struct PartialInteriorMut { + zst: [i32; 0], + cell: UnsafeCell, +} + +#[rustc_mir(rustc_peek_indirectly_mutable,stop_after_dataflow)] +#[rustc_mir(borrowck_graphviz_postflow="indirect.dot")] +const BOO: i32 = { + let x = PartialInteriorMut { + zst: [], + cell: UnsafeCell::new(0), + }; + + let p_zst: *const _ = &x.zst ; // Doesn't cause `x` to get marked as indirectly mutable. + + let rmut_cell = unsafe { + // Take advantage of the fact that `zst` and `cell` are at the same location in memory. + // This trick would work with any size type if miri implemented `ptr::offset`. + let p_cell = p_zst as *const UnsafeCell; + + let pmut_cell = (*p_cell).get(); + &mut *pmut_cell + }; + + *rmut_cell = 42; // Mutates `x` indirectly even though `x` is not marked indirectly mutable!!! + let val = *rmut_cell; + unsafe { rustc_peek(x) }; //~ ERROR rustc_peek: bit not set + + val +}; + +fn main() { + println!("{}", BOO); +} diff --git a/src/test/ui/mir-dataflow/indirect-mutation-offset.stderr b/src/test/ui/mir-dataflow/indirect-mutation-offset.stderr new file mode 100644 index 0000000000000..16bd17813134a --- /dev/null +++ b/src/test/ui/mir-dataflow/indirect-mutation-offset.stderr @@ -0,0 +1,10 @@ +error: rustc_peek: bit not set + --> $DIR/indirect-mutation-offset.rs:35:14 + | +LL | unsafe { rustc_peek(x) }; + | ^^^^^^^^^^^^^ + +error: stop_after_dataflow ended compilation + +error: aborting due to 2 previous errors +