librustc: WIP for write barriers #4454

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@pcwalton
Contributor

This is a preliminary review only—do not pull!

r? @nikomatsakis. I'd like to know whether you feel this is on the right track. This includes the dynamic component to the @mut&imm borrow only and not the write barriers. I'd like to get your opinion as to the best way to implement the write barriers—another pass that uses mem_categorization perhaps?

@nikomatsakis
Contributor

This seems like the right approach to me. As for the write barriers, that's a good question. I had always imagined that trans would compute where they are needed but I guess there's some computation involved, so perhaps it's best to put that into the check_loans phase of the borrow checker. After all, it already computes the cmt for each lvalue, so it's already got the info you need. It can just stash into a side-table whether a write-barrier is needed for a given assignment. We should think carefully about when the write barrier occurs---it probably doesn't matter so much but clearly it must be well-defined! It seems to me the write barrier should probably occur at the last moment before the write occurs: so after the RHS is evaluated.

I imagine that to actually implement the write barriers you might want to use a similar table to the rooting table, since you will have to insert the write barriers potentially on auto-derefs of intermediate expressions. So if someone wrote:

struct Foo { f: int };
let x = @mut @mut Foo { f: 2 };
let y = &mut x.f;
x.f += 2; // (1)

then the write barrier at (1) would have to check *x. So you could add the code that actually checks the write barrier to Datum::deref(), I think. We have to be sure that all the relevant paths go through there, but they are supposed to---the main place I'd be concerned is the pattern matching code, which may invoke Datum::box_body() directly? We could also have box_body() take an expr id and be responsible for this. Anyway, something like that.

@pcwalton
Contributor

Here's some more code. There is currently a problem whereby basic blocks are appended to after termination, preventing self-hosting—I need to work that out before this is done. However, the test case works.

r? @nikomatsakis

@pcwalton
Contributor

This implements enough to get @mut borrowing to &imm. Review requested. r? @nikomatsakis

@nikomatsakis
Contributor

r+

This looks pretty good. As I wrote in the comments, I think the current modifications to check_mutbl are not as general as they should be, but what's there should work for simple cases, so that's an area to expand in the future (we can discuss this more on IRC or in person).

I notice there seems to be no tests for write barriers. I think you should add some tests that freeze an @mut and then perform mutation on its fields. Ideally you could test such mutation through various amounts of indirection and explicitness, e.g., *x = S { f: 1 };, x.f = 1; (*x).f = 1; etc.

@pcwalton
Contributor
@nikomatsakis
Contributor

r- but pretty close: this is unsound unless you loan out b at least as const. I detailed the required change and two test cases.

Question: Are there run-fail tests corresponding to the various write-guard failures that can take place? I feel like in general there are a host of new tests that should be added but I guess I have to step back and review what's there.

Oh, and one nit: I personally would prefer if the tests that are testing the borrow checker have a name like borrowck-*.rs rather than write-guard-*. I want to be able to find all borrow check related tests with ls borrowck*rs. Perhaps borrowck-wg-foo-bar.rs would be best, if you want to specify that it's particularly related to write guards. But basically my rule of thumb is that if the test is expecting an error that will be reported by the borrow checker, or checking that the borrow checker does not issue an error in some circumstance, it's name should begin with borrowck-.

@nikomatsakis
Contributor

One other thing---can you add the paragraph I wrote explaining why re_free is sound as the scope of a loan as a comment on the relevant bit of code where you had the "XXX"?

@pcwalton
Contributor
@nikomatsakis
Contributor

r+, but can you add a compile-fail test like the following and also mention its name in the comment about loans with scope re_free. This test is testing the scenario that the comment refers to: in a(), the field p.f is lent out with scope v, which extends beyond the callee. This means that the caller knows nothing about it, so it's not obvious that this would be sound. Maybe the caller will not respect the loan. But in b(), we see that this does not happen. b() has lent out p and given away the borrowed pointer to a(), and hence b() now lacks the ability to modify the pointer p anymore. I think a test like this would be good. This is testing the scenario that the comment refers to.

struct Foo { f: int }

fn a(p: &v/mut Foo) -> &v/mut int { &mut p.f }

fn b(p: &v/mut Foo) -> &v/mut int {
    let q = a(&mut *p);
    p.f += 1; //~ ERROR p should be lent out here
    return q;
}

fn c() {
    let mut f = Foo { f: 3 };
    let b = b(&mut f);
    f.f += 1; //~ ERROR, f is lent out
    *b += 1; // OK
}

fn main() {}
@graydon
Contributor
graydon commented Jan 23, 2013

This landed, yes? Close if so.

@pcwalton
Contributor

Not yet.

@pcwalton pcwalton closed this Jan 24, 2013
@pcwalton pcwalton deleted the pcwalton:inhtwama-wip branch Jan 24, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment