From 59c746030a1f1330f2037f09c2a7a6def8a5fc04 Mon Sep 17 00:00:00 2001
From: Dylan MacKenzie <ecstaticmorse@gmail.com>
Date: Tue, 21 Apr 2020 10:15:07 -0700
Subject: [PATCH] Use a ref-counted pointer for ownership of the predecessor
 cache

...instead of a `LockGuard` which means the lock is held for longer than
necessary.
---
 src/librustc_middle/mir/predecessors.rs | 37 +++++++++++++++----------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/src/librustc_middle/mir/predecessors.rs b/src/librustc_middle/mir/predecessors.rs
index 629b5c2efb711..8f1f927852119 100644
--- a/src/librustc_middle/mir/predecessors.rs
+++ b/src/librustc_middle/mir/predecessors.rs
@@ -1,5 +1,5 @@
 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;
@@ -11,7 +11,7 @@ pub type Predecessors = IndexVec<BasicBlock, SmallVec<[BasicBlock; 4]>>;
 
 #[derive(Clone, Debug)]
 pub struct PredecessorCache {
-    cache: Lock<Option<Predecessors>>,
+    cache: Lock<Option<Lrc<Predecessors>>>,
 }
 
 impl PredecessorCache {
@@ -20,30 +20,39 @@ impl PredecessorCache {
         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) {
         *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(
         &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)
+        }))
     }
 }