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 codegen breaking aliasing rules for functions with sret results #18250

Merged
merged 1 commit into from
Oct 28, 2014

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented Oct 23, 2014

This reverts commit a0ec902 "Avoid
unnecessary temporary on assignments".

Leaving out the temporary for the functions return value can lead to a
situation that conflicts with rust's aliasing rules.

Given this:

fn func(f: &mut Foo) -> Foo { /* ... */ }

fn bar() {
    let mut foo = Foo { /* ... */ };

    foo = func(&mut foo);
}

We effectively get two mutable references to the same variable foo at
the same time. One for the parameter f, and one for the hidden
out-pointer. So we can't just trans_into the destination directly, but
must use trans to get a new temporary slot from which the result can
be copied.

This reverts commit a0ec902 "Avoid
unnecessary temporary on assignments".

Leaving out the temporary for the functions return value can lead to a
situation that conflicts with rust's aliasing rules.

Given this:

````rust
fn func(f: &mut Foo) -> Foo { /* ... */ }

fn bar() {
    let mut foo = Foo { /* ... */ };

    foo = func(&mut foo);
}
````

We effectively get two mutable references to the same variable `foo` at
the same time. One for the parameter `f`, and one for the hidden
out-pointer. So we can't just `trans_into` the destination directly, but
must use `trans` to get a new temporary slot from which the result can
be copied.
@thestinger
Copy link
Contributor

@dotdash: It would be nice if this checked for an outstanding borrow and performed the optimization if there wasn't one. I don't know how hard that would be...

@dotdash
Copy link
Contributor Author

dotdash commented Oct 23, 2014

@thestinger could you elaborate on that check? I'm not sure I understand why an outstanding mutable borrow would make the optzn valid. Also, you said something about unwinding invalidating it as well. What about that?

@thestinger
Copy link
Contributor

@dotdash: Sorry, I meant if there wasn't an outstanding borrow. I don't think this optimization is problematic for unwinding if there's no aliasing &mut, which is problematic for other reasons anyway.

@alexchandel
Copy link

Optimizing returning large structs would seem to be important for Rust's constructor idiom.

@huonw
Copy link
Member

huonw commented Oct 24, 2014

@alexchandel, being correct is far more important; we should revert to correctness quickly, and can add optimisations, progressively, later.

@alexchandel
Copy link

@huonw Sure, no one could reasonably disagree with this revert. I think @thestinger's optimization—or a working version of what this PR reverts—would be good, and would seem to benefit every large struct implementation that has a method like fn new() -> Foo.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 27, 2014
@bors bors merged commit 70fe20a into rust-lang:master Oct 28, 2014
@dotdash dotdash deleted the fix_aliasing branch February 4, 2015 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants