Skip to content
This repository

Liveness, borrowck and last use interact badly #2633

Closed
catamorphism opened this Issue · 17 comments

4 participants

Tim Chevalier Niko Matsakis Patrick Walton Graydon Hoare
Tim Chevalier
Collaborator

This arose from #2584. This code snippet from trans::base::trans_assign_op illustrates the problem:

        ret move_val(bcx, DROP_EXISTING, lhs_res.val,
                     // FIXME: should kind be owned?
                     {bcx: bcx, val: target, kind: owned},
                     dty);

bcx has type block, which is now a newtype-like enum whose LHS is an @ of a class. The bug is exposed by the fact that block doesn't get treated as immediate even though its representation is a box, but that's not the main bug. The main problem is that there's an implicit move because of the second use of bcx as a field in the record literal: it's a last use, implying that bcx gets zeroed at that point. Normally, borrowck would flag a move of something that has a reference to it (the reference gets brought about by the first instance of bcx, as the first argument to move_val). But since the implicit moves due to last-use information aren't visible to borrowck, the code happily compiles, runs, and then segfaults when move_val dereferences its first argument.

@nikomatsakis and I discussed this a bit on IRC and there's not a clear solution (though there is an obvious workaround for #2584). Perhaps the answer is to just remove last-use analysis.

Niko Matsakis
Collaborator

Here is a targeted test case:

fn a_val(&&x: ~int, +y: ~int) -> int {
    *x + *y
}

fn main() {
    let z = ~22;
    a_val(z, z);
}
Niko Matsakis
Collaborator

I see two possible solutions. One is to have borrowck remove the entries from last_use. This sounds both complex and a bit fragile. The other is to remove last_use and ask users to manually annotate moves.

It is a bit unclear to me what will be the impact of removing last_use on usability. On the one hand, it's a nice way to avoid excess use of the unary move operator. On the other hand, it's a common source of confusion ("why am I getting a copy warning here but not there? oh, I see, that's a last-use"). Also, when you add borrowck into the equation, it's one more thing that users have to think about ("it's not a last use because there is an existing alias in scope"). On the other hand, people will probably end up understanding the idea of "alias in scope" anyway.

Tim Chevalier
Collaborator

A standalone test case for this is under run-pass/issue-2633.rs (currently xfailed).

Niko Matsakis
Collaborator

Another example making use of by-value:

fn a_val(++x: @int, +y: @int) -> int {
    free(y);
    #debug["deref"];
    *x
}

fn free(-x: @int) {}

fn main() {
    let z = @22;
    assert 22 == a_val(z, z);
}
Patrick Walton
Owner

I'm in favor of getting rid of the last use analysis. Programmers should not have to run a liveness pass in their heads to figure out where the moves are.

A possibility here is to re-purpose the last use analysis into a fixit for the no-implicit-copies warning. That is, you might get warnings like so if liveness detects that the variable in question is dead:

foo.rs:20: warning: copying a value of type '~str' requires memory allocation
foo.rs:20: note: suggest use of 'move' to avoid the copy
Tim Chevalier
Collaborator

I like the idea of using last-use as a source of data for improving error messages, but not for guiding code generation at all.

Tim Chevalier catamorphism referenced this issue from a commit
Tim Chevalier Make move_val take its first argument by copy
Workaround for #2633 -- should allow changes on eholk's branch to
compile without segfaulting.
a14df27
Niko Matsakis
Collaborator

I think I like @pcwalton's idea as well. However, that seems like a decision that should be brought up at the Tuesday meeting. For the very short term, it's probably easy enough to have borrowck remove unsafe entries from the last_use table. I'm not thrilled about the idea because we tend to make tables that are built-up by a pass and then read-only thereafter, which seems like reasonable practice.

Niko Matsakis nikomatsakis referenced this issue from a commit
Niko Matsakis Undo workaround for #2633 since it is fixed.
This reverts commit a14df27.

Conflicts:

	src/rustc/middle/trans/base.rs
b0e66a6
Tim Chevalier
Collaborator

At the meeting today, we agreed to adopt @pcwalton 's idea to make last-use solely a hint source for error messages.

Graydon Hoare
Owner

I'm not sure if a fixit justifies a full pass; could just as easily amend the no-implicit-copies message to say "(try a move or copy?)"

In any case, yeah, I would like last-use to not have semantic weight. It was suspiciously non-obvious when we added it and I think it's proven no more obvious after regular use. I like explicit signals from the user in these contexts.

Tim Chevalier
Collaborator

I'm going to start making moves explicit in preparation for making last-use semantically non-meaningful.

Tim Chevalier
Collaborator

I'm going to take this over, but it's blocked on #3413

Graydon Hoare
Owner

Punting to 0.5; last-use should go but it seems like it'll take a little while to stabilize when it happens.

Niko Matsakis
Collaborator

Deferring to 0.5

Tim Chevalier
Collaborator

I have a patch for this (at least, removing any code that looks at the results of last-use, and making all moves explicit), but sadly it's a huge performance hit. Apparently there is a difference between how explicit moves are compiled, and how implicit (last-use) moves are compiled: even if I change the code so that trans still pays attention to the results of last-use, but the kind checker requires explicit moves on last uses, performance still gets much worse (for example, the typechecking pass takes 18s in stage 1, as opposed to 5s in stage 0).

So I'm not checking anything in yet :-(

Graydon Hoare
Owner

Can you push to a private branch? I'm happy to perf diff it to find the culprit.

Tim Chevalier
Collaborator

@graydon Oops, sorry, missed your comment. I will push to my fork once I get a chance to rebase it.

Tim Chevalier
Collaborator

c6780fb closes this. I opened #3755 to cover the remaining part (about hints in error messages).

Tim Chevalier catamorphism closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.