From 3aedfbecfd3eaf8a0d4174d2a42a91d3eac77c25 Mon Sep 17 00:00:00 2001 From: Nathan Corbyn Date: Mon, 8 Jun 2020 13:54:20 +0100 Subject: [PATCH 1/4] Enforce unwind invariants --- src/librustc_mir/transform/validate.rs | 60 ++++++++++++++++++-------- 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 1433d39abfbba..339b1469f1237 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -11,6 +11,11 @@ use rustc_middle::{ }; use rustc_span::def_id::DefId; +enum EdgeKind { + Unwind, + Other, +} + pub struct Validator { /// Describes at which point in the pipeline this validation is happening. pub when: String, @@ -49,8 +54,25 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { ); } - fn check_bb(&self, location: Location, bb: BasicBlock) { - if self.body.basic_blocks().get(bb).is_none() { + fn check_bb(&self, location: Location, bb: BasicBlock, edge_kind: EdgeKind) { + if let Some(bb) = self.body.basic_blocks().get(bb) { + let src = self.body.basic_blocks().get(location.block).unwrap(); + match (src.is_cleanup, bb.is_cleanup, edge_kind) { + // Non-cleanup blocks can jump to non-cleanup blocks along non-unwind edges + (false, false, EdgeKind::Other) + // Non-cleanup blocks can jump to cleanup blocks along unwind edges + | (false, true, EdgeKind::Unwind) + // Cleanup blocks can jump to cleanup blocks along unwind edges + | (true, true, EdgeKind::Unwind) => {} + // All other jumps are invalid + _ => { + self.fail( + location, + format!("encountered jump that does not respect unwind invariants {:?}", bb) + ) + } + } + } else { self.fail(location, format!("encountered jump to invalid basic block {:?}", bb)) } } @@ -92,7 +114,7 @@ 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(location, *target); + self.check_bb(location, *target, EdgeKind::Other); } TerminatorKind::SwitchInt { targets, values, .. } => { if targets.len() != values.len() + 1 { @@ -106,19 +128,19 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ); } for target in targets { - self.check_bb(location, *target); + self.check_bb(location, *target, EdgeKind::Other); } } TerminatorKind::Drop { target, unwind, .. } => { - self.check_bb(location, *target); + self.check_bb(location, *target, EdgeKind::Other); if let Some(unwind) = unwind { - self.check_bb(location, *unwind); + self.check_bb(location, *unwind, EdgeKind::Unwind); } } TerminatorKind::DropAndReplace { target, unwind, .. } => { - self.check_bb(location, *target); + self.check_bb(location, *target, EdgeKind::Other); if let Some(unwind) = unwind { - self.check_bb(location, *unwind); + self.check_bb(location, *unwind, EdgeKind::Unwind); } } TerminatorKind::Call { func, destination, cleanup, .. } => { @@ -131,10 +153,10 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ), } if let Some((_, target)) = destination { - self.check_bb(location, *target); + self.check_bb(location, *target, EdgeKind::Other); } if let Some(cleanup) = cleanup { - self.check_bb(location, *cleanup); + self.check_bb(location, *cleanup, EdgeKind::Unwind); } } TerminatorKind::Assert { cond, target, cleanup, .. } => { @@ -148,30 +170,30 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ), ); } - self.check_bb(location, *target); + self.check_bb(location, *target, EdgeKind::Other); if let Some(cleanup) = cleanup { - self.check_bb(location, *cleanup); + self.check_bb(location, *cleanup, EdgeKind::Unwind); } } TerminatorKind::Yield { resume, drop, .. } => { - self.check_bb(location, *resume); + self.check_bb(location, *resume, EdgeKind::Other); if let Some(drop) = drop { - self.check_bb(location, *drop); + self.check_bb(location, *drop, EdgeKind::Other); } } TerminatorKind::FalseEdge { real_target, imaginary_target } => { - self.check_bb(location, *real_target); - self.check_bb(location, *imaginary_target); + self.check_bb(location, *real_target, EdgeKind::Other); + self.check_bb(location, *imaginary_target, EdgeKind::Other); } TerminatorKind::FalseUnwind { real_target, unwind } => { - self.check_bb(location, *real_target); + self.check_bb(location, *real_target, EdgeKind::Other); if let Some(unwind) = unwind { - self.check_bb(location, *unwind); + self.check_bb(location, *unwind, EdgeKind::Unwind); } } TerminatorKind::InlineAsm { destination, .. } => { if let Some(destination) = destination { - self.check_bb(location, *destination); + self.check_bb(location, *destination, EdgeKind::Other); } } // Nothing to validate for these. From 4158bb0f0b06a3a47042ab5e5cbc70075bec8805 Mon Sep 17 00:00:00 2001 From: Nathan Corbyn Date: Mon, 8 Jun 2020 16:00:09 +0100 Subject: [PATCH 2/4] Relax cleanup to cleanup check --- src/librustc_mir/transform/validate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 339b1469f1237..252ac2a00b292 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -62,8 +62,8 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { (false, false, EdgeKind::Other) // Non-cleanup blocks can jump to cleanup blocks along unwind edges | (false, true, EdgeKind::Unwind) - // Cleanup blocks can jump to cleanup blocks along unwind edges - | (true, true, EdgeKind::Unwind) => {} + // Cleanup blocks can jump to cleanup blocks along any edges + | (true, true, _) => {} // All other jumps are invalid _ => { self.fail( From 1c4fd22618730f63689d8dc36972bc2a56d46067 Mon Sep 17 00:00:00 2001 From: Nathan Corbyn Date: Mon, 8 Jun 2020 16:04:41 +0100 Subject: [PATCH 3/4] Strengthen cleanup to cleanup check --- src/librustc_mir/transform/validate.rs | 32 +++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 252ac2a00b292..dda416b01a21f 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -13,7 +13,7 @@ use rustc_span::def_id::DefId; enum EdgeKind { Unwind, - Other, + Normal, } pub struct Validator { @@ -59,11 +59,11 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { let src = self.body.basic_blocks().get(location.block).unwrap(); match (src.is_cleanup, bb.is_cleanup, edge_kind) { // Non-cleanup blocks can jump to non-cleanup blocks along non-unwind edges - (false, false, EdgeKind::Other) + (false, false, EdgeKind::Normal) // Non-cleanup blocks can jump to cleanup blocks along unwind edges | (false, true, EdgeKind::Unwind) - // Cleanup blocks can jump to cleanup blocks along any edges - | (true, true, _) => {} + // Cleanup blocks can jump to cleanup blocks along non-unwind edges + | (true, true, EdgeKind::Normal) => {} // All other jumps are invalid _ => { self.fail( @@ -114,7 +114,7 @@ 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(location, *target, EdgeKind::Other); + self.check_bb(location, *target, EdgeKind::Normal); } TerminatorKind::SwitchInt { targets, values, .. } => { if targets.len() != values.len() + 1 { @@ -128,17 +128,17 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ); } for target in targets { - self.check_bb(location, *target, EdgeKind::Other); + self.check_bb(location, *target, EdgeKind::Normal); } } TerminatorKind::Drop { target, unwind, .. } => { - self.check_bb(location, *target, EdgeKind::Other); + self.check_bb(location, *target, EdgeKind::Normal); if let Some(unwind) = unwind { self.check_bb(location, *unwind, EdgeKind::Unwind); } } TerminatorKind::DropAndReplace { target, unwind, .. } => { - self.check_bb(location, *target, EdgeKind::Other); + self.check_bb(location, *target, EdgeKind::Normal); if let Some(unwind) = unwind { self.check_bb(location, *unwind, EdgeKind::Unwind); } @@ -153,7 +153,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ), } if let Some((_, target)) = destination { - self.check_bb(location, *target, EdgeKind::Other); + self.check_bb(location, *target, EdgeKind::Normal); } if let Some(cleanup) = cleanup { self.check_bb(location, *cleanup, EdgeKind::Unwind); @@ -170,30 +170,30 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ), ); } - self.check_bb(location, *target, EdgeKind::Other); + self.check_bb(location, *target, EdgeKind::Normal); if let Some(cleanup) = cleanup { self.check_bb(location, *cleanup, EdgeKind::Unwind); } } TerminatorKind::Yield { resume, drop, .. } => { - self.check_bb(location, *resume, EdgeKind::Other); + self.check_bb(location, *resume, EdgeKind::Normal); if let Some(drop) = drop { - self.check_bb(location, *drop, EdgeKind::Other); + self.check_bb(location, *drop, EdgeKind::Normal); } } TerminatorKind::FalseEdge { real_target, imaginary_target } => { - self.check_bb(location, *real_target, EdgeKind::Other); - self.check_bb(location, *imaginary_target, EdgeKind::Other); + self.check_bb(location, *real_target, EdgeKind::Normal); + self.check_bb(location, *imaginary_target, EdgeKind::Normal); } TerminatorKind::FalseUnwind { real_target, unwind } => { - self.check_bb(location, *real_target, EdgeKind::Other); + self.check_bb(location, *real_target, EdgeKind::Normal); if let Some(unwind) = unwind { self.check_bb(location, *unwind, EdgeKind::Unwind); } } TerminatorKind::InlineAsm { destination, .. } => { if let Some(destination) = destination { - self.check_bb(location, *destination, EdgeKind::Other); + self.check_bb(location, *destination, EdgeKind::Normal); } } // Nothing to validate for these. From 9495ee21b740fc1a8bcec651c5f2135806a71b03 Mon Sep 17 00:00:00 2001 From: Nathan Corbyn Date: Tue, 9 Jun 2020 11:21:36 +0100 Subject: [PATCH 4/4] Address comments --- src/librustc_mir/transform/validate.rs | 45 +++++++++++++++----------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index dda416b01a21f..8150c328316cb 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -11,6 +11,7 @@ use rustc_middle::{ }; use rustc_span::def_id::DefId; +#[derive(Copy, Clone, Debug)] enum EdgeKind { Unwind, Normal, @@ -54,7 +55,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { ); } - fn check_bb(&self, location: Location, bb: BasicBlock, edge_kind: EdgeKind) { + fn check_edge(&self, location: Location, bb: BasicBlock, edge_kind: EdgeKind) { if let Some(bb) = self.body.basic_blocks().get(bb) { let src = self.body.basic_blocks().get(location.block).unwrap(); match (src.is_cleanup, bb.is_cleanup, edge_kind) { @@ -68,7 +69,13 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { _ => { self.fail( location, - format!("encountered jump that does not respect unwind invariants {:?}", bb) + format!( + "{:?} edge to {:?} violates unwind invariants (cleanup {:?} -> {:?})", + edge_kind, + bb, + src.is_cleanup, + bb.is_cleanup, + ) ) } } @@ -114,7 +121,7 @@ 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(location, *target, EdgeKind::Normal); + self.check_edge(location, *target, EdgeKind::Normal); } TerminatorKind::SwitchInt { targets, values, .. } => { if targets.len() != values.len() + 1 { @@ -128,19 +135,19 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ); } for target in targets { - self.check_bb(location, *target, EdgeKind::Normal); + self.check_edge(location, *target, EdgeKind::Normal); } } TerminatorKind::Drop { target, unwind, .. } => { - self.check_bb(location, *target, EdgeKind::Normal); + self.check_edge(location, *target, EdgeKind::Normal); if let Some(unwind) = unwind { - self.check_bb(location, *unwind, EdgeKind::Unwind); + self.check_edge(location, *unwind, EdgeKind::Unwind); } } TerminatorKind::DropAndReplace { target, unwind, .. } => { - self.check_bb(location, *target, EdgeKind::Normal); + self.check_edge(location, *target, EdgeKind::Normal); if let Some(unwind) = unwind { - self.check_bb(location, *unwind, EdgeKind::Unwind); + self.check_edge(location, *unwind, EdgeKind::Unwind); } } TerminatorKind::Call { func, destination, cleanup, .. } => { @@ -153,10 +160,10 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ), } if let Some((_, target)) = destination { - self.check_bb(location, *target, EdgeKind::Normal); + self.check_edge(location, *target, EdgeKind::Normal); } if let Some(cleanup) = cleanup { - self.check_bb(location, *cleanup, EdgeKind::Unwind); + self.check_edge(location, *cleanup, EdgeKind::Unwind); } } TerminatorKind::Assert { cond, target, cleanup, .. } => { @@ -170,30 +177,30 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ), ); } - self.check_bb(location, *target, EdgeKind::Normal); + self.check_edge(location, *target, EdgeKind::Normal); if let Some(cleanup) = cleanup { - self.check_bb(location, *cleanup, EdgeKind::Unwind); + self.check_edge(location, *cleanup, EdgeKind::Unwind); } } TerminatorKind::Yield { resume, drop, .. } => { - self.check_bb(location, *resume, EdgeKind::Normal); + self.check_edge(location, *resume, EdgeKind::Normal); if let Some(drop) = drop { - self.check_bb(location, *drop, EdgeKind::Normal); + self.check_edge(location, *drop, EdgeKind::Normal); } } TerminatorKind::FalseEdge { real_target, imaginary_target } => { - self.check_bb(location, *real_target, EdgeKind::Normal); - self.check_bb(location, *imaginary_target, EdgeKind::Normal); + self.check_edge(location, *real_target, EdgeKind::Normal); + self.check_edge(location, *imaginary_target, EdgeKind::Normal); } TerminatorKind::FalseUnwind { real_target, unwind } => { - self.check_bb(location, *real_target, EdgeKind::Normal); + self.check_edge(location, *real_target, EdgeKind::Normal); if let Some(unwind) = unwind { - self.check_bb(location, *unwind, EdgeKind::Unwind); + self.check_edge(location, *unwind, EdgeKind::Unwind); } } TerminatorKind::InlineAsm { destination, .. } => { if let Some(destination) = destination { - self.check_bb(location, *destination, EdgeKind::Normal); + self.check_edge(location, *destination, EdgeKind::Normal); } } // Nothing to validate for these.