Skip to content

Commit 9ac5e98

Browse files
JakobDegenJakobDegen
committed
Extend the MIR validator to check many more things around rvalues.
1 parent 6343691 commit 9ac5e98

File tree

3 files changed

+182
-60
lines changed

3 files changed

+182
-60
lines changed

‎compiler/rustc_const_eval/src/transform/validate.rs

+161-39
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,13 @@ use rustc_index::bit_set::BitSet;
44
use rustc_infer::infer::TyCtxtInferExt;
55
use rustc_middle::mir::interpret::Scalar;
66
use rustc_middle::mir::visit::{PlaceContext, Visitor};
7-
use rustc_middle::mir::{traversal, Place};
87
use rustc_middle::mir::{
9-
AggregateKind, BasicBlock, Body, BorrowKind, Local, Location, MirPass, MirPhase, Operand,
10-
PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement, StatementKind, Terminator,
11-
TerminatorKind, START_BLOCK,
8+
traversal, AggregateKind, BasicBlock, BinOp, Body, BorrowKind, Local, Location, MirPass,
9+
MirPhase, Operand, Place, PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement,
10+
StatementKind, Terminator, TerminatorKind, UnOp, START_BLOCK,
1211
};
1312
use rustc_middle::ty::fold::BottomUpFolder;
14-
use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt, TypeFoldable};
13+
use rustc_middle::ty::{self, InstanceDef, ParamEnv, Ty, TyCtxt, TypeFoldable};
1514
use rustc_mir_dataflow::impls::MaybeStorageLive;
1615
use rustc_mir_dataflow::storage::AlwaysLiveLocals;
1716
use rustc_mir_dataflow::{Analysis, ResultsCursor};
@@ -36,6 +35,13 @@ pub struct Validator {
3635

3736
impl<'tcx> MirPass<'tcx> for Validator {
3837
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
38+
// FIXME(JakobDegen): These bodies never instantiated in codegend anyway, so it's not
39+
// terribly important that they pass the validator. However, I think other passes might
40+
// still see them, in which case they might be surprised. It would probably be better if we
41+
// didn't put this through the MIR pipeline at all.
42+
if matches!(body.source.instance, InstanceDef::Intrinsic(..) | InstanceDef::Virtual(..)) {
43+
return;
44+
}
3945
let def_id = body.source.def_id();
4046
let param_env = tcx.param_env(def_id);
4147
let mir_phase = self.mir_phase;
@@ -248,58 +254,174 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
248254
}
249255
}
250256

251-
fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
252-
match &statement.kind {
253-
StatementKind::Assign(box (dest, rvalue)) => {
254-
// LHS and RHS of the assignment must have the same type.
255-
let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty;
256-
let right_ty = rvalue.ty(&self.body.local_decls, self.tcx);
257-
if !self.mir_assign_valid_types(right_ty, left_ty) {
257+
fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) {
258+
macro_rules! check_kinds {
259+
($t:expr, $text:literal, $($patterns:tt)*) => {
260+
if !matches!(($t).kind(), $($patterns)*) {
261+
self.fail(location, format!($text, $t));
262+
}
263+
};
264+
}
265+
match rvalue {
266+
Rvalue::Use(_) => {}
267+
Rvalue::Aggregate(agg_kind, _) => {
268+
let disallowed = match **agg_kind {
269+
AggregateKind::Array(..) => false,
270+
AggregateKind::Generator(..) => self.mir_phase >= MirPhase::GeneratorsLowered,
271+
_ => self.mir_phase >= MirPhase::Deaggregated,
272+
};
273+
if disallowed {
258274
self.fail(
259275
location,
260-
format!(
261-
"encountered `{:?}` with incompatible types:\n\
262-
left-hand side has type: {}\n\
263-
right-hand side has type: {}",
264-
statement.kind, left_ty, right_ty,
265-
),
276+
format!("{:?} have been lowered to field assignments", rvalue),
277+
)
278+
}
279+
}
280+
Rvalue::Ref(_, BorrowKind::Shallow, _) => {
281+
if self.mir_phase >= MirPhase::DropsLowered {
282+
self.fail(
283+
location,
284+
"`Assign` statement with a `Shallow` borrow should have been removed after drop lowering phase",
266285
);
267286
}
268-
match rvalue {
269-
// The sides of an assignment must not alias. Currently this just checks whether the places
270-
// are identical.
271-
Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => {
272-
if dest == src {
287+
}
288+
Rvalue::Len(p) => {
289+
let pty = p.ty(&self.body.local_decls, self.tcx).ty;
290+
check_kinds!(
291+
pty,
292+
"Cannot compute length of non-array type {:?}",
293+
ty::Array(..) | ty::Slice(..)
294+
);
295+
}
296+
Rvalue::BinaryOp(op, vals) | Rvalue::CheckedBinaryOp(op, vals) => {
297+
use BinOp::*;
298+
let a = vals.0.ty(&self.body.local_decls, self.tcx);
299+
let b = vals.1.ty(&self.body.local_decls, self.tcx);
300+
match op {
301+
Offset => {
302+
check_kinds!(a, "Cannot offset non-pointer type {:?}", ty::RawPtr(..));
303+
if b != self.tcx.types.isize && b != self.tcx.types.usize {
304+
self.fail(location, format!("Cannot offset by non-isize type {:?}", b));
305+
}
306+
}
307+
Eq | Lt | Le | Ne | Ge | Gt => {
308+
for x in [a, b] {
309+
check_kinds!(
310+
x,
311+
"Cannot compare type {:?}",
312+
ty::Bool
313+
| ty::Char
314+
| ty::Int(..)
315+
| ty::Uint(..)
316+
| ty::Float(..)
317+
| ty::RawPtr(..)
318+
| ty::FnPtr(..)
319+
)
320+
}
321+
// None of the possible types have lifetimes, so we can just compare
322+
// directly
323+
if a != b {
273324
self.fail(
274325
location,
275-
"encountered `Assign` statement with overlapping memory",
326+
format!("Cannot compare unequal types {:?} and {:?}", a, b),
276327
);
277328
}
278329
}
279-
Rvalue::Aggregate(agg_kind, _) => {
280-
let disallowed = match **agg_kind {
281-
AggregateKind::Array(..) => false,
282-
AggregateKind::Generator(..) => {
283-
self.mir_phase >= MirPhase::GeneratorsLowered
284-
}
285-
_ => self.mir_phase >= MirPhase::Deaggregated,
286-
};
287-
if disallowed {
330+
Shl | Shr => {
331+
for x in [a, b] {
332+
check_kinds!(
333+
x,
334+
"Cannot shift non-integer type {:?}",
335+
ty::Uint(..) | ty::Int(..)
336+
)
337+
}
338+
}
339+
BitAnd | BitOr | BitXor => {
340+
for x in [a, b] {
341+
check_kinds!(
342+
x,
343+
"Cannot perform bitwise op on type {:?}",
344+
ty::Uint(..) | ty::Int(..) | ty::Bool
345+
)
346+
}
347+
if a != b {
288348
self.fail(
289349
location,
290-
format!("{:?} have been lowered to field assignments", rvalue),
291-
)
350+
format!(
351+
"Cannot perform bitwise op on unequal types {:?} and {:?}",
352+
a, b
353+
),
354+
);
292355
}
293356
}
294-
Rvalue::Ref(_, BorrowKind::Shallow, _) => {
295-
if self.mir_phase >= MirPhase::DropsLowered {
357+
Add | Sub | Mul | Div | Rem => {
358+
for x in [a, b] {
359+
check_kinds!(
360+
x,
361+
"Cannot perform op on type {:?}",
362+
ty::Uint(..) | ty::Int(..) | ty::Float(..)
363+
)
364+
}
365+
if a != b {
296366
self.fail(
297367
location,
298-
"`Assign` statement with a `Shallow` borrow should have been removed after drop lowering phase",
368+
format!("Cannot perform op on unequal types {:?} and {:?}", a, b),
299369
);
300370
}
301371
}
302-
_ => {}
372+
}
373+
}
374+
Rvalue::UnaryOp(op, operand) => {
375+
let a = operand.ty(&self.body.local_decls, self.tcx);
376+
match op {
377+
UnOp::Neg => {
378+
check_kinds!(a, "Cannot negate type {:?}", ty::Int(..) | ty::Float(..))
379+
}
380+
UnOp::Not => {
381+
check_kinds!(
382+
a,
383+
"Cannot binary not type {:?}",
384+
ty::Int(..) | ty::Uint(..) | ty::Bool
385+
);
386+
}
387+
}
388+
}
389+
Rvalue::ShallowInitBox(operand, _) => {
390+
let a = operand.ty(&self.body.local_decls, self.tcx);
391+
check_kinds!(a, "Cannot shallow init type {:?}", ty::RawPtr(..));
392+
}
393+
_ => {}
394+
}
395+
self.super_rvalue(rvalue, location);
396+
}
397+
398+
fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
399+
match &statement.kind {
400+
StatementKind::Assign(box (dest, rvalue)) => {
401+
// LHS and RHS of the assignment must have the same type.
402+
let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty;
403+
let right_ty = rvalue.ty(&self.body.local_decls, self.tcx);
404+
if !self.mir_assign_valid_types(right_ty, left_ty) {
405+
self.fail(
406+
location,
407+
format!(
408+
"encountered `{:?}` with incompatible types:\n\
409+
left-hand side has type: {}\n\
410+
right-hand side has type: {}",
411+
statement.kind, left_ty, right_ty,
412+
),
413+
);
414+
}
415+
// FIXME(JakobDegen): Check this for all rvalues, not just this one.
416+
if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
417+
// The sides of an assignment must not alias. Currently this just checks whether
418+
// the places are identical.
419+
if dest == src {
420+
self.fail(
421+
location,
422+
"encountered `Assign` statement with overlapping memory",
423+
);
424+
}
303425
}
304426
}
305427
StatementKind::AscribeUserType(..) => {

‎src/test/mir-opt/lower_intrinsics.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
#![crate_type = "lib"]
44

55
// EMIT_MIR lower_intrinsics.wrapping.LowerIntrinsics.diff
6-
pub fn wrapping<T: Copy>(a: T, b: T) {
6+
pub fn wrapping(a: i32, b: i32) {
77
let _x = core::intrinsics::wrapping_add(a, b);
88
let _y = core::intrinsics::wrapping_sub(a, b);
99
let _z = core::intrinsics::wrapping_mul(a, b);

‎src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff

+20-20
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,23 @@
11
- // MIR for `wrapping` before LowerIntrinsics
22
+ // MIR for `wrapping` after LowerIntrinsics
33

4-
fn wrapping(_1: T, _2: T) -> () {
5-
debug a => _1; // in scope 0 at $DIR/lower_intrinsics.rs:6:26: 6:27
6-
debug b => _2; // in scope 0 at $DIR/lower_intrinsics.rs:6:32: 6:33
7-
let mut _0: (); // return place in scope 0 at $DIR/lower_intrinsics.rs:6:38: 6:38
8-
let _3: T; // in scope 0 at $DIR/lower_intrinsics.rs:7:9: 7:11
9-
let mut _4: T; // in scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46
10-
let mut _5: T; // in scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49
11-
let mut _7: T; // in scope 0 at $DIR/lower_intrinsics.rs:8:45: 8:46
12-
let mut _8: T; // in scope 0 at $DIR/lower_intrinsics.rs:8:48: 8:49
13-
let mut _10: T; // in scope 0 at $DIR/lower_intrinsics.rs:9:45: 9:46
14-
let mut _11: T; // in scope 0 at $DIR/lower_intrinsics.rs:9:48: 9:49
4+
fn wrapping(_1: i32, _2: i32) -> () {
5+
debug a => _1; // in scope 0 at $DIR/lower_intrinsics.rs:6:17: 6:18
6+
debug b => _2; // in scope 0 at $DIR/lower_intrinsics.rs:6:25: 6:26
7+
let mut _0: (); // return place in scope 0 at $DIR/lower_intrinsics.rs:6:33: 6:33
8+
let _3: i32; // in scope 0 at $DIR/lower_intrinsics.rs:7:9: 7:11
9+
let mut _4: i32; // in scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46
10+
let mut _5: i32; // in scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49
11+
let mut _7: i32; // in scope 0 at $DIR/lower_intrinsics.rs:8:45: 8:46
12+
let mut _8: i32; // in scope 0 at $DIR/lower_intrinsics.rs:8:48: 8:49
13+
let mut _10: i32; // in scope 0 at $DIR/lower_intrinsics.rs:9:45: 9:46
14+
let mut _11: i32; // in scope 0 at $DIR/lower_intrinsics.rs:9:48: 9:49
1515
scope 1 {
1616
debug _x => _3; // in scope 1 at $DIR/lower_intrinsics.rs:7:9: 7:11
17-
let _6: T; // in scope 1 at $DIR/lower_intrinsics.rs:8:9: 8:11
17+
let _6: i32; // in scope 1 at $DIR/lower_intrinsics.rs:8:9: 8:11
1818
scope 2 {
1919
debug _y => _6; // in scope 2 at $DIR/lower_intrinsics.rs:8:9: 8:11
20-
let _9: T; // in scope 2 at $DIR/lower_intrinsics.rs:9:9: 9:11
20+
let _9: i32; // in scope 2 at $DIR/lower_intrinsics.rs:9:9: 9:11
2121
scope 3 {
2222
debug _z => _9; // in scope 3 at $DIR/lower_intrinsics.rs:9:9: 9:11
2323
}
@@ -30,10 +30,10 @@
3030
_4 = _1; // scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46
3131
StorageLive(_5); // scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49
3232
_5 = _2; // scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49
33-
- _3 = wrapping_add::<T>(move _4, move _5) -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50
33+
- _3 = wrapping_add::<i32>(move _4, move _5) -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50
3434
- // mir::Constant
3535
- // + span: $DIR/lower_intrinsics.rs:7:14: 7:44
36-
- // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_add::<T>}, val: Value(Scalar(<ZST>)) }
36+
- // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_add::<i32>}, val: Value(Scalar(<ZST>)) }
3737
+ _3 = Add(move _4, move _5); // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50
3838
+ goto -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50
3939
}
@@ -46,10 +46,10 @@
4646
_7 = _1; // scope 1 at $DIR/lower_intrinsics.rs:8:45: 8:46
4747
StorageLive(_8); // scope 1 at $DIR/lower_intrinsics.rs:8:48: 8:49
4848
_8 = _2; // scope 1 at $DIR/lower_intrinsics.rs:8:48: 8:49
49-
- _6 = wrapping_sub::<T>(move _7, move _8) -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50
49+
- _6 = wrapping_sub::<i32>(move _7, move _8) -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50
5050
- // mir::Constant
5151
- // + span: $DIR/lower_intrinsics.rs:8:14: 8:44
52-
- // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_sub::<T>}, val: Value(Scalar(<ZST>)) }
52+
- // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_sub::<i32>}, val: Value(Scalar(<ZST>)) }
5353
+ _6 = Sub(move _7, move _8); // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50
5454
+ goto -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50
5555
}
@@ -62,18 +62,18 @@
6262
_10 = _1; // scope 2 at $DIR/lower_intrinsics.rs:9:45: 9:46
6363
StorageLive(_11); // scope 2 at $DIR/lower_intrinsics.rs:9:48: 9:49
6464
_11 = _2; // scope 2 at $DIR/lower_intrinsics.rs:9:48: 9:49
65-
- _9 = wrapping_mul::<T>(move _10, move _11) -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50
65+
- _9 = wrapping_mul::<i32>(move _10, move _11) -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50
6666
- // mir::Constant
6767
- // + span: $DIR/lower_intrinsics.rs:9:14: 9:44
68-
- // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_mul::<T>}, val: Value(Scalar(<ZST>)) }
68+
- // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_mul::<i32>}, val: Value(Scalar(<ZST>)) }
6969
+ _9 = Mul(move _10, move _11); // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50
7070
+ goto -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50
7171
}
7272

7373
bb3: {
7474
StorageDead(_11); // scope 2 at $DIR/lower_intrinsics.rs:9:49: 9:50
7575
StorageDead(_10); // scope 2 at $DIR/lower_intrinsics.rs:9:49: 9:50
76-
_0 = const (); // scope 0 at $DIR/lower_intrinsics.rs:6:38: 10:2
76+
_0 = const (); // scope 0 at $DIR/lower_intrinsics.rs:6:33: 10:2
7777
StorageDead(_9); // scope 2 at $DIR/lower_intrinsics.rs:10:1: 10:2
7878
StorageDead(_6); // scope 1 at $DIR/lower_intrinsics.rs:10:1: 10:2
7979
StorageDead(_3); // scope 0 at $DIR/lower_intrinsics.rs:10:1: 10:2

0 commit comments

Comments
 (0)