Ignore borrowck for static lvalues and allow assignment to static muts #46032

KiChjang · 2017-11-16T10:57:39Z

nikomatsakis

Looks good! Just a few nits.

nikomatsakis · 2017-11-16T11:03:08Z

src/librustc_mir/borrow_check.rs

-                                    context, common_prefix, lvalue_span, bk,
-                                    &borrow, end_issued_loan_span)
+        // Ignore all borrows rooted in statics
+        if let Lvalue::Local(_) = *self.prefixes(lvalue_span.0, PrefixSet::All).last().unwrap() {


this if let makes me nervous -- if we add more lvalues in the future, it might silently squash them. I'd rather see an exhaustive match.

Alternatively, something like:

if let Lvalue::Static(_) = ... { // Skip checks on statics. See #123. } else { ... }

nikomatsakis · 2017-11-16T11:03:30Z

src/librustc_mir/borrow_check.rs

-                                    &borrow, end_issued_loan_span)
+        // Ignore all borrows rooted in statics
+        if let Lvalue::Local(_) = *self.prefixes(lvalue_span.0, PrefixSet::All).last().unwrap() {
+            self.each_borrow_involving_path(


also, I think this should be factored into a helper function personally

nikomatsakis · 2017-11-16T11:21:13Z

r? @nikomatsakis

KiChjang · 2017-11-16T11:27:06Z

Done. r? @nikomatsakis

arielb1 · 2017-11-16T12:15:40Z

src/test/run-pass/borrowck/borrowck-unsafe-static-mutable-borrows.rs

+// revisions: ast mir
+//[mir]compile-flags: -Z emit-end-regions -Z borrowck-mir
+
+// Test file taken from issue 45129 (https://github.com/rust-lang/rust/issues/45129)


This code is UB. Please don't have it as a run-pass test, because verified compilation will correctly complain.

to make it non-UB, e.g. use an array and access different array indices:

struct Foo { x: [usize; 2] } static mut SFOO: Foo = Foo { x: [23, 32] }; impl Foo { fn x(&mut self) -> &mut usize { &mut self.x[0] } } fn main() { unsafe { let x = SFOO.x(); SFOO.x[1] += 1; *x += 1; } }

arielb1 · 2017-11-16T12:21:01Z

src/librustc_mir/borrow_check.rs

+        // Check permissions
+        self.check_access_permissions(lvalue_span, rw);
+
+        match *self.prefixes(lvalue_span.0, PrefixSet::All).last().unwrap() {


Shouldn't borrows involving statics be handled in the same way as borrows involving raw pointers? I could open another issue for borrows through raw pointers, but they should be handled similarly.

struct Foo { x: [usize; 2] } static mut SFOO: Foo = Foo { x: [23, 32] }; impl Foo { fn x(&mut self) -> &mut usize { &mut self.x[0] } } fn main() { // this compiles with AST borrowck, should also compile with MIR borrowck unsafe { let sfoo: *mut Foo = &mut SFOO; let x = (*sfoo).x(); (*sfoo).x[1] += 1; *x += 1; } }

Yes, I agree they should be handled the same. Do you think that means however that we should unify the codepaths that make those two cases behave the same? If so, how do you think we should do it? (I think that sounds like a nice idea)

(In particular, if we decided to offer a lint at some point for obvious UB, I'd want it to apply to both.)

I guess if we just factor out a helper function unsafe_access_path(&Lvalue) it can test for either a deref of a raw pointer or the use of a static mut.

@nikomatsakis

One I think good way of doing it is just not registering borrows that go through an unsafe access path.

Note that for unsafe pointers this is different from checking the lvalue, but it is what AST borrowck does, e.g. in:

fn main() { let mut x = 1 as *const (); let y = unsafe { &*x }; // borrow through an unsafe pointer let _ = &mut x; // then mutate everything - should be OK println!("{:p}", y); }

I was avoiding recommending that because I wanted to consider adding a lint later on. But we could do it that way. Either way, we basically need the helper function I talked about, I imagine. It's just a question of whether we try to avoid registering the borrows in the first place, or try to ignore them when they are registered. =)

arielb1 · 2017-11-16T12:23:25Z

btw,

[00:02:41] tidy error: /checkout/src/test/run-pass/borrowck/borrowck-unsafe-static-mutable-borrows.rs:18: trailing whitespace

[00:02:41] tidy error: /checkout/src/test/run-pass/borrowck/borrowck-unsafe-static-mutable-borrows.rs:28: trailing whitespace

KiChjang · 2017-11-16T23:33:52Z

Alright, I've made changes to the test file, and am now also not registering borrows that are from an unsafe lvalue.

nikomatsakis

Looks good! I just think one part of it is no longer needed.

nikomatsakis · 2017-11-17T10:15:54Z

src/librustc_mir/dataflow/impls/borrows.rs

+
+        match *lvalue {
+            Local(_) => false,
+            Static(_) => true,


Only static mut is an unsafe lvalue, not any static at all.

nikomatsakis · 2017-11-17T10:16:36Z

src/librustc_mir/borrow_check.rs

-                                    context, common_prefix, lvalue_span, bk,
-                                    &borrow, end_issued_loan_span)
-                            }
+        fn check_borrows_readability_or_writability<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a>(


If we are suppressing the borrows, we shouldn't need any changes to this function.

nikomatsakis · 2017-11-17T10:16:57Z

src/test/run-pass/borrowck/borrowck-unsafe-static-mutable-borrows.rs

+        let sfoo: *mut Foo = &mut SFOO;
+        let x = (*sfoo).x();
+        (*sfoo).x[1] += 1;
+        *x += 1;


Looks good! =)

nikomatsakis

Lookin' good. One nit.

nikomatsakis · 2017-11-17T20:55:19Z

src/librustc_mir/dataflow/impls/borrows.rs

@@ -164,6 +203,34 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> {
            }
        }
    }
+
+    fn is_unsafe_lvalue(&self, lvalue: &mir::Lvalue<'tcx>) -> bool {


Why do we have this same function twice? Copy-and-paste error?

nikomatsakis · 2017-11-18T02:31:54Z

@bors r+

bors · 2017-11-18T02:31:55Z

📌 Commit f8ba371 has been approved by nikomatsakis

bors · 2017-11-18T17:30:32Z

⌛ Testing commit f8ba371 with merge b1409af...

Ignore borrowck for static lvalues and allow assignment to static muts Fixes #45129. Fixes #45641.

bors · 2017-11-18T19:51:34Z

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

KiChjang force-pushed the ignore-borrowck-statics branch 3 times, most recently from 54d3a29 to 64b7ac5 Compare November 16, 2017 11:02

nikomatsakis requested changes Nov 16, 2017

View reviewed changes

rust-highfive assigned nikomatsakis Nov 16, 2017

KiChjang force-pushed the ignore-borrowck-statics branch 2 times, most recently from e4190a5 to 0c62bfd Compare November 16, 2017 11:26

arielb1 reviewed Nov 16, 2017

View reviewed changes

pnkfelix mentioned this pull request Nov 16, 2017

MIR-borrowck: paths off of statics should not be borrow-checked #45129

Closed

kennytm added the S-waiting-on-author label Nov 16, 2017

KiChjang force-pushed the ignore-borrowck-statics branch from 0c62bfd to 086b92c Compare November 16, 2017 23:32

nikomatsakis requested changes Nov 17, 2017

View reviewed changes

Allow assignment to static muts

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

3be597a

KiChjang force-pushed the ignore-borrowck-statics branch 2 times, most recently from f22266b to 374047d Compare November 17, 2017 20:04

nikomatsakis requested changes Nov 17, 2017

View reviewed changes

Do not registor borrows for unsafe lvalues

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

c9d1db7

KiChjang force-pushed the ignore-borrowck-statics branch from 374047d to c9d1db7 Compare November 17, 2017 22:16

Add run-pass test for assignment to static mut

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

f8ba371

KiChjang force-pushed the ignore-borrowck-statics branch from e11a29a to f8ba371 Compare November 17, 2017 22:22

nikomatsakis approved these changes Nov 18, 2017

View reviewed changes

kennytm added S-waiting-on-bors and removed S-waiting-on-author labels Nov 18, 2017

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

Auto merge of #46032 - KiChjang:ignore-borrowck-statics, r=nikomatsakis

b1409af

Ignore borrowck for static lvalues and allow assignment to static muts Fixes #45129. Fixes #45641.

bors merged commit f8ba371 into rust-lang:master Nov 18, 2017

bors mentioned this pull request Nov 18, 2017

[MIR Borrowck] Handle borrows of array elements #46014

Closed

KiChjang deleted the ignore-borrowck-statics branch November 18, 2017 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore borrowck for static lvalues and allow assignment to static muts #46032

Ignore borrowck for static lvalues and allow assignment to static muts #46032

KiChjang commented Nov 16, 2017

nikomatsakis left a comment

nikomatsakis Nov 16, 2017

nikomatsakis Nov 16, 2017

nikomatsakis Nov 16, 2017

nikomatsakis commented Nov 16, 2017

KiChjang commented Nov 16, 2017

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 Nov 16, 2017 •

edited

Loading

nikomatsakis Nov 16, 2017

nikomatsakis Nov 16, 2017

nikomatsakis Nov 16, 2017

arielb1 Nov 16, 2017 •

edited

Loading

nikomatsakis Nov 16, 2017

arielb1 commented Nov 16, 2017

KiChjang commented Nov 16, 2017

nikomatsakis left a comment

nikomatsakis Nov 17, 2017

nikomatsakis Nov 17, 2017

nikomatsakis Nov 17, 2017

nikomatsakis left a comment

nikomatsakis Nov 17, 2017

nikomatsakis commented Nov 18, 2017

bors commented Nov 18, 2017

bors commented Nov 18, 2017

bors commented Nov 18, 2017

Ignore borrowck for static lvalues and allow assignment to static muts #46032

Ignore borrowck for static lvalues and allow assignment to static muts #46032

Conversation

KiChjang commented Nov 16, 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

nikomatsakis commented Nov 16, 2017

KiChjang commented Nov 16, 2017

arielb1 Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

arielb1 Nov 16, 2017 • edited Loading

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 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Nov 16, 2017

KiChjang commented Nov 16, 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

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Nov 18, 2017

bors commented Nov 18, 2017

bors commented Nov 18, 2017

bors commented Nov 18, 2017

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 Nov 16, 2017 •

edited

Loading