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

Document the current MIR semantics that are clear from existing code #95320

Merged
merged 11 commits into from
Apr 12, 2022
Merged

Document the current MIR semantics that are clear from existing code #95320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dae5c84
Improve documentation of `Place` and `Operand`
JakobDegen Mar 24, 2022
2f4a7a0
Adjust computation of place types to detect more invalid places
JakobDegen Mar 24, 2022
6343691
Add documentation for the semantics of MIR rvalues
JakobDegen Mar 25, 2022
9ac5e98
Extend the MIR validator to check many more things around rvalues.
JakobDegen Mar 25, 2022
8e01cd6
Improve documentation for MIR statement kinds.
JakobDegen Mar 25, 2022
f1f25c0
Improve documentation for MIR terminators
JakobDegen Mar 26, 2022
f2d7908
Adjust MIR validator to check a few more things for terminators
JakobDegen Mar 26, 2022
1d318e4
Improve MIR phases documentation with summaries of changes
JakobDegen Mar 26, 2022
411ae6f
Address various comments and change some details around place to valu…
JakobDegen Mar 27, 2022
4bce639
Add more clarifications in response to Ralf's comments
JakobDegen Apr 8, 2022
8732bf5
Remove rule that place loads may not happen with variant index set
JakobDegen Apr 9, 2022
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
229 changes: 185 additions & 44 deletions compiler/rustc_const_eval/src/transform/validate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@
use rustc_index::bit_set::BitSet;
use rustc_infer::infer::TyCtxtInferExt;
use rustc_middle::mir::interpret::Scalar;
use rustc_middle::mir::traversal;
use rustc_middle::mir::visit::{PlaceContext, Visitor};
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};
Expand All @@ -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(..)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intrinsics may be codegened in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I think at that point this check should be refined to verify those bodies that do get codegened only. Even better would be if code elsewhere got changed to stop passing intrinsics that don't get codegened through the MIR validator. cc @oli-obk

return;
}
let def_id = body.source.def_id();
let param_env = tcx.param_env(def_id);
let mir_phase = self.mir_phase;
Expand Down Expand Up @@ -240,58 +246,179 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
self.super_projection_elem(local, proj_base, elem, context, 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) {
fn visit_place(&mut self, place: &Place<'tcx>, _: PlaceContext, _: Location) {
// Set off any `bug!`s in the type computation code
let _ = place.ty(&self.body.local_decls, self.tcx);
}

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(..) => {
Expand Down Expand Up @@ -512,6 +639,9 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
}
}
TerminatorKind::Yield { resume, drop, .. } => {
if self.body.generator.is_none() {
self.fail(location, "`Yield` cannot appear outside generator bodies");
}
if self.mir_phase >= MirPhase::GeneratorsLowered {
self.fail(location, "`Yield` should have been replaced by generator lowering");
}
Expand Down Expand Up @@ -551,18 +681,29 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
}
}
TerminatorKind::GeneratorDrop => {
if self.body.generator.is_none() {
self.fail(location, "`GeneratorDrop` cannot appear outside generator bodies");
}
if self.mir_phase >= MirPhase::GeneratorsLowered {
self.fail(
location,
"`GeneratorDrop` should have been replaced by generator lowering",
);
}
}
// Nothing to validate for these.
TerminatorKind::Resume
| TerminatorKind::Abort
| TerminatorKind::Return
| TerminatorKind::Unreachable => {}
TerminatorKind::Resume | TerminatorKind::Abort => {
let bb = location.block;
if !self.body.basic_blocks()[bb].is_cleanup {
self.fail(location, "Cannot `Resume` or `Abort` from non-cleanup basic block")
}
}
TerminatorKind::Return => {
let bb = location.block;
if self.body.basic_blocks()[bb].is_cleanup {
self.fail(location, "Cannot `Return` from cleanup basic block")
}
}
TerminatorKind::Unreachable => {}
}

self.super_terminator(terminator, location);
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
#![feature(unwrap_infallible)]
#![feature(decl_macro)]
#![feature(drain_filter)]
#![feature(intra_doc_pointers)]
#![recursion_limit = "512"]
#![allow(rustc::potential_query_instability)]

Expand Down
Loading