Don't hold the predecessor cache lock longer than necessary

src/librustc_codegen_ssa/mir/mod.rs

@@ -155,7 +155,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
     let cleanup_kinds = analyze::cleanup_kinds(&mir);
     // Allocate a `Block` for every basic block, except
     // the start block, if nothing loops back to it.
-    let reentrant_start_block = !mir.predecessors_for(mir::START_BLOCK).is_empty();
+    let reentrant_start_block = !mir.predecessors()[mir::START_BLOCK].is_empty();
     let block_bxs: IndexVec<mir::BasicBlock, Bx::BasicBlock> = mir
         .basic_blocks()
         .indices()

src/librustc_middle/mir/mod.rs

@@ -23,14 +23,12 @@ use rustc_ast::ast::Name;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_data_structures::graph::dominators::{dominators, Dominators};
 use rustc_data_structures::graph::{self, GraphSuccessors};
-use rustc_data_structures::sync::MappedLockGuard;
 use rustc_index::bit_set::BitMatrix;
 use rustc_index::vec::{Idx, IndexVec};
 use rustc_macros::HashStable;
 use rustc_serialize::{Decodable, Encodable};
 use rustc_span::symbol::Symbol;
 use rustc_span::{Span, DUMMY_SP};
-use smallvec::SmallVec;
 use std::borrow::Cow;
 use std::fmt::{self, Debug, Display, Formatter, Write};
 use std::ops::{Index, IndexMut};
@@ -166,7 +164,7 @@ pub struct Body<'tcx> {
     /// FIXME(oli-obk): rewrite the promoted during promotion to eliminate the cell components.
     pub ignore_interior_mut_in_const_validation: bool,
 
-    pub predecessor_cache: PredecessorCache,
+    predecessor_cache: PredecessorCache,
 }
 
 impl<'tcx> Body<'tcx> {
@@ -392,15 +390,6 @@ impl<'tcx> Body<'tcx> {
         Location { block: bb, statement_index: self[bb].statements.len() }
     }
 
-    #[inline]
-    pub fn predecessors_for(
-        &self,
-        bb: BasicBlock,
-    ) -> impl std::ops::Deref<Target = SmallVec<[BasicBlock; 4]>> + '_ {
-        let predecessors = self.predecessor_cache.compute(&self.basic_blocks);
-        MappedLockGuard::map(predecessors, |preds| &mut preds[bb])
-    }
-
     #[inline]
     pub fn predecessors(&self) -> impl std::ops::Deref<Target = Predecessors> + '_ {
         self.predecessor_cache.compute(&self.basic_blocks)
@@ -2676,7 +2665,7 @@ impl graph::GraphPredecessors<'graph> for Body<'tcx> {
 impl graph::WithPredecessors for Body<'tcx> {
     #[inline]
     fn predecessors(&self, node: Self::Node) -> <Self as graph::GraphPredecessors<'_>>::Iter {
-        self.predecessors_for(node).clone().into_iter()
+        self.predecessors()[node].clone().into_iter()
     }
 }
 

src/librustc_middle/mir/predecessors.rs

@@ -1,5 +1,7 @@
+//! Lazily compute the reverse control-flow graph for the MIR.
+
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
-use rustc_data_structures::sync::{Lock, LockGuard, MappedLockGuard};
+use rustc_data_structures::sync::{Lock, Lrc};
 use rustc_index::vec::IndexVec;
 use rustc_serialize as serialize;
 use smallvec::SmallVec;
@@ -10,40 +12,49 @@ use crate::mir::{BasicBlock, BasicBlockData};
 pub type Predecessors = IndexVec<BasicBlock, SmallVec<[BasicBlock; 4]>>;
 
 #[derive(Clone, Debug)]
-pub struct PredecessorCache {
-    cache: Lock<Option<Predecessors>>,
+pub(super) struct PredecessorCache {
+    cache: Lock<Option<Lrc<Predecessors>>>,
 }
 
 impl PredecessorCache {
     #[inline]
-    pub fn new() -> Self {
+    pub(super) fn new() -> Self {
         PredecessorCache { cache: Lock::new(None) }
     }
 
+    /// Invalidates the predecessor cache.
+    ///
+    /// Invalidating the predecessor cache requires mutating the MIR, which in turn requires a
+    /// unique reference (`&mut`) to the `mir::Body`. Because of this, we can assume that all
+    /// callers of `invalidate` have a unique reference to the MIR and thus to the predecessor
+    /// cache. This means we don't actually need to take a lock when `invalidate` is called.
     #[inline]
-    pub fn invalidate(&mut self) {
+    pub(super) fn invalidate(&mut self) {
         *self.cache.get_mut() = None;
     }
 
+    /// Returns a ref-counted smart pointer containing the predecessor graph for this MIR.
+    ///
+    /// We use ref-counting instead of a mapped `LockGuard` here to ensure that the lock for
+    /// `cache` is only held inside this function. As long as no other locks are taken while
+    /// computing the predecessor graph, deadlock is impossible.
     #[inline]
-    pub fn compute(
+    pub(super) fn compute(
         &self,
         basic_blocks: &IndexVec<BasicBlock, BasicBlockData<'_>>,
-    ) -> MappedLockGuard<'_, Predecessors> {
-        LockGuard::map(self.cache.lock(), |cache| {
-            cache.get_or_insert_with(|| {
-                let mut preds = IndexVec::from_elem(SmallVec::new(), basic_blocks);
-                for (bb, data) in basic_blocks.iter_enumerated() {
-                    if let Some(term) = &data.terminator {
-                        for &succ in term.successors() {
-                            preds[succ].push(bb);
-                        }
+    ) -> Lrc<Predecessors> {
+        Lrc::clone(self.cache.lock().get_or_insert_with(|| {
+            let mut preds = IndexVec::from_elem(SmallVec::new(), basic_blocks);
+            for (bb, data) in basic_blocks.iter_enumerated() {
+                if let Some(term) = &data.terminator {
+                    for &succ in term.successors() {
+                        preds[succ].push(bb);
                     }
                 }
+            }
 
-                preds
-            })
-        })
+            Lrc::new(preds)
+        }))
     }
 }
 

src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs

@@ -1268,7 +1268,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             location: Location,
         ) -> impl Iterator<Item = Location> + 'a {
             if location.statement_index == 0 {
-                let predecessors = body.predecessors_for(location.block).to_vec();
+                let predecessors = body.predecessors()[location.block].to_vec();
                 Either::Left(predecessors.into_iter().map(move |bb| body.terminator_loc(bb)))
             } else {
                 Either::Right(std::iter::once(Location {

src/librustc_mir/borrow_check/region_infer/values.rs

@@ -89,7 +89,7 @@ impl RegionValueElements {
             // If this is a basic block head, then the predecessors are
             // the terminators of other basic blocks
             stack.extend(
-                body.predecessors_for(block)
+                body.predecessors()[block]
                     .iter()
                     .map(|&pred_bb| body.terminator_loc(pred_bb))
                     .map(|pred_loc| self.point_from_location(pred_loc)),

src/librustc_mir/borrow_check/type_check/liveness/trace.rs

@@ -303,7 +303,7 @@ impl LivenessResults<'me, 'typeck, 'flow, 'tcx> {
         }
 
         let body = self.cx.body;
-        for &pred_block in body.predecessors_for(block).iter() {
+        for &pred_block in body.predecessors()[block].iter() {
             debug!("compute_drop_live_points_for_block: pred_block = {:?}", pred_block,);
 
             // Check whether the variable is (at least partially)