Move predecessors cache invalidation back to basic_blocks_mut, add a couple more ensure_predecessors to prevent panics

Author: Nashenas88

src/librustc/mir/mod.rs

@@ -203,23 +203,9 @@ impl<'tcx> Body<'tcx> {
 
     #[inline]
     pub fn basic_blocks_mut(&mut self) -> &mut IndexVec<BasicBlock, BasicBlockData<'tcx>> {
-        &mut self.basic_blocks
-    }
-
-    pub fn basic_block_terminator_opt_mut(
-        &mut self, bb: BasicBlock
-    ) -> &mut Option<Terminator<'tcx>> {
-        // FIXME we should look into improving the cache invalidation
-        debug!("Invalidating predecessors cache through opt terminator for block at : {:?}", self.span.data());
-        self.predecessors_cache = None;
-        &mut self.basic_blocks[bb].terminator
-    }
-
-    pub fn basic_block_terminator_mut(&mut self, bb: BasicBlock) -> &mut Terminator<'tcx> {
-        // FIXME we should look into improving the cache invalidation
-        debug!("Invalidating predecessors cache through terminator for block at : {:?}", self.span.data());
+        debug!("Clearing predecessors cache at: {:?}", self.span.data());
         self.predecessors_cache = None;
-        self.basic_blocks[bb].terminator_mut()
+        &mut self.basic_blocks
     }
 
     #[inline]
@@ -1370,10 +1356,6 @@ impl<'tcx> BasicBlockData<'tcx> {
         BasicBlockData { statements: vec![], terminator, is_cleanup: false }
     }
 
-    pub fn terminator_opt(&self) -> &Option<Terminator<'tcx>> {
-        &self.terminator
-    }
-
     /// Accessor for terminator.
     ///
     /// Terminator may not be None after construction of the basic block is complete. This accessor
@@ -1382,17 +1364,10 @@ impl<'tcx> BasicBlockData<'tcx> {
         self.terminator.as_ref().expect("invalid terminator state")
     }
 
-    // This cannot be public since changing the terminator will break the predecessors cache in Body
-    // To do so outside of this module, use Body::basic_block_terminator_mut(BasicBlock)
-    fn terminator_mut(&mut self) -> &mut Terminator<'tcx> {
+    pub fn terminator_mut(&mut self) -> &mut Terminator<'tcx> {
         self.terminator.as_mut().expect("invalid terminator state")
     }
 
-    // This can be public since changing the kind will not break the predecessors cache in Body
-    pub fn terminator_kind_mut(&mut self) -> &mut TerminatorKind<'tcx> {
-        &mut self.terminator_mut().kind
-    }
-
     pub fn retain_statements<F>(&mut self, mut f: F)
     where
         F: FnMut(&mut Statement<'_>) -> bool,

src/librustc/mir/traversal.rs

@@ -55,7 +55,7 @@ impl<'a, 'tcx> Iterator for Preorder<'a, 'tcx> {
 
             let data = &self.body[idx];
 
-            if let Some(ref term) = data.terminator_opt() {
+            if let Some(ref term) = data.terminator {
                 self.worklist.extend(term.successors());
             }
 
@@ -117,7 +117,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> {
 
         let data = &po.body[root];
 
-        if let Some(ref term) = data.terminator_opt() {
+        if let Some(ref term) = data.terminator {
             po.visited.insert(root);
             po.visit_stack.push((root, term.successors()));
             po.traverse_successor();
@@ -186,7 +186,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> {
             };
 
             if self.visited.insert(bb) {
-                if let Some(term) = &self.body[bb].terminator_opt() {
+                if let Some(term) = &self.body[bb].terminator {
                     self.visit_stack.push((bb, term.successors()));
                 }
             }

src/librustc/mir/visit.rs

@@ -789,31 +789,18 @@ macro_rules! make_mir_visitor {
             }
 
             // Convenience methods
-            make_mir_visitor!{@make_visit_location, $($mutability)?}
-        }
-    };
-    (@make_visit_location, mut) => {
-       fn visit_location(&mut self, body: &mut Body<'tcx>, location: Location) {
-          if body[location.block].statements.len() == location.statement_index {
-            if let Some(ref mut terminator) = body.basic_block_terminator_opt_mut(location.block) {
-                self.visit_terminator(terminator, location)
-            }
-          } else {
-            let statement = &mut body[location.block].statements[location.statement_index];
-            self.visit_statement(statement, location)
-          }
-       }
-    };
-    (@make_visit_location, ) => {
-        fn visit_location(&mut self, body: &Body<'tcx>, location: Location) {
-            let basic_block = &body[location.block];
-            if basic_block.statements.len() == location.statement_index {
-                if let Some(ref terminator) = basic_block.terminator_opt() {
-                    self.visit_terminator(terminator, location)
+
+            fn visit_location(&mut self, body: & $($mutability)? Body<'tcx>, location: Location) {
+                let basic_block = & $($mutability)? body[location.block];
+                if basic_block.statements.len() == location.statement_index {
+                    if let Some(ref $($mutability)? terminator) = basic_block.terminator {
+                        self.visit_terminator(terminator, location)
+                    }
+                } else {
+                    let statement = & $($mutability)?
+                        basic_block.statements[location.statement_index];
+                    self.visit_statement(statement, location)
                 }
-            } else {
-                let statement = &basic_block.statements[location.statement_index];
-                self.visit_statement(statement, location)
             }
         }
     }

src/librustc_codegen_ssa/mir/mod.rs

@@ -265,7 +265,7 @@ fn create_funclets<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
 
         let funclet;
         let ret_llbb;
-        match mir[bb].terminator_opt().as_ref().map(|t| &t.kind) {
+        match mir[bb].terminator.as_ref().map(|t| &t.kind) {
             // This is a basic block that we're aborting the program for,
             // notably in an `extern` function. These basic blocks are inserted
             // so that we assert that `extern` functions do indeed not panic,

src/librustc_mir/borrow_check/error_reporting.rs

@@ -497,7 +497,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                             ..
                         },
                         ..
-                    }) = bbd.terminator_opt() {
+                    }) = bbd.terminator {
                         if let Some(source)
                             = BorrowedContentSource::from_call(func.ty(self.body, tcx), tcx)
                         {

src/librustc_mir/borrow_check/mod.rs

@@ -166,6 +166,11 @@ fn do_mir_borrowck<'a, 'tcx>(
     let mut promoted: IndexVec<Promoted, Body<'tcx>> = input_promoted.clone();
     let free_regions =
         nll::replace_regions_in_mir(infcx, def_id, param_env, &mut body, &mut promoted);
+
+    // Region replacement above very likely invalidated the predecessors cache. It's used later on
+    // when retrieving the dominators from the body, so we need to ensure it exists before locking
+    // the body for changes.
+    body.ensure_predecessors();
     let body = &body; // no further changes
     let location_table = &LocationTable::new(body);
 

src/librustc_mir/build/cfg.rs

@@ -64,7 +64,7 @@ impl<'tcx> CFG<'tcx> {
                      source_info: SourceInfo,
                      kind: TerminatorKind<'tcx>) {
         debug!("terminating block {:?} <- {:?}", block, kind);
-        debug_assert!(self.block_data(block).terminator_opt().is_none(),
+        debug_assert!(self.block_data(block).terminator.is_none(),
                       "terminate: block {:?}={:?} already has a terminator set",
                       block,
                       self.block_data(block));

src/librustc_mir/build/mod.rs

@@ -731,7 +731,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
     fn finish(self) -> Body<'tcx> {
         for (index, block) in self.cfg.basic_blocks.iter().enumerate() {
-            if block.terminator_opt().is_none() {
+            if block.terminator.is_none() {
                 span_bug!(self.fn_span, "no terminator on block {:?}", index);
             }
         }

src/librustc_mir/lints.rs

@@ -79,7 +79,7 @@ fn check_fn_for_unconditional_recursion(
 
         let block = &basic_blocks[bb];
 
-        if let Some(ref terminator) = block.terminator_opt() {
+        if let Some(ref terminator) = block.terminator {
             match terminator.kind {
                 TerminatorKind::Call { ref func, .. } => {
                     let func_ty = func.ty(body, tcx);

src/librustc_mir/shim.rs

@@ -123,6 +123,15 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> &'tcx
         &add_call_guards::CriticalCallEdges,
     ]);
 
+    // The `ensure_predecessors_cache::EnsurePredecessorsCache` MirPass wasn't used in the
+    // `run_passes` above because the above pass is not always guaranteed to run. There can be
+    // instances where, e.g. a `MirPhase::Validated` pass has already been run on a `Body` by the
+    // time it arrived at this line, and so the above `run_passes` call will NOT run any of the
+    // passes (They do not run if a same or later pass has already been executed on a `Body`).
+    // Adding the ensure pass during the `run_passes` for `MirPhase::Validated` would not
+    // help because the predecessors cache would be invalidated between that pass and this call.
+    // Having the single ensure outside of the `run_passes` list here guarantees that anyone
+    // using this `Body` could call `Body::unwrap_predecessors()` without worrying about a panic.
     result.ensure_predecessors();
 
     debug!("make_shim({:?}) = {:?}", instance, result);