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

Improve liveness analysis for generators #84333

Merged
merged 1 commit into from
Aug 25, 2021
Merged

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Apr 19, 2021

Liveness analysis for generators assumes that execution always continues
normally after a yield point, not accounting for the fact that generator
could be dropped before completion.

If generators captures any variables by reference, those variables could
be used within a generator, or when the generator completes, but also
after each yield point in the case the generator is dropped.

Account for the case when generator is dropped after yielding, but
before running to the completion. This effectively considers all
variables captured by reference to be used after a yield point.

Fixes #84292.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2021
@tmandry
Copy link
Member

tmandry commented Aug 14, 2021

r? @tmandry

cc @eholk fyi

@rust-highfive rust-highfive assigned tmandry and unassigned estebank Aug 14, 2021
@eholk
Copy link
Contributor

eholk commented Aug 17, 2021

This looks good to me. To make sure I understand it, it looks like the main thing this does is adds an edge from every yield expression to the exit node, which models the fact that execution may not resume after a yield. Is this correct?

@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 18, 2021

it looks like the main thing this does is adds an edge from every yield expression to the exit node, which models the fact that execution may not resume after a yield. Is this correct?

Yes, that's correct. Variables captured by reference are already marked as live in the exit node at the beginning of Liveness::compute. To fix the issue we just need to model the control flow from yield to the exit.

@bors
Copy link
Contributor

bors commented Aug 24, 2021

☔ The latest upstream changes (presumably #85556) made this pull request unmergeable. Please resolve the merge conflicts.

Liveness analysis for generators assumes that execution always continues
normally after a yield point, not accounting for the fact that generator
could be dropped before completion.

If generators captures any variables by reference, those variables could
be used within a generator, or when the generator completes, but also
after each yield point in the case the generator is dropped.

Account for the case when generator is dropped after yielding, but
before running to the completion. This effectively considers all
variables captured by reference to be used after a yield point.
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 24, 2021

Rebased to resolve conflicts with #85556.

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@tmandry
Copy link
Member

tmandry commented Aug 25, 2021

Looks good, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 25, 2021

📌 Commit 9f6f862 has been approved by tmandry

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 25, 2021
@bors
Copy link
Contributor

bors commented Aug 25, 2021

⌛ Testing commit 9f6f862 with merge 1a9ac38...

@bors
Copy link
Contributor

bors commented Aug 25, 2021

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 1a9ac38 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 25, 2021
@bors bors merged commit 1a9ac38 into rust-lang:master Aug 25, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 25, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 25, 2021

Thanks for reviewing @eholk, @tmandry.

@tmiasko tmiasko deleted the liveness-yield branch August 25, 2021 14:14
eholk added a commit to eholk/rust that referenced this pull request Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unused_assignments false positive in async blocks
10 participants