From 0caba178dfd5983403564de82a7d11184c7964e2 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Wed, 18 Oct 2017 13:54:36 +0300 Subject: [PATCH] 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 --- src/librustc_mir/build/scope.rs | 104 ++++++++++-------- .../borrowck/borrowck-unary-move.rs | 7 +- 2 files changed, 66 insertions(+), 45 deletions(-) diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 772438d5252c..c0d17a1590f8 100644 --- a/src/librustc_mir/build/scope.rs +++ b/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, + + /// 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 { - 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); diff --git a/src/test/compile-fail/borrowck/borrowck-unary-move.rs b/src/test/compile-fail/borrowck/borrowck-unary-move.rs index 5b5c5f4da912..6cab5a8bf602 100644 --- a/src/test/compile-fail/borrowck/borrowck-unary-move.rs +++ b/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 { 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 }