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

Handle partial initialization correctly in generators #63616

Closed
tmandry opened this issue Aug 15, 2019 · 3 comments
Closed

Handle partial initialization correctly in generators #63616

tmandry opened this issue Aug 15, 2019 · 3 comments
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Aug 15, 2019

Problem: consider something like the following,

let gen = || {
  let _x = (yield 5; panic!());
};

This can result in a local saved in our generator of type ((), !), which is partially initialized across the yield. In the layout code we claim that fields are all fully initialized across yields. (This is still a problem even if the second field is not of type !, but is particularly egregious in that case.)

Today this doesn't result in any miscompilations. MIR generation is quite conservative, and never seems to do partial initialization of aggregates across blocks. That might change in the future, and the layouts we create for generators do not take this into account.

The simplest correct thing we can do is to wrap all saved Generator locals in MaybeUninit. This means we need to project through the union in the generator MIR transform, which makes it different from the way we wrap with MaybeUninit "internally" today, to avoid propagating uninhabitedness (see #63035).

The only downside of wrapping everything is that we lose information that might be useful for optimizations and in miri. We can do better than this by only wrapping fields of an aggregate type, or maybe even restricting to fields which contain a field projection in the generator code.

#63230 was a naive attempt at avoiding issues like this, but avoiding partial initialization in surface Rust is not enough to prevent it in MIR.

@jonas-schievink jonas-schievink added A-coroutines Area: Coroutines A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 15, 2019
@RalfJung
Copy link
Member

Cc @eddyb

@tmandry tmandry added the A-async-await Area: Async & Await label Dec 31, 2019
@tmandry tmandry added AsyncAwait-Polish Async-await issues that are part of the "polish" area AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. and removed AsyncAwait-Polish Async-await issues that are part of the "polish" area labels Jan 7, 2020
@nikomatsakis
Copy link
Contributor

It seems like we have a few options here:

  • We never assume that the fields of a generator are initialized
  • We disable MIR optimizations on generators, or we wait to perform them until things are desugared

Specifically, I don't think we can actually optimize MIR construction to eliminate aggregates, or at least doing so would mean we also have to rework the drop elaboration and so forth.

@tmiasko
Copy link
Contributor

tmiasko commented Dec 13, 2023

Since #63035 and #118871 the coroutine layout no longer assumes that any of saved locals are initialized. In general, we simply don't know whether locals are definitely initialized in particular states of a coroutine.

To give a two more examples, consider:

let mut a: T;
loop {
    yield;
    a = f();
}

and

let a: T;
if b {
    a = f();
}
yield;

@tmiasko tmiasko closed this as completed Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants