MIR borrowck: finalize `check_access_permissions()` #46041

zilbuz · 2017-11-16T16:45:58Z

Fix #44837 (hopefully for good)

arielb1 · 2017-11-16T18:22:11Z

src/librustc_mir/borrow_check.rs

+                    err.emit();
+                }
+            },
+            Write(WriteKind::MutableBorrow(BorrowKind::Shared)) |


This should be a delay_span_bug if this would cause an error - all 3 of these cases indicate an erroneous program.

arielb1 · 2017-11-16T18:25:19Z

src/librustc_mir/borrow_check.rs

@@ -501,7 +513,9 @@ impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> MirBorrowckCtxt<'c, 'b, 'a, 'gcx, 'tcx>
                     lvalue_span: (&Lvalue<'tcx>, Span),
                     kind: ShallowOrDeep,
                     mode: MutateMode,
+                     is_local_mutation_allowed: LocalMutationIsAllowed,


This function checks for mutation of locals anyway, so it should always pass LocalMutationIsAllowed::Yes to avoid double errors.

arielb1 · 2017-11-16T18:26:02Z

src/librustc_mir/borrow_check.rs

        let lvalue = lvalue_span.0;
        let ty = lvalue.ty(self.mir, self.tcx).to_ty(self.tcx);
        let moves_by_default =
            self.fake_infer_ctxt.type_moves_by_default(self.param_env, ty, DUMMY_SP);
        if moves_by_default {
            // move of lvalue: check if this is move of already borrowed path
-            self.access_lvalue(context, lvalue_span, (Deep, Write(WriteKind::Move)), flow_state);
+            self.access_lvalue(context, lvalue_span, (Deep, Write(WriteKind::Move)), LocalMutationIsAllowed::No, flow_state);


it is 100% OK to move out of an immutable local - this should be a yes.

arielb1 · 2017-11-16T18:27:57Z

src/librustc_mir/borrow_check.rs

@@ -549,7 +564,7 @@ impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> MirBorrowckCtxt<'c, 'b, 'a, 'gcx, 'tcx>
                    _ => unreachable!(),
                };
                self.access_lvalue(
-                    context, (lvalue, span), (Shallow(Some(af)), Read(ReadKind::Copy)), flow_state);
+                    context, (lvalue, span), (Shallow(Some(af)), Read(ReadKind::Copy)), LocalMutationIsAllowed::No, flow_state);


~~dropandreplace is an assignment of a type with a destructor, so this should be an error only if the value is already initialized.~~

This is a read, ignore all of this.

arielb1 · 2017-11-16T18:33:03Z

Commented. Need to leave for today.

bors · 2017-11-16T23:30:46Z

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

zilbuz · 2017-11-17T13:31:55Z

@arielb1 Sorry I won't be available for the rest of the week and I didn't have the time to finalize my PR. I have some changes corresponding to your comments but not yet pushed. Some tests aren't passing.

If this is blocking feel free to take the PR and finalize it yourself. Otherwise I'll finish it sunday or monday evening.

nikomatsakis · 2017-11-20T11:03:03Z

src/librustc_mir/borrow_check.rs

@@ -444,6 +446,12 @@ enum WriteKind {
    Move,
 }

+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+enum LocalMutationIsAllowed {


Nit: can you add a comment explaining what this flag is about?

zilbuz · 2017-11-25T15:26:24Z

@nikomatsakis @arielb1 There is an ICE on the following test: https://github.com/rust-lang/rust/blob/master/src/test/run-pass/borrowck/borrowck-unsafe-static-mutable-borrows.rs

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 sfoo: *mut Foo = &mut SFOO;
        let x = (*sfoo).x();
        (*sfoo).x[1] += 1;
        *x += 1;
    }
}

The delay_span_bug() in check_access_permissions() is triggered for sfoo with Write(StorageDeadOrDrop): https://github.com/rust-lang/rust/pull/46041/files#diff-e91f32610bea593e68f8e93f87419a29R1048

How should I fix this? Is the Write(StorageDeadOrDrop) authorized in certain cases?

arielb1 · 2017-11-25T20:10:35Z

@zilbuz

~~That's weird because the delay_span_bug should only occur for non-mut statics, and I don't see any non-mut static in this code.~~

arielb1 · 2017-11-25T20:17:37Z

src/librustc_mir/borrow_check.rs

+            Write(WriteKind::Move) |
+            Write(WriteKind::StorageDeadOrDrop) |
+            Write(WriteKind::MutableBorrow(BorrowKind::Shared)) => {
+                let item_msg = match self.describe_lvalue(lvalue) {


I think you're missing a check for whether the access is actually permitted, similarly to the previous lines. i.e. an

if let Err(lvalue_err) = self.is_mutable(lvalue, is_local_mutation_allowed) {

There's no reason to use pretty-printing in delay_span_bug - it can hide information and it just makes the code uglier. Just format the lvalue directly (format!("Accessing {:?} with the kind {:?} shouldn't be possible", lvalue, kind)).

arielb1 · 2017-11-25T20:18:06Z

src/librustc_mir/borrow_check.rs

+                        Some(name) => format!("`{}`", name),
+                        None => "item".to_owned()
+                    };
+                    span_bug!(span, "&unique borrow for {} should not fail", item_msg);


There's no reason to use pretty-printing in delay_span_bug - it can hide information and it just makes the code uglier. Just format the lvalue directly (format!("Accessing {:?} with the kind {:?} shouldn't be possible", lvalue, kind)).

arielb1 · 2017-11-25T20:19:28Z

Oops wrong delay_span_bug. However, I think I found the problem.

zilbuz · 2017-11-26T20:17:31Z

@arielb1 I added LocalMutationIsAllowed::Yes for the access_lvalue() calls in visit_terminator_entry(): https://github.com/rust-lang/rust/pull/46041/files#diff-e91f32610bea593e68f8e93f87419a29R397

bors · 2017-11-26T21:33:59Z

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

arielb1 · 2017-11-27T14:45:13Z

@bors r+

bors · 2017-11-27T14:45:14Z

📌 Commit abd07b7 has been approved by arielb1

bors · 2017-11-27T19:53:25Z

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

bors · 2017-11-28T02:13:10Z

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

…name

arielb1 · 2017-11-29T18:55:54Z

@bors r+

bors · 2017-11-29T18:55:55Z

📌 Commit 1cd9d74 has been approved by arielb1

bors · 2017-11-30T05:26:35Z

⌛ Testing commit 1cd9d74 with merge 909b94b...

@arielb1

MIR borrowck: finalize `check_access_permissions()` Fix #44837 (hopefully for good) r? @arielb1

bors · 2017-11-30T08:00:23Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 909b94b to master...

rust-highfive assigned arielb1 Nov 16, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2017

zilbuz force-pushed the issue-44837 branch from d7fe03b to 468281c Compare November 16, 2017 18:17

arielb1 reviewed Nov 16, 2017

View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2017

zilbuz force-pushed the issue-44837 branch from 468281c to 7e29213 Compare November 20, 2017 10:29

nikomatsakis reviewed Nov 20, 2017

View reviewed changes

zilbuz force-pushed the issue-44837 branch 3 times, most recently from d3135c6 to d4cc8b0 Compare November 25, 2017 10:31

zilbuz changed the title ~~[WIP] MIR borrowck: finalize check_access_permissions()~~ MIR borrowck: finalize check_access_permissions() Nov 25, 2017

arielb1 reviewed Nov 25, 2017

View reviewed changes

zilbuz force-pushed the issue-44837 branch 2 times, most recently from 65b5a73 to 4bd53ae Compare November 26, 2017 20:15

zilbuz force-pushed the issue-44837 branch from 4bd53ae to abd07b7 Compare November 27, 2017 11:54

arielb1 mentioned this pull request Nov 27, 2017

Rollup of 2 pull requests #46304

Closed

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 27, 2017

zilbuz force-pushed the issue-44837 branch from abd07b7 to 2368c58 Compare November 27, 2017 21:50

zilbuz added 2 commits November 28, 2017 13:44

mir-borrowck: describe_lvalue() can fail if a local doesn't have a …

503d25c

…name

mir-borrowck: Add permission check for WriteKind::Mutate

34f56c4

zilbuz force-pushed the issue-44837 branch from 2368c58 to 6ffa8f9 Compare November 28, 2017 14:13

mir-borrowck: Update tests

1cd9d74

zilbuz force-pushed the issue-44837 branch from 6ffa8f9 to 1cd9d74 Compare November 28, 2017 15:20

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

Auto merge of #46041 - zilbuz:issue-44837, r=arielb1

909b94b

MIR borrowck: finalize `check_access_permissions()` Fix #44837 (hopefully for good) r? @arielb1

bors merged commit 1cd9d74 into rust-lang:master Nov 30, 2017

This was referenced Nov 30, 2017

MIR-borrowck: immutable unique closure upvars can be mutated #46236

Merged

Always unwind through a Resume and other fixes #46320

Merged

zilbuz deleted the issue-44837 branch January 15, 2018 14:05

davidtwco mentioned this pull request Dec 17, 2018

Add a note describing the type of the non-Copy moved variable #56843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIR borrowck: finalize `check_access_permissions()` #46041

MIR borrowck: finalize `check_access_permissions()` #46041

zilbuz commented Nov 16, 2017

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 commented Nov 16, 2017

bors commented Nov 16, 2017

zilbuz commented Nov 17, 2017

nikomatsakis Nov 20, 2017

zilbuz commented Nov 25, 2017

arielb1 commented Nov 25, 2017 •

edited

Loading

arielb1 Nov 25, 2017

arielb1 Nov 25, 2017

arielb1 commented Nov 25, 2017

zilbuz commented Nov 26, 2017

bors commented Nov 26, 2017

arielb1 commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 28, 2017

arielb1 commented Nov 29, 2017

bors commented Nov 29, 2017

bors commented Nov 30, 2017

bors commented Nov 30, 2017

MIR borrowck: finalize check_access_permissions() #46041

MIR borrowck: finalize check_access_permissions() #46041

Conversation

zilbuz 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

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

arielb1 commented Nov 16, 2017

bors commented Nov 16, 2017

zilbuz commented Nov 17, 2017

nikomatsakis Nov 20, 2017

Choose a reason for hiding this comment

zilbuz commented Nov 25, 2017

arielb1 commented Nov 25, 2017 • edited Loading

arielb1 Nov 25, 2017

Choose a reason for hiding this comment

arielb1 Nov 25, 2017

Choose a reason for hiding this comment

arielb1 commented Nov 25, 2017

zilbuz commented Nov 26, 2017

bors commented Nov 26, 2017

arielb1 commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 28, 2017

arielb1 commented Nov 29, 2017

bors commented Nov 29, 2017

bors commented Nov 30, 2017

bors commented Nov 30, 2017

MIR borrowck: finalize `check_access_permissions()` #46041

MIR borrowck: finalize `check_access_permissions()` #46041

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 Nov 16, 2017 •

edited

Loading

arielb1 commented Nov 25, 2017 •

edited

Loading