coverage: Simplify the loop that combines blocks into BCBs

Author: Zalathar

compiler/rustc_mir_transform/src/coverage/graph.rs

@@ -91,74 +91,41 @@ impl CoverageGraph {
         // FIXME(#78544): MIR InstrumentCoverage: Improve coverage of `#[should_panic]` tests and
         // `catch_unwind()` handlers.
 
+        // Accumulates a chain of blocks that will be combined into one BCB.
         let mut basic_blocks = Vec::new();
+
         let filtered_successors = |bb| bcb_filtered_successors(mir_body[bb].terminator());
         for bb in short_circuit_preorder(mir_body, filtered_successors)
             .filter(|&bb| mir_body[bb].terminator().kind != TerminatorKind::Unreachable)
         {
-            if let Some(last) = basic_blocks.last() {
-                let predecessors = &mir_body.basic_blocks.predecessors()[bb];
-                if predecessors.len() > 1 || !predecessors.contains(last) {
-                    // The `bb` has more than one _incoming_ edge, and should start its own
-                    // `BasicCoverageBlockData`. (Note, the `basic_blocks` vector does not yet
-                    // include `bb`; it contains a sequence of one or more sequential basic_blocks
-                    // with no intermediate branches in or out. Save these as a new
-                    // `BasicCoverageBlockData` before starting the new one.)
-                    Self::add_basic_coverage_block(
-                        &mut bcbs,
-                        &mut bb_to_bcb,
-                        basic_blocks.split_off(0),
-                    );
-                    debug!(
-                        "  because {}",
-                        if predecessors.len() > 1 {
-                            "predecessors.len() > 1".to_owned()
-                        } else {
-                            format!("bb {} is not in predecessors: {:?}", bb.index(), predecessors)
-                        }
-                    );
-                }
+            // If the previous block can't be chained into `bb`, flush the accumulated
+            // blocks into a new BCB, then start building the next chain.
+            if let Some(&prev) = basic_blocks.last()
+                && (!filtered_successors(prev).is_chainable() || {
+                    // If `bb` has multiple predecessor blocks, or `prev` isn't
+                    // one of its predecessors, we can't chain and must flush.
+                    let predecessors = &mir_body.basic_blocks.predecessors()[bb];
+                    predecessors.len() > 1 || !predecessors.contains(&prev)
+                })
+            {
+                debug!(
+                    terminator_kind = ?mir_body[prev].terminator().kind,
+                    predecessors = ?&mir_body.basic_blocks.predecessors()[bb],
+                    "can't chain from {prev:?} to {bb:?}"
+                );
+                Self::add_basic_coverage_block(
+                    &mut bcbs,
+                    &mut bb_to_bcb,
+                    basic_blocks.split_off(0),
+                );
             }
-            basic_blocks.push(bb);
-
-            let term = mir_body[bb].terminator();
-
-            match bcb_filtered_successors(term) {
-                CoverageSuccessors::NotChainable(_) => {
-                    // The `bb` has more than one _outgoing_ edge, or exits the function. Save the
-                    // current sequence of `basic_blocks` gathered to this point, as a new
-                    // `BasicCoverageBlockData`.
-                    Self::add_basic_coverage_block(
-                        &mut bcbs,
-                        &mut bb_to_bcb,
-                        basic_blocks.split_off(0),
-                    );
-                    debug!("  because term.kind = {:?}", term.kind);
-                    // Note that this condition is based on `TerminatorKind`, even though it
-                    // theoretically boils down to `successors().len() != 1`; that is, either zero
-                    // (e.g., `Return`, `Terminate`) or multiple successors (e.g., `SwitchInt`), but
-                    // since the BCB CFG ignores things like unwind branches (which exist in the
-                    // `Terminator`s `successors()` list) checking the number of successors won't
-                    // work.
-                }
 
-                // The following `TerminatorKind`s are either not expected outside an unwind branch,
-                // or they should not (under normal circumstances) branch. Coverage graphs are
-                // simplified by assuring coverage results are accurate for program executions that
-                // don't panic.
-                //
-                // Programs that panic and unwind may record slightly inaccurate coverage results
-                // for a coverage region containing the `Terminator` that began the panic. This
-                // is as intended. (See Issue #78544 for a possible future option to support
-                // coverage in test programs that panic.)
-                CoverageSuccessors::Chainable(_) => {}
-            }
+            basic_blocks.push(bb);
         }
 
         if !basic_blocks.is_empty() {
-            // process any remaining basic_blocks into a final `BasicCoverageBlockData`
+            debug!("flushing accumulated blocks into one last BCB");
             Self::add_basic_coverage_block(&mut bcbs, &mut bb_to_bcb, basic_blocks.split_off(0));
-            debug!("  because the end of the MIR CFG was reached while traversing");
         }
 
         (bcbs, bb_to_bcb)
@@ -350,6 +317,15 @@ enum CoverageSuccessors<'a> {
     NotChainable(&'a [BasicBlock]),
 }
 
+impl CoverageSuccessors<'_> {
+    fn is_chainable(&self) -> bool {
+        match self {
+            Self::Chainable(_) => true,
+            Self::NotChainable(_) => false,
+        }
+    }
+}
+
 impl IntoIterator for CoverageSuccessors<'_> {
     type Item = BasicBlock;
     type IntoIter = impl DoubleEndedIterator<Item = Self::Item>;