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.

Sign up for GitHub

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

Jump to bottom

Remove both StorageLive and StorageDead in CopyProp. #107524

Merged
merged 3 commits into from
Feb 2, 2023
Merged

Remove both StorageLive and StorageDead in CopyProp. #107524

merged 3 commits into from
Feb 2, 2023

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jan 31, 2023 •
edited
Loading

Fixes #107511

#106908 removed StorageDead without the accompanying StorageLive. In loops, execution would see repeated StorageLive, without any StorageDead, which is UB.

So when removing storage statements, we have to remove both StorageLive and StorageDead.

I also added a MIR validation pass for StorageLive. It may be a bit overzealous.

@rustbot
Copy link
Collaborator

rustbot commented Jan 31, 2023

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 31, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 31, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@cjgillot
Copy link
Contributor Author

cc @RalfJung

@RalfJung
Copy link
Member

Thanks for the quick fix!

#106908 removed StorageDead without the accompanying StorageLive. In loops, execution would see repeated StorageLive, without any StorageDead, which is UB.

Ah, I see. Makes sense.
(FWIW whether we really want redundant StorageLive to be UB is kind of an open question, but for now it's an invariant we want to enforce -- see #99160. If having to remove StorageLive here is for some reason a burden or disadvantage, please leave a note in that issue. It would be nice to enforce the same for StorageDead but due to #98896 that is not currently possible.)

@RalfJung
Copy link
Member

I also added a MIR validation pass for StorageLive. It may be a bit overzealous.

I'm afraid I can't review that part, I don't know the APIs it uses so I don't even know what it actually checks.

So either r=me with that part moved to a separate PR, or we wait for another reviewer.

RalfJung
RalfJung reviewed Jan 31, 2023
View reviewed changes
tests/mir-opt/copy-prop/issue_107511.rs Outdated
sum += a[i];
}

let _ = sum;
Copy link
Member

Choose a reason for hiding this comment

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

let _val = sum might be better, _ patterns can be quite surprising...

@cjgillot
Copy link
Contributor Author

I don't really have an opinion on whether this should be UB. This is not a (currently) difficult invariant to preserve, so optimizations should probably preserve it.

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 31, 2023

📌 Commit e8ac040 has been approved by RalfJung

It is now in the queue for this repository.

@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 Jan 31, 2023
@bors
Copy link
Contributor

bors commented Feb 1, 2023

⌛ Testing commit e8ac040 with merge 9fcf4e35c49a654a7cbdfaf59a2e5d8882938b9b...

@bors
Copy link
Contributor

bors commented Feb 1, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 1, 2023
@compiler-errors
Copy link
Member

@bors retry CI was broken

@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 Feb 1, 2023
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@cjgillot
Copy link
Contributor Author

cjgillot commented Feb 2, 2023

@bors retry

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 2, 2023
@matthiaskrgr
Rollup merge of rust-lang#107524 - cjgillot:both-storage, r=RalfJung
4cf01cf 
Remove both StorageLive and StorageDead in CopyProp.

Fixes rust-lang#107511

rust-lang#106908 removed StorageDead without the accompanying StorageLive. In loops, execution would see repeated StorageLive, without any StorageDead, which is UB.

So when removing storage statements, we have to remove both StorageLive and StorageDead.

~I also added a MIR validation pass for StorageLive. It may be a bit overzealous.~
This was referenced Feb 2, 2023
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 2, 2023
@bors
Auto merge of rust-lang#107601 - matthiaskrgr:rollup-07zaafe, r=matth…
f312650 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#106919 (Recover `_` as `..` in field pattern)
 - rust-lang#107493 (Improve diagnostic for missing space in range pattern)
 - rust-lang#107515 (Improve pretty-printing of `HirIdValidator` errors)
 - rust-lang#107524 (Remove both StorageLive and StorageDead in CopyProp.)
 - rust-lang#107532 (Erase regions before doing uninhabited check in borrowck)
 - rust-lang#107559 (Rename `rust_2015` → `is_rust_2015`)
 - rust-lang#107577 (Reinstate the `hir-stats.rs` tests on stage 1.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6917040 into rust-lang:master Feb 2, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 2, 2023
@cjgillot cjgillot deleted the both-storage branch February 2, 2023 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@davidtwco davidtwco

Labels
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
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.

Bad MIR: StorageLive on a local that was already live with opt-level=4
7 participants
@cjgillot @rustbot @RalfJung @bors @compiler-errors @rust-log-analyzer @davidtwco