diff options
| -rw-r--r-- | compiler/rustc_const_eval/src/transform/validate.rs | 50 | ||||
| -rw-r--r-- | compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs | 12 | ||||
| -rw-r--r-- | compiler/rustc_mir_transform/src/lib.rs | 1 | ||||
| -rw-r--r-- | compiler/rustc_mir_transform/src/lint.rs | 119 | ||||
| -rw-r--r-- | compiler/rustc_mir_transform/src/pass_manager.rs | 12 | ||||
| -rw-r--r-- | compiler/rustc_mir_transform/src/ref_prop.rs | 3 | ||||
| -rw-r--r-- | compiler/rustc_session/src/options.rs | 2 | ||||
| -rw-r--r-- | src/tools/compiletest/src/runtest.rs | 1 | ||||
| -rw-r--r-- | tests/mir-opt/reference_prop.rs | 1 | ||||
| -rw-r--r-- | tests/ui/mir/lint/no-storage.rs | 30 | ||||
| -rw-r--r-- | tests/ui/mir/lint/storage-live.rs (renamed from tests/ui/mir/validate/storage-live.rs) | 2 | ||||
| -rw-r--r-- | tests/ui/mir/lint/storage-live.stderr (renamed from tests/ui/mir/validate/storage-live.stderr) | 0 | ||||
| -rw-r--r-- | tests/ui/mir/lint/storage-return.rs | 19 |
13 files changed, 196 insertions, 56 deletions
diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index cca5b90abb9..2f538cebaa6 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -8,9 +8,6 @@ use rustc_middle::mir::interpret::Scalar; use rustc_middle::mir::visit::{NonUseContext, PlaceContext, Visitor}; use rustc_middle::mir::*; use rustc_middle::ty::{self, InstanceDef, ParamEnv, Ty, TyCtxt, TypeVisitableExt, Variance}; -use rustc_mir_dataflow::impls::MaybeStorageLive; -use rustc_mir_dataflow::storage::always_storage_live_locals; -use rustc_mir_dataflow::{Analysis, ResultsCursor}; use rustc_target::abi::{Size, FIRST_VARIANT}; use rustc_target::spec::abi::Abi; @@ -51,12 +48,6 @@ impl<'tcx> MirPass<'tcx> for Validator { Reveal::All => tcx.param_env_reveal_all_normalized(def_id), }; - let always_live_locals = always_storage_live_locals(body); - let storage_liveness = MaybeStorageLive::new(std::borrow::Cow::Owned(always_live_locals)) - .into_engine(tcx, body) - .iterate_to_fixpoint() - .into_results_cursor(body); - let can_unwind = if mir_phase <= MirPhase::Runtime(RuntimePhase::Initial) { // In this case `AbortUnwindingCalls` haven't yet been executed. true @@ -83,7 +74,6 @@ impl<'tcx> MirPass<'tcx> for Validator { mir_phase, unwind_edge_count: 0, reachable_blocks: traversal::reachable_as_bitset(body), - storage_liveness, place_cache: FxHashSet::default(), value_cache: FxHashSet::default(), can_unwind, @@ -116,7 +106,6 @@ struct CfgChecker<'a, 'tcx> { mir_phase: MirPhase, unwind_edge_count: usize, reachable_blocks: BitSet<BasicBlock>, - storage_liveness: ResultsCursor<'a, 'tcx, MaybeStorageLive<'static>>, place_cache: FxHashSet<PlaceRef<'tcx>>, value_cache: FxHashSet<u128>, // If `false`, then the MIR must not contain `UnwindAction::Continue` or @@ -294,28 +283,13 @@ impl<'a, 'tcx> CfgChecker<'a, 'tcx> { } impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> { - fn visit_local(&mut self, local: Local, context: PlaceContext, location: Location) { + fn visit_local(&mut self, local: Local, _context: PlaceContext, location: Location) { if self.body.local_decls.get(local).is_none() { self.fail( location, format!("local {local:?} has no corresponding declaration in `body.local_decls`"), ); } - - if self.reachable_blocks.contains(location.block) && context.is_use() { - // We check that the local is live whenever it is used. Technically, violating this - // restriction is only UB and not actually indicative of not well-formed MIR. This means - // that an optimization which turns MIR that already has UB into MIR that fails this - // check is not necessarily wrong. However, we have no such optimizations at the moment, - // and so we include this check anyway to help us catch bugs. If you happen to write an - // optimization that might cause this to incorrectly fire, feel free to remove this - // check. - self.storage_liveness.seek_after_primary_effect(location); - let locals_with_storage = self.storage_liveness.get(); - if !locals_with_storage.contains(local) { - self.fail(location, format!("use of local {local:?}, which has no storage here")); - } - } } fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { @@ -367,26 +341,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> { self.fail(location, format!("explicit `{kind:?}` is forbidden")); } } - StatementKind::StorageLive(local) => { - // We check that the local is not live when entering a `StorageLive` for it. - // Technically, violating this restriction is only UB and not actually indicative - // of not well-formed MIR. This means that an optimization which turns MIR that - // already has UB into MIR that fails this check is not necessarily wrong. However, - // we have no such optimizations at the moment, and so we include this check anyway - // to help us catch bugs. If you happen to write an optimization that might cause - // this to incorrectly fire, feel free to remove this check. - if self.reachable_blocks.contains(location.block) { - self.storage_liveness.seek_before_primary_effect(location); - let locals_with_storage = self.storage_liveness.get(); - if locals_with_storage.contains(*local) { - self.fail( - location, - format!("StorageLive({local:?}) which already has storage here"), - ); - } - } - } - StatementKind::StorageDead(_) + StatementKind::StorageLive(_) + | StatementKind::StorageDead(_) | StatementKind::Intrinsic(_) | StatementKind::Coverage(_) | StatementKind::ConstEvalCounter diff --git a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs index 646c70eb88f..595c2ff5bf7 100644 --- a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs +++ b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs @@ -81,17 +81,17 @@ impl<'tcx, 'a> crate::GenKillAnalysis<'tcx> for MaybeStorageLive<'a> { } #[derive(Clone)] -pub struct MaybeStorageDead { - always_live_locals: BitSet<Local>, +pub struct MaybeStorageDead<'a> { + always_live_locals: Cow<'a, BitSet<Local>>, } -impl MaybeStorageDead { - pub fn new(always_live_locals: BitSet<Local>) -> Self { +impl<'a> MaybeStorageDead<'a> { + pub fn new(always_live_locals: Cow<'a, BitSet<Local>>) -> Self { MaybeStorageDead { always_live_locals } } } -impl<'tcx> crate::AnalysisDomain<'tcx> for MaybeStorageDead { +impl<'tcx, 'a> crate::AnalysisDomain<'tcx> for MaybeStorageDead<'a> { type Domain = BitSet<Local>; const NAME: &'static str = "maybe_storage_dead"; @@ -112,7 +112,7 @@ impl<'tcx> crate::AnalysisDomain<'tcx> for MaybeStorageDead { } } -impl<'tcx> crate::GenKillAnalysis<'tcx> for MaybeStorageDead { +impl<'tcx, 'a> crate::GenKillAnalysis<'tcx> for MaybeStorageDead<'a> { type Idx = Local; fn domain_size(&self, body: &Body<'tcx>) -> usize { diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index 89e897191e8..98d4d96d0c7 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -86,6 +86,7 @@ pub mod inline; mod instsimplify; mod jump_threading; mod large_enums; +mod lint; mod lower_intrinsics; mod lower_slice_len; mod match_branches; diff --git a/compiler/rustc_mir_transform/src/lint.rs b/compiler/rustc_mir_transform/src/lint.rs new file mode 100644 index 00000000000..3940d0ddbf3 --- /dev/null +++ b/compiler/rustc_mir_transform/src/lint.rs @@ -0,0 +1,119 @@ +//! This pass statically detects code which has undefined behaviour or is likely to be erroneous. +//! It can be used to locate problems in MIR building or optimizations. It assumes that all code +//! can be executed, so it has false positives. +use rustc_index::bit_set::BitSet; +use rustc_middle::mir::visit::{PlaceContext, Visitor}; +use rustc_middle::mir::*; +use rustc_middle::ty::TyCtxt; +use rustc_mir_dataflow::impls::{MaybeStorageDead, MaybeStorageLive}; +use rustc_mir_dataflow::storage::always_storage_live_locals; +use rustc_mir_dataflow::{Analysis, ResultsCursor}; +use std::borrow::Cow; + +pub fn lint_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, when: String) { + let reachable_blocks = traversal::reachable_as_bitset(body); + let always_live_locals = &always_storage_live_locals(body); + + let maybe_storage_live = MaybeStorageLive::new(Cow::Borrowed(always_live_locals)) + .into_engine(tcx, body) + .iterate_to_fixpoint() + .into_results_cursor(body); + + let maybe_storage_dead = MaybeStorageDead::new(Cow::Borrowed(always_live_locals)) + .into_engine(tcx, body) + .iterate_to_fixpoint() + .into_results_cursor(body); + + Lint { + tcx, + when, + body, + is_fn_like: tcx.def_kind(body.source.def_id()).is_fn_like(), + always_live_locals, + reachable_blocks, + maybe_storage_live, + maybe_storage_dead, + } + .visit_body(body); +} + +struct Lint<'a, 'tcx> { + tcx: TyCtxt<'tcx>, + when: String, + body: &'a Body<'tcx>, + is_fn_like: bool, + always_live_locals: &'a BitSet<Local>, + reachable_blocks: BitSet<BasicBlock>, + maybe_storage_live: ResultsCursor<'a, 'tcx, MaybeStorageLive<'a>>, + maybe_storage_dead: ResultsCursor<'a, 'tcx, MaybeStorageDead<'a>>, +} + +impl<'a, 'tcx> Lint<'a, 'tcx> { + #[track_caller] + fn fail(&self, location: Location, msg: impl AsRef<str>) { + let span = self.body.source_info(location).span; + self.tcx.sess.dcx().span_delayed_bug( + span, + format!( + "broken MIR in {:?} ({}) at {:?}:\n{}", + self.body.source.instance, + self.when, + location, + msg.as_ref() + ), + ); + } +} + +impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> { + fn visit_local(&mut self, local: Local, context: PlaceContext, location: Location) { + if self.reachable_blocks.contains(location.block) && context.is_use() { + self.maybe_storage_dead.seek_after_primary_effect(location); + if self.maybe_storage_dead.get().contains(local) { + self.fail(location, format!("use of local {local:?}, which has no storage here")); + } + } + } + + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { + match statement.kind { + StatementKind::StorageLive(local) => { + if self.reachable_blocks.contains(location.block) { + self.maybe_storage_live.seek_before_primary_effect(location); + if self.maybe_storage_live.get().contains(local) { + self.fail( + location, + format!("StorageLive({local:?}) which already has storage here"), + ); + } + } + } + _ => {} + } + + self.super_statement(statement, location); + } + + fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { + match terminator.kind { + TerminatorKind::Return => { + if self.is_fn_like && self.reachable_blocks.contains(location.block) { + self.maybe_storage_live.seek_after_primary_effect(location); + for local in self.maybe_storage_live.get().iter() { + if !self.always_live_locals.contains(local) { + self.fail( + location, + format!( + "local {local:?} still has storage when returning from function" + ), + ); + } + } + } + } + _ => {} + } + + self.super_terminator(terminator, location); + } +} diff --git a/compiler/rustc_mir_transform/src/pass_manager.rs b/compiler/rustc_mir_transform/src/pass_manager.rs index c4eca18ff27..1da1c1920b2 100644 --- a/compiler/rustc_mir_transform/src/pass_manager.rs +++ b/compiler/rustc_mir_transform/src/pass_manager.rs @@ -2,7 +2,7 @@ use rustc_middle::mir::{self, Body, MirPhase, RuntimePhase}; use rustc_middle::ty::TyCtxt; use rustc_session::Session; -use crate::{validate, MirPass}; +use crate::{lint::lint_body, validate, MirPass}; /// Just like `MirPass`, except it cannot mutate `Body`. pub trait MirLint<'tcx> { @@ -109,6 +109,7 @@ fn run_passes_inner<'tcx>( phase_change: Option<MirPhase>, validate_each: bool, ) { + let lint = tcx.sess.opts.unstable_opts.lint_mir & !body.should_skip(); let validate = validate_each & tcx.sess.opts.unstable_opts.validate_mir & !body.should_skip(); let overridden_passes = &tcx.sess.opts.unstable_opts.mir_enable_passes; trace!(?overridden_passes); @@ -131,6 +132,9 @@ fn run_passes_inner<'tcx>( if validate { validate_body(tcx, body, format!("before pass {name}")); } + if lint { + lint_body(tcx, body, format!("before pass {name}")); + } if let Some(prof_arg) = &prof_arg { tcx.sess @@ -147,6 +151,9 @@ fn run_passes_inner<'tcx>( if validate { validate_body(tcx, body, format!("after pass {name}")); } + if lint { + lint_body(tcx, body, format!("after pass {name}")); + } body.pass_count += 1; } @@ -164,6 +171,9 @@ fn run_passes_inner<'tcx>( if validate || new_phase == MirPhase::Runtime(RuntimePhase::Optimized) { validate_body(tcx, body, format!("after phase change to {}", new_phase.name())); } + if lint { + lint_body(tcx, body, format!("after phase change to {}", new_phase.name())); + } body.pass_count = 1; } diff --git a/compiler/rustc_mir_transform/src/ref_prop.rs b/compiler/rustc_mir_transform/src/ref_prop.rs index f13ab5b0f1f..05a3ac3cc75 100644 --- a/compiler/rustc_mir_transform/src/ref_prop.rs +++ b/compiler/rustc_mir_transform/src/ref_prop.rs @@ -7,6 +7,7 @@ use rustc_middle::ty::TyCtxt; use rustc_mir_dataflow::impls::MaybeStorageDead; use rustc_mir_dataflow::storage::always_storage_live_locals; use rustc_mir_dataflow::Analysis; +use std::borrow::Cow; use crate::ssa::{SsaLocals, StorageLiveLocals}; @@ -120,7 +121,7 @@ fn compute_replacement<'tcx>( // Compute `MaybeStorageDead` dataflow to check that we only replace when the pointee is // definitely live. - let mut maybe_dead = MaybeStorageDead::new(always_live_locals) + let mut maybe_dead = MaybeStorageDead::new(Cow::Owned(always_live_locals)) .into_engine(tcx, body) .iterate_to_fixpoint() .into_results_cursor(body); diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index 06b554e8e63..4626da52850 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -1694,6 +1694,8 @@ options! { "link native libraries in the linker invocation (default: yes)"), link_only: bool = (false, parse_bool, [TRACKED], "link the `.rlink` file generated by `-Z no-link` (default: no)"), + lint_mir: bool = (false, parse_bool, [UNTRACKED], + "lint MIR before and after each transformation"), llvm_module_flag: Vec<(String, u32, String)> = (Vec::new(), parse_llvm_module_flag, [TRACKED], "a list of module flags to pass to LLVM (space separated)"), llvm_plugins: Vec<String> = (Vec::new(), parse_list, [TRACKED], diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 5d53a4d28f2..ca80328f3ac 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -2445,6 +2445,7 @@ impl<'test> TestCx<'test> { "-Copt-level=1", &zdump_arg, "-Zvalidate-mir", + "-Zlint-mir", "-Zdump-mir-exclude-pass-number", ]); if let Some(pass) = &self.props.mir_unit_test { diff --git a/tests/mir-opt/reference_prop.rs b/tests/mir-opt/reference_prop.rs index 1b9c8fe15c2..8adfbb4535b 100644 --- a/tests/mir-opt/reference_prop.rs +++ b/tests/mir-opt/reference_prop.rs @@ -1,3 +1,4 @@ +// compile-flags: -Zlint-mir=no // unit-test: ReferencePropagation // needs-unwind diff --git a/tests/ui/mir/lint/no-storage.rs b/tests/ui/mir/lint/no-storage.rs new file mode 100644 index 00000000000..246a0b34e5d --- /dev/null +++ b/tests/ui/mir/lint/no-storage.rs @@ -0,0 +1,30 @@ +// compile-flags: -Zlint-mir --crate-type=lib -Ztreat-err-as-bug +// failure-status: 101 +// dont-check-compiler-stderr +// regex-error-pattern: use of local .*, which has no storage here +#![feature(custom_mir, core_intrinsics)] +extern crate core; +use core::intrinsics::mir::*; + +#[custom_mir(dialect = "built")] +pub fn f(a: bool) { + mir!( + let b: (); + { + match a { true => bb1, _ => bb2 } + } + bb1 = { + StorageLive(b); + Goto(bb3) + } + bb2 = { + Goto(bb3) + } + bb3 = { + b = (); + RET = b; + StorageDead(b); + Return() + } + ) +} diff --git a/tests/ui/mir/validate/storage-live.rs b/tests/ui/mir/lint/storage-live.rs index ed3c26ed6da..f34a2c7de37 100644 --- a/tests/ui/mir/validate/storage-live.rs +++ b/tests/ui/mir/lint/storage-live.rs @@ -1,4 +1,4 @@ -// compile-flags: -Zvalidate-mir -Ztreat-err-as-bug +// compile-flags: -Zlint-mir -Ztreat-err-as-bug // failure-status: 101 // error-pattern: broken MIR in // error-pattern: StorageLive(_1) which already has storage here diff --git a/tests/ui/mir/validate/storage-live.stderr b/tests/ui/mir/lint/storage-live.stderr index 1037ddc88ef..1037ddc88ef 100644 --- a/tests/ui/mir/validate/storage-live.stderr +++ b/tests/ui/mir/lint/storage-live.stderr diff --git a/tests/ui/mir/lint/storage-return.rs b/tests/ui/mir/lint/storage-return.rs new file mode 100644 index 00000000000..a2f63b449b4 --- /dev/null +++ b/tests/ui/mir/lint/storage-return.rs @@ -0,0 +1,19 @@ +// compile-flags: -Zlint-mir -Ztreat-err-as-bug +// failure-status: 101 +// dont-check-compiler-stderr +// error-pattern: has storage when returning +#![feature(custom_mir, core_intrinsics)] +extern crate core; +use core::intrinsics::mir::*; + +#[custom_mir(dialect = "built")] +fn main() { + mir!( + let a: (); + { + StorageLive(a); + RET = a; + Return() + } + ) +} |