From 4ca626258ae9a677c4d63fa278847eb90e514868 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sat, 23 May 2020 20:01:36 +0200 Subject: [PATCH 1/3] Avoid `Operand::Copy` with `&mut T` --- src/librustc_mir/shim.rs | 2 +- src/librustc_mir/transform/instcombine.rs | 19 ++++++++++++------- .../rustc.a.Inline.after.mir | 2 +- .../rustc.b.Inline.after.mir | 2 +- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index e3982c654d5fa..b439e919050c0 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -700,7 +700,7 @@ fn build_call_shim<'tcx>( let rcvr = rcvr_adjustment.map(|rcvr_adjustment| match rcvr_adjustment { Adjustment::Identity => Operand::Move(rcvr_place()), - Adjustment::Deref => Operand::Copy(tcx.mk_place_deref(rcvr_place())), + Adjustment::Deref => Operand::Move(tcx.mk_place_deref(rcvr_place())), // Can't copy `&mut` Adjustment::DerefMove => Operand::Move(tcx.mk_place_deref(rcvr_place())), Adjustment::RefMut => { // let rcvr = &mut rcvr; diff --git a/src/librustc_mir/transform/instcombine.rs b/src/librustc_mir/transform/instcombine.rs index dee37f767e979..a016892d982d1 100644 --- a/src/librustc_mir/transform/instcombine.rs +++ b/src/librustc_mir/transform/instcombine.rs @@ -1,11 +1,11 @@ //! Performs various peephole optimizations. use crate::transform::{MirPass, MirSource}; -use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::fx::FxHashMap; use rustc_index::vec::Idx; use rustc_middle::mir::visit::{MutVisitor, Visitor}; use rustc_middle::mir::{ - Body, Constant, Local, Location, Operand, Place, PlaceRef, ProjectionElem, Rvalue, + Body, Constant, Local, Location, Mutability, Operand, Place, PlaceRef, ProjectionElem, Rvalue, }; use rustc_middle::ty::{self, TyCtxt}; use std::mem; @@ -39,7 +39,7 @@ impl<'tcx> MutVisitor<'tcx> for InstCombineVisitor<'tcx> { } fn visit_rvalue(&mut self, rvalue: &mut Rvalue<'tcx>, location: Location) { - if self.optimizations.and_stars.remove(&location) { + if let Some(mtbl) = self.optimizations.and_stars.remove(&location) { debug!("replacing `&*`: {:?}", rvalue); let new_place = match rvalue { Rvalue::Ref(_, _, place) => { @@ -57,7 +57,10 @@ impl<'tcx> MutVisitor<'tcx> for InstCombineVisitor<'tcx> { } _ => bug!("Detected `&*` but didn't find `&*`!"), }; - *rvalue = Rvalue::Use(Operand::Copy(new_place)) + *rvalue = Rvalue::Use(match mtbl { + Mutability::Mut => Operand::Move(new_place), + Mutability::Not => Operand::Copy(new_place), + }); } if let Some(constant) = self.optimizations.arrays_lengths.remove(&location) { @@ -88,8 +91,10 @@ impl Visitor<'tcx> for OptimizationFinder<'b, 'tcx> { if let PlaceRef { local, projection: &[ref proj_base @ .., ProjectionElem::Deref] } = place.as_ref() { - if Place::ty_from(local, proj_base, self.body, self.tcx).ty.is_region_ptr() { - self.optimizations.and_stars.insert(location); + // The dereferenced place must have type `&_`. + let ty = Place::ty_from(local, proj_base, self.body, self.tcx).ty; + if let ty::Ref(_, _, mtbl) = ty.kind { + self.optimizations.and_stars.insert(location, mtbl); } } } @@ -109,6 +114,6 @@ impl Visitor<'tcx> for OptimizationFinder<'b, 'tcx> { #[derive(Default)] struct OptimizationList<'tcx> { - and_stars: FxHashSet, + and_stars: FxHashMap, arrays_lengths: FxHashMap>, } diff --git a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.a.Inline.after.mir b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.a.Inline.after.mir index 2eebf3f0eceb9..8751469d265a2 100644 --- a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.a.Inline.after.mir +++ b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.a.Inline.after.mir @@ -15,7 +15,7 @@ fn a(_1: &mut [T]) -> &mut [T] { StorageLive(_3); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:15 StorageLive(_4); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:6 _4 = &mut (*_1); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:6 - _3 = _4; // scope 1 at $SRC_DIR/libcore/convert/mod.rs:LL:COL + _3 = move _4; // scope 1 at $SRC_DIR/libcore/convert/mod.rs:LL:COL _2 = &mut (*_3); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:15 StorageDead(_4); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:14: 3:15 _0 = &mut (*_2); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:15 diff --git a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.b.Inline.after.mir b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.b.Inline.after.mir index f9e1699c55dfa..743da27a049f9 100644 --- a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.b.Inline.after.mir +++ b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.b.Inline.after.mir @@ -18,7 +18,7 @@ fn b(_1: &mut std::boxed::Box) -> &mut T { _4 = &mut (*_1); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:8:5: 8:6 StorageLive(_5); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL _5 = &mut (*(*_4)); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL - _3 = _5; // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL + _3 = move _5; // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL StorageDead(_5); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL _2 = &mut (*_3); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:8:5: 8:15 StorageDead(_4); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:8:14: 8:15 From e04318e0fa5429e1ac44b624b9a775cfeeed66ed Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 24 May 2020 00:55:44 +0200 Subject: [PATCH 2/3] Add a small MIR validation pass --- src/librustc_interface/tests.rs | 1 + src/librustc_mir/transform/mod.rs | 14 ++++- src/librustc_mir/transform/validate.rs | 80 ++++++++++++++++++++++++++ src/librustc_session/options.rs | 2 + 4 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 src/librustc_mir/transform/validate.rs diff --git a/src/librustc_interface/tests.rs b/src/librustc_interface/tests.rs index 0394821d09569..d573e11fc4b24 100644 --- a/src/librustc_interface/tests.rs +++ b/src/librustc_interface/tests.rs @@ -511,6 +511,7 @@ fn test_debugging_options_tracking_hash() { untracked!(ui_testing, true); untracked!(unpretty, Some("expanded".to_string())); untracked!(unstable_options, true); + untracked!(validate_mir, true); untracked!(verbose, true); macro_rules! tracked { diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 0551ed5a15ddb..95a5752348374 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -39,6 +39,7 @@ pub mod simplify_branches; pub mod simplify_try; pub mod uninhabited_enum_branching; pub mod unreachable_prop; +pub mod validate; pub(crate) fn provide(providers: &mut Providers<'_>) { self::check_unsafety::provide(providers); @@ -147,12 +148,18 @@ pub fn run_passes( passes: &[&[&dyn MirPass<'tcx>]], ) { let phase_index = mir_phase.phase_index(); + let source = MirSource { instance, promoted }; + let validate = tcx.sess.opts.debugging_opts.validate_mir; if body.phase >= mir_phase { return; } - let source = MirSource { instance, promoted }; + if validate { + validate::Validator { when: format!("input to phase {:?}", mir_phase) } + .run_pass(tcx, source, body); + } + let mut index = 0; let mut run_pass = |pass: &dyn MirPass<'tcx>| { let run_hooks = |body: &_, index, is_after| { @@ -169,6 +176,11 @@ pub fn run_passes( pass.run_pass(tcx, source, body); run_hooks(body, index, true); + if validate { + validate::Validator { when: format!("after {} in phase {:?}", pass.name(), mir_phase) } + .run_pass(tcx, source, body); + } + index += 1; }; diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs new file mode 100644 index 0000000000000..a25edd131baa1 --- /dev/null +++ b/src/librustc_mir/transform/validate.rs @@ -0,0 +1,80 @@ +//! Validates the MIR to ensure that invariants are upheld. + +use super::{MirPass, MirSource}; +use rustc_middle::mir::visit::Visitor; +use rustc_middle::{ + mir::{Body, Location, Operand, Rvalue, Statement, StatementKind}, + ty::{ParamEnv, TyCtxt}, +}; +use rustc_span::{def_id::DefId, Span, DUMMY_SP}; + +pub struct Validator { + /// Describes at which point in the pipeline this validation is happening. + pub when: String, +} + +impl<'tcx> MirPass<'tcx> for Validator { + fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) { + let def_id = source.def_id(); + let param_env = tcx.param_env(def_id); + TypeChecker { when: &self.when, def_id, body, tcx, param_env }.visit_body(body); + } +} + +struct TypeChecker<'a, 'tcx> { + when: &'a str, + def_id: DefId, + body: &'a Body<'tcx>, + tcx: TyCtxt<'tcx>, + param_env: ParamEnv<'tcx>, +} + +impl<'a, 'tcx> TypeChecker<'a, 'tcx> { + fn fail(&self, span: Span, msg: impl AsRef) { + // We use `delay_span_bug` as we might see broken MIR when other errors have already + // occurred. + self.tcx.sess.diagnostic().delay_span_bug( + span, + &format!("broken MIR in {:?} ({}): {}", self.def_id, self.when, msg.as_ref()), + ); + } +} + +impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { + fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) { + // `Operand::Copy` is only supposed to be used with `Copy` types. + if let Operand::Copy(place) = operand { + let ty = place.ty(&self.body.local_decls, self.tcx).ty; + + if !ty.is_copy_modulo_regions(self.tcx, self.param_env, DUMMY_SP) { + self.fail( + DUMMY_SP, + format!("`Operand::Copy` with non-`Copy` type {} at {:?}", ty, location), + ); + } + } + + self.super_operand(operand, location); + } + + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { + // The sides of an assignment must not alias. Currently this just checks whether the places + // are identical. + if let StatementKind::Assign(box (dest, rvalue)) = &statement.kind { + match rvalue { + Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => { + if dest == src { + self.fail( + DUMMY_SP, + format!( + "encountered `Assign` statement with overlapping memory at {:?}", + location + ), + ); + } + } + _ => {} + } + } + } +} diff --git a/src/librustc_session/options.rs b/src/librustc_session/options.rs index 3b6c21e7de008..95f9ff00fb8d4 100644 --- a/src/librustc_session/options.rs +++ b/src/librustc_session/options.rs @@ -1045,6 +1045,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "adds unstable command line options to rustc interface (default: no)"), use_ctors_section: Option = (None, parse_opt_bool, [TRACKED], "use legacy .ctors section for initializers rather than .init_array"), + validate_mir: bool = (false, parse_bool, [UNTRACKED], + "validate MIR after each transformation"), verbose: bool = (false, parse_bool, [UNTRACKED], "in general, enable more debug printouts (default: no)"), verify_llvm_ir: bool = (false, parse_bool, [TRACKED], From fe1753af840527bb2beba3ee603971312299b2e7 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Mon, 25 May 2020 22:04:48 +0200 Subject: [PATCH 3/3] Always validate MIR after optimizing --- src/librustc_mir/transform/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 95a5752348374..af9436d404180 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -191,6 +191,11 @@ pub fn run_passes( } body.phase = mir_phase; + + if mir_phase == MirPhase::Optimized { + validate::Validator { when: format!("end of phase {:?}", mir_phase) } + .run_pass(tcx, source, body); + } } fn mir_const_qualif(tcx: TyCtxt<'_>, def_id: DefId) -> ConstQualifs {