From 7e2fc9f90b9348326cf78baf93e2871cc24c3753 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 30 Sep 2020 14:29:05 -0700 Subject: [PATCH 1/3] Mark inactive enum variants along "otherwise" edge as unintitialized This means all enum variants whose discriminant has a dedicated outgoing edge in the `SwitchInt` terminator. --- compiler/rustc_mir/src/dataflow/impls/mod.rs | 171 +++++++++++-------- 1 file changed, 102 insertions(+), 69 deletions(-) diff --git a/compiler/rustc_mir/src/dataflow/impls/mod.rs b/compiler/rustc_mir/src/dataflow/impls/mod.rs index 185f0edfeb6bc..a391360727a7f 100644 --- a/compiler/rustc_mir/src/dataflow/impls/mod.rs +++ b/compiler/rustc_mir/src/dataflow/impls/mod.rs @@ -11,7 +11,7 @@ use super::MoveDataParamEnv; use crate::util::elaborate_drops::DropFlagState; -use super::move_paths::{HasMoveData, InitIndex, InitKind, MoveData, MovePathIndex}; +use super::move_paths::{HasMoveData, InitIndex, InitKind, LookupResult, MoveData, MovePathIndex}; use super::{lattice, AnalysisDomain, GenKill, GenKillAnalysis}; use super::drop_flag_effects_for_function_entry; @@ -362,40 +362,17 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { return; } - let enum_ = discr.place().and_then(|discr| { - switch_on_enum_discriminant(self.tcx, &self.body, &self.body[block], discr) - }); - - let (enum_place, enum_def) = match enum_ { - Some(x) => x, - None => return, - }; - - let mut discriminants = enum_def.discriminants(self.tcx); - edge_effects.apply(|trans, edge| { - let value = match edge.value { - Some(x) => x, - None => return, - }; - - // MIR building adds discriminants to the `values` array in the same order as they - // are yielded by `AdtDef::discriminants`. We rely on this to match each - // discriminant in `values` to its corresponding variant in linear time. - let (variant, _) = discriminants - .find(|&(_, discr)| discr.val == value) - .expect("Order of `AdtDef::discriminants` differed from `SwitchInt::values`"); - - // 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), - ); - }); + // Kill all move paths that correspond to variants we know to be inactive along a + // particular outgoing edge of a `SwitchInt`. + enum_discriminant_switch_inactive_variant_effects( + self.tcx, + self.body, + self.move_data(), + block, + discr, + edge_effects, + |trans, mpi| trans.kill(mpi), + ); } } @@ -481,40 +458,17 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> { return; } - let enum_ = discr.place().and_then(|discr| { - switch_on_enum_discriminant(self.tcx, &self.body, &self.body[block], discr) - }); - - let (enum_place, enum_def) = match enum_ { - Some(x) => x, - None => return, - }; - - let mut discriminants = enum_def.discriminants(self.tcx); - edge_effects.apply(|trans, edge| { - let value = match edge.value { - Some(x) => x, - None => return, - }; - - // MIR building adds discriminants to the `values` array in the same order as they - // are yielded by `AdtDef::discriminants`. We rely on this to match each - // discriminant in `values` to its corresponding variant in linear time. - let (variant, _) = discriminants - .find(|&(_, discr)| discr.val == value) - .expect("Order of `AdtDef::discriminants` differed from `SwitchInt::values`"); - - // 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), - ); - }); + // Mark all move paths that correspond to variants that are inactive along a given edge as + // maybe uninitialized (in reality, they are *definitely* uninitialized). + enum_discriminant_switch_inactive_variant_effects( + self.tcx, + self.body, + self.move_data(), + block, + discr, + edge_effects, + |trans, mpi| trans.gen(mpi), + ); } } @@ -715,3 +669,82 @@ fn switch_on_enum_discriminant( _ => None, } } + +fn enum_discriminant_switch_inactive_variant_effects( + tcx: TyCtxt<'tcx>, + body: &mir::Body<'tcx>, + move_data: &MoveData<'tcx>, + block: mir::BasicBlock, + discr: &mir::Operand<'tcx>, + edge_effects: &mut impl SwitchIntEdgeEffects, + mut on_uninitialized_variant: impl FnMut(&mut D, MovePathIndex), +) { + let enum_ = + discr.place().and_then(|discr| switch_on_enum_discriminant(tcx, body, &body[block], discr)); + + let (enum_place, enum_def) = match enum_ { + Some(x) => x, + None => return, + }; + + 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]; + let mut discriminants = enum_def.discriminants(tcx); + + // `MovePathIndex`s for those variants with their own outgoing edge. These + // get marked as uninitialized along the "otherwise" edge. + let mut variant_mpis_with_edge = vec![]; + + edge_effects.apply(|trans, edge| { + if let Some(value) = edge.value { + // MIR building adds discriminants to the `values` array in the same order as they + // are yielded by `AdtDef::discriminants`. We rely on this to match each + // discriminant in `values` to its corresponding variant in linear time. + let (active_variant, _) = discriminants + .find(|&(_, discr)| discr.val == value) + .expect("Order of `AdtDef::discriminants` differed from `SwitchInt::values`"); + + 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 { + // If this is the move path for the variant that is active along this edge + // of the `SwitchInt` terminator, remember that it is not active as part of + // the "otherwise" edge. + variant_mpis_with_edge.push(variant_mpi); + } else { + // Otherwise, it is the move path for an *inactive* variant. Mark it and + // all of its descendants as uninitialzied. + drop_flag_effects::on_all_children_bits( + tcx, + body, + move_data, + variant_mpi, + |mpi| on_uninitialized_variant(trans, mpi), + ); + } + } + } else { + // We're on the otherwise branch. All variants that we visited above are known to be + // uninitialized. + for &variant_mpi in &variant_mpis_with_edge { + drop_flag_effects::on_all_children_bits(tcx, body, move_data, variant_mpi, |mpi| { + on_uninitialized_variant(trans, mpi) + }); + } + } + }); +} From d5ee7236bb7afaee92816fb6b3a482e551c5e809 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 30 Sep 2020 14:32:39 -0700 Subject: [PATCH 2/3] Remove unused helper function --- .../src/dataflow/drop_flag_effects.rs | 40 ------------------- 1 file changed, 40 deletions(-) diff --git a/compiler/rustc_mir/src/dataflow/drop_flag_effects.rs b/compiler/rustc_mir/src/dataflow/drop_flag_effects.rs index d1d507e54ef5f..51a8c3c7b21b8 100644 --- a/compiler/rustc_mir/src/dataflow/drop_flag_effects.rs +++ b/compiler/rustc_mir/src/dataflow/drop_flag_effects.rs @@ -1,7 +1,6 @@ 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}; @@ -229,42 +228,3 @@ 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) - }); - } - } -} From fac0ad6035c95a69743b0053f54bed6f2a9fd9ae Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 30 Sep 2020 14:52:17 -0700 Subject: [PATCH 3/3] Bless MIR test --- .../issue_41888.main.ElaborateDrops.after.mir | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/test/mir-opt/issue_41888.main.ElaborateDrops.after.mir b/src/test/mir-opt/issue_41888.main.ElaborateDrops.after.mir index 84ba4d78ba041..d74e7d093b120 100644 --- a/src/test/mir-opt/issue_41888.main.ElaborateDrops.after.mir +++ b/src/test/mir-opt/issue_41888.main.ElaborateDrops.after.mir @@ -9,9 +9,8 @@ fn main() -> () { let mut _5: isize; // in scope 0 at $DIR/issue-41888.rs:10:16: 10:24 let mut _7: bool; // in scope 0 at $DIR/issue-41888.rs:15:1: 15:2 let mut _8: bool; // in scope 0 at $DIR/issue-41888.rs:15:1: 15:2 - let mut _9: bool; // in scope 0 at $DIR/issue-41888.rs:15:1: 15:2 + let mut _9: isize; // in scope 0 at $DIR/issue-41888.rs:15:1: 15:2 let mut _10: isize; // in scope 0 at $DIR/issue-41888.rs:15:1: 15:2 - let mut _11: isize; // in scope 0 at $DIR/issue-41888.rs:15:1: 15:2 scope 1 { debug e => _1; // in scope 1 at $DIR/issue-41888.rs:7:9: 7:10 let _6: K; // in scope 1 at $DIR/issue-41888.rs:10:21: 10:23 @@ -21,7 +20,6 @@ fn main() -> () { } bb0: { - _9 = const false; // scope 0 at $DIR/issue-41888.rs:7:9: 7:10 _7 = const false; // scope 0 at $DIR/issue-41888.rs:7:9: 7:10 _8 = const false; // scope 0 at $DIR/issue-41888.rs:7:9: 7:10 StorageLive(_1); // scope 0 at $DIR/issue-41888.rs:7:9: 7:10 @@ -79,7 +77,6 @@ fn main() -> () { bb10: { StorageLive(_6); // scope 1 at $DIR/issue-41888.rs:10:21: 10:23 - _9 = const false; // scope 1 at $DIR/issue-41888.rs:10:21: 10:23 _6 = move ((_1 as F).0: K); // scope 1 at $DIR/issue-41888.rs:10:21: 10:23 _0 = const (); // scope 2 at $DIR/issue-41888.rs:10:29: 13:10 StorageDead(_6); // scope 1 at $DIR/issue-41888.rs:13:9: 13:10 @@ -93,7 +90,6 @@ fn main() -> () { bb12: { _7 = const false; // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 _8 = const false; // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 - _9 = const false; // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 StorageDead(_1); // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 StorageDead(_2); // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 return; // scope 0 at $DIR/issue-41888.rs:15:2: 15:2 @@ -102,7 +98,6 @@ fn main() -> () { bb13 (cleanup): { _7 = const true; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 _8 = const true; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 - _9 = const true; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 _1 = move _3; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 goto -> bb7; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 } @@ -110,7 +105,6 @@ fn main() -> () { bb14: { _7 = const true; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 _8 = const true; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 - _9 = const true; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 _1 = move _3; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 goto -> bb6; // scope 1 at $DIR/issue-41888.rs:9:9: 9:10 } @@ -138,8 +132,8 @@ fn main() -> () { } bb20: { - _10 = discriminant(_1); // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 - switchInt(move _10) -> [0_isize: bb15, otherwise: bb18]; // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 + _9 = discriminant(_1); // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 + switchInt(move _9) -> [0_isize: bb15, otherwise: bb18]; // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 } bb21: { @@ -147,8 +141,8 @@ fn main() -> () { } bb22 (cleanup): { - _11 = discriminant(_1); // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 - switchInt(move _11) -> [0_isize: bb17, otherwise: bb19]; // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 + _10 = discriminant(_1); // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 + switchInt(move _10) -> [0_isize: bb17, otherwise: bb19]; // scope 0 at $DIR/issue-41888.rs:15:1: 15:2 } bb23 (cleanup): {