ICE when const evaluating UB

[oli-obk]

This ICEs on stable, probably since March 2018, but it requires the user to do UB in constants, so...

I think a possible fix would be to enable validation in const eval (cc @RalfJung ), though we should benchmark the perf considerations around that first. Alternatively we can just not ICE here and instead emit an error.

#[derive(Copy, Clone)]
pub struct ChildStdin {
    inner: AnonPipe,
}

#[derive(Copy, Clone)]
enum AnonPipe {}

const FOO: () = {
    union Foo {
        a: ChildStdin,
        b: (),
    }
    let x = unsafe { Foo { b: () }.a };
    let x = &x.inner;
};

fn main() {}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
warning: field is never used: `inner`
 --> src/main.rs:3:5
  |
3 |     inner: AnonPipe,
  |     ^^^^^^^^^^^^^^^
  |
  = note: #[warn(dead_code)] on by default

warning: constant item is never used: `FOO`
  --> src/main.rs:9:1
   |
9  | / const FOO: () = {
10 | |     union Foo {
11 | |         a: ChildStdin,
12 | |         b: (),
...  |
15 | |     let x = &x.inner;
16 | | };
   | |__^

thread 'rustc' panicked at 'Tried to access field 0 of union with 0 fields', src/librustc_mir/interpret/place.rs:360:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.37.0 (eae3437df 2019-08-13) running on x86_64-unknown-linux-gnu

note: compiler flags: -C codegen-units=1 -C debuginfo=2 --crate-type bin

note: some of the compiler flags provided by cargo are hidden

error: Could not compile `playground`.

To learn more, run the command again with --verbose.

[RalfJung]

Interesting. ICEs in Miri with -Zmiri-disable-validation are also bugs, so I'd say doing more validation is not the right call.

Do you have a backtrace? Which code is responsible for ICEing here and how hard/bad would it be to just error instead?

[oli-obk]

It's ICEing in

rust/src/librustc_mir/interpret/place.rs

Line 387 in 64c0969

"Tried to access field {} of union with {} fields", field, count);

and we could indeed just emit a UB error here, but it seems very... odd, since the field access check has very little to do with UB at a first glance. The layout is a union with zero fields, because it's essentially !.

[RalfJung]

The layout is a union with zero fields, because it's essentially !.

TBH this looks like a bug to me. That's the layout of which type, ChildStdin? It should have a field, and it should not be a union.

Cc @eddyb

[oli-obk]

It's not a bug. Just like ScalarPair layout for a newtype with a single field is not a bug (E.g. struct Foo(&'static [u8]);).

[RalfJung]

Precisely by analogy with ScalarPair, I think this is a bug. Your Foo has a TyLayout with exactly 1 field, matching what is declared. It does not have 2 fields which would be the case if it had a "pair layout". So, transparent layout changes the abi, but it does not change how many "fields" a type is considered to have. Similarly, struct Foo(&'static [u8], (), ()) has 3 fields in its layout, despite still being a ScalarPair.

So, correspondingly, ChildStdin should have 1 field.

[oli-obk]

oh... right. Yea we should totally fix that

[eddyb]

This being a layout bug would trivially break codegen, why is only miri affected?

[RalfJung]

Codegen might skip the field access because the type is uninhabited?

[eddyb]

I wouldn't be so sure, codegen calls .field on layouts a lot without checking.

Can the ICE print the whole TyLayout? That might be more useful here.

EDIT: oh this might be why: we don't check the index at all:

rust/src/librustc_target/abi/mod.rs

Line 741 in 42ec683

FieldPlacement::Union(_) => Size::ZERO,

[oli-obk]

yea but we can't use field because that ICEs during const prop in the array case. I have a local branch that I'm playing with to get the entire picture

[eddyb]

You can literally remove the assert to get the same effect. But if the field count is wrong I'd rather have an assert in both places and fix the count (if possible).

[oli-obk]

The full TyLayout is

TyLayout {
    ty: ChildStdin,
    details: LayoutDetails {
        variants: Single {
            index: 0,
        },
        fields: Union(
            0,
        ),
        abi: Uninhabited,
        largest_niche: None,
        align: AbiAndPrefAlign {
            abi: Align {
                pow2: 0,
            },
            pref: Align {
                pow2: 0,
            },
        },
        size: Size {
            raw: 0,
        },
    },

and so far I haven't figured out how that can happen

[oli-obk]

Ah, nevermind, I found it:

rust/src/librustc/ty/layout.rs

Line 830 in 64f61c7

return tcx.layout_raw(param_env.and(tcx.types.never));

[eddyb]

Ah so that's a micro-optimization. Maybe we should compute the right thing.

[oli-obk]

#64987 computes the layout for structs but keeps returning the never layout for enums without inhabited variants