Emit StorageDead for all locals

Some errors in tests now get suppressed as a side-effect of this change.
We should discuss in review whether we are okay with this.

Author: tmandry

Diff for: src/librustc_mir/build/scope.rs

@@ -83,9 +83,10 @@ use rustc::middle::region;
 use rustc::ty::Ty;
 use rustc::hir;
 use rustc::mir::*;
-use syntax_pos::{Span};
+use syntax_pos::{Span, DUMMY_SP};
 use rustc_data_structures::fx::FxHashMap;
 use std::collections::hash_map::Entry;
+use std::mem;
 
 #[derive(Debug)]
 pub struct Scope<'tcx> {
@@ -98,16 +99,10 @@ pub struct Scope<'tcx> {
     /// the span of that region_scope
     region_scope_span: Span,
 
-    /// Whether there's anything to do for the cleanup path, that is,
+    /// Whether we need landing pads for the cleanup path, that is,
     /// when unwinding through this scope. This includes destructors,
-    /// but not StorageDead statements, which don't get emitted at all
-    /// for unwinding, for several reasons:
-    ///  * clang doesn't emit llvm.lifetime.end for C++ unwinding
-    ///  * LLVM's memory dependency analysis can't handle it atm
-    ///  * polluting the cleanup MIR with StorageDead creates
-    ///    landing pads even though there's no actual destructors
-    ///  * freeing up stack space has no effect during unwinding
-    needs_cleanup: bool,
+    /// but not StorageDead statements.
+    cleanup_may_panic: bool,
 
     /// set of places to drop when exiting this scope. This starts
     /// out empty but grows as variables are declared during the
@@ -348,7 +343,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             source_scope: vis_scope,
             region_scope: region_scope.0,
             region_scope_span: region_scope.1.span,
-            needs_cleanup: false,
+            cleanup_may_panic: false,
             drops: vec![],
             cached_generator_drop: None,
             cached_exits: Default::default(),
@@ -411,7 +406,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 
         // If we are emitting a `drop` statement, we need to have the cached
         // diverge cleanup pads ready in case that drop panics.
-        let may_panic = self.scopes[(len - scope_count)..].iter().any(|s| s.needs_cleanup);
+        let may_panic = self.scopes[(len - scope_count)..].iter().any(|s| s.cleanup_may_panic);
         if may_panic {
             self.diverge_cleanup();
         }
@@ -466,10 +461,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
     /// This path terminates in GeneratorDrop. Returns the start of the path.
     /// None indicates there’s no cleanup to do at this point.
     pub fn generator_drop_cleanup(&mut self) -> Option<BasicBlock> {
-        if !self.scopes.iter().any(|scope| scope.needs_cleanup) {
-            return None;
-        }
-
         // Fill in the cache for unwinds
         self.diverge_cleanup_gen(true);
 
@@ -480,10 +471,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         let result = block;
 
         while let Some(scope) = scopes.next() {
-            if !scope.needs_cleanup {
-                continue;
-            }
-
             block = if let Some(b) = scope.cached_generator_drop {
                 self.cfg.terminate(block, src_info,
                                    TerminatorKind::Goto { target: b });
@@ -623,7 +610,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 
     // Schedule an abort block - this is used for some ABIs that cannot unwind
     pub fn schedule_abort(&mut self) -> BasicBlock {
-        self.scopes[0].needs_cleanup = true;
+        self.scopes[0].cleanup_may_panic = true;
         let abortblk = self.cfg.start_new_cleanup_block();
         let source_info = self.scopes[0].source_info(self.fn_span);
         self.cfg.terminate(abortblk, source_info, TerminatorKind::Abort);
@@ -735,7 +722,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             scope.invalidate_cache(!needs_drop, this_scope);
             if this_scope {
                 if let DropKind::Value { .. } = drop_kind {
-                    scope.needs_cleanup = true;
+                    scope.cleanup_may_panic = true;
                 }
 
                 let region_scope_span = region_scope.span(self.hir.tcx(),
@@ -900,12 +887,6 @@ fn build_scope_drops<'tcx>(
     // drops panic (panicking while unwinding will abort, so there's no need for
     // another set of arrows). The drops for the unwind path should have already
     // been generated by `diverge_cleanup_gen`.
-    //
-    // The code in this function reads from right to left.
-    // Storage dead drops have to be done left to right (since we can only push
-    // to the end of a Vec). So, we find the next drop and then call
-    // push_storage_deads which will iterate backwards through them so that
-    // they are added in the correct order.
 
     let mut unwind_blocks = scope.drops.iter().rev().filter_map(|drop_data| {
         if let DropKind::Value { cached_block } = drop_data.kind {
@@ -936,11 +917,6 @@ fn build_scope_drops<'tcx>(
                 block = next;
             }
             DropKind::Storage => {
-                // We do not need to emit StorageDead for generator drops
-                if generator_drop {
-                    continue
-                }
-
                 // Drop the storage for both value and storage drops.
                 // Only temps and vars need their storage dead.
                 match drop_data.location {
@@ -958,12 +934,12 @@ fn build_scope_drops<'tcx>(
     block.unit()
 }
 
-fn build_diverge_scope<'tcx>(cfg: &mut CFG<'tcx>,
-                             span: Span,
-                             scope: &mut Scope<'tcx>,
-                             mut target: BasicBlock,
-                             generator_drop: bool)
-                             -> BasicBlock
+fn build_diverge_scope(cfg: &mut CFG<'tcx>,
+                       span: Span,
+                       scope: &mut Scope<'tcx>,
+                       mut target: BasicBlock,
+                       generator_drop: bool)
+                       -> BasicBlock
 {
     // Build up the drops in **reverse** order. The end result will
     // look like:
@@ -981,7 +957,13 @@ fn build_diverge_scope<'tcx>(cfg: &mut CFG<'tcx>,
         scope: source_scope
     };
 
-    // Next, build up the drops. Here we iterate the vector in
+    // We keep track of StorageDead statements to prepend to our current block
+    // and store them here, in reverse order.
+    let mut storage_deads = vec![];
+
+    let mut target_built_by_us = false;
+
+    // Build up the drops. Here we iterate the vector in
     // *forward* order, so that we generate drops[0] first (right to
     // left in diagram above).
     for (j, drop_data) in scope.drops.iter_mut().enumerate() {
@@ -994,28 +976,70 @@ fn build_diverge_scope<'tcx>(cfg: &mut CFG<'tcx>,
         // frozen stack)? In particular, the intent may have been to
         // match the behavior of clang, but on inspection eddyb says
         // this is not what clang does.
-        let cached_block = match drop_data.kind {
-            DropKind::Value { ref mut cached_block } => cached_block.ref_mut(generator_drop),
-            DropKind::Storage => continue
-        };
-        target = if let Some(cached_block) = *cached_block {
-            cached_block
-        } else {
-            let block = cfg.start_new_cleanup_block();
-            cfg.terminate(block, source_info(drop_data.span),
-                          TerminatorKind::Drop {
-                              location: drop_data.location.clone(),
-                              target,
-                              unwind: None
-                          });
-            *cached_block = Some(block);
-            block
+        match drop_data.kind {
+            DropKind::Storage => {
+                // Only temps and vars need their storage dead.
+                match drop_data.location {
+                    Place::Base(PlaceBase::Local(index)) => {
+                        storage_deads.push(Statement {
+                            source_info: source_info(drop_data.span),
+                            kind: StatementKind::StorageDead(index)
+                        });
+                    }
+                    _ => unreachable!(),
+                };
+            }
+            DropKind::Value { ref mut cached_block } => {
+                let cached_block = cached_block.ref_mut(generator_drop);
+                target = if let Some(cached_block) = *cached_block {
+                    storage_deads.clear();
+                    target_built_by_us = false;
+                    cached_block
+                } else {
+                    push_storage_deads(
+                        cfg, &mut target, &mut storage_deads, target_built_by_us, source_scope);
+                    let block = cfg.start_new_cleanup_block();
+                    cfg.terminate(block, source_info(drop_data.span),
+                                  TerminatorKind::Drop {
+                                      location: drop_data.location.clone(),
+                                      target,
+                                      unwind: None
+                                  });
+                    *cached_block = Some(block);
+                    target_built_by_us = true;
+                    block
+                };
+            }
         };
     }
-
+    push_storage_deads(cfg, &mut target, &mut storage_deads, target_built_by_us, source_scope);
     *scope.cached_unwind.ref_mut(generator_drop) = Some(target);
 
+    assert!(storage_deads.is_empty());
     debug!("build_diverge_scope({:?}, {:?}) = {:?}", scope, span, target);
 
     target
 }
+
+fn push_storage_deads(cfg: &mut CFG<'tcx>,
+                      target: &mut BasicBlock,
+                      storage_deads: &mut Vec<Statement<'tcx>>,
+                      target_built_by_us: bool,
+                      source_scope: SourceScope) {
+    if storage_deads.is_empty() { return; }
+    if !target_built_by_us {
+        // We cannot add statements to an existing block, so we create a new
+        // block for our StorageDead statements.
+        let block = cfg.start_new_cleanup_block();
+        let source_info = SourceInfo { span: DUMMY_SP, scope: source_scope };
+        cfg.terminate(block, source_info, TerminatorKind::Goto { target: *target });
+        *target = block;
+    }
+    let statements = &mut cfg.block_data_mut(*target).statements;
+    storage_deads.reverse();
+    debug!("push_storage_deads({:?}), storage_deads={:?}, statements={:?}",
+           *target, storage_deads, statements);
+    storage_deads.append(statements);
+    mem::swap(statements, storage_deads);
+    assert!(storage_deads.is_empty());
+}

Diff for: src/test/ui/dropck/dropck_trait_cycle_checked.stderr

@@ -21,30 +21,6 @@ LL |     o1.set1(&o3);
 LL | }
    | - `o3` dropped here while still borrowed
 
-error[E0597]: `o2` does not live long enough
-  --> $DIR/dropck_trait_cycle_checked.rs:113:13
-   |
-LL |     let (o1, o2, o3): (Box<Obj>, Box<Obj>, Box<Obj>) = (O::new(), O::new(), O::new());
-   |                                                                   -------- cast requires that `o2` is borrowed for `'static`
-...
-LL |     o2.set0(&o2);
-   |             ^^^ borrowed value does not live long enough
-...
-LL | }
-   | - `o2` dropped here while still borrowed
-
-error[E0597]: `o3` does not live long enough
-  --> $DIR/dropck_trait_cycle_checked.rs:114:13
-   |
-LL |     let (o1, o2, o3): (Box<Obj>, Box<Obj>, Box<Obj>) = (O::new(), O::new(), O::new());
-   |                                                                             -------- cast requires that `o3` is borrowed for `'static`
-...
-LL |     o2.set1(&o3);
-   |             ^^^ borrowed value does not live long enough
-...
-LL | }
-   | - `o3` dropped here while still borrowed
-
 error[E0597]: `o1` does not live long enough
   --> $DIR/dropck_trait_cycle_checked.rs:115:13
    |
@@ -57,17 +33,6 @@ LL |     o3.set1(&o2);
 LL | }
    | - `o1` dropped here while still borrowed
 
-error[E0597]: `o2` does not live long enough
-  --> $DIR/dropck_trait_cycle_checked.rs:116:13
-   |
-LL |     let (o1, o2, o3): (Box<Obj>, Box<Obj>, Box<Obj>) = (O::new(), O::new(), O::new());
-   |                                                                   -------- cast requires that `o2` is borrowed for `'static`
-...
-LL |     o3.set1(&o2);
-   |             ^^^ borrowed value does not live long enough
-LL | }
-   | - `o2` dropped here while still borrowed
-
-error: aborting due to 6 previous errors
+error: aborting due to 3 previous errors
 
 For more information about this error, try `rustc --explain E0597`.

Diff for: src/test/ui/nll/user-annotations/constant-in-expr-inherent-2.stderr

@@ -10,41 +10,6 @@ LL |     FUN(&x);
 LL | }
    | - `x` dropped here while still borrowed
 
-error[E0597]: `x` does not live long enough
-  --> $DIR/constant-in-expr-inherent-2.rs:26:23
-   |
-LL |     A::ASSOCIATED_FUN(&x);
-   |     ------------------^^-
-   |     |                 |
-   |     |                 borrowed value does not live long enough
-   |     argument requires that `x` is borrowed for `'static`
-...
-LL | }
-   | - `x` dropped here while still borrowed
-
-error[E0597]: `x` does not live long enough
-  --> $DIR/constant-in-expr-inherent-2.rs:27:28
-   |
-LL |     B::ALSO_ASSOCIATED_FUN(&x);
-   |     -----------------------^^-
-   |     |                      |
-   |     |                      borrowed value does not live long enough
-   |     argument requires that `x` is borrowed for `'static`
-LL |     <_>::TRAIT_ASSOCIATED_FUN(&x);
-LL | }
-   | - `x` dropped here while still borrowed
-
-error[E0597]: `x` does not live long enough
-  --> $DIR/constant-in-expr-inherent-2.rs:28:31
-   |
-LL |     <_>::TRAIT_ASSOCIATED_FUN(&x);
-   |     --------------------------^^-
-   |     |                         |
-   |     |                         borrowed value does not live long enough
-   |     argument requires that `x` is borrowed for `'static`
-LL | }
-   | - `x` dropped here while still borrowed
-
-error: aborting due to 4 previous errors
+error: aborting due to previous error
 
 For more information about this error, try `rustc --explain E0597`.

Diff for: src/test/ui/nll/user-annotations/method-ufcs-inherent-2.stderr

@@ -13,21 +13,6 @@ LL |     let x = A::<'a>::new::<&'a u32>(&v, &v);
 LL | }
    | - `v` dropped here while still borrowed
 
-error[E0597]: `v` does not live long enough
-  --> $DIR/method-ufcs-inherent-2.rs:16:41
-   |
-LL | fn foo<'a>() {
-   |        -- lifetime `'a` defined here
-LL |     let v = 22;
-LL |     let x = A::<'a>::new::<&'a u32>(&v, &v);
-   |             ----------------------------^^-
-   |             |                           |
-   |             |                           borrowed value does not live long enough
-   |             argument requires that `v` is borrowed for `'a`
-...
-LL | }
-   | - `v` dropped here while still borrowed
-
-error: aborting due to 2 previous errors
+error: aborting due to previous error
 
 For more information about this error, try `rustc --explain E0597`.

Diff for: src/test/ui/nll/user-annotations/method-ufcs-inherent-4.stderr

@@ -13,21 +13,6 @@ LL |     let x = <A<'a>>::new::<&'a u32>(&v, &v);
 LL | }
    | - `v` dropped here while still borrowed
 
-error[E0597]: `v` does not live long enough
-  --> $DIR/method-ufcs-inherent-4.rs:17:41
-   |
-LL | fn foo<'a>() {
-   |        -- lifetime `'a` defined here
-LL |     let v = 22;
-LL |     let x = <A<'a>>::new::<&'a u32>(&v, &v);
-   |             ----------------------------^^-
-   |             |                           |
-   |             |                           borrowed value does not live long enough
-   |             argument requires that `v` is borrowed for `'a`
-...
-LL | }
-   | - `v` dropped here while still borrowed
-
-error: aborting due to 2 previous errors
+error: aborting due to previous error
 
 For more information about this error, try `rustc --explain E0597`.