Fix end region emission order #44129

pnkfelix · 2017-08-28T13:25:39Z

rust-highfive · 2017-08-28T13:25:50Z

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

arielb1 · 2017-08-28T14:10:15Z

Can you have a test that shows how this fits with the destruction code extent? Maybe even in the same test?

It should be

endregion for 'rce
drop/storagedead - 'dce is live here
endregion for 'dce

nikomatsakis · 2017-08-28T15:08:44Z

r=me modulo @arielb1's concern

bors · 2017-08-28T19:27:18Z

☔ The latest upstream changes (presumably #43076) made this pull request unmergeable. Please resolve the merge conflicts.

pnkfelix · 2017-08-30T12:43:36Z

Sorry its taking me so long to finish this off. I spent a while trying to create a case matching @arielb1's expectation. Now I think I have one, but I also am not sure whether the outline @arielb1 gave is what we should actually want to see in practice.

Here is some input code I've come up with:

fn main() {
    // This case illustrates one particular ordering of interest:
    D1(&S1("tmp"));
}

#[derive(Debug)]
struct S1(&'static str);

#[derive(Debug)]
struct D1<'a>(&'a S1);

impl<'a> Drop for D1<'a> {
    fn drop(&mut self) {
        println!("D1({:?})", self.0); 
    }
}

Here is the output MIR (compiled via: ./build/x86_64-apple-darwin/stage1/bin/rustc -Z identify_regions -Z span_free_formats -Z dump-mir=main ../src/test/mir-opt/end_region_destruction_extents_1.rs):

// MIR for `main`
// source = Fn(NodeId(4))
// pass_name = SimplifyCfg-initial
// disambiguator = after

fn main() -> () {
    let mut _0: ();
    let mut _1: D1<'160dce>;
    let mut _2: &'160dce S1;
    let mut _3: &'160dce S1;
    let mut _4: S1;

    bb0: {
        StorageLive(_2);
        StorageLive(_3);
        StorageLive(_4);
        _4 = S1::{{constructor}}(const "tmp",);
        _3 = &'160dce _4;
        _2 = &'160dce (*_3);
        _1 = D1<'160dce>::{{constructor}}(_2,);
        drop(_1) -> bb1;
    }

    bb1: {
        StorageDead(_2);
        StorageDead(_3);
        StorageDead(_4);
        EndRegion('160dce);
        _0 = ();
        return;
    }
}

So, here's my problem: I agree that EndRegion('160dce) should come after the drop(_1) in the control flow (since we need that region to live across the destructor invocation).

But it should precede the StorageDead(_4), right?

...

(Also, I just realized that this code actually only started compiling relatively recently; on the current stable, it is rejected with a suggestion that the S1("tmp") doesn't live long enough. So maybe this is a case where EndRegion emission needs to account for r-value promotion, which, if fixed, would result in the ordering drop(_1); EndRegion('160dce); StorageDead(_4); ... maybe?

nikomatsakis · 2017-08-30T14:51:21Z

(Also, I just realized that this code actually only started compiling relatively recently; on the current stable, it is rejected with a suggestion that the S1("tmp") doesn't live long enough. So maybe this is a case where EndRegion emission needs to account for r-value promotion, which, if fixed, would result in the ordering drop(_1); EndRegion('160dce); StorageDead(_4); ... maybe?

I think what I would expect is that we would run the qualification pass after construction, and hence wind up removing _4 in favor of a new static. In that case, the fact that StorageDead(_4) precedes EndRegion('160dce) wouldn't cause an error, because _4 is not being borrowed anymore.

arielb1 · 2017-08-30T15:47:09Z

@pnkfelix

Your example only compiles because of rvalue promotion. An rvalue-promotion version (or this one, on stable) errors as it should - today's borrowck basically puts the 'dce endregion after all the drops.

fn main() {
    // This case illustrates one particular ordering of interest:
    let x = "tmp";
    D1(&S1(x)); //~ ERROR
}

#[derive(Debug)]
struct S1(&'static str);

#[derive(Debug)]
struct D1<'a>(&'a S1);

impl<'a> Drop for D1<'a> {
    fn drop(&mut self) {
        println!("D1({:?})", self.0); 
    }
}

Also, you can easily have drop -> storagedead -> drop -> storagedead orderings, as in:

fn main() {
    D1((&D1(&S1("red")), &S1("tmp"), &D1(&S1("herring"))).1);
}

In which case there is no "between the drop and storagedead" place.

Therefore, I prefer having the drop & storagedead as 1 unit and placing the EndRegion after them. I believe this should work for all code that does not exploit soundness issues in the borrow checker.

arielb1 · 2017-08-30T15:50:55Z

I'll also note that moving the EndRegion to after the StorageDead can only make less code compile - you won't have any new "liveness" requirements (because StorageDead doesn't have any) but now the StorageDead can conflict with pre-existing borrows.

pnkfelix · 2017-08-30T15:54:42Z

@arielb1 ... okay, but isn't that in conflict with the property (taken from #43481 (comment) ):

For all 'rgn, and all lval such that the code has borrow(s) of form &'rgn lval, whereever StorageDead(lval) occurs, the region 'rgn has already ended (via EndRegion('rgn)).

pnkfelix · 2017-08-30T15:56:48Z

@arielb1 or maybe I misunderstand (once again)? Are you saying that we will enforce the property, because the compiler will reject such code?

arielb1 · 2017-08-30T15:59:58Z

@pnkfelix

Thats... what the borrow-check does. Or at least what it's supposed to do. You can't have a live borrow to something while you EndRegion it.

pnkfelix · 2017-08-30T16:09:10Z

@arielb1 okay, I was confused because I thought you were suggesting an ordering for {Drop,StorageDead,EndRegion} that you intended to be legal to the borrowck in cases like this.

pnkfelix · 2017-08-31T12:49:28Z

@bors r=nmatsakis

bors · 2017-08-31T12:49:29Z

📌 Commit 4a7c28c has been approved by nmatsakis

bors · 2017-08-31T12:54:29Z

⌛ Testing commit 4a7c28c9b1f956fb7577bd118adb9b2823f37bfe with merge 8318fdad5733a80e7d7449bfdbc08bd42cc0a47c...

bors · 2017-08-31T15:40:33Z

💔 Test failed - status-travis

kennytm · 2017-08-31T15:46:44Z

@bors retry

check i686-apple-darwin hung during doc test of libstd. Reason unknown.

[02:08:15] test collections/hash/set.rs - collections::hash::set::HashSet<T, S>::is_disjoint (line 542) ... ok
[02:08:15] test collections/hash/set.rs - collections::hash::set::HashSet<T, S>::is_empty (line 442) ... ok


No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

bors · 2017-08-31T17:00:33Z

⌛ Testing commit 4a7c28c9b1f956fb7577bd118adb9b2823f37bfe with merge 19db440bb17f57e851875ab641283c24abc621f2...

bors · 2017-08-31T17:56:14Z

💔 Test failed - status-travis

kennytm · 2017-08-31T18:02:47Z

@bors retry #44221

check i686-apple-darwin stuck again.

[00:18:16]    Compiling rustc_llvm v0.0.0 (file:///Users/travis/build/rust-lang/rust/src/librustc_llvm)
[00:18:20]    Compiling flate2 v0.2.19
[00:18:20]    Compiling rustc_errors v0.0.0 (file:///Users/travis/build/rust-lang/rust/src/librustc_errors)
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

bors · 2017-08-31T19:10:23Z

⌛ Testing commit 4a7c28c9b1f956fb7577bd118adb9b2823f37bfe with merge 055f72be4a23c82a6a4d6278440439b1fa3a69f3...

bors · 2017-08-31T21:08:09Z

💔 Test failed - status-travis

bors · 2017-09-01T09:56:20Z

⌛ Testing commit 4a7c28c9b1f956fb7577bd118adb9b2823f37bfe with merge 05446c7ffd17549af517b5bed93fb5066910a430...

bors · 2017-09-01T10:40:16Z

💔 Test failed - status-travis

kennytm · 2017-09-01T12:01:49Z

@bors ~~retry~~ ~~p=-1~~ p=0 #42117

[00:04:00] extracting /Users/travis/build/rust-lang/rust/build/cache/2017-07-18/cargo-beta-i686-apple-darwin.tar.gz
[00:04:01]     Updating registry `https://github.com/rust-lang/crates.io-index`
[00:05:16]     Updating git repository `https://github.com/rust-lang/cargo`

No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

~~(reduce priority because it has failed too many times, even if spurious.)~~ (Restore priority as re-r+ is expected)

bors · 2017-09-01T13:20:55Z

☔ The latest upstream changes (presumably #44171) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2017-09-01T17:59:17Z

@pnkfelix merge conflict :(

bors · 2017-09-04T10:18:04Z

🔒 Merge conflict

pnkfelix · 2017-09-05T13:14:18Z

@bors r=nmatsakis

bors · 2017-09-05T13:14:19Z

📌 Commit e2f00fc has been approved by nmatsakis

aidanhs · 2017-09-07T02:04:18Z

@bors r-

Failure seems related?

Check compiletest suite=mir-opt mode=mir-opt (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:49:22] 
[00:49:22] running 32 tests
[00:49:27] thread 'main' panicked at 'Some tests failed', /checkout/src/tools/compiletest/src/main.rs:323:21
[00:49:27] ..............F.F.F.............
[00:49:27] failures:
[00:49:27] 
[00:49:27] ---- [mir-opt] mir-opt/end_region_9.rs stdout ----
[00:49:27] 	thread '[mir-opt] mir-opt/end_region_9.rs' panicked at 'ran out of mir dump output to match against.
[00:49:27] Did not find expected line: "    let mut _4: &\'13_0rs i32;"

bors · 2017-09-07T15:57:55Z

☔ The latest upstream changes (presumably #44249) made this pull request unmergeable. Please resolve the merge conflicts.

…or a scope. (The idea is that the StorageDead marks the point where the memory can be deallocated, and the EndRegion is marking where borrows of that memory can no longer legally exist.)

Driveby fix to end_region_9.rs; it was missing END marker and was therefore always passing (regardless of output correctness).

pnkfelix · 2017-09-13T12:52:04Z

Thanks @aidanhs sorry about that!

pnkfelix · 2017-09-13T12:52:21Z

@bors r=nmatsakis

bors · 2017-09-13T12:52:22Z

📌 Commit 5fa0b66 has been approved by nmatsakis

bors · 2017-09-13T13:01:48Z

⌛ Testing commit 5fa0b66 with merge 539f208...

…sakis Fix end region emission order Fix #43481

bors · 2017-09-13T18:59:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nmatsakis
Pushing 539f208 to master...

rust-highfive assigned nikomatsakis Aug 28, 2017

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 28, 2017

pnkfelix force-pushed the fix-end-region-emission-order branch from 369e87d to 96e4f35 Compare August 29, 2017 08:48

pnkfelix force-pushed the fix-end-region-emission-order branch from 96e4f35 to 4a7c28c Compare August 31, 2017 11:34

kennytm mentioned this pull request Aug 31, 2017

Spurious failure with stalled macOS builds (specifically the check i686-apple-darwin job) #44221

Closed

pnkfelix force-pushed the fix-end-region-emission-order branch from 4a7c28c to e2f00fc Compare September 5, 2017 13:13

pnkfelix 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 5, 2017

aidanhs added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 7, 2017

pnkfelix added 4 commits September 13, 2017 12:55

Fix issue rust-lang#43481: emit the EndRegion *before* StorageDeads f…

25aa86d

…or a scope. (The idea is that the StorageDead marks the point where the memory can be deallocated, and the EndRegion is marking where borrows of that memory can no longer legally exist.)

Update mir-opt tests to reflect change to EndRegion emission order.

ab46142

Driveby fix to end_region_9.rs; it was missing END marker and was therefore always passing (regardless of output correctness).

Unit test for proper EndRegion emission on a cyclic reference.

88080bd

Test case illustrating some destruction code extent stuff.

5fa0b66

pnkfelix force-pushed the fix-end-region-emission-order branch from e2f00fc to 5fa0b66 Compare September 13, 2017 12:51

pnkfelix 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 13, 2017

bors added a commit that referenced this pull request Sep 13, 2017

Auto merge of #44129 - pnkfelix:fix-end-region-emission-order, r=nmat…

539f208

…sakis Fix end region emission order Fix #43481

bors merged commit 5fa0b66 into rust-lang:master Sep 13, 2017

Fix end region emission order #44129

Fix end region emission order #44129

Conversation

pnkfelix commented Aug 28, 2017

rust-highfive commented Aug 28, 2017

arielb1 commented Aug 28, 2017 • edited Loading

nikomatsakis commented Aug 28, 2017

bors commented Aug 28, 2017

pnkfelix commented Aug 30, 2017 • edited Loading

nikomatsakis commented Aug 30, 2017

arielb1 commented Aug 30, 2017 • edited Loading

arielb1 commented Aug 30, 2017

pnkfelix commented Aug 30, 2017

pnkfelix commented Aug 30, 2017

arielb1 commented Aug 30, 2017 • edited Loading

pnkfelix commented Aug 30, 2017

pnkfelix commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

kennytm commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

kennytm commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Sep 1, 2017

bors commented Sep 1, 2017

kennytm commented Sep 1, 2017 • edited Loading

bors commented Sep 1, 2017

nikomatsakis commented Sep 1, 2017

bors commented Sep 4, 2017

pnkfelix commented Sep 5, 2017

bors commented Sep 5, 2017

aidanhs commented Sep 7, 2017

bors commented Sep 7, 2017

pnkfelix commented Sep 13, 2017

pnkfelix commented Sep 13, 2017

bors commented Sep 13, 2017

bors commented Sep 13, 2017

bors commented Sep 13, 2017

arielb1 commented Aug 28, 2017 •

edited

Loading

pnkfelix commented Aug 30, 2017 •

edited

Loading

arielb1 commented Aug 30, 2017 •

edited

Loading

arielb1 commented Aug 30, 2017 •

edited

Loading

kennytm commented Sep 1, 2017 •

edited

Loading