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

Smaller and more correct generator codegen #69814

Merged
merged 7 commits into from
Mar 19, 2020
Merged

Smaller and more correct generator codegen #69814

merged 7 commits into from
Mar 19, 2020

Conversation

jonas-schievink
Copy link
Contributor

This removes unnecessary panicking branches in the resume function when the generator can not return or unwind, respectively.

Closes #66100

It also addresses the correctness concerns wrt poisoning on unwind. These are not currently a soundness issue because any operation inside a generator that could possibly unwind will result in a cleanup path for dropping it, ultimately reaching a Resume terminator, which we already handled correctly. Future MIR optimizations might optimize that out, though.

r? @Zoxc

This didn't cause issues before since generator types were always
considered to "need drop", leading to unwind paths
(including a `Resume` block) always getting generated.
The indices do not matter here, and this fixes an index out of bounds
panic when compiling a generator that can unwind but not return.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2020
@jonas-schievink jonas-schievink changed the title Smaller and more correct generator code Smaller and more correct generator codegen Mar 8, 2020
src/librustc_mir/transform/generator.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/generator.rs Outdated Show resolved Hide resolved
Co-Authored-By: bjorn3 <bjorn3@users.noreply.github.com>
@Zoxc
Copy link
Contributor

Zoxc commented Mar 8, 2020

You should be able to write a test that generators can't be resumed once unwound (covering the case fixed here).

@jonas-schievink
Copy link
Contributor Author

Hmm, not sure how I'd do that. Besides a call to box_free I haven't encountered any terminator with unwind = None, and that doesn't unwind (or rather, it's UB if the global allocator unwinds).

src/test/ui/generator/panic-safe.rs tests this case already (but works with the previous code as well, since poisoning on Resume is enough here).

1,
(POISONED, insert_panic_block(tcx, body, ResumedAfterPanic(generator_kind))),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe swap the order of inserts here so the same ordering is kept.


if can_return {
cases.insert(
1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If can_unwind and can_return, this will overwrite the POISONED case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insert on a Vec can't overwrite.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be moved to index 2 though, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed that reply

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought cases was an FxHashMap.

@Zoxc
Copy link
Contributor

Zoxc commented Mar 14, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Mar 14, 2020

📌 Commit 38fa378 has been approved by Zoxc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2020
@jonas-schievink
Copy link
Contributor Author

I'm currently looking into making needs_drop smarter when called on a generator, and it seems that that does indeed expose the soundness problem here: ui/async-await/issues/issue-65419/issue-65419-async-fn-resume-after-panic.rs starts failing because the generator isn't poisoned when it panics (since no destructors need to be called, no unwind path is created, so the Resume terminator is missing).

Centril added a commit to Centril/rust that referenced this pull request Mar 18, 2020
Smaller and more correct generator codegen

This removes unnecessary panicking branches in the resume function when the generator can not return or unwind, respectively.

Closes rust-lang#66100

It also addresses the correctness concerns wrt poisoning on unwind. These are not currently a soundness issue because any operation *inside* a generator that could possibly unwind will result in a cleanup path for dropping it, ultimately reaching a `Resume` terminator, which we already handled correctly. Future MIR optimizations might optimize that out, though.

r? @Zoxc
Centril added a commit to Centril/rust that referenced this pull request Mar 19, 2020
Smaller and more correct generator codegen

This removes unnecessary panicking branches in the resume function when the generator can not return or unwind, respectively.

Closes rust-lang#66100

It also addresses the correctness concerns wrt poisoning on unwind. These are not currently a soundness issue because any operation *inside* a generator that could possibly unwind will result in a cleanup path for dropping it, ultimately reaching a `Resume` terminator, which we already handled correctly. Future MIR optimizations might optimize that out, though.

r? @Zoxc
bors added a commit that referenced this pull request Mar 19, 2020
Rollup of 9 pull requests

Successful merges:

 - #69036 (rustc: don't resolve Instances which would produce malformed shims.)
 - #69443 (tidy: Better license checks.)
 - #69814 (Smaller and more correct generator codegen)
 - #69929 (Regenerate tables for Unicode 13.0.0)
 - #69959 (std: Don't abort process when printing panics in tests)
 - #69969 (unix: Set a guard page at the end of signal stacks)
 - #70005 ([rustdoc] Improve visibility for code blocks warnings)
 - #70088 (Use copy bound in atomic operations to generate simpler MIR)
 - #70095 (Implement -Zlink-native-libraries)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Mar 19, 2020
Rollup of 9 pull requests

Successful merges:

 - #68941 (Properly handle Spans that reference imported SourceFiles)
 - #69036 (rustc: don't resolve Instances which would produce malformed shims.)
 - #69443 (tidy: Better license checks.)
 - #69814 (Smaller and more correct generator codegen)
 - #69929 (Regenerate tables for Unicode 13.0.0)
 - #69959 (std: Don't abort process when printing panics in tests)
 - #69969 (unix: Set a guard page at the end of signal stacks)
 - #70005 ([rustdoc] Improve visibility for code blocks warnings)
 - #70088 (Use copy bound in atomic operations to generate simpler MIR)

Failed merges:

r? @ghost
@bors bors merged commit 5570a23 into rust-lang:master Mar 19, 2020
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
@jonas-schievink jonas-schievink deleted the gen-ret-unw branch April 17, 2020 18:36
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2020
…thewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[codegen] Unnecessary panicking branches in resumption of infinite generator (stored in static variable)
5 participants