Kill the storage for all locals on returning terminators #46100

KiChjang · 2017-11-19T12:26:53Z

nikomatsakis

Looks nice! I left a few minor nits as well as a request for some new tests.

nikomatsakis · 2017-11-19T12:38:25Z

src/librustc_mir/dataflow/impls/borrows.rs

+                    }
+                }
+            }
+            _ => {}


Nit: I'd prefer to see the variants here written out exhaustively. My rule of thumb: if a new terminator kind is added, what is the change this code might want to change? If it is not "rather low", make it exhaustive. I rate this chance as "moderate".

nikomatsakis · 2017-11-19T12:40:10Z

src/librustc_mir/dataflow/impls/borrows.rs

+            mir::TerminatorKind::Resume |
+            mir::TerminatorKind::Return |
+            mir::TerminatorKind::GeneratorDrop => {
+                for (borrow_index, borrow_data) in self.borrows.iter_enumerated() {


This would be a good place for a comment. Something like:

// When we return from the function, then all `ReScope`-style regions are guaranteed to have ended. // Normally, there would be `EndRegion` statements that come before, and hence most of these loans // will already be dead -- but, in some cases like unwind paths, we do not always emit `EndRegion` // statements, so we add some kills here as a "backup" and to avoid spurious error messages.

nikomatsakis · 2017-11-19T12:41:08Z

src/librustc_mir/borrow_check.rs

            TerminatorKind::Resume |
            TerminatorKind::Return |
-            TerminatorKind::GeneratorDrop |
+            TerminatorKind::GeneratorDrop => {
+                for local in self.mir.local_decls.indices() {


Good place for a comment, me thinks.

// Returning from the function implicitly kills storage for all locals. // Often, the storage will already have been killed by an explicit // StorageDead, but we don't always emit those (notably on unwind paths), // so this "extra check" serves as a kind of backup.

nikomatsakis · 2017-11-19T12:45:44Z

src/librustc_mir/borrow_check.rs

+                        (&Lvalue::Local(local), span),
+                        (Shallow(None), Write(WriteKind::StorageDeadOrDrop)),
+                        flow_state
+                    );


This PR is also aiming to check for in-scope loans of "thread-local statics" and report an error. It may be that it already prevents those. To check, can you add the following test as src/test/compile-fail/borrowck/thread-local-static-borrow-outlives-fn.rs (I don't believe we have any such test already):

// Test that borrows of `#[thread_local]` statics cannot // outlive the function. See also #17954. // revisions: ast mir #![feature(thread_local)] #[thread_local] static FOO: u8 = 3; fn assert_static(_t: &'static u8) {} fn main() { assert_static(&FOO); }

This currently gives a somewhat obscure error about the lifetime of a temporary value, which makes me think that it will likely generate an error under your PR as well, presuming MIR introduces a temporary at the same spot. I'm actually a bit unsure just why we are generating a temporary here -- presumably something to do with how we lower access to #[thread_local] statics. Anyway, let's add the test. If it gets an error in both checkers, great, otherwise, we'll have to add a bit of code right around here I think to check for outstanding borrows of #[thread_local] statics.

nikomatsakis · 2017-11-19T12:52:47Z

src/test/run-fail/borrowck-local-borrow.rs

+    let x = 2;
+    let y = &x;
+    panic!("panic 1");
+}


Can you also add the test that @arielb1 added to the issue and check that it generates two errors (one for each function) -- both in AST and MIR mode? A quick test with play seems to suggest that it does not generate two errors today, but it'd be good to verify that your test is not working before your patch, and works after while you are at it.

fn cplusplus_mode(x: isize) -> &'static isize { &x } fn cplusplus_mode_exceptionally_unsafe(x: &mut Option<&'static mut isize>) { let z = 0; *x = Some(&mut z); panic!("catch me for a dangling pointer!") }

KiChjang · 2017-11-19T20:34:20Z

Ok, so I added two tests, both of which are failing; borrowck-local-borrow-outlives-fn.rs fails because the MIR borrowck error message differs from AST borrowck, whereas borrowck-local-static-borrow-outlives-fn.rs fails because MIR borrowck is actually not catching the borrow correctly for static locals.

KiChjang · 2017-11-20T00:24:40Z

The latest force-push contains changes to the logic in checking for StorageDeads; instead of looping through the local declarations, we loop through the borrows. This is because it's much easier to get the span of the borrow for error reporting purposes.

KiChjang · 2017-11-20T00:26:09Z

src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs

+    let mut z = 0;
+    *x = Some(&mut z); //[ast]~ ERROR `z` does not live long enough
+                       //[mir]~^ ERROR `z` does not live long enough (Ast)
+                       //[mir]~| ERROR `z` does not live long enough (Mir)


The only problem left here now is this - MIR borrowck doesn't generate an error for this.

The problem here is TerminatorKind::Call with a None unwind terminator, which just means that the call immediately jumps to a resume.

I feel it's a good idea to have MIR construction instead link all None call terminators to a resume block, and have a later pass (e.g. SimplifyBranches) clean up unwind paths that don't actually do anything so they won't choke LLVM.

I suppose you can remove this test and I'll handle the change myself.

KiChjang · 2017-11-20T00:29:19Z

src/librustc_mir/borrow_check.rs

+                            self.access_lvalue(
+                                ContextKind::StorageDead.new(loc),
+                                (&root_lvalue, self.mir.source_info(borrow.location).span),
+                                (Deep, Write(WriteKind::StorageDeadOrDrop)),


The other thing I'm not sure about is whether this should be a shallow write instead.

This should be a drop - basically a shallow write for structs without a destructor, and a deep write for structs with one.

This is because you want to avoid the likes of #31567 while still permitting borrowing the interior of types such as &mut references, as in:

fn foo<'a>(x: &'a mut u32) -> &'a mut u32 { let result : &'a mut u32 = &mut *x; if true { result } else { panic!() // this destroys `x`, but not `*x` } }

Ah so this code only runs for thread-locals. In that case, I suppose there won't be much harm from it remaining a deep write.

arielb1 · 2017-11-20T18:02:58Z

nice PR!

r=me with the cplusplus_mode_exceptionally_unsafe test removed - I'll fix it myself.

QuietMisdreavus · 2017-11-21T01:52:50Z

@bors r=arielb1

bors · 2017-11-21T01:52:51Z

📌 Commit 99d44aa has been approved by arielb1

bors · 2017-11-24T18:50:35Z

⌛ Testing commit 99d44aa353d2dc399b695624b5138ca56c089f60 with merge ae47198f7629abf0b226f7f1565fd734213e5ffb...

bors · 2017-11-24T19:50:22Z

💔 Test failed - status-travis

kennytm · 2017-11-24T19:58:50Z

mir-opt/nll/region-liveness-drop-no-may-dangle.rs failed on i686-musl. The test doesn't seem 32-bit- or musl-specific though, so the failure is probably affected by recent merges. Try rebase on the latest master.

[00:54:59] failures:
[00:54:59] 
[00:54:59] ---- [mir-opt] mir-opt/nll/region-liveness-drop-no-may-dangle.rs stdout ----
[00:54:59] 	
[00:54:59] error: compilation failed!
[00:54:59] status: exit code: 101
[00:54:59] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/mir-opt/nll/region-liveness-drop-no-may-dangle.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt" "--target=i686-unknown-linux-musl" "-Zdump-mir=all" "-Zmir-opt-level=3" "-Zdump-mir-exclude-pass-number" "-Zdump-mir-dir=/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/nll/region-liveness-drop-no-may-dangle" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/nll/region-liveness-drop-no-may-dangle.stage2-i686-unknown-linux-musl" "-Crpath" "-O" "-Lnative=/checkout/obj/build/i686-unknown-linux-musl/native/rust-test-helpers" "-Clinker=/musl-i686/bin/musl-gcc" "-Znll" "-Zverbose" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/nll/region-liveness-drop-no-may-dangle.stage2-i686-unknown-linux-musl.aux"
[00:54:59] stdout:
[00:54:59] ------------------------------------------
[00:54:59] 
[00:54:59] ------------------------------------------
[00:54:59] stderr:
[00:54:59] ------------------------------------------
[00:54:59] error[E0597]: borrowed value does not live long enough (Mir)
[00:54:59]   --> /checkout/src/test/mir-opt/nll/region-liveness-drop-no-may-dangle.rs:25:51
[00:54:59]    |
[00:54:59] thread 'main' panicked at 'Some tests failed', /checkout/src/tools/compiletest/src/main.rs:331:21
[00:54:59] 24 |     let mut v = [1, 2, 3];
[00:54:59]    |         ----- temporary value created here
[00:54:59] 25 |     let p: Wrap<& /* R1 */ usize> = Wrap { value: &v[0] };
[00:54:59]    |                                                   ^^^^^ temporary value dropped here while still borrowed
[00:54:59]    |
[00:54:59]    = note: consider using a `let` binding to increase its lifetime
[00:54:59] 
[00:54:59] error: aborting due to previous error
[00:54:59] 
[00:54:59] 
[00:54:59] ------------------------------------------
[00:54:59] 
[00:54:59] thread '[mir-opt] mir-opt/nll/region-liveness-drop-no-may-dangle.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2570:8
[00:54:59] note: Run with `RUST_BACKTRACE=1` for a backtrace.

KiChjang · 2017-11-24T20:01:25Z

Rebased.

KiChjang · 2017-11-24T23:36:04Z

Same error as above - I wonder if my patch is causing the temporary rvalue to be dropped too soon...

kennytm · 2017-11-25T06:01:28Z

@KiChjang rebasing won't fix the problem, it just exposes the problem on the CI early on instead of wait until bors r+ (ensure the CI status is truly red instead of falsely green). It also allows one to debug it locally easier.

est31 · 2017-11-25T07:53:17Z

src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs

+// except according to those terms.
+
+// revisions: ast mir
+//[mir]compile-flags: -Z emit-end-regions -Z borrowck-mir


You don't need the emit-end-regions flag any more btw, thanks to this very nice PR: #44717. My PR #46106 is removing them everywhere but I guess I should start spreading the word so that the line doesn't keep getting added :p.

The comment applies to the other places where you have -Z emit-end-regions as well, but I won't comment on each of them.

arielb1 · 2017-11-25T20:47:40Z

@KiChjang

How are you doing debugging this? Should I check in and try to get this working?

arielb1 · 2017-11-25T22:03:13Z

@bors r=nikomatsakis

bors · 2017-11-25T22:03:14Z

📌 Commit efa3ed2 has been approved by nikomatsakis

bors · 2017-11-26T11:43:25Z

⌛ Testing commit efa3ed2 with merge 2ca00a9...

Kill the storage for all locals on returning terminators Fixes #45704.

bors · 2017-11-26T14:10:32Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 2ca00a9 to master...

nikomatsakis requested changes Nov 19, 2017

View reviewed changes

nikomatsakis self-assigned this Nov 19, 2017

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

KiChjang force-pushed the mass-dead-check branch from 15223c1 to 15494cd Compare November 19, 2017 20:24

KiChjang force-pushed the mass-dead-check branch from 15494cd to 768a89a Compare November 20, 2017 00:22

KiChjang commented Nov 20, 2017

View reviewed changes

KiChjang force-pushed the mass-dead-check branch from 768a89a to 99d44aa Compare November 20, 2017 21:28

kennytm 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 Nov 21, 2017

nikomatsakis approved these changes Nov 22, 2017

View reviewed changes

arielb1 mentioned this pull request Nov 22, 2017

Add a MIR-borrowck-only output mode #46106

Merged

kennytm 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 Nov 24, 2017

Kill the storage for all locals on returning terminators

0996bb0

KiChjang force-pushed the mass-dead-check branch from 99d44aa to 0996bb0 Compare November 24, 2017 20:01

est31 reviewed Nov 25, 2017

View reviewed changes

KiChjang mentioned this pull request Nov 25, 2017

region-liveness-drop-no-may-dangle.rs compilation failure on MIR borrowck #46267

Closed

Disable region-liveness-drop-no-may-dangle.rs

efa3ed2

kennytm 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 Nov 26, 2017

bors added a commit that referenced this pull request Nov 26, 2017

Auto merge of #46100 - KiChjang:mass-dead-check, r=nikomatsakis

2ca00a9

Kill the storage for all locals on returning terminators Fixes #45704.

bors merged commit efa3ed2 into rust-lang:master Nov 26, 2017

KiChjang deleted the mass-dead-check branch November 26, 2017 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kill the storage for all locals on returning terminators #46100

Kill the storage for all locals on returning terminators #46100

KiChjang commented Nov 19, 2017

nikomatsakis left a comment

nikomatsakis Nov 19, 2017

nikomatsakis Nov 19, 2017

nikomatsakis Nov 19, 2017

nikomatsakis Nov 19, 2017

nikomatsakis Nov 19, 2017

KiChjang commented Nov 19, 2017

KiChjang commented Nov 20, 2017

KiChjang Nov 20, 2017

arielb1 Nov 20, 2017

arielb1 Nov 20, 2017

KiChjang Nov 20, 2017

arielb1 Nov 20, 2017 •

edited

Loading

arielb1 Nov 20, 2017

arielb1 commented Nov 20, 2017 •

edited

Loading

QuietMisdreavus commented Nov 21, 2017

bors commented Nov 21, 2017

bors commented Nov 24, 2017

bors commented Nov 24, 2017

kennytm commented Nov 24, 2017

KiChjang commented Nov 24, 2017

KiChjang commented Nov 24, 2017

kennytm commented Nov 25, 2017 •

edited

Loading

est31 Nov 25, 2017

arielb1 commented Nov 25, 2017

arielb1 commented Nov 25, 2017

bors commented Nov 25, 2017

bors commented Nov 26, 2017

bors commented Nov 26, 2017

Kill the storage for all locals on returning terminators #46100

Kill the storage for all locals on returning terminators #46100

Conversation

KiChjang commented Nov 19, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiChjang commented Nov 19, 2017

KiChjang commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Nov 20, 2017 • edited Loading

QuietMisdreavus commented Nov 21, 2017

bors commented Nov 21, 2017

bors commented Nov 24, 2017

bors commented Nov 24, 2017

kennytm commented Nov 24, 2017

KiChjang commented Nov 24, 2017

KiChjang commented Nov 24, 2017

kennytm commented Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

arielb1 commented Nov 25, 2017

arielb1 commented Nov 25, 2017

bors commented Nov 25, 2017

bors commented Nov 26, 2017

bors commented Nov 26, 2017

arielb1 Nov 20, 2017 •

edited

Loading

arielb1 commented Nov 20, 2017 •

edited

Loading

kennytm commented Nov 25, 2017 •

edited

Loading