run EndRegion when unwinding otherwise-empty scopes

Improves #44832

borrowck-overloaded-index-move-index.rs - fixed
borrowck-multiple-captures.rs - still ICE
borrowck-issue-2657-1.rs - fixed
borrowck-loan-blocks-move.rs - fixed
borrowck-move-from-subpath-of-borrowed-path.rs - fixed
borrowck-mut-borrow-linear-errors.rs - still ICE
borrowck-no-cycle-in-exchange-heap.rs - fixed
borrowck-unary-move.rs - fixed
borrowck-loan-blocks-move-cc.rs - fixed
borrowck-vec-pattern-element-loan.rs - still broken

Author: arielb1

src/librustc_mir/build/scope.rs

@@ -131,6 +131,9 @@ pub struct Scope<'tcx> {
 
     /// The cache for drop chain on "generator drop" exit.
     cached_generator_drop: Option<BasicBlock>,
+
+    /// The cache for drop chain on "unwind" exit.
+    cached_unwind: CachedBlock,
 }
 
 #[derive(Debug)]
@@ -233,8 +236,10 @@ impl<'tcx> Scope<'tcx> {
         self.cached_exits.clear();
 
         if !storage_only {
-            // the current generator drop ignores storage but refers to top-of-scope
+            // the current generator drop and unwind ignore
+            // storage but refer to top-of-scope
             self.cached_generator_drop = None;
+            self.cached_unwind.invalidate();
         }
 
         if !storage_only && !this_scope_only {
@@ -246,26 +251,6 @@ impl<'tcx> Scope<'tcx> {
         }
     }
 
-    /// Returns the cached entrypoint for diverging exit from this scope.
-    ///
-    /// Precondition: the caches must be fully filled (i.e. diverge_cleanup is called) in order for
-    /// this method to work correctly.
-    fn cached_block(&self, generator_drop: bool) -> Option<BasicBlock> {
-        let mut drops = self.drops.iter().rev().filter_map(|data| {
-            match data.kind {
-                DropKind::Value { cached_block } => {
-                    Some(cached_block.get(generator_drop))
-                }
-                DropKind::Storage => None
-            }
-        });
-        if let Some(cached_block) = drops.next() {
-            Some(cached_block.expect("drop cache is not filled"))
-        } else {
-            None
-        }
-    }
-
     /// Given a span and this scope's visibility scope, make a SourceInfo.
     fn source_info(&self, span: Span) -> SourceInfo {
         SourceInfo {
@@ -374,7 +359,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             needs_cleanup: false,
             drops: vec![],
             cached_generator_drop: None,
-            cached_exits: FxHashMap()
+            cached_exits: FxHashMap(),
+            cached_unwind: CachedBlock::default(),
         });
     }
 
@@ -500,15 +486,16 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                                    TerminatorKind::Goto { target: b });
                 b
             };
+
+            // End all regions for scopes out of which we are breaking.
+            self.cfg.push_end_region(self.hir.tcx(), block, src_info, scope.region_scope);
+
             unpack!(block = build_scope_drops(&mut self.cfg,
                                               scope,
                                               rest,
                                               block,
                                               self.arg_count,
                                               true));
-
-            // End all regions for scopes out of which we are breaking.
-            self.cfg.push_end_region(self.hir.tcx(), block, src_info, scope.region_scope);
         }
 
         self.cfg.terminate(block, src_info, TerminatorKind::GeneratorDrop);
@@ -841,23 +828,45 @@ fn build_scope_drops<'tcx>(cfg: &mut CFG<'tcx>,
         let source_info = scope.source_info(drop_data.span);
         match drop_data.kind {
             DropKind::Value { .. } => {
-                // Try to find the next block with its cached block
-                // for us to diverge into in case the drop panics.
+                // Try to find the next block with its cached block for us to
+                // diverge into, either a previous block in this current scope or
+                // the top of the previous scope.
+                //
+                // If it wasn't for EndRegion, we could just chain all the DropData
+                // together and pick the first DropKind::Value. Please do that
+                // when we replace EndRegion with NLL.
                 let on_diverge = iter.clone().filter_map(|dd| {
                     match dd.kind {
                         DropKind::Value { cached_block } => Some(cached_block),
                         DropKind::Storage => None
                     }
-                }).map(|cached_block| {
-                    cached_block
-                        .get(generator_drop)
-                        .unwrap_or_else(|| span_bug!(drop_data.span, "cached block not present?"))
-                }).next();
-                // If there’s no `cached_block`s within current scope,
-                // we must look for one in the enclosing scope.
-                let on_diverge = on_diverge.or_else(|| {
-                    earlier_scopes.iter().rev().flat_map(|s| s.cached_block(generator_drop)).next()
+                }).next().or_else(|| {
+                    if earlier_scopes.iter().any(|scope| scope.needs_cleanup) {
+                        // If *any* scope requires cleanup code to be run,
+                        // we must use the cached unwind from the *topmost*
+                        // scope, to ensure all EndRegions from surrounding
+                        // scopes are executed before the drop code runs.
+                        Some(earlier_scopes.last().unwrap().cached_unwind)
+                    } else {
+                        // We don't need any further cleanup, so return None
+                        // to avoid creating a landing pad. We can skip
+                        // EndRegions because all local regions end anyway
+                        // when the function unwinds.
+                        //
+                        // This is an important optimization because LLVM is
+                        // terrible at optimizing landing pads. FIXME: I think
+                        // it would be cleaner and better to do this optimization
+                        // in SimplifyCfg instead of here.
+                        None
+                    }
+                });
+
+                let on_diverge = on_diverge.map(|cached_block| {
+                    cached_block.get(generator_drop).unwrap_or_else(|| {
+                        span_bug!(drop_data.span, "cached block not present?")
+                    })
                 });
+
                 let next = cfg.start_new_block();
                 cfg.terminate(block, source_info, TerminatorKind::Drop {
                     location: drop_data.location.clone(),
@@ -948,14 +957,21 @@ fn build_diverge_scope<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
         };
     }
 
-    // Finally, push the EndRegion block, used by mir-borrowck. (Block
-    // becomes trivial goto after pass that removes all EndRegions.)
-    {
-        let block = cfg.start_new_cleanup_block();
-        cfg.push_end_region(tcx, block, source_info(span), scope.region_scope);
-        cfg.terminate(block, source_info(span), TerminatorKind::Goto { target: target });
-        target = block
-    }
+    // Finally, push the EndRegion block, used by mir-borrowck, and set
+    // `cached_unwind` to point to it (Block becomes trivial goto after
+    // pass that removes all EndRegions).
+    target = {
+        let cached_block = scope.cached_unwind.ref_mut(generator_drop);
+        if let Some(cached_block) = *cached_block {
+            cached_block
+        } else {
+            let block = cfg.start_new_cleanup_block();
+            cfg.push_end_region(tcx, block, source_info(span), scope.region_scope);
+            cfg.terminate(block, source_info(span), TerminatorKind::Goto { target: target });
+            *cached_block = Some(block);
+            block
+        }
+    };
 
     debug!("build_diverge_scope({:?}, {:?}) = {:?}", scope, span, target);
 

src/test/compile-fail/borrowck/borrowck-unary-move.rs

@@ -8,10 +8,15 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+// ignore-tidy-linelength
+// revisions: ast mir
+//[mir]compile-flags: -Z emit-end-regions -Z borrowck-mir
 
 fn foo(x: Box<isize>) -> isize {
     let y = &*x;
-    free(x); //~ ERROR cannot move out of `x` because it is borrowed
+    free(x); //[ast]~ ERROR cannot move out of `x` because it is borrowed
+    //[mir]~^ ERROR cannot move out of `x` because it is borrowed (Ast)
+    //[mir]~| ERROR cannot move out of `x` because it is borrowed (Mir)
     *y
 }