Auto merge of #55150 - pnkfelix:issues-47215-54797-fix-ice-from-moving-out-of-thread-local-under-ast-borrowck, r=nikomatsakis

bors · bors · commit f32f1113c93f · 2018-10-27T09:56:37.000Z
Do not allow moving out of thread local under ast borrowck AST borrowck failed to prevent moving out of a thread-local static. This was broken. And it also (sometimes?) caused an ICE during drop elaboration. Fix #47215 Fix #54797
diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs
@@ -93,6 +93,7 @@ use util::nodemap::ItemLocalSet;
 #[derive(Clone, Debug, PartialEq)]
 pub enum Categorization<'tcx> {
     Rvalue(ty::Region<'tcx>),            // temporary val, argument is its scope
+    ThreadLocal(ty::Region<'tcx>),       // value that cannot move, but still restricted in scope
     StaticItem,
     Upvar(Upvar),                        // upvar referenced by closure env
     Local(ast::NodeId),                  // local variable
@@ -268,6 +269,7 @@ impl<'tcx> cmt_<'tcx> {
             Categorization::Deref(ref base_cmt, _) => {
                 base_cmt.immutability_blame()
             }
+            Categorization::ThreadLocal(..) |
             Categorization::StaticItem => {
                 // Do we want to do something here?
                 None
@@ -715,17 +717,23 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
             }
 
             Def::Static(def_id, mutbl) => {
-                // `#[thread_local]` statics may not outlive the current function.
-                for attr in &self.tcx.get_attrs(def_id)[..] {
-                    if attr.check_name("thread_local") {
-                        return Ok(self.cat_rvalue_node(hir_id, span, expr_ty));
-                    }
-                }
+                // `#[thread_local]` statics may not outlive the current function, but
+                // they also cannot be moved out of.
+                let is_thread_local = self.tcx.get_attrs(def_id)[..]
+                    .iter()
+                    .any(|attr| attr.check_name("thread_local"));
+
+                let cat = if is_thread_local {
+                    let re = self.temporary_scope(hir_id.local_id);
+                    Categorization::ThreadLocal(re)
+                } else {
+                    Categorization::StaticItem
+                };
 
                 Ok(cmt_ {
                     hir_id,
-                    span:span,
-                    cat:Categorization::StaticItem,
+                    span,
+                    cat,
                     mutbl: if mutbl { McDeclared } else { McImmutable},
                     ty:expr_ty,
                     note: NoteNone
@@ -1408,6 +1416,7 @@ impl<'tcx> cmt_<'tcx> {
         match self.cat {
             Categorization::Rvalue(..) |
             Categorization::StaticItem |
+            Categorization::ThreadLocal(..) |
             Categorization::Local(..) |
             Categorization::Deref(_, UnsafePtr(..)) |
             Categorization::Deref(_, BorrowedPtr(..)) |
@@ -1439,6 +1448,7 @@ impl<'tcx> cmt_<'tcx> {
             }
 
             Categorization::Rvalue(..) |
+            Categorization::ThreadLocal(..) |
             Categorization::Local(..) |
             Categorization::Upvar(..) |
             Categorization::Deref(_, UnsafePtr(..)) => { // yes, it's aliasable, but...
@@ -1485,6 +1495,9 @@ impl<'tcx> cmt_<'tcx> {
             Categorization::StaticItem => {
                 "static item".into()
             }
+            Categorization::ThreadLocal(..) => {
+                "thread-local static item".into()
+            }
             Categorization::Rvalue(..) => {
                 "non-place".into()
             }
diff --git a/src/librustc_borrowck/borrowck/check_loans.rs b/src/librustc_borrowck/borrowck/check_loans.rs
@@ -377,6 +377,7 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
                 // by-move upvars, which is local data for generators
                 Categorization::Upvar(..) => true,
 
+                Categorization::ThreadLocal(region) |
                 Categorization::Rvalue(region) => {
                     // Rvalues promoted to 'static are no longer local
                     if let RegionKind::ReStatic = *region {
diff --git a/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs b/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs
@@ -177,6 +177,7 @@ fn check_and_get_illegal_move_origin<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
     match cmt.cat {
         Categorization::Deref(_, mc::BorrowedPtr(..)) |
         Categorization::Deref(_, mc::UnsafePtr(..)) |
+        Categorization::ThreadLocal(..) |
         Categorization::StaticItem => {
             Some(cmt.clone())
         }
diff --git a/src/librustc_borrowck/borrowck/gather_loans/lifetime.rs b/src/librustc_borrowck/borrowck/gather_loans/lifetime.rs
@@ -70,6 +70,7 @@ impl<'a, 'tcx> GuaranteeLifetimeContext<'a, 'tcx> {
 
         match cmt.cat {
             Categorization::Rvalue(..) |
+            Categorization::ThreadLocal(..) |
             Categorization::Local(..) |                     // L-Local
             Categorization::Upvar(..) |
             Categorization::Deref(_, mc::BorrowedPtr(..)) | // L-Deref-Borrowed
@@ -105,6 +106,7 @@ impl<'a, 'tcx> GuaranteeLifetimeContext<'a, 'tcx> {
         //! rooting etc, and presuming `cmt` is not mutated.
 
         match cmt.cat {
+            Categorization::ThreadLocal(temp_scope) |
             Categorization::Rvalue(temp_scope) => {
                 temp_scope
             }
diff --git a/src/librustc_borrowck/borrowck/gather_loans/move_error.rs b/src/librustc_borrowck/borrowck/gather_loans/move_error.rs
@@ -145,6 +145,8 @@ fn report_cannot_move_out_of<'a, 'tcx>(bccx: &'a BorrowckCtxt<'a, 'tcx>,
     match move_from.cat {
         Categorization::Deref(_, mc::BorrowedPtr(..)) |
         Categorization::Deref(_, mc::UnsafePtr(..)) |
+        Categorization::Deref(_, mc::Unique) |
+        Categorization::ThreadLocal(..) |
         Categorization::StaticItem => {
             bccx.cannot_move_out_of(
                 move_from.span, &move_from.descriptive_string(bccx.tcx), Origin::Ast)
@@ -166,7 +168,10 @@ fn report_cannot_move_out_of<'a, 'tcx>(bccx: &'a BorrowckCtxt<'a, 'tcx>,
                 }
             }
         }
-        _ => {
+
+        Categorization::Rvalue(..) |
+        Categorization::Local(..) |
+        Categorization::Upvar(..) => {
             span_bug!(move_from.span, "this path should not cause illegal move");
         }
     }
diff --git a/src/librustc_borrowck/borrowck/gather_loans/restrictions.rs b/src/librustc_borrowck/borrowck/gather_loans/restrictions.rs
@@ -70,6 +70,12 @@ impl<'a, 'tcx> RestrictionsContext<'a, 'tcx> {
                 RestrictionResult::Safe
             }
 
+            Categorization::ThreadLocal(..) => {
+                // Thread-locals are statics that have a scope, with
+                // no underlying structure to provide restrictions.
+                RestrictionResult::Safe
+            }
+
             Categorization::Local(local_id) => {
                 // R-Variable, locally declared
                 let lp = new_lp(LpVar(local_id));
diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs
@@ -520,6 +520,7 @@ pub fn opt_loan_path_is_field<'tcx>(cmt: &mc::cmt_<'tcx>) -> (Option<Rc<LoanPath
 
     match cmt.cat {
         Categorization::Rvalue(..) |
+        Categorization::ThreadLocal(..) |
         Categorization::StaticItem => {
             (None, false)
         }
diff --git a/src/librustc_passes/rvalue_promotion.rs b/src/librustc_passes/rvalue_promotion.rs
@@ -663,6 +663,7 @@ impl<'a, 'gcx, 'tcx> euv::Delegate<'tcx> for CheckCrateVisitor<'a, 'gcx> {
         let mut cur = cmt;
         loop {
             match cur.cat {
+                Categorization::ThreadLocal(..) |
                 Categorization::Rvalue(..) => {
                     if loan_cause == euv::MatchDiscriminant {
                         // Ignore the dummy immutable borrow created by EUV.
diff --git a/src/librustc_typeck/check/regionck.rs b/src/librustc_typeck/check/regionck.rs
@@ -1243,6 +1243,7 @@ impl<'a, 'gcx, 'tcx> RegionCtxt<'a, 'gcx, 'tcx> {
                 | Categorization::StaticItem
                 | Categorization::Upvar(..)
                 | Categorization::Local(..)
+                | Categorization::ThreadLocal(..)
                 | Categorization::Rvalue(..) => {
                     // These are all "base cases" with independent lifetimes
                     // that are not subject to inference
diff --git a/src/librustc_typeck/check/upvar.rs b/src/librustc_typeck/check/upvar.rs
@@ -401,6 +401,7 @@ impl<'a, 'gcx, 'tcx> InferBorrowKind<'a, 'gcx, 'tcx> {
 
             Categorization::Deref(_, mc::UnsafePtr(..)) |
             Categorization::StaticItem |
+            Categorization::ThreadLocal(..) |
             Categorization::Rvalue(..) |
             Categorization::Local(_) |
             Categorization::Upvar(..) => {
@@ -431,6 +432,7 @@ impl<'a, 'gcx, 'tcx> InferBorrowKind<'a, 'gcx, 'tcx> {
 
             Categorization::Deref(_, mc::UnsafePtr(..)) |
             Categorization::StaticItem |
+            Categorization::ThreadLocal(..) |
             Categorization::Rvalue(..) |
             Categorization::Local(_) |
             Categorization::Upvar(..) => {}
diff --git a/src/test/ui/borrowck/borrowck-thread-local-static-borrow-outlives-fn.ast.stderr b/src/test/ui/borrowck/borrowck-thread-local-static-borrow-outlives-fn.ast.stderr
@@ -2,9 +2,9 @@ error[E0597]: borrowed value does not live long enough
   --> $DIR/borrowck-thread-local-static-borrow-outlives-fn.rs:21:21
    |
 LL |      assert_static(&FOO); //[ast]~ ERROR [E0597]
-   |                     ^^^ - temporary value only lives until here
+   |                     ^^^ - borrowed value only lives until here
    |                     |
-   |                     temporary value does not live long enough
+   |                     borrowed value does not live long enough
    |
    = note: borrowed value must be valid for the static lifetime...
 
diff --git a/src/test/ui/borrowck/issue-47215-ice-from-drop-elab.nll.stderr b/src/test/ui/borrowck/issue-47215-ice-from-drop-elab.nll.stderr
@@ -0,0 +1,12 @@
+error[E0507]: cannot move out of static item
+  --> $DIR/issue-47215-ice-from-drop-elab.rs:17:21
+   |
+LL |         let mut x = X; //~ ERROR cannot move out of thread-local static item [E0507]
+   |                     ^
+   |                     |
+   |                     cannot move out of static item
+   |                     help: consider borrowing here: `&X`
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0507`.
diff --git a/src/test/ui/borrowck/issue-47215-ice-from-drop-elab.rs b/src/test/ui/borrowck/issue-47215-ice-from-drop-elab.rs
@@ -0,0 +1,20 @@
+// rust-lang/rust#47215: at one time, the compiler categorized
+// thread-local statics as a temporary rvalue, as a way to enforce
+// that they are only valid for a given lifetime.
+//
+// The problem with this is that you cannot move out of static items,
+// but you *can* move temporary rvalues. I.e., the categorization
+// above only solves half of the problem presented by thread-local
+// statics.
+
+#![feature(thread_local)]
+
+#[thread_local]
+static mut X: ::std::sync::atomic::AtomicUsize = ::std::sync::atomic::ATOMIC_USIZE_INIT;
+
+fn main() {
+    unsafe {
+        let mut x = X; //~ ERROR cannot move out of thread-local static item [E0507]
+        let _y = x.get_mut();
+    }
+}
diff --git a/src/test/ui/borrowck/issue-47215-ice-from-drop-elab.stderr b/src/test/ui/borrowck/issue-47215-ice-from-drop-elab.stderr
@@ -0,0 +1,12 @@
+error[E0507]: cannot move out of thread-local static item
+  --> $DIR/issue-47215-ice-from-drop-elab.rs:17:21
+   |
+LL |         let mut x = X; //~ ERROR cannot move out of thread-local static item [E0507]
+   |                     ^
+   |                     |
+   |                     cannot move out of thread-local static item
+   |                     help: consider using a reference instead: `&X`
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0507`.
diff --git a/src/test/ui/issues/issue-17954.rs b/src/test/ui/issues/issue-17954.rs
@@ -23,4 +23,4 @@ fn main() {
         println!("{}", a);
     });
 }
-//~^ NOTE temporary value only lives until here
+//~^ NOTE borrowed value only lives until here
diff --git a/src/test/ui/issues/issue-17954.stderr b/src/test/ui/issues/issue-17954.stderr
@@ -2,10 +2,10 @@ error[E0597]: borrowed value does not live long enough
   --> $DIR/issue-17954.rs:17:14
    |
 LL |     let a = &FOO;
-   |              ^^^ temporary value does not live long enough
+   |              ^^^ borrowed value does not live long enough
 ...
 LL | }
-   | - temporary value only lives until here
+   | - borrowed value only lives until here
    |
    = note: borrowed value must be valid for the static lifetime...
 

Original file line number	Diff line number	Diff line change
`@@ -177,6 +177,7 @@ fn check_and_get_illegal_move_origin<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,`
`177`	`177`	`match cmt.cat {`
`178`	`178`	`Categorization::Deref(_, mc::BorrowedPtr(..)) \|`
`179`	`179`	`Categorization::Deref(_, mc::UnsafePtr(..)) \|`
	`180`	`+ Categorization::ThreadLocal(..) \|`
`180`	`181`	`Categorization::StaticItem => {`
`181`	`182`	`Some(cmt.clone())`
`182`	`183`	`}`
Original file line number	Diff line number	Diff line change
`@@ -145,6 +145,8 @@ fn report_cannot_move_out_of<'a, 'tcx>(bccx: &'a BorrowckCtxt<'a, 'tcx>,`
`145`	`145`	`match move_from.cat {`
`146`	`146`	`Categorization::Deref(_, mc::BorrowedPtr(..)) \|`
`147`	`147`	`Categorization::Deref(_, mc::UnsafePtr(..)) \|`
	`148`	`+ Categorization::Deref(_, mc::Unique) \|`
	`149`	`+ Categorization::ThreadLocal(..) \|`
`148`	`150`	`Categorization::StaticItem => {`
`149`	`151`	`bccx.cannot_move_out_of(`
`150`	`152`	`move_from.span, &move_from.descriptive_string(bccx.tcx), Origin::Ast)`
`@@ -166,7 +168,10 @@ fn report_cannot_move_out_of<'a, 'tcx>(bccx: &'a BorrowckCtxt<'a, 'tcx>,`
`166`	`168`	`}`
`167`	`169`	`}`
`168`	`170`	`}`
`169`		`- _ => {`
	`171`	`+`
	`172`	`+ Categorization::Rvalue(..) \|`
	`173`	`+ Categorization::Local(..) \|`
	`174`	`+ Categorization::Upvar(..) => {`
`170`	`175`	`span_bug!(move_from.span, "this path should not cause illegal move");`
`171`	`176`	`}`
`172`	`177`	`}`
Original file line number	Diff line number	Diff line change
`@@ -520,6 +520,7 @@ pub fn opt_loan_path_is_field<'tcx>(cmt: &mc::cmt_<'tcx>) -> (Option<Rc<LoanPath`
`520`	`520`
`521`	`521`	`match cmt.cat {`
`522`	`522`	`Categorization::Rvalue(..) \|`
	`523`	`+ Categorization::ThreadLocal(..) \|`
`523`	`524`	`Categorization::StaticItem => {`
`524`	`525`	`(None, false)`
`525`	`526`	`}`