Skip to content

Commit

Permalink
Auto merge of #72093 - jonas-schievink:unmut, r=oli-obk
Browse files Browse the repository at this point in the history
Avoid `Operand::Copy` with `&mut T`

This is generally unsound to do, as the copied type is assumed to implement
`Copy`.

Closes #46420
  • Loading branch information
bors committed May 26, 2020
2 parents 9eedd13 + fe1753a commit cb272d5
Show file tree
Hide file tree
Showing 8 changed files with 116 additions and 11 deletions.
1 change: 1 addition & 0 deletions src/librustc_interface/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/shim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
19 changes: 12 additions & 7 deletions src/librustc_mir/transform/instcombine.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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) => {
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
}
}
}
Expand All @@ -109,6 +114,6 @@ impl Visitor<'tcx> for OptimizationFinder<'b, 'tcx> {

#[derive(Default)]
struct OptimizationList<'tcx> {
and_stars: FxHashSet<Location>,
and_stars: FxHashMap<Location, Mutability>,
arrays_lengths: FxHashMap<Location, Constant<'tcx>>,
}
19 changes: 18 additions & 1 deletion src/librustc_mir/transform/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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| {
Expand All @@ -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;
};

Expand All @@ -179,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 {
Expand Down
80 changes: 80 additions & 0 deletions src/librustc_mir/transform/validate.rs
Original file line number Diff line number Diff line change
@@ -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<str>) {
// 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
),
);
}
}
_ => {}
}
}
}
}
2 changes: 2 additions & 0 deletions src/librustc_session/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"adds unstable command line options to rustc interface (default: no)"),
use_ctors_section: Option<bool> = (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],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ fn b(_1: &mut std::boxed::Box<T>) -> &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
Expand Down

0 comments on commit cb272d5

Please sign in to comment.