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

Fix MIR CopyPropagation errneously propagating assignments to function args #45753

Merged
merged 2 commits into from
Nov 12, 2017

Conversation

sinkuu
Copy link
Contributor

@sinkuu sinkuu commented Nov 4, 2017

Compiling this code with MIR CopyPropagation activated will result in printing 5,
because CopyProp errneously propagates the assignment of 5 to all x:

fn bar(mut x: u8) {
    println!("{}", x);
    x = 5;
}

fn main() {
    bar(123);
}

If a local is propagated, it will result in an ICE at trans due to an use-before-def:

fn dummy(x: u8) -> u8 { x }

fn foo(mut x: u8) {
    x = dummy(x); // this will assign a local to `x`
}

Currently CopyProp conservatively gives up if there are multiple assignments to a local,
but it is not took into account that arguments are already assigned from the beginning.
This PR fixes the problem by preventing propagation of assignments to function arguments.

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2017
@arielb1
Copy link
Contributor

arielb1 commented Nov 5, 2017

I think it's cleaner to have the check with the rest of the destination checks at ~L85:

let dest_def_count = dest_use_info.def_count_not_including_drop();
if dest_def_count == 0 {
debug!(" Can't copy-propagate local: dest {:?} undefined",
dest_local);
continue
}
if dest_def_count > 1 {
debug!(" Can't copy-propagate local: dest {:?} defined {} times",
dest_local,
dest_use_info.def_count());
continue
}

@sinkuu
Copy link
Contributor Author

sinkuu commented Nov 5, 2017

@arielb1 I want the copyprop to keep eliminating (arg) = (arg) assignment if possible (it would be ok if lhs and rhs are the same variable, right?), so it needs to check the src of assign statement.

(I noticed I should move the check from the current weird place to just before

let maybe_action = match *operand {
at least.)

@arielb1
Copy link
Contributor

arielb1 commented Nov 5, 2017

@sinkuu

Eliminating self-assignment is ok in even more cases (e.g. even if there are multiple definitions of the value, or multiple uses - I actually think that self-assignment can be eliminated in all cases), so I'll rather do its elimination separately. Unless for some reason we generate a lot of arg = arg; assignments, but I doubt that.

@sinkuu
Copy link
Contributor Author

sinkuu commented Nov 8, 2017

Depends on #45757

@arielb1
Copy link
Contributor

arielb1 commented Nov 12, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Nov 12, 2017

📌 Commit 1142524 has been approved by arielb1

@bors
Copy link
Contributor

bors commented Nov 12, 2017

⌛ Testing commit 1142524 with merge 3d2dc6e...

bors added a commit that referenced this pull request Nov 12, 2017
Fix MIR CopyPropagation errneously propagating assignments to function args

Compiling this code with MIR CopyPropagation activated will result in printing `5`,
because CopyProp errneously propagates the assignment of `5` to all `x`:

```rust
fn bar(mut x: u8) {
    println!("{}", x);
    x = 5;
}

fn main() {
    bar(123);
}

```

If a local is propagated, it will result in an ICE at trans due to an use-before-def:

```rust
fn dummy(x: u8) -> u8 { x }

fn foo(mut x: u8) {
    x = dummy(x); // this will assign a local to `x`
}
```
Currently CopyProp conservatively gives up if there are multiple assignments to a local,
but it is not took into account that arguments are already assigned from the beginning.
This PR fixes the problem by preventing propagation of assignments to function arguments.
@bors
Copy link
Contributor

bors commented Nov 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 3d2dc6e to master...

@bors bors merged commit 1142524 into rust-lang:master Nov 12, 2017
@sinkuu
Copy link
Contributor Author

sinkuu commented Nov 12, 2017

Thank you for reviewing.

@sinkuu sinkuu deleted the mir_copyprop_arg branch November 12, 2017 18:08
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Dec 3, 2017
Fix CopyPropagation regression (2)

Remaining part of MIR copyprop regression by (I think) rust-lang#45380, which I missed in rust-lang#45753.

```rust
fn foo(mut x: i32) -> i32 {
    let y = x;
    x = 123; // `x` is assigned only once in MIR, but cannot be propagated to `y`
    y
}
```

So any assignment to an argument cannot be propagated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants