Auto merge of #73879 - ecstatic-morse:discr-switch-uninit, r=oli-obk

Handle inactive enum variants in `MaybeUninitializedPlaces` Resolves the first part of #69715. This is the equivalent of #68528 but for `MaybeUninitializedPlaces`. Because we now notify drop elaboration that inactive enum variants might be uninitialized, some drops get marked as ["open" that were previously "static"](https://github.com/rust-lang/rust/blob/e0e5d82e1677c82d209b214bbfc2cc5705c2336a/src/librustc_mir/transform/elaborate_drops.rs#L191). Unlike in #69715, this isn't strictly better: An "open" drop expands to more MIR than a simple call to the drop shim. However, because drop elaboration considers each field of an "open" drop separately, it can sometimes eliminate unnecessary drops of moved-from or unit-like enum variants. This is the case for `Option::unwrap`, which is reflected in the `mir-opt` test. cc @eddyb r? @oli-obk
rust-lang · Jul 5, 2020 · 2753fab · 2753fab
2 parents 0cd7ff7 + eb4d28b
commit 2753fab
Show file tree

Hide file tree

Showing 6 changed files with 133 additions and 39 deletions.
diff --git 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
@@ -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<Self::Idx>,
+        _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
@@ -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()

diff --git a/...t/mir-opt/no-drop-for-inactive-variant/rustc.unwrap.SimplifyCfg-elaborate-drops.after.mir b/...t/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>) -> 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(<ZST>))
@@ -38,20 +35,21 @@ fn unwrap(_1: std::option::Option<T>) -> 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/...opt/simplify-locals-removes-unused-discriminant-reads/32bit/rustc.map.SimplifyLocals.diff b/...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 => ((_0 as Some).0: std::boxed::Box<()>); // 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/...opt/simplify-locals-removes-unused-discriminant-reads/64bit/rustc.map.SimplifyLocals.diff b/...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 => ((_0 as Some).0: std::boxed::Box<()>); // 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
       }
   }