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

Layout and MIR field accesses are incoherent #69763

Closed
3 tasks done
RalfJung opened this issue Mar 6, 2020 · 11 comments
Closed
3 tasks done

Layout and MIR field accesses are incoherent #69763

RalfJung opened this issue Mar 6, 2020 · 11 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2020

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

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

eddyb commented Mar 6, 2020

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
Copy link
Member Author

RalfJung commented Mar 6, 2020

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
Copy link
Member

eddyb commented Mar 6, 2020

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
Copy link
Member Author

RalfJung commented Mar 6, 2020

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. :(

Centril added a commit to Centril/rust that referenced this issue Mar 11, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Centril added a commit to Centril/rust that referenced this issue Mar 11, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 14, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
JohnTitor added a commit to JohnTitor/rust that referenced this issue Mar 14, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 17, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Manishearth added a commit to Manishearth/rust that referenced this issue Mar 17, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Centril added a commit to Centril/rust that referenced this issue Mar 19, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
JohnTitor added a commit to JohnTitor/rust that referenced this issue Mar 20, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
@RalfJung
Copy link
Member Author

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
Copy link
Member

eddyb commented Mar 25, 2020

@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
Copy link
Member Author

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
Copy link
Member

eddyb commented Mar 25, 2020

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
Copy link
Member Author

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
Copy link
Member

eddyb commented Mar 25, 2020

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
Copy link
Member Author

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
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants