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.

Sign up for GitHub

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

validate basic sanity for TerminatorKind #72810

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5707838
validate basic sanity for TerminatorKind
RalfJung May 31, 2020
e07e424
replace DUMMY_SP by proper span
RalfJung May 31, 2020
9a4bdbf
more checks for SwitchInt
RalfJung May 31, 2020
f793c0b
always print MIR Location when validator finds a problem
RalfJung May 31, 2020
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
View file
Open in desktop
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
103 changes: 101 additions & 2 deletions src/librustc_mir/transform/validate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
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};

Expand Down Expand Up @@ -38,6 +41,12 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
&format!("broken MIR in {:?} ({}): {}", self.def_id, self.when, msg.as_ref()),
);
}

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

impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
Expand Down Expand Up @@ -77,4 +86,94 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
}
}
}

fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, _location: Location) {
match &terminator.kind {
TerminatorKind::Goto { target } => {
self.check_bb(terminator.source_info.span, *target);
}
TerminatorKind::SwitchInt { targets, .. } => {
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
if targets.is_empty() {
self.fail(
terminator.source_info.span,
"encountered `SwitchInt` terminator with no target to jump to",
);
}
for target in targets {
self.check_bb(terminator.source_info.span, *target);
}
}
TerminatorKind::Drop { target, unwind, .. } => {
self.check_bb(terminator.source_info.span, *target);
if let Some(unwind) = unwind {
self.check_bb(terminator.source_info.span, *unwind);
}
}
TerminatorKind::DropAndReplace { target, unwind, .. } => {
self.check_bb(terminator.source_info.span, *target);
if let Some(unwind) = unwind {
self.check_bb(terminator.source_info.span, *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(
terminator.source_info.span,
format!("encountered non-callable type {} in `Call` terminator", func_ty),
),
}
if let Some((_, target)) = destination {
self.check_bb(terminator.source_info.span, *target);
}
if let Some(cleanup) = cleanup {
self.check_bb(terminator.source_info.span, *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(
terminator.source_info.span,
format!(
"encountered non-boolean condition of type {} in `Assert` terminator",
cond_ty
),
);
}
self.check_bb(terminator.source_info.span, *target);
if let Some(cleanup) = cleanup {
self.check_bb(terminator.source_info.span, *cleanup);
}
}
TerminatorKind::Yield { resume, drop, .. } => {
self.check_bb(terminator.source_info.span, *resume);
if let Some(drop) = drop {
self.check_bb(terminator.source_info.span, *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(terminator.source_info.span, *real_target);
self.check_bb(terminator.source_info.span, *imaginary_target);
}
TerminatorKind::FalseUnwind { real_target, unwind } => {
self.check_bb(terminator.source_info.span, *real_target);
if let Some(unwind) = unwind {
self.check_bb(terminator.source_info.span, *unwind);
}
}
TerminatorKind::InlineAsm { destination, .. } => {
if let Some(destination) = destination {
self.check_bb(terminator.source_info.span, *destination);
}
}
// Nothing to validate for these.
TerminatorKind::Resume
| TerminatorKind::Abort
| TerminatorKind::Return
| TerminatorKind::Unreachable
| TerminatorKind::GeneratorDrop => {}
}
}
}