From ca8678b23e5f49c65da1e0d2aa01d3fa0d5178e9 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Mon, 29 Jun 2020 17:20:41 -0700 Subject: [PATCH 1/2] Handle inactive enum variants in `MaybeUninitializedPlaces` --- .../dataflow/drop_flag_effects.rs | 40 +++++++++++ src/librustc_mir/dataflow/impls/mod.rs | 71 +++++++++++++------ src/librustc_mir/transform/elaborate_drops.rs | 1 + 3 files changed, 89 insertions(+), 23 deletions(-) diff --git a/src/librustc_mir/dataflow/drop_flag_effects.rs b/src/librustc_mir/dataflow/drop_flag_effects.rs index 6dd06743e2d5b..707e136678e9c 100644 --- a/src/librustc_mir/dataflow/drop_flag_effects.rs +++ b/src/librustc_mir/dataflow/drop_flag_effects.rs @@ -1,6 +1,7 @@ use crate::util::elaborate_drops::DropFlagState; use rustc_middle::mir::{self, Body, Location}; use rustc_middle::ty::{self, TyCtxt}; +use rustc_target::abi::VariantIdx; use super::indexes::MovePathIndex; use super::move_paths::{InitKind, LookupResult, MoveData}; @@ -228,3 +229,42 @@ pub(crate) fn for_location_inits<'tcx, F>( } } } + +/// Calls `handle_inactive_variant` for each descendant move path of `enum_place` that contains a +/// `Downcast` to a variant besides the `active_variant`. +/// +/// NOTE: If there are no move paths corresponding to an inactive variant, +/// `handle_inactive_variant` will not be called for that variant. +pub(crate) fn on_all_inactive_variants<'tcx>( + tcx: TyCtxt<'tcx>, + body: &mir::Body<'tcx>, + move_data: &MoveData<'tcx>, + enum_place: mir::Place<'tcx>, + active_variant: VariantIdx, + mut handle_inactive_variant: impl FnMut(MovePathIndex), +) { + let enum_mpi = match move_data.rev_lookup.find(enum_place.as_ref()) { + LookupResult::Exact(mpi) => mpi, + LookupResult::Parent(_) => return, + }; + + let enum_path = &move_data.move_paths[enum_mpi]; + for (variant_mpi, variant_path) in enum_path.children(&move_data.move_paths) { + // Because of the way we build the `MoveData` tree, each child should have exactly one more + // projection than `enum_place`. This additional projection must be a downcast since the + // base is an enum. + let (downcast, base_proj) = variant_path.place.projection.split_last().unwrap(); + assert_eq!(enum_place.projection.len(), base_proj.len()); + + let variant_idx = match *downcast { + mir::ProjectionElem::Downcast(_, idx) => idx, + _ => unreachable!(), + }; + + if variant_idx != active_variant { + on_all_children_bits(tcx, body, move_data, variant_mpi, |mpi| { + handle_inactive_variant(mpi) + }); + } + } +} diff --git a/src/librustc_mir/dataflow/impls/mod.rs b/src/librustc_mir/dataflow/impls/mod.rs index d5def0389126a..8975faec48765 100644 --- a/src/librustc_mir/dataflow/impls/mod.rs +++ b/src/librustc_mir/dataflow/impls/mod.rs @@ -12,7 +12,7 @@ use super::MoveDataParamEnv; use crate::util::elaborate_drops::DropFlagState; -use super::move_paths::{HasMoveData, InitIndex, InitKind, LookupResult, MoveData, MovePathIndex}; +use super::move_paths::{HasMoveData, InitIndex, InitKind, MoveData, MovePathIndex}; use super::{AnalysisDomain, BottomValue, GenKill, GenKillAnalysis}; use super::drop_flag_effects_for_function_entry; @@ -124,11 +124,23 @@ pub struct MaybeUninitializedPlaces<'a, 'tcx> { tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>, + + mark_inactive_variants_as_uninit: bool, } impl<'a, 'tcx> MaybeUninitializedPlaces<'a, 'tcx> { pub fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>) -> Self { - MaybeUninitializedPlaces { tcx, body, mdpe } + MaybeUninitializedPlaces { tcx, body, mdpe, mark_inactive_variants_as_uninit: false } + } + + /// Causes inactive enum variants to be marked as "maybe uninitialized" after a switch on an + /// enum discriminant. + /// + /// This is correct in a vacuum but is not the default because it causes problems in the borrow + /// checker, where this information gets propagated along `FakeEdge`s. + pub fn mark_inactive_variants_as_uninit(mut self) -> Self { + self.mark_inactive_variants_as_uninit = true; + self } } @@ -350,27 +362,16 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { _adt: &ty::AdtDef, variant: VariantIdx, ) { - let enum_mpi = match self.move_data().rev_lookup.find(enum_place.as_ref()) { - LookupResult::Exact(mpi) => mpi, - LookupResult::Parent(_) => return, - }; - - // Kill all move paths that correspond to variants other than this one - let move_paths = &self.move_data().move_paths; - let enum_path = &move_paths[enum_mpi]; - for (mpi, variant_path) in enum_path.children(move_paths) { - trans.kill(mpi); - match variant_path.place.projection.last().unwrap() { - mir::ProjectionElem::Downcast(_, idx) if *idx == variant => continue, - _ => drop_flag_effects::on_all_children_bits( - self.tcx, - self.body, - self.move_data(), - mpi, - |mpi| trans.kill(mpi), - ), - } - } + // Kill all move paths that correspond to variants we know to be inactive along this + // particular outgoing edge of a `SwitchInt`. + drop_flag_effects::on_all_inactive_variants( + self.tcx, + self.body, + self.move_data(), + enum_place, + variant, + |mpi| trans.kill(mpi), + ); } } @@ -443,6 +444,30 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> { }, ); } + + fn discriminant_switch_effect( + &self, + trans: &mut impl GenKill, + _block: mir::BasicBlock, + enum_place: mir::Place<'tcx>, + _adt: &ty::AdtDef, + variant: VariantIdx, + ) { + if !self.mark_inactive_variants_as_uninit { + return; + } + + // Mark all move paths that correspond to variants other than this one as maybe + // uninitialized (in reality, they are *definitely* uninitialized). + drop_flag_effects::on_all_inactive_variants( + self.tcx, + self.body, + self.move_data(), + enum_place, + variant, + |mpi| trans.gen(mpi), + ); + } } impl<'a, 'tcx> AnalysisDomain<'tcx> for DefinitelyInitializedPlaces<'a, 'tcx> { diff --git a/src/librustc_mir/transform/elaborate_drops.rs b/src/librustc_mir/transform/elaborate_drops.rs index 1704d8baabdc8..d3bfd872d16c2 100644 --- a/src/librustc_mir/transform/elaborate_drops.rs +++ b/src/librustc_mir/transform/elaborate_drops.rs @@ -48,6 +48,7 @@ impl<'tcx> MirPass<'tcx> for ElaborateDrops { .into_results_cursor(body); let uninits = MaybeUninitializedPlaces::new(tcx, body, &env) + .mark_inactive_variants_as_uninit() .into_engine(tcx, body, def_id) .dead_unwinds(&dead_unwinds) .iterate_to_fixpoint() From eb4d28bce0a73ddbdbcd107dd7875ddd97a603d8 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 30 Jun 2020 11:31:00 -0700 Subject: [PATCH 2/2] Bless mir-opt tests --- ...wrap.SimplifyCfg-elaborate-drops.after.mir | 22 +++++++++---------- .../32bit/rustc.map.SimplifyLocals.diff | 19 ++++++++++++++-- .../64bit/rustc.map.SimplifyLocals.diff | 19 ++++++++++++++-- 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/src/test/mir-opt/no-drop-for-inactive-variant/rustc.unwrap.SimplifyCfg-elaborate-drops.after.mir b/src/test/mir-opt/no-drop-for-inactive-variant/rustc.unwrap.SimplifyCfg-elaborate-drops.after.mir index ce987a57d5fc7..2e8cfaea937d7 100644 --- a/src/test/mir-opt/no-drop-for-inactive-variant/rustc.unwrap.SimplifyCfg-elaborate-drops.after.mir +++ b/src/test/mir-opt/no-drop-for-inactive-variant/rustc.unwrap.SimplifyCfg-elaborate-drops.after.mir @@ -8,22 +8,19 @@ fn unwrap(_1: std::option::Option) -> T { let mut _4: !; // in scope 0 at $SRC_DIR/libstd/macros.rs:LL:COL let mut _5: isize; // in scope 0 at $DIR/no-drop-for-inactive-variant.rs:12:1: 12:2 let mut _6: isize; // in scope 0 at $DIR/no-drop-for-inactive-variant.rs:12:1: 12:2 + let mut _7: isize; // in scope 0 at $DIR/no-drop-for-inactive-variant.rs:12:1: 12:2 scope 1 { debug x => _3; // in scope 1 at $DIR/no-drop-for-inactive-variant.rs:9:14: 9:15 } bb0: { _2 = discriminant(_1); // scope 0 at $DIR/no-drop-for-inactive-variant.rs:9:9: 9:16 - switchInt(move _2) -> [0_isize: bb2, 1_isize: bb4, otherwise: bb3]; // scope 0 at $DIR/no-drop-for-inactive-variant.rs:9:9: 9:16 + switchInt(move _2) -> [0_isize: bb1, 1_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/no-drop-for-inactive-variant.rs:9:9: 9:16 } - bb1 (cleanup): { - resume; // scope 0 at $DIR/no-drop-for-inactive-variant.rs:7:1: 12:2 - } - - bb2: { + bb1: { StorageLive(_4); // scope 0 at $SRC_DIR/libstd/macros.rs:LL:COL - const std::rt::begin_panic::<&str>(const "explicit panic") -> bb5; // scope 0 at $SRC_DIR/libstd/macros.rs:LL:COL + const std::rt::begin_panic::<&str>(const "explicit panic") -> bb4; // scope 0 at $SRC_DIR/libstd/macros.rs:LL:COL // ty::Const // + ty: fn(&str) -> ! {std::rt::begin_panic::<&str>} // + val: Value(Scalar()) @@ -38,20 +35,21 @@ fn unwrap(_1: std::option::Option) -> T { // + literal: Const { ty: &str, val: Value(Slice { data: Allocation { bytes: [101, 120, 112, 108, 105, 99, 105, 116, 32, 112, 97, 110, 105, 99], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [16383], len: Size { raw: 14 } }, size: Size { raw: 14 }, align: Align { pow2: 0 }, mutability: Not, extra: () }, start: 0, end: 14 }) } } - bb3: { + bb2: { unreachable; // scope 0 at $DIR/no-drop-for-inactive-variant.rs:8:11: 8:14 } - bb4: { + bb3: { StorageLive(_3); // scope 0 at $DIR/no-drop-for-inactive-variant.rs:9:14: 9:15 _3 = move ((_1 as Some).0: T); // scope 0 at $DIR/no-drop-for-inactive-variant.rs:9:14: 9:15 _0 = move _3; // scope 1 at $DIR/no-drop-for-inactive-variant.rs:9:20: 9:21 StorageDead(_3); // scope 0 at $DIR/no-drop-for-inactive-variant.rs:9:21: 9:22 - _5 = discriminant(_1); // scope 0 at $DIR/no-drop-for-inactive-variant.rs:12:1: 12:2 + _6 = discriminant(_1); // scope 0 at $DIR/no-drop-for-inactive-variant.rs:12:1: 12:2 return; // scope 0 at $DIR/no-drop-for-inactive-variant.rs:12:2: 12:2 } - bb5 (cleanup): { - drop(_1) -> bb1; // scope 0 at $DIR/no-drop-for-inactive-variant.rs:12:1: 12:2 + bb4 (cleanup): { + _5 = discriminant(_1); // scope 0 at $DIR/no-drop-for-inactive-variant.rs:12:1: 12:2 + resume; // scope 0 at $DIR/no-drop-for-inactive-variant.rs:7:1: 12:2 } } diff --git a/src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads/32bit/rustc.map.SimplifyLocals.diff b/src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads/32bit/rustc.map.SimplifyLocals.diff index f0b696118e996..01df5a2fce051 100644 --- a/src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads/32bit/rustc.map.SimplifyLocals.diff +++ b/src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads/32bit/rustc.map.SimplifyLocals.diff @@ -7,13 +7,28 @@ let mut _2: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 let _3: std::boxed::Box<()>; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:14: 4:15 - let mut _4: std::boxed::Box<()>; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:25: 4:26 -- let mut _5: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 +- let mut _5: bool; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 - let mut _6: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 +- let mut _7: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 scope 1 { debug x => _3; // in scope 1 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:14: 4:15 } bb0: { +- _5 = const false; // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 +- // ty::Const +- // + ty: bool +- // + val: Value(Scalar(0x00)) +- // mir::Constant +- // + span: $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 +- // + literal: Const { ty: bool, val: Value(Scalar(0x00)) } +- _5 = const true; // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 +- // ty::Const +- // + ty: bool +- // + val: Value(Scalar(0x01)) +- // mir::Constant +- // + span: $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 +- // + literal: Const { ty: bool, val: Value(Scalar(0x01)) } _2 = discriminant(_1); // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 } @@ -35,7 +50,7 @@ } bb3: { -- _5 = discriminant(_1); // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 +- _6 = discriminant(_1); // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 return; // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:2: 6:2 } } diff --git a/src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads/64bit/rustc.map.SimplifyLocals.diff b/src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads/64bit/rustc.map.SimplifyLocals.diff index 1ac6eb85441f5..e46b3067fc467 100644 --- a/src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads/64bit/rustc.map.SimplifyLocals.diff +++ b/src/test/mir-opt/simplify-locals-removes-unused-discriminant-reads/64bit/rustc.map.SimplifyLocals.diff @@ -7,13 +7,28 @@ let mut _2: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 let _3: std::boxed::Box<()>; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:14: 4:15 - let mut _4: std::boxed::Box<()>; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:25: 4:26 -- let mut _5: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 +- let mut _5: bool; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 - let mut _6: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 +- let mut _7: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 scope 1 { debug x => _3; // in scope 1 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:14: 4:15 } bb0: { +- _5 = const false; // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 +- // ty::Const +- // + ty: bool +- // + val: Value(Scalar(0x00)) +- // mir::Constant +- // + span: $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 +- // + literal: Const { ty: bool, val: Value(Scalar(0x00)) } +- _5 = const true; // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 +- // ty::Const +- // + ty: bool +- // + val: Value(Scalar(0x01)) +- // mir::Constant +- // + span: $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 +- // + literal: Const { ty: bool, val: Value(Scalar(0x01)) } _2 = discriminant(_1); // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:3:9: 3:13 } @@ -35,7 +50,7 @@ } bb3: { -- _5 = discriminant(_1); // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 +- _6 = discriminant(_1); // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2 return; // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:2: 6:2 } }