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.

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

interpret: project_downcast: do not ICE for uninhabited variants #120367

Merged
merged 2 commits into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member Author

Choose a reason for hiding this comment

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

This here is a bit unfortunate, but enum layouts are weird...

// 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,
Expand Down
21 changes: 2 additions & 19 deletions compiler/rustc_const_eval/src/interpret/projection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 <https://github.com/rust-lang/rust/issues/120337>.

// This cannot be `transmute` as variants *can* have a smaller size than the entire enum.
base.offset(Size::ZERO, layout, self)
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1074,6 +1074,8 @@ pub enum ProjectionElem<V, T> {
/// "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<Symbol>, VariantIdx),

/// Like an explicit cast from an opaque type to a concrete type, but without
Expand Down
7 changes: 1 addition & 6 deletions compiler/rustc_mir_transform/src/dataflow_const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
@@ -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 };
}
34 changes: 34 additions & 0 deletions tests/mir-opt/gvn_uninhabited.f.GVN.panic-abort.diff
Original file line number Diff line number Diff line change
@@ -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;
}
}

34 changes: 34 additions & 0 deletions tests/mir-opt/gvn_uninhabited.f.GVN.panic-unwind.diff
Original file line number Diff line number Diff line change
@@ -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;
}
}

24 changes: 24 additions & 0 deletions tests/mir-opt/gvn_uninhabited.rs
Original file line number Diff line number Diff line change
@@ -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() {}
10 changes: 10 additions & 0 deletions tests/ui/consts/let-irrefutable-pattern-ice-120337.rs
Original file line number Diff line number Diff line change
@@ -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() {}