Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate basic sanity for TerminatorKind #72810

Merged
merged 4 commits into from
Jun 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/librustc_mir/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.go_to_block(target_block);
}

Call { ref func, ref args, destination, ref cleanup, .. } => {
Call {
ref func,
ref args,
destination,
ref cleanup,
from_hir_call: _from_hir_call,
} => {
let old_stack = self.frame_idx();
let old_loc = self.frame().loc;
let func = self.eval_operand(func, None)?;
Expand Down
135 changes: 120 additions & 15 deletions src/librustc_mir/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
use super::{MirPass, MirSource};
use rustc_middle::mir::visit::Visitor;
use rustc_middle::{
mir::{Body, Location, Operand, Rvalue, Statement, StatementKind},
ty::{ParamEnv, TyCtxt},
mir::{
BasicBlock, Body, Location, Operand, Rvalue, Statement, StatementKind, Terminator,
TerminatorKind,
},
ty::{self, ParamEnv, TyCtxt},
};
use rustc_span::{def_id::DefId, Span, DUMMY_SP};
use rustc_span::def_id::DefId;

pub struct Validator {
/// Describes at which point in the pipeline this validation is happening.
Expand All @@ -30,27 +33,38 @@ struct TypeChecker<'a, 'tcx> {
}

impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
fn fail(&self, span: Span, msg: impl AsRef<str>) {
fn fail(&self, location: Location, msg: impl AsRef<str>) {
let span = self.body.source_info(location).span;
// We use `delay_span_bug` as we might see broken MIR when other errors have already
// occurred.
self.tcx.sess.diagnostic().delay_span_bug(
span,
&format!("broken MIR in {:?} ({}): {}", self.def_id, self.when, msg.as_ref()),
&format!(
"broken MIR in {:?} ({}) at {:?}:\n{}",
self.def_id,
self.when,
location,
msg.as_ref()
),
);
}

fn check_bb(&self, location: Location, bb: BasicBlock) {
if self.body.basic_blocks().get(bb).is_none() {
self.fail(location, format!("encountered jump to invalid basic block {:?}", bb))
}
}
}

impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) {
// `Operand::Copy` is only supposed to be used with `Copy` types.
if let Operand::Copy(place) = operand {
let ty = place.ty(&self.body.local_decls, self.tcx).ty;
let span = self.body.source_info(location).span;

if !ty.is_copy_modulo_regions(self.tcx, self.param_env, DUMMY_SP) {
self.fail(
DUMMY_SP,
format!("`Operand::Copy` with non-`Copy` type {} at {:?}", ty, location),
);
if !ty.is_copy_modulo_regions(self.tcx, self.param_env, span) {
self.fail(location, format!("`Operand::Copy` with non-`Copy` type {}", ty));
}
}

Expand All @@ -65,16 +79,107 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => {
if dest == src {
self.fail(
DUMMY_SP,
format!(
"encountered `Assign` statement with overlapping memory at {:?}",
location
),
location,
"encountered `Assign` statement with overlapping memory",
);
}
}
_ => {}
}
}
}

fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
match &terminator.kind {
TerminatorKind::Goto { target } => {
self.check_bb(location, *target);
}
TerminatorKind::SwitchInt { targets, values, .. } => {
if targets.len() != values.len() + 1 {
self.fail(
location,
format!(
"encountered `SwitchInt` terminator with {} values, but {} targets (should be values+1)",
values.len(),
targets.len(),
),
);
}
for target in targets {
self.check_bb(location, *target);
}
}
TerminatorKind::Drop { target, unwind, .. } => {
self.check_bb(location, *target);
if let Some(unwind) = unwind {
self.check_bb(location, *unwind);
}
}
TerminatorKind::DropAndReplace { target, unwind, .. } => {
self.check_bb(location, *target);
if let Some(unwind) = unwind {
self.check_bb(location, *unwind);
}
}
TerminatorKind::Call { func, destination, cleanup, .. } => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call (and perhaps others?) could also validate that the destination doesn't overlap with any of the arguments. This would have caught a miscompilation in #72632.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DropAndReplace (and Yield?) can do the same check. Not sure about inline assembly, but it seems like that's a candidate for such a check as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, now I wonder if Miri would have caught that (for Call). Unfortunately we can't test Miri on MIR that rustc does not generate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably would have, but I haven't yet figured out how to use a locally built miri with external code (like with cargo-miri).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably would have, but I haven't yet figured out how to use a locally built miri with external code (like with cargo-miri).

I usually do ./miri install and then use cargo miri.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

./src/tools/miri/miri install fails with "can't find crate for rustc_apfloat" for me, and running the miri tests ICEs the compiler

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I've only ever used the script in a standalone clone... yeah it probably doesn't work in the rustc folder layout.

running the miri tests ICEs the compiler

How do you run them? ./x.py test --stage 0 src/tools/miri should work -- except when the Miri toolstate is broken for the commit you're basing your work on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what I tried to run, but it enters unreachable code while decoding or encoding something. Not really sure what's happening, but I'll do more digging.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like an incremental problem or --keep-stage gone wrong... strange.

let func_ty = func.ty(&self.body.local_decls, self.tcx);
match func_ty.kind {
ty::FnPtr(..) | ty::FnDef(..) => {}
_ => self.fail(
location,
format!("encountered non-callable type {} in `Call` terminator", func_ty),
),
}
if let Some((_, target)) = destination {
self.check_bb(location, *target);
}
if let Some(cleanup) = cleanup {
self.check_bb(location, *cleanup);
}
}
TerminatorKind::Assert { cond, target, cleanup, .. } => {
let cond_ty = cond.ty(&self.body.local_decls, self.tcx);
if cond_ty != self.tcx.types.bool {
self.fail(
location,
format!(
"encountered non-boolean condition of type {} in `Assert` terminator",
cond_ty
),
);
}
self.check_bb(location, *target);
if let Some(cleanup) = cleanup {
self.check_bb(location, *cleanup);
}
}
TerminatorKind::Yield { resume, drop, .. } => {
self.check_bb(location, *resume);
if let Some(drop) = drop {
self.check_bb(location, *drop);
}
}
TerminatorKind::FalseEdges { real_target, imaginary_target } => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, does anyone know why this is named in plural? Shouldn't it be FalseEdge instead?
Cc @matthewjasper

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it may have been accurate in the past. Feel free to change it.

self.check_bb(location, *real_target);
self.check_bb(location, *imaginary_target);
}
TerminatorKind::FalseUnwind { real_target, unwind } => {
self.check_bb(location, *real_target);
if let Some(unwind) = unwind {
self.check_bb(location, *unwind);
}
}
TerminatorKind::InlineAsm { destination, .. } => {
if let Some(destination) = destination {
self.check_bb(location, *destination);
}
}
// Nothing to validate for these.
TerminatorKind::Resume
| TerminatorKind::Abort
| TerminatorKind::Return
| TerminatorKind::Unreachable
| TerminatorKind::GeneratorDrop => {}
}
}
}