From 57078384809fffafac4e90e18cc37a91a1dd5200 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 31 May 2020 09:52:51 +0200 Subject: [PATCH 1/4] validate basic sanity for TerminatorKind --- src/librustc_mir/interpret/terminator.rs | 8 +- src/librustc_mir/transform/validate.rs | 103 ++++++++++++++++++++++- 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index b048240ca8dc1..3db16a71bab15 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -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)?; diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index a25edd131baa1..046889193dac3 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -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}; @@ -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> { @@ -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, .. } => { + 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, .. } => { + 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 } => { + 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 => {} + } + } } From e07e42433f9f8e2845d4941c59dc64918d467228 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 31 May 2020 14:26:41 +0200 Subject: [PATCH 2/4] replace DUMMY_SP by proper span --- src/librustc_mir/transform/validate.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 046889193dac3..7a1393468cec6 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -54,12 +54,10 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { // `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(span, format!("`Operand::Copy` with non-`Copy` type {}", ty)); } } From 9a4bdbff9e52fe2bb2977ea6edddb13064726a25 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 31 May 2020 15:07:16 +0200 Subject: [PATCH 3/4] more checks for SwitchInt --- src/librustc_mir/transform/validate.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 7a1393468cec6..4039d1b50e8f5 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -90,11 +90,15 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { TerminatorKind::Goto { target } => { self.check_bb(terminator.source_info.span, *target); } - TerminatorKind::SwitchInt { targets, .. } => { - if targets.is_empty() { + TerminatorKind::SwitchInt { targets, values, .. } => { + if targets.len() != values.len() + 1 { self.fail( terminator.source_info.span, - "encountered `SwitchInt` terminator with no target to jump to", + format!( + "encountered `SwitchInt` terminator with {} values, but {} targets (should be values+1)", + values.len(), + targets.len(), + ), ); } for target in targets { From f793c0b1bf3392c19eb11331fddc8c9f561361ee Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 31 May 2020 15:21:09 +0200 Subject: [PATCH 4/4] always print MIR Location when validator finds a problem --- src/librustc_mir/transform/validate.rs | 68 ++++++++++++++------------ 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 4039d1b50e8f5..7d301b2f49648 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -9,7 +9,7 @@ use rustc_middle::{ }, 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. @@ -33,18 +33,25 @@ struct TypeChecker<'a, 'tcx> { } impl<'a, 'tcx> TypeChecker<'a, 'tcx> { - fn fail(&self, span: Span, msg: impl AsRef) { + fn fail(&self, location: Location, msg: impl AsRef) { + 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, span: Span, bb: BasicBlock) { + fn check_bb(&self, location: Location, bb: BasicBlock) { if self.body.basic_blocks().get(bb).is_none() { - self.fail(span, format!("encountered jump to invalid basic block {:?}", bb)) + self.fail(location, format!("encountered jump to invalid basic block {:?}", bb)) } } } @@ -57,7 +64,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { let span = self.body.source_info(location).span; if !ty.is_copy_modulo_regions(self.tcx, self.param_env, span) { - self.fail(span, format!("`Operand::Copy` with non-`Copy` type {}", ty)); + self.fail(location, format!("`Operand::Copy` with non-`Copy` type {}", ty)); } } @@ -72,11 +79,8 @@ 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", ); } } @@ -85,15 +89,15 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { } } - fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, _location: Location) { + fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { match &terminator.kind { TerminatorKind::Goto { target } => { - self.check_bb(terminator.source_info.span, *target); + self.check_bb(location, *target); } TerminatorKind::SwitchInt { targets, values, .. } => { if targets.len() != values.len() + 1 { self.fail( - terminator.source_info.span, + location, format!( "encountered `SwitchInt` terminator with {} values, but {} targets (should be values+1)", values.len(), @@ -102,19 +106,19 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ); } for target in targets { - self.check_bb(terminator.source_info.span, *target); + self.check_bb(location, *target); } } TerminatorKind::Drop { target, unwind, .. } => { - self.check_bb(terminator.source_info.span, *target); + self.check_bb(location, *target); if let Some(unwind) = unwind { - self.check_bb(terminator.source_info.span, *unwind); + self.check_bb(location, *unwind); } } TerminatorKind::DropAndReplace { target, unwind, .. } => { - self.check_bb(terminator.source_info.span, *target); + self.check_bb(location, *target); if let Some(unwind) = unwind { - self.check_bb(terminator.source_info.span, *unwind); + self.check_bb(location, *unwind); } } TerminatorKind::Call { func, destination, cleanup, .. } => { @@ -122,52 +126,52 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { match func_ty.kind { ty::FnPtr(..) | ty::FnDef(..) => {} _ => self.fail( - terminator.source_info.span, + location, format!("encountered non-callable type {} in `Call` terminator", func_ty), ), } if let Some((_, target)) = destination { - self.check_bb(terminator.source_info.span, *target); + self.check_bb(location, *target); } if let Some(cleanup) = cleanup { - self.check_bb(terminator.source_info.span, *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( - terminator.source_info.span, + location, format!( "encountered non-boolean condition of type {} in `Assert` terminator", cond_ty ), ); } - self.check_bb(terminator.source_info.span, *target); + self.check_bb(location, *target); if let Some(cleanup) = cleanup { - self.check_bb(terminator.source_info.span, *cleanup); + self.check_bb(location, *cleanup); } } TerminatorKind::Yield { resume, drop, .. } => { - self.check_bb(terminator.source_info.span, *resume); + self.check_bb(location, *resume); if let Some(drop) = drop { - self.check_bb(terminator.source_info.span, *drop); + self.check_bb(location, *drop); } } TerminatorKind::FalseEdges { real_target, imaginary_target } => { - self.check_bb(terminator.source_info.span, *real_target); - self.check_bb(terminator.source_info.span, *imaginary_target); + self.check_bb(location, *real_target); + self.check_bb(location, *imaginary_target); } TerminatorKind::FalseUnwind { real_target, unwind } => { - self.check_bb(terminator.source_info.span, *real_target); + self.check_bb(location, *real_target); if let Some(unwind) = unwind { - self.check_bb(terminator.source_info.span, *unwind); + self.check_bb(location, *unwind); } } TerminatorKind::InlineAsm { destination, .. } => { if let Some(destination) = destination { - self.check_bb(terminator.source_info.span, *destination); + self.check_bb(location, *destination); } } // Nothing to validate for these.