Skip to content

Commit

Permalink
Rollup merge of rust-lang#64980 - ecstatic-morse:better-rustc-peek, r…
Browse files Browse the repository at this point in the history 
…=oli-obk

Enable support for `IndirectlyMutableLocals` in `rustc_peek`

This PR allows `rustc_peek` tests to be written for the `IndirectlyMutableLocals` analysis implemented in rust-lang#64470. See any of the tests in [`test/ui/mir-dataflow`](https://github.com/rust-lang/rust/blob/master/src/test/ui/mir-dataflow/inits-1.rs) for an example.

Included in this PR is a major rewrite of the `rustc_peek` module. This was motivated by the differences between the `IndirectlyMutableLocals` analysis and the initialized places ones.

To properly test `IndirectlyMutableLocals`, we must pass locals by-value to `rustc_peek`, since any local that is not `Freeze` will be marked as indirectly mutable as soon as a reference to it is taken. Unfortunately, `UnsafeCell` is not `Copy`, so we can only do one `rustc_peek` on each value with interior mutability inside a test. I'm not sure how to deal with this restriction; perhaps I need to special case borrows preceding a call to `rustc_peek` in the analysis itself?

`rustc_peek` also assumed that the analysis was done on move paths and that its transfer function only needed to be applied at assignment statements. This PR removes both of those restrictions by adding a trait, `RustcPeekAt`, that controls how the peeked at `Place` maps to the current dataflow state and using a dataflow cursor to retrieve the state itself.

Finally, this PR adds a test which demonstrates some unsoundness in the `IndirectlyMutableLocals` analysis by converting a reference to a `Freeze` field to a reference to a `!Freeze` field by offsetting a pointer (or in this case transmuting a pointer to a ZST field with the same address as a `!Freeze` field). This does not represent a hole in the language proper, since this analysis is only used to validate `const` bodies, in which the unsound code will only compile with `-Zunleash-the-miri-inside-of-you`. Nevertheless, this should get fixed.

r? @oli-obk
  • Loading branch information
@Centril
Centril authored Oct 2, 2019
2 parents 10b0fe9 + 33aa5e8 commit 7daf2e8
Show file tree
Hide file tree
Showing 4 changed files with 237 additions and 130 deletions.
314 changes: 184 additions & 130 deletions src/librustc_mir/transform/rustc_peek.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,21 @@ 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};

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
};
use crate::dataflow::IndirectlyMutableLocals;
use crate::dataflow::move_paths::{MovePathIndex, LookupResult};
use crate::dataflow::move_paths::{HasMoveData, MoveData};

Expand Down Expand Up @@ -50,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);
Expand All @@ -60,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");
}
Expand Down Expand Up @@ -88,151 +97,196 @@ 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<mir::Terminator<'tcx>>,
) -> 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<Self> {
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<Self::Idx>,
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<Self::Idx>,
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");
}
}
}
}

impl<'tcx> RustcPeekAt<'tcx> for IndirectlyMutableLocals<'_, 'tcx> {
fn peek_at(
&self,
tcx: TyCtxt<'tcx>,
place: &mir::Place<'tcx>,
flow_state: &BitSet<Local>,
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");
}
}
return None;
}
1 change: 1 addition & 0 deletions src/libsyntax_pos/symbol.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 7daf2e8

Please sign in to comment.