From 1025a12b64a7e5d852e02d59d86aca558733bed1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 26 Jan 2024 08:22:05 +0100 Subject: [PATCH 1/2] interpret: project_downcast: do not ICE for uninhabited variants --- .../src/interpret/projection.rs | 21 ++----------------- compiler/rustc_middle/src/mir/syntax.rs | 2 ++ .../issue-120337-irrefutable-let-ice.rs | 16 ++++++++++++++ .../let-irrefutable-pattern-ice-120337.rs | 10 +++++++++ 4 files changed, 30 insertions(+), 19 deletions(-) create mode 100644 src/tools/miri/tests/pass/issues/issue-120337-irrefutable-let-ice.rs create mode 100644 tests/ui/consts/let-irrefutable-pattern-ice-120337.rs diff --git a/compiler/rustc_const_eval/src/interpret/projection.rs b/compiler/rustc_const_eval/src/interpret/projection.rs index bd60e066f72..3a441c1d649 100644 --- a/compiler/rustc_const_eval/src/interpret/projection.rs +++ b/compiler/rustc_const_eval/src/interpret/projection.rs @@ -201,25 +201,8 @@ where // see https://github.com/rust-lang/rust/issues/93688#issuecomment-1032929496.) // So we just "offset" by 0. let layout = base.layout().for_variant(self, variant); - // In the future we might want to allow this to permit code like this: - // (this is a Rust/MIR pseudocode mix) - // ``` - // enum Option2 { - // Some(i32, !), - // None, - // } - // - // fn panic() -> ! { panic!() } - // - // let x: Option2; - // x.Some.0 = 42; - // x.Some.1 = panic(); - // SetDiscriminant(x, Some); - // ``` - // However, for now we don't generate such MIR, and this check here *has* found real - // bugs (see https://github.com/rust-lang/rust/issues/115145), so we will keep rejecting - // it. - assert!(!layout.abi.is_uninhabited()); + // This variant may in fact be uninhabited. + // See . // This cannot be `transmute` as variants *can* have a smaller size than the entire enum. base.offset(Size::ZERO, layout, self) diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index a5c229879a7..a4b6c4f9c3f 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -1074,6 +1074,8 @@ pub enum ProjectionElem { /// "Downcast" to a variant of an enum or a coroutine. /// /// The included Symbol is the name of the variant, used for printing MIR. + /// + /// This operation itself is never UB, all it does is change the type of the place. Downcast(Option, VariantIdx), /// Like an explicit cast from an opaque type to a concrete type, but without diff --git a/src/tools/miri/tests/pass/issues/issue-120337-irrefutable-let-ice.rs b/src/tools/miri/tests/pass/issues/issue-120337-irrefutable-let-ice.rs new file mode 100644 index 00000000000..5af0d0e4bbd --- /dev/null +++ b/src/tools/miri/tests/pass/issues/issue-120337-irrefutable-let-ice.rs @@ -0,0 +1,16 @@ +// Validation stops the test before the ICE we used to hit +//@compile-flags: -Zmiri-disable-validation + +#![feature(never_type)] +#[derive(Copy, Clone)] +pub enum E { + A(!), +} +pub union U { + u: (), + e: E, +} + +fn main() { + let E::A(ref _a) = unsafe { &(&U { u: () }).e }; +} diff --git a/tests/ui/consts/let-irrefutable-pattern-ice-120337.rs b/tests/ui/consts/let-irrefutable-pattern-ice-120337.rs new file mode 100644 index 00000000000..7da6b7ca285 --- /dev/null +++ b/tests/ui/consts/let-irrefutable-pattern-ice-120337.rs @@ -0,0 +1,10 @@ +// check-pass +#![feature(never_type)] +#[derive(Copy, Clone)] +pub enum E { A(!), } +pub union U { u: (), e: E, } +pub const C: () = { + let E::A(ref a) = unsafe { &(&U { u: () }).e}; +}; + +fn main() {} From 64cd13ff3b9b5ca0ed88cd1cfd0d15aca846da7e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 26 Jan 2024 10:30:46 +0100 Subject: [PATCH 2/2] add test for GVN issue; cleanup in dataflow_const_prop --- .../rustc_const_eval/src/interpret/operand.rs | 6 +++- .../src/dataflow_const_prop.rs | 7 +--- .../gvn_uninhabited.f.GVN.panic-abort.diff | 34 +++++++++++++++++++ .../gvn_uninhabited.f.GVN.panic-unwind.diff | 34 +++++++++++++++++++ tests/mir-opt/gvn_uninhabited.rs | 24 +++++++++++++ 5 files changed, 98 insertions(+), 7 deletions(-) create mode 100644 tests/mir-opt/gvn_uninhabited.f.GVN.panic-abort.diff create mode 100644 tests/mir-opt/gvn_uninhabited.f.GVN.panic-unwind.diff create mode 100644 tests/mir-opt/gvn_uninhabited.rs diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 80d4bda4827..4653c9016c6 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -260,8 +260,12 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> { // This makes several assumptions about what layouts we will encounter; we match what // codegen does as good as we can (see `extract_field` in `rustc_codegen_ssa/src/mir/operand.rs`). let inner_val: Immediate<_> = match (**self, self.layout.abi) { - // if the entire value is uninit, then so is the field (can happen in ConstProp) + // If the entire value is uninit, then so is the field (can happen in ConstProp). (Immediate::Uninit, _) => Immediate::Uninit, + // If the field is uninhabited, we can forget the data (can happen in ConstProp). + // `enum S { A(!), B, C }` is an example of an enum with Scalar layout that + // has an `Uninhabited` variant, which means this case is possible. + _ if layout.abi.is_uninhabited() => Immediate::Uninit, // the field contains no information, can be left uninit // (Scalar/ScalarPair can contain even aligned ZST, not just 1-ZST) _ if layout.is_zst() => Immediate::Uninit, diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index d5f22b2cdbc..ad12bce9b02 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -403,12 +403,7 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { operand, &mut |elem, op| match elem { TrackElem::Field(idx) => self.ecx.project_field(op, idx.as_usize()).ok(), - TrackElem::Variant(idx) => { - if op.layout.for_variant(&self.ecx, idx).abi.is_uninhabited() { - return None; - } - self.ecx.project_downcast(op, idx).ok() - } + TrackElem::Variant(idx) => self.ecx.project_downcast(op, idx).ok(), TrackElem::Discriminant => { let variant = self.ecx.read_discriminant(op).ok()?; let discr_value = diff --git a/tests/mir-opt/gvn_uninhabited.f.GVN.panic-abort.diff b/tests/mir-opt/gvn_uninhabited.f.GVN.panic-abort.diff new file mode 100644 index 00000000000..0b6819ad483 --- /dev/null +++ b/tests/mir-opt/gvn_uninhabited.f.GVN.panic-abort.diff @@ -0,0 +1,34 @@ +- // MIR for `f` before GVN ++ // MIR for `f` after GVN + + fn f() -> u32 { + let mut _0: u32; + let _1: u32; + let mut _2: E; + let mut _3: &U; + let _4: U; + scope 1 { + debug i => _1; + } + scope 2 { + let mut _5: &U; + } + + bb0: { + StorageLive(_2); + StorageLive(_3); + _5 = const _; + _3 = &(*_5); + _2 = ((*_3).1: E); + StorageLive(_1); +- _1 = ((_2 as A).1: u32); ++ _1 = const 0_u32; + StorageDead(_3); + StorageDead(_2); +- _0 = _1; ++ _0 = const 0_u32; + StorageDead(_1); + return; + } + } + diff --git a/tests/mir-opt/gvn_uninhabited.f.GVN.panic-unwind.diff b/tests/mir-opt/gvn_uninhabited.f.GVN.panic-unwind.diff new file mode 100644 index 00000000000..0b6819ad483 --- /dev/null +++ b/tests/mir-opt/gvn_uninhabited.f.GVN.panic-unwind.diff @@ -0,0 +1,34 @@ +- // MIR for `f` before GVN ++ // MIR for `f` after GVN + + fn f() -> u32 { + let mut _0: u32; + let _1: u32; + let mut _2: E; + let mut _3: &U; + let _4: U; + scope 1 { + debug i => _1; + } + scope 2 { + let mut _5: &U; + } + + bb0: { + StorageLive(_2); + StorageLive(_3); + _5 = const _; + _3 = &(*_5); + _2 = ((*_3).1: E); + StorageLive(_1); +- _1 = ((_2 as A).1: u32); ++ _1 = const 0_u32; + StorageDead(_3); + StorageDead(_2); +- _0 = _1; ++ _0 = const 0_u32; + StorageDead(_1); + return; + } + } + diff --git a/tests/mir-opt/gvn_uninhabited.rs b/tests/mir-opt/gvn_uninhabited.rs new file mode 100644 index 00000000000..a55b2dd763a --- /dev/null +++ b/tests/mir-opt/gvn_uninhabited.rs @@ -0,0 +1,24 @@ +// unit-test: GVN +// compile-flags: -O +// EMIT_MIR_FOR_EACH_PANIC_STRATEGY +// skip-filecheck + +#![feature(never_type)] + +#[derive(Copy, Clone)] +pub enum E { + A(!, u32), +} + +pub union U { + i: u32, + e: E, +} + +// EMIT_MIR gvn_uninhabited.f.GVN.diff +pub const fn f() -> u32 { + let E::A(_, i) = unsafe { (&U { i: 0 }).e }; + i +} + +fn main() {}