From 9ac5e986ed8e7d589787532857ef74576473adcf Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Thu, 24 Mar 2022 22:30:23 -0400 Subject: [PATCH] Extend the MIR validator to check many more things around rvalues. --- .../src/transform/validate.rs | 200 ++++++++++++++---- src/test/mir-opt/lower_intrinsics.rs | 2 +- ...r_intrinsics.wrapping.LowerIntrinsics.diff | 40 ++-- 3 files changed, 182 insertions(+), 60 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index af58bcabdf2..7eb91385653 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -4,14 +4,13 @@ use rustc_infer::infer::TyCtxtInferExt; use rustc_middle::mir::interpret::Scalar; use rustc_middle::mir::visit::{PlaceContext, Visitor}; -use rustc_middle::mir::{traversal, Place}; use rustc_middle::mir::{ - AggregateKind, BasicBlock, Body, BorrowKind, Local, Location, MirPass, MirPhase, Operand, - PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement, StatementKind, Terminator, - TerminatorKind, START_BLOCK, + traversal, AggregateKind, BasicBlock, BinOp, Body, BorrowKind, Local, Location, MirPass, + MirPhase, Operand, Place, PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement, + StatementKind, Terminator, TerminatorKind, UnOp, START_BLOCK, }; use rustc_middle::ty::fold::BottomUpFolder; -use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt, TypeFoldable}; +use rustc_middle::ty::{self, InstanceDef, ParamEnv, Ty, TyCtxt, TypeFoldable}; use rustc_mir_dataflow::impls::MaybeStorageLive; use rustc_mir_dataflow::storage::AlwaysLiveLocals; use rustc_mir_dataflow::{Analysis, ResultsCursor}; @@ -36,6 +35,13 @@ pub struct Validator { impl<'tcx> MirPass<'tcx> for Validator { fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { + // FIXME(JakobDegen): These bodies never instantiated in codegend anyway, so it's not + // terribly important that they pass the validator. However, I think other passes might + // still see them, in which case they might be surprised. It would probably be better if we + // didn't put this through the MIR pipeline at all. + if matches!(body.source.instance, InstanceDef::Intrinsic(..) | InstanceDef::Virtual(..)) { + return; + } let def_id = body.source.def_id(); let param_env = tcx.param_env(def_id); let mir_phase = self.mir_phase; @@ -248,58 +254,174 @@ fn visit_place(&mut self, place: &Place<'tcx>, _: PlaceContext, location: Locati } } - fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { - match &statement.kind { - StatementKind::Assign(box (dest, rvalue)) => { - // LHS and RHS of the assignment must have the same type. - let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty; - let right_ty = rvalue.ty(&self.body.local_decls, self.tcx); - if !self.mir_assign_valid_types(right_ty, left_ty) { + fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { + macro_rules! check_kinds { + ($t:expr, $text:literal, $($patterns:tt)*) => { + if !matches!(($t).kind(), $($patterns)*) { + self.fail(location, format!($text, $t)); + } + }; + } + match rvalue { + Rvalue::Use(_) => {} + Rvalue::Aggregate(agg_kind, _) => { + let disallowed = match **agg_kind { + AggregateKind::Array(..) => false, + AggregateKind::Generator(..) => self.mir_phase >= MirPhase::GeneratorsLowered, + _ => self.mir_phase >= MirPhase::Deaggregated, + }; + if disallowed { self.fail( location, - format!( - "encountered `{:?}` with incompatible types:\n\ - left-hand side has type: {}\n\ - right-hand side has type: {}", - statement.kind, left_ty, right_ty, - ), + format!("{:?} have been lowered to field assignments", rvalue), + ) + } + } + Rvalue::Ref(_, BorrowKind::Shallow, _) => { + if self.mir_phase >= MirPhase::DropsLowered { + self.fail( + location, + "`Assign` statement with a `Shallow` borrow should have been removed after drop lowering phase", ); } - match rvalue { - // The sides of an assignment must not alias. Currently this just checks whether the places - // are identical. - Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => { - if dest == src { + } + Rvalue::Len(p) => { + let pty = p.ty(&self.body.local_decls, self.tcx).ty; + check_kinds!( + pty, + "Cannot compute length of non-array type {:?}", + ty::Array(..) | ty::Slice(..) + ); + } + Rvalue::BinaryOp(op, vals) | Rvalue::CheckedBinaryOp(op, vals) => { + use BinOp::*; + let a = vals.0.ty(&self.body.local_decls, self.tcx); + let b = vals.1.ty(&self.body.local_decls, self.tcx); + match op { + Offset => { + check_kinds!(a, "Cannot offset non-pointer type {:?}", ty::RawPtr(..)); + if b != self.tcx.types.isize && b != self.tcx.types.usize { + self.fail(location, format!("Cannot offset by non-isize type {:?}", b)); + } + } + Eq | Lt | Le | Ne | Ge | Gt => { + for x in [a, b] { + check_kinds!( + x, + "Cannot compare type {:?}", + ty::Bool + | ty::Char + | ty::Int(..) + | ty::Uint(..) + | ty::Float(..) + | ty::RawPtr(..) + | ty::FnPtr(..) + ) + } + // None of the possible types have lifetimes, so we can just compare + // directly + if a != b { self.fail( location, - "encountered `Assign` statement with overlapping memory", + format!("Cannot compare unequal types {:?} and {:?}", a, b), ); } } - Rvalue::Aggregate(agg_kind, _) => { - let disallowed = match **agg_kind { - AggregateKind::Array(..) => false, - AggregateKind::Generator(..) => { - self.mir_phase >= MirPhase::GeneratorsLowered - } - _ => self.mir_phase >= MirPhase::Deaggregated, - }; - if disallowed { + Shl | Shr => { + for x in [a, b] { + check_kinds!( + x, + "Cannot shift non-integer type {:?}", + ty::Uint(..) | ty::Int(..) + ) + } + } + BitAnd | BitOr | BitXor => { + for x in [a, b] { + check_kinds!( + x, + "Cannot perform bitwise op on type {:?}", + ty::Uint(..) | ty::Int(..) | ty::Bool + ) + } + if a != b { self.fail( location, - format!("{:?} have been lowered to field assignments", rvalue), - ) + format!( + "Cannot perform bitwise op on unequal types {:?} and {:?}", + a, b + ), + ); } } - Rvalue::Ref(_, BorrowKind::Shallow, _) => { - if self.mir_phase >= MirPhase::DropsLowered { + Add | Sub | Mul | Div | Rem => { + for x in [a, b] { + check_kinds!( + x, + "Cannot perform op on type {:?}", + ty::Uint(..) | ty::Int(..) | ty::Float(..) + ) + } + if a != b { self.fail( location, - "`Assign` statement with a `Shallow` borrow should have been removed after drop lowering phase", + format!("Cannot perform op on unequal types {:?} and {:?}", a, b), ); } } - _ => {} + } + } + Rvalue::UnaryOp(op, operand) => { + let a = operand.ty(&self.body.local_decls, self.tcx); + match op { + UnOp::Neg => { + check_kinds!(a, "Cannot negate type {:?}", ty::Int(..) | ty::Float(..)) + } + UnOp::Not => { + check_kinds!( + a, + "Cannot binary not type {:?}", + ty::Int(..) | ty::Uint(..) | ty::Bool + ); + } + } + } + Rvalue::ShallowInitBox(operand, _) => { + let a = operand.ty(&self.body.local_decls, self.tcx); + check_kinds!(a, "Cannot shallow init type {:?}", ty::RawPtr(..)); + } + _ => {} + } + self.super_rvalue(rvalue, location); + } + + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { + match &statement.kind { + StatementKind::Assign(box (dest, rvalue)) => { + // LHS and RHS of the assignment must have the same type. + let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty; + let right_ty = rvalue.ty(&self.body.local_decls, self.tcx); + if !self.mir_assign_valid_types(right_ty, left_ty) { + self.fail( + location, + format!( + "encountered `{:?}` with incompatible types:\n\ + left-hand side has type: {}\n\ + right-hand side has type: {}", + statement.kind, left_ty, right_ty, + ), + ); + } + // FIXME(JakobDegen): Check this for all rvalues, not just this one. + if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue { + // The sides of an assignment must not alias. Currently this just checks whether + // the places are identical. + if dest == src { + self.fail( + location, + "encountered `Assign` statement with overlapping memory", + ); + } } } StatementKind::AscribeUserType(..) => { diff --git a/src/test/mir-opt/lower_intrinsics.rs b/src/test/mir-opt/lower_intrinsics.rs index 8a8880dad02..eab51b65f1a 100644 --- a/src/test/mir-opt/lower_intrinsics.rs +++ b/src/test/mir-opt/lower_intrinsics.rs @@ -3,7 +3,7 @@ #![crate_type = "lib"] // EMIT_MIR lower_intrinsics.wrapping.LowerIntrinsics.diff -pub fn wrapping(a: T, b: T) { +pub fn wrapping(a: i32, b: i32) { let _x = core::intrinsics::wrapping_add(a, b); let _y = core::intrinsics::wrapping_sub(a, b); let _z = core::intrinsics::wrapping_mul(a, b); diff --git a/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff b/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff index a531a19bd78..5a0286bad2f 100644 --- a/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff +++ b/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff @@ -1,23 +1,23 @@ - // MIR for `wrapping` before LowerIntrinsics + // MIR for `wrapping` after LowerIntrinsics - fn wrapping(_1: T, _2: T) -> () { - debug a => _1; // in scope 0 at $DIR/lower_intrinsics.rs:6:26: 6:27 - debug b => _2; // in scope 0 at $DIR/lower_intrinsics.rs:6:32: 6:33 - let mut _0: (); // return place in scope 0 at $DIR/lower_intrinsics.rs:6:38: 6:38 - let _3: T; // in scope 0 at $DIR/lower_intrinsics.rs:7:9: 7:11 - let mut _4: T; // in scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46 - let mut _5: T; // in scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49 - let mut _7: T; // in scope 0 at $DIR/lower_intrinsics.rs:8:45: 8:46 - let mut _8: T; // in scope 0 at $DIR/lower_intrinsics.rs:8:48: 8:49 - let mut _10: T; // in scope 0 at $DIR/lower_intrinsics.rs:9:45: 9:46 - let mut _11: T; // in scope 0 at $DIR/lower_intrinsics.rs:9:48: 9:49 + fn wrapping(_1: i32, _2: i32) -> () { + debug a => _1; // in scope 0 at $DIR/lower_intrinsics.rs:6:17: 6:18 + debug b => _2; // in scope 0 at $DIR/lower_intrinsics.rs:6:25: 6:26 + let mut _0: (); // return place in scope 0 at $DIR/lower_intrinsics.rs:6:33: 6:33 + let _3: i32; // in scope 0 at $DIR/lower_intrinsics.rs:7:9: 7:11 + let mut _4: i32; // in scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46 + let mut _5: i32; // in scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49 + let mut _7: i32; // in scope 0 at $DIR/lower_intrinsics.rs:8:45: 8:46 + let mut _8: i32; // in scope 0 at $DIR/lower_intrinsics.rs:8:48: 8:49 + let mut _10: i32; // in scope 0 at $DIR/lower_intrinsics.rs:9:45: 9:46 + let mut _11: i32; // in scope 0 at $DIR/lower_intrinsics.rs:9:48: 9:49 scope 1 { debug _x => _3; // in scope 1 at $DIR/lower_intrinsics.rs:7:9: 7:11 - let _6: T; // in scope 1 at $DIR/lower_intrinsics.rs:8:9: 8:11 + let _6: i32; // in scope 1 at $DIR/lower_intrinsics.rs:8:9: 8:11 scope 2 { debug _y => _6; // in scope 2 at $DIR/lower_intrinsics.rs:8:9: 8:11 - let _9: T; // in scope 2 at $DIR/lower_intrinsics.rs:9:9: 9:11 + let _9: i32; // in scope 2 at $DIR/lower_intrinsics.rs:9:9: 9:11 scope 3 { debug _z => _9; // in scope 3 at $DIR/lower_intrinsics.rs:9:9: 9:11 } @@ -30,10 +30,10 @@ _4 = _1; // scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46 StorageLive(_5); // scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49 _5 = _2; // scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49 -- _3 = wrapping_add::(move _4, move _5) -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50 +- _3 = wrapping_add::(move _4, move _5) -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50 - // mir::Constant - // + span: $DIR/lower_intrinsics.rs:7:14: 7:44 -- // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_add::}, val: Value(Scalar()) } +- // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_add::}, val: Value(Scalar()) } + _3 = Add(move _4, move _5); // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50 + goto -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50 } @@ -46,10 +46,10 @@ _7 = _1; // scope 1 at $DIR/lower_intrinsics.rs:8:45: 8:46 StorageLive(_8); // scope 1 at $DIR/lower_intrinsics.rs:8:48: 8:49 _8 = _2; // scope 1 at $DIR/lower_intrinsics.rs:8:48: 8:49 -- _6 = wrapping_sub::(move _7, move _8) -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50 +- _6 = wrapping_sub::(move _7, move _8) -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50 - // mir::Constant - // + span: $DIR/lower_intrinsics.rs:8:14: 8:44 -- // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_sub::}, val: Value(Scalar()) } +- // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_sub::}, val: Value(Scalar()) } + _6 = Sub(move _7, move _8); // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50 + goto -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50 } @@ -62,10 +62,10 @@ _10 = _1; // scope 2 at $DIR/lower_intrinsics.rs:9:45: 9:46 StorageLive(_11); // scope 2 at $DIR/lower_intrinsics.rs:9:48: 9:49 _11 = _2; // scope 2 at $DIR/lower_intrinsics.rs:9:48: 9:49 -- _9 = wrapping_mul::(move _10, move _11) -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50 +- _9 = wrapping_mul::(move _10, move _11) -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50 - // mir::Constant - // + span: $DIR/lower_intrinsics.rs:9:14: 9:44 -- // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_mul::}, val: Value(Scalar()) } +- // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_mul::}, val: Value(Scalar()) } + _9 = Mul(move _10, move _11); // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50 + goto -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50 } @@ -73,7 +73,7 @@ bb3: { StorageDead(_11); // scope 2 at $DIR/lower_intrinsics.rs:9:49: 9:50 StorageDead(_10); // scope 2 at $DIR/lower_intrinsics.rs:9:49: 9:50 - _0 = const (); // scope 0 at $DIR/lower_intrinsics.rs:6:38: 10:2 + _0 = const (); // scope 0 at $DIR/lower_intrinsics.rs:6:33: 10:2 StorageDead(_9); // scope 2 at $DIR/lower_intrinsics.rs:10:1: 10:2 StorageDead(_6); // scope 1 at $DIR/lower_intrinsics.rs:10:1: 10:2 StorageDead(_3); // scope 0 at $DIR/lower_intrinsics.rs:10:1: 10:2 -- 2.44.0