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

Futures involving uninhabited variables are incorrectly considered uninhabited. #59972

Closed
Lymia opened this issue Apr 14, 2019 · 8 comments
Closed

Comments

@Lymia
Copy link
Contributor

@Lymia Lymia commented Apr 14, 2019

The following code crashes with an illegal instruction, presumably because all the code after contains_never is considered unreachable:

#![feature(futures_api, async_await, await_macro)]

pub enum Uninhabited { }

fn uninhabited_async() -> Uninhabited {
    unreachable!()
}

async fn noop() { }

async fn contains_never() {
    let error = uninhabited_async();
    await!(noop());
    let error2 = error;
}

fn main() {
    contains_never();
}

It seems the root cause is that the struct representing the future contains a field of type Uninhabited as it is preserved across the (unreachable) yield point and thus is considered uninhabited itself, despite being perfectly constructible.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 14, 2019

This sounds like it is caused by the fact that generators have an "incorrect" layout (claiming to have fields with certain types when really these might not be initialized), and some code not taking that special case into account properly.

Cc @eddyb @cramertj

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 15, 2019

@tmandry
Copy link
Contributor

@tmandry tmandry commented Apr 15, 2019

This is actually already fixed in #59897 (currently a WIP), around here. I'll make sure to add this as a test to that PR!

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 16, 2019

@tmandry does that mean that generators will no longer "cheat" in their layout, and

// Locals variables which live across yields are stored
can be removed? That'd be cool :D

I'd also have to adjust

ty::Generator(..) => {
-- can that be treated like any other type then because the variants have all the right information?

@eddyb
Copy link
Member

@eddyb eddyb commented Apr 17, 2019

@RalfJung I don't think it's that simple, we'd likely still need to keep uninitialized variables for now.
Although... I suppose the state variants, for yields across which they're not live, could just omit those variables, despite not using that space for anything else.

@tmandry
Copy link
Contributor

@tmandry tmandry commented Apr 17, 2019

@RalfJung I don't think it's that simple, we'd likely still need to keep uninitialized variables for now.
Although... I suppose the state variants, for yields across which they're not live, could just omit those variables, despite not using that space for anything else.

We could do that, yes, with some further changes to my PR. I would like to get there, and then we can have UB checking for our generators as well :)

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Apr 18, 2019

triage: P-high. removing nominated tag.

bors added a commit that referenced this issue May 4, 2019
Multi-variant layouts for generators

This allows generators to overlap fields using variants, but doesn't do any such overlapping yet. It creates one variant for every state of the generator (unresumed, returned, panicked, plus one for every yield), and puts every stored local in each of the yield-point variants.

Required for optimizing generator layouts (#52924).

There was quite a lot of refactoring needed for this change. I've done my best in later commits to eliminate assumptions in the code that only certain kinds of types are multi-variant, and to centralize knowledge of the inner mechanics of generators in as few places as possible.

This change also emits debuginfo about the fields contained in each variant, as well as preserving debuginfo about stored locals while running in the generator.

Also, fixes #59972.

Future work:
- Use this change for an optimization pass that actually overlaps locals within the generator struct (#52924)
- In the type layout fields, don't include locals that are uninitialized for a particular variant, so miri and UB sanitizers can check our memory (see #59972 (comment))
- Preserve debuginfo scopes across generator yield points
@bors bors closed this in #59897 May 4, 2019
@Centril
Copy link
Contributor

@Centril Centril commented May 4, 2019

Reopening for the test that #59897 didn't add (as far as I can tell; cc @eddyb & @tmandry)

@Centril Centril reopened this May 4, 2019
tmandry added a commit to tmandry/rust that referenced this issue May 6, 2019
tmandry added a commit to tmandry/rust that referenced this issue May 7, 2019
Centril added a commit to Centril/rust that referenced this issue May 8, 2019
…-obk

Add test for rust-lang#59972

This PR adds a test for rust-lang#59972, which was fixed in rust-lang#59897.

Closes rust-lang#59972.

r? @eddyb
bors added a commit that referenced this issue May 8, 2019
Rollup of 4 pull requests

Successful merges:

 - #60491 (std: Update compiler-builtins crate)
 - #60572 (Add test for #59972)
 - #60585 (Omit the vendor component in the WASI triple)
 - #60634 (Document + Cleanup lang_items.rs)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this issue May 8, 2019
…-obk

Add test for rust-lang#59972

This PR adds a test for rust-lang#59972, which was fixed in rust-lang#59897.

Closes rust-lang#59972.

r? @eddyb
bors added a commit that referenced this issue May 8, 2019
Rollup of 6 pull requests

Successful merges:

 - #59979 (to_xe_bytes for isize and usize returns an array of different size)
 - #60491 (std: Update compiler-builtins crate)
 - #60572 (Add test for #59972)
 - #60585 (Omit the vendor component in the WASI triple)
 - #60627 (test for #50518)
 - #60634 (Document + Cleanup lang_items.rs)

Failed merges:

r? @ghost
bors added a commit that referenced this issue May 8, 2019
Rollup of 6 pull requests

Successful merges:

 - #59979 (to_xe_bytes for isize and usize returns an array of different size)
 - #60491 (std: Update compiler-builtins crate)
 - #60572 (Add test for #59972)
 - #60585 (Omit the vendor component in the WASI triple)
 - #60627 (test for #50518)
 - #60634 (Document + Cleanup lang_items.rs)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this issue May 8, 2019
…-obk

Add test for rust-lang#59972

This PR adds a test for rust-lang#59972, which was fixed in rust-lang#59897.

Closes rust-lang#59972.

r? @eddyb
Centril added a commit to Centril/rust that referenced this issue May 8, 2019
…-obk

Add test for rust-lang#59972

This PR adds a test for rust-lang#59972, which was fixed in rust-lang#59897.

Closes rust-lang#59972.

r? @eddyb
bors added a commit that referenced this issue May 8, 2019
Rollup of 8 pull requests

Successful merges:

 - #59979 (to_xe_bytes for isize and usize returns an array of different size)
 - #60491 (std: Update compiler-builtins crate)
 - #60550 (Add tests for concrete const types)
 - #60572 (Add test for #59972)
 - #60627 (test for #50518)
 - #60634 (Document + Cleanup lang_items.rs)
 - #60641 (Instead of ICEing on incorrect pattern, use delay_span_bug)
 - #60644 (Use `delay_span_bug` for "Failed to unify obligation")

Failed merges:

r? @ghost
@bors bors closed this in #60572 May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants