Fix ICE on const eval of union field

[etaoins]

MIR's Const::get_field() attempts to retrieve the value for a given field in a constant. In the case of a union constant it was falling through to a generic const_get_elt based on the field index. As union fields don't have an index this caused an ICE in llvm_field_index.

Fix by simply returning the current value when accessing any field in a union. This works because all union fields start at byte offset 0.

The added test uses const_fn it ensure the field is extracted using MIR's const evaluation. The crash is reproducible without it, however.

Fixes #47788

r? @eddyb

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[eddyb]

What happens if you make them different types, say, i64 for the second field`, and maybe do some arithmetic on it?

[eddyb]

Thank you for the PR, however #46882 should fix the ICE and implement the correct semantics in all cases, so I'm not sure we should merge this partial fix. cc @oli-obk

[etaoins]

@eddyb

I can confirm that #46882 fixes this. My only concern is this a regression against stable as it's possible to crash nightly and beta (but not 1.23) with the following:

union DummyUnion {
    field1: i32,
    field2: i32,
}

const UNION: DummyUnion = DummyUnion { field1: 1 };

fn read_field1() -> i32 {
    const FIELD1: i32 = unsafe { UNION.field1 };
    FIELD1
}

fn main() {
    read_field1();
}

[eddyb]

Yes but what does stable do if the types aren't the same? It can't possibly work. Oh well.

cc @rust-lang/compiler

[eddyb]

Also I think your test is still subtly cheating, try an union between i64 and (i32, i32), where you initialize the former and read the latter. Should fail on both stable and this PR.

[etaoins]

This passes on both stable and this PR but ICEs on nightly and beta:

type Field1 = u64;
type Field2 = (u32, u32);

union DummyUnion {
    field1: Field1,
    field2: Field2,
}

const UNION: DummyUnion = DummyUnion {
    field1: 0xdeadbeeff00fb00f,
};

fn read_field1() -> Field1 {
    const FIELD1: Field1 = unsafe { UNION.field1 };
    FIELD1
}

fn read_field2() -> Field2 {
    const FIELD2: Field2 = unsafe { UNION.field2 };
    FIELD2
}

fn main() {
    assert_eq!(read_field1(), 0xdeadbeeff00fb00f);

    let (first, second) = read_field2();
    assert_eq!(first, 0xf00fb00f);
    assert_eq!(second, 0xdeadbeef);
}

[petrochenkov]

This is not supposed to work on stable.
There's a test ensuring this doesn't work - https://github.com/rust-lang/rust/blob/master/src/test/ui/union/union-const-eval.rs.
Const functions however change the intended error into a warning somehow and it's a bug.

[eddyb]

@etaoins Sorry, what I meant was this:

const FIELD2_1: u32 = unsafe { UNION.field2.1 };

I'm willing to backport this PR to beta just to get the half-broken behavior back, and the code changed here should get removed by #46882 anyway, so it should all work out.

[oli-obk]

This code will get removed by #46882 and the underlying issue will be fixed. I'll add this test to my PR, it should simply work

[etaoins]

@eddyb

Yup, that finally kills it. Running this will segfault on stable and fail the assertion on my PR:

type Field1 = u64;
type Field2 = (u32, u32);

union DummyUnion {
    field1: Field1,
    field2: Field2,
}

const UNION: DummyUnion = DummyUnion {
    field1: 0xdeadbeeff00fb00f,
};

fn read_field2_1() -> u32 {
    const FIELD2_1: u32 = unsafe { UNION.field2.1 };
    FIELD2_1
}

fn main() {
    assert_eq!(read_field2_1(), 0xdeadbeef);
}

[eddyb]

@bors r+

[bors]

📌 Commit ed7e4e1 has been approved by eddyb

[eddyb]

I think we should merge this and backport it, it shouldn't allow anything that didn't work before.

[bors]

⌛ Testing commit ed7e4e1 with merge 5e7fd65...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 5e7fd65 to master...

[etaoins]

@eddyb What's the process for backporting? Should I open a PR against the beta branch?

[eddyb]

@etaoins We first have to accept it and then we typically batch them.

[nikomatsakis]

Accepting for backport.