Skip to content

Layout and MIR field accesses are incoherent #69763

Closed
@RalfJung

Description

@RalfJung
Member

I propose that we should have the following invariant: Whenever, anywhere in MIR, there is a field projection with some index i, then i is actually a valid field index in the (statically known) layout of the type being projected into.

Right now, this is not the case. This code:

pub enum Void {}

enum UninhabitedUnivariant { _Variant(Void), }

fn main() {
    let _seed: UninhabitedUnivariant = None.unwrap();
    match _seed {
        UninhabitedUnivariant::_Variant(_x) => {}
    }
}

generates the following MIR:

    bb1: {
        StorageDead(_2);                 // bb1[0]: scope 0 at src/main.rs:6:52: 6:53
        StorageLive(_3);                 // bb1[1]: scope 1 at src/main.rs:8:41: 8:43
        _3 = move ((_1 as _Variant).0: Void); // bb1[2]: scope 1 at src/main.rs:8:41: 8:43
        StorageDead(_3);                 // bb1[3]: scope 1 at src/main.rs:9:5: 9:6
        StorageDead(_1);                 // bb1[4]: scope 0 at src/main.rs:10:1: 10:2
        return;                          // bb1[5]: scope 0 at src/main.rs:10:2: 10:2
    }

but the layout of _Variant is that of a union with 0 fields.

A consequence of this incoherence is that all codegen backends and Miri (and possibly more MIR consumers) all need to be on their guard when considering field projections, always protecting against the case where the projection is ill-formed (usually that happens by special-casing uninhabited types before considering the projection).

I think instead of putting that burden on every MIR consumer, we should fix either the layout or the MIR to not violate the invariant in the first place.

Cc @eddyb with whom I anyway recently talked about those odd Union layouts. Is there some fundamental issue with the invariant I am proposing? Also Cc @oli-obk @wesleywiser

If we decide to go with this, we should

  • Check for other cases in Miri/const-prop that just work around such broken MIR/layout
    Check for the corresponding cases in codegen and other MIR consumers -- mostly, add assertion here.

Activity

added
C-cleanupCategory: PRs that clean code up or issues documenting cleanup.
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
A-MIRArea: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html
on Mar 6, 2020
eddyb

eddyb commented on Mar 6, 2020

@eddyb
Member

I wrote #69753 (comment), before taking another look at the code, which revealed the problem:

So this is the relevant code, that (I thought) would kick in and compute the right number of fields:

rust/src/librustc/ty/layout.rs

Lines 2025 to 2044 in 4a1b69d

Variants::Single { index } => {
// Deny calling for_variant more than once for non-Single enums.
if let Ok(layout) = cx.layout_of(this.ty).to_result() {
assert_eq!(layout.variants, Variants::Single { index });
}
let fields = match this.ty.kind {
ty::Adt(def, _) => def.variants[variant_index].fields.len(),
_ => bug!(),
};
let tcx = cx.tcx();
tcx.intern_layout(LayoutDetails {
variants: Variants::Single { index: variant_index },
fields: FieldPlacement::Union(fields),
abi: Abi::Uninhabited,
largest_niche: None,
align: tcx.data_layout.i8_align,
size: Size::ZERO,
})
}

However, just before it we have:

Variants::Single { index } if index == variant_index => this.details,

So what's happening is the first variant of every uninhabited enum will get confused with the enum as a whole (which right now has the layout of !, i.e. fields: Union(0)).

Removing that line will probably fix this, but I haven't tried it.
EDIT: removing would break inhabited univariant enums, it should be made conditional on fields != Union(0) instead (i.e. add that to the existing match guard of that arm).

RalfJung

RalfJung commented on Mar 6, 2020

@RalfJung
MemberAuthor

Glad to hear that we agree on this invariant. :)

We fixed this months ago. I don't understand why this is a problem again now.

Doesn't the fact that codegen, borrowck and so on didn't ICE here indicate that they all still contain some "is_uninhabited" check that ought to be unnecessary? If we can find and remove that check, it would mean we'd see such issues much earlier (we'd see them on any MIR statement that codegen runs on, not just any statement that const-prop runs on).

eddyb

eddyb commented on Mar 6, 2020

@eddyb
Member

We fixed this months ago. I don't understand why this is a problem again now.

I was wrong when I said that, in that it's still broken for variant index 0.

they all still contain some "is_uninhabited" check that ought to be unnecessary?

I'm not sure I could see how that factors into this too much.
A lot of stuff skips dead code (for good reason), so it would be hard to get good coverage.


In case you didn't see it, I edited #69763 (comment) with the right fix, AFAICT.

Long-term we'll want to change the layout to use Variants::Multiple more uniformly, and have:

  • a () DiscriminantKind for univariant enums
  • a ! DiscriminantKind for uninhabited enums

(we'd probably have to move discr: Scalar into DiscriminantKind::{Tag,NicheFilling})

RalfJung

RalfJung commented on Mar 6, 2020

@RalfJung
MemberAuthor

So looks like #66250 removed the assertion that demonstrates the incoherence, instead of fixing the issue -- and no issue was opened to track the problem so it just got forgotten until someone re-discovered the ICE. :(

added 2 commits that reference this issue on Mar 11, 2020

Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…

3dc5218

Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…

7210c6c
added a commit that references this issue on Mar 14, 2020

Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…

8f06c62
added a commit that references this issue on Mar 14, 2020

Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…

5ef02a5
added a commit that references this issue on Mar 17, 2020

Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…

dd0d904
added a commit that references this issue on Mar 17, 2020

Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…

45ed7e1
added a commit that references this issue on Mar 19, 2020

Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…

46d8095
added a commit that references this issue on Mar 20, 2020

Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…

3554f2d
RalfJung

RalfJung commented on Mar 24, 2020

@RalfJung
MemberAuthor

The fixes for asserting field access sanity in Miri and codegen have landed.
I am not aware of any other such hacks in Miri (where we "work around" weird layout). There is the issue with SetDiscriminant and uninhabited variants bailing out early, but I got convinced some time ago that that is just needed -- and saying "SetDiscriminant for an uninhabited variant is insta-UB" makes enough sense. I just went over place.rs and operand.rs and everything seems good.

So, I think this can be closed. :)

eddyb

eddyb commented on Mar 25, 2020

@eddyb
Member

@RalfJung What about the second half of #69763 (comment)? Right now the layouts for enums with 0 or 1 present (inhabited or with non-ZST fields) variants are pretty artificial, and I think they should be fixed.

RalfJung

RalfJung commented on Mar 25, 2020

@RalfJung
MemberAuthor

Agreed, but that sounds like a separate issue to me? At least there aren't field accesses to variants that don't exist, or stuff like that.

Would that proposal also help with SetDiscriminant, i.e. would we still need to bail out explicitly and early for uninhabited variants?

eddyb

eddyb commented on Mar 25, 2020

@eddyb
Member

would we still need to bail out explicitly and early for uninhabited variants

If the whole enum is uninhabited, it would be an exhaustive match like Tag vs NicheFilling today.

If a specific variant is uninhabited, you could probably let the regular discriminant writing code handle it, although there's always the risk that the way that code is implemented would write a different variant's tag/niche (e.g. by truncation, if the uninhabited variant is just outside).

Agreed, but that sounds like a separate issue to me?

Hmm I thought you wanted to keep this open until layout reflected what MIR could access more directly, instead of faking it with layout_of(Enum) = layout_of(!) etc.

RalfJung

RalfJung commented on Mar 25, 2020

@RalfJung
MemberAuthor

I was mostly concerned with fields for this issue, not variants. I guess I got used to variants being a mess. ;) None of the text in the OP makes sense for the variant case though, I think?

And then there's also the thing where primitives are "fieldless unions", and maybe instead there should be FieldPlacement::Atom or so to indicate that there are no fields (but there is still data).

eddyb

eddyb commented on Mar 25, 2020

@eddyb
Member

I was mostly concerned with fields for this issue, not variants.

Variants are what contain fields. The reason for any oddities with fields is because the variants they're in aren't actually in the layout so they have to be faked on the spot (which is what I think we should fix).

RalfJung

RalfJung commented on Mar 25, 2020

@RalfJung
MemberAuthor

Okay, I made a new issue for that: #70399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-MIRArea: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.htmlC-cleanupCategory: PRs that clean code up or issues documenting cleanup.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @eddyb@RalfJung

      Issue actions

        Layout and MIR field accesses are incoherent · Issue #69763 · rust-lang/rust