Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark inactive enum variants along "otherwise" edge as uninitialized #77377

Closed
wants to merge 3 commits into from
Closed

Mark inactive enum variants along "otherwise" edge as uninitialized #77377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e2fc9f
Mark inactive enum variants along "otherwise" edge as unintitialized
ecstatic-morse Sep 30, 2020
d5ee723
Remove unused helper function
ecstatic-morse Sep 30, 2020
fac0ad6
Bless MIR test
ecstatic-morse Sep 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 0 additions & 40 deletions compiler/rustc_mir/src/dataflow/drop_flag_effects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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)
});
}
}
}
171 changes: 102 additions & 69 deletions compiler/rustc_mir/src/dataflow/impls/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
);
}
}

Expand Down Expand Up @@ -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),
);
}
}

Expand Down Expand Up @@ -715,3 +669,82 @@ fn switch_on_enum_discriminant(
_ => None,
}
}

fn enum_discriminant_switch_inactive_variant_effects<D>(
tcx: TyCtxt<'tcx>,
body: &mir::Body<'tcx>,
move_data: &MoveData<'tcx>,
block: mir::BasicBlock,
discr: &mir::Operand<'tcx>,
edge_effects: &mut impl SwitchIntEdgeEffects<D>,
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a guarantee that the otherwise branch is always invoked last during the iteration performed by edge_effects.apply?

Because, as best as I can tell, it seems like you are relying here on such a guarantee (since you are accumulating all the values in variant_mpis_with_edge during the callback invocations on the non-otherwise edges.

But skimming over the doc comments added in PR #77242, I do not see any such guarantee in the documentation for apply_switch_int_edge_effects.

(And overall, I think the logic here would be easier to understand if the block structure in the code reflected that ordering constraint, even if it requires changes to the dataflow API. I don't know the best way to express it; perhaps by two distinct callbacks instead of just one, where the first callback is invoked for all the edge.value.is_some() cases here, and the second callback for the otherwise case?)

drop_flag_effects::on_all_children_bits(tcx, body, move_data, variant_mpi, |mpi| {
on_uninitialized_variant(trans, mpi)
});
}
}
});
}
16 changes: 5 additions & 11 deletions src/test/mir-opt/issue_41888.main.ElaborateDrops.after.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -102,15 +98,13 @@ 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
}

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
}
Expand Down Expand Up @@ -138,17 +132,17 @@ 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: {
switchInt(_7) -> [false: bb15, otherwise: bb20]; // scope 0 at $DIR/issue-41888.rs:15:1: 15:2
}

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): {
Expand Down