From 529c35331bb3817e90b5099c33d97aa55ad2713d Mon Sep 17 00:00:00 2001 From: Fabian Wolff Date: Sun, 3 Oct 2021 15:29:56 +0200 Subject: [PATCH 1/3] Fix unsound optimization with explicit variant discriminants --- .../rustc_mir_transform/src/simplify_try.rs | 18 ++++++++++++++---- src/test/ui/mir/issue-89485.rs | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/mir/issue-89485.rs diff --git a/compiler/rustc_mir_transform/src/simplify_try.rs b/compiler/rustc_mir_transform/src/simplify_try.rs index fd36671b36f54..8da90a432cea9 100644 --- a/compiler/rustc_mir_transform/src/simplify_try.rs +++ b/compiler/rustc_mir_transform/src/simplify_try.rs @@ -706,12 +706,22 @@ impl<'a, 'tcx> SimplifyBranchSameOptimizationFinder<'a, 'tcx> { let helper = |rhs: &Rvalue<'tcx>, place: &Place<'tcx>, variant_index: &VariantIdx, + switch_value: u128, side_to_choose| { let place_type = place.ty(self.body, self.tcx).ty; let adt = match *place_type.kind() { ty::Adt(adt, _) if adt.is_enum() => adt, _ => return StatementEquality::NotEqual, }; + let variant_discr = adt.discriminant_for_variant(self.tcx, *variant_index).val; + if variant_discr != switch_value { + trace!( + "NO: variant discriminant {} does not equal switch value {}", + variant_discr, + switch_value + ); + return StatementEquality::NotEqual; + } let variant_is_fieldless = adt.variants[*variant_index].fields.is_empty(); if !variant_is_fieldless { trace!("NO: variant {:?} was not fieldless", variant_index); @@ -742,18 +752,18 @@ impl<'a, 'tcx> SimplifyBranchSameOptimizationFinder<'a, 'tcx> { StatementKind::SetDiscriminant { place, variant_index }, ) // we need to make sure that the switch value that targets the bb with SetDiscriminant (y), is the same as the variant index - if Some(variant_index.index() as u128) == y_target_and_value.value => { + if y_target_and_value.value.is_some() => { // choose basic block of x, as that has the assign - helper(rhs, place, variant_index, x_target_and_value.target) + helper(rhs, place, variant_index, y_target_and_value.value.unwrap(), x_target_and_value.target) } ( StatementKind::SetDiscriminant { place, variant_index }, StatementKind::Assign(box (_, rhs)), ) // we need to make sure that the switch value that targets the bb with SetDiscriminant (x), is the same as the variant index - if Some(variant_index.index() as u128) == x_target_and_value.value => { + if x_target_and_value.value.is_some() => { // choose basic block of y, as that has the assign - helper(rhs, place, variant_index, y_target_and_value.target) + helper(rhs, place, variant_index, x_target_and_value.value.unwrap(), y_target_and_value.target) } _ => { trace!("NO: statements `{:?}` and `{:?}` not considered equal", x, y); diff --git a/src/test/ui/mir/issue-89485.rs b/src/test/ui/mir/issue-89485.rs new file mode 100644 index 0000000000000..cb507eefebbe5 --- /dev/null +++ b/src/test/ui/mir/issue-89485.rs @@ -0,0 +1,18 @@ +// Regression test for issue #89485. + +// run-pass + +#[derive(Debug, Eq, PartialEq)] +pub enum Type { + A = 1, + B = 2, +} +pub fn encode(v: Type) -> Type { + match v { + Type::A => Type::B, + _ => v, + } +} +fn main() { + assert_eq!(Type::B, encode(Type::A)); +} From 20489eaca2ad541a35059d38e2b064c46d3bcc9f Mon Sep 17 00:00:00 2001 From: Fabian Wolff Date: Sun, 3 Oct 2021 21:06:49 +0200 Subject: [PATCH 2/3] Update comments --- .../rustc_mir_transform/src/simplify_try.rs | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_mir_transform/src/simplify_try.rs b/compiler/rustc_mir_transform/src/simplify_try.rs index 8da90a432cea9..f8c616a19e937 100644 --- a/compiler/rustc_mir_transform/src/simplify_try.rs +++ b/compiler/rustc_mir_transform/src/simplify_try.rs @@ -713,6 +713,8 @@ impl<'a, 'tcx> SimplifyBranchSameOptimizationFinder<'a, 'tcx> { ty::Adt(adt, _) if adt.is_enum() => adt, _ => return StatementEquality::NotEqual, }; + // We need to make sure that the switch value that targets the bb with + // SetDiscriminant is the same as the variant discriminant. let variant_discr = adt.discriminant_for_variant(self.tcx, *variant_index).val; if variant_discr != switch_value { trace!( @@ -750,20 +752,28 @@ impl<'a, 'tcx> SimplifyBranchSameOptimizationFinder<'a, 'tcx> { ( StatementKind::Assign(box (_, rhs)), StatementKind::SetDiscriminant { place, variant_index }, - ) - // we need to make sure that the switch value that targets the bb with SetDiscriminant (y), is the same as the variant index - if y_target_and_value.value.is_some() => { + ) if y_target_and_value.value.is_some() => { // choose basic block of x, as that has the assign - helper(rhs, place, variant_index, y_target_and_value.value.unwrap(), x_target_and_value.target) + helper( + rhs, + place, + variant_index, + y_target_and_value.value.unwrap(), + x_target_and_value.target, + ) } ( StatementKind::SetDiscriminant { place, variant_index }, StatementKind::Assign(box (_, rhs)), - ) - // we need to make sure that the switch value that targets the bb with SetDiscriminant (x), is the same as the variant index - if x_target_and_value.value.is_some() => { + ) if x_target_and_value.value.is_some() => { // choose basic block of y, as that has the assign - helper(rhs, place, variant_index, x_target_and_value.value.unwrap(), y_target_and_value.target) + helper( + rhs, + place, + variant_index, + x_target_and_value.value.unwrap(), + y_target_and_value.target, + ) } _ => { trace!("NO: statements `{:?}` and `{:?}` not considered equal", x, y); From dd9b4763a4805e508cafd9aa49eebda27e5298c0 Mon Sep 17 00:00:00 2001 From: Fabian Wolff Date: Sun, 3 Oct 2021 22:58:24 +0200 Subject: [PATCH 3/3] Disable `SimplifyBranchSame` optimization for now --- compiler/rustc_mir_transform/src/simplify_try.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/compiler/rustc_mir_transform/src/simplify_try.rs b/compiler/rustc_mir_transform/src/simplify_try.rs index f8c616a19e937..e436d73226a55 100644 --- a/compiler/rustc_mir_transform/src/simplify_try.rs +++ b/compiler/rustc_mir_transform/src/simplify_try.rs @@ -544,6 +544,12 @@ pub struct SimplifyBranchSame; impl<'tcx> MirPass<'tcx> for SimplifyBranchSame { fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { + // This optimization is disabled by default for now due to + // soundness concerns; see issue #89485 and PR #89489. + if !tcx.sess.opts.debugging_opts.unsound_mir_opts { + return; + } + trace!("Running SimplifyBranchSame on {:?}", body.source); let finder = SimplifyBranchSameOptimizationFinder { body, tcx }; let opts = finder.find();