coverage: Detach MC/DC branch spans from regular branch spans

MC/DC's reliance on the existing branch coverage types is making it much harder
to improve branch coverage.

Author: Zalathar

compiler/rustc_middle/src/mir/coverage.rs

@@ -314,7 +314,9 @@ impl Default for ConditionInfo {
 #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
 pub struct MCDCBranchSpan {
     pub span: Span,
-    pub condition_info: ConditionInfo,
+    /// If `None`, this actually represents a normal branch span inserted for
+    /// code that was too complex for MC/DC.
+    pub condition_info: Option<ConditionInfo>,
     pub true_marker: BlockMarkerId,
     pub false_marker: BlockMarkerId,
 }

compiler/rustc_middle/src/mir/pretty.rs

@@ -491,7 +491,7 @@ fn write_coverage_branch_info(
         writeln!(
             w,
             "{INDENT}coverage mcdc branch {{ condition_id: {:?}, true: {true_marker:?}, false: {false_marker:?} }} => {span:?}",
-            condition_info.condition_id
+            condition_info.map(|info| info.condition_id)
         )?;
     }
 

compiler/rustc_mir_build/src/build/coverageinfo.rs

@@ -22,6 +22,7 @@ pub(crate) struct BranchInfoBuilder {
 
     num_block_markers: usize,
     branch_spans: Vec<BranchSpan>,
+
     mcdc_branch_spans: Vec<MCDCBranchSpan>,
     mcdc_decision_spans: Vec<MCDCDecisionSpan>,
     mcdc_state: Option<MCDCState>,
@@ -95,13 +96,7 @@ impl BranchInfoBuilder {
         }
     }
 
-    fn record_conditions_operation(&mut self, logical_op: LogicalOp, span: Span) {
-        if let Some(mcdc_state) = self.mcdc_state.as_mut() {
-            mcdc_state.record_conditions(logical_op, span);
-        }
-    }
-
-    fn fetch_condition_info(
+    fn fetch_mcdc_condition_info(
         &mut self,
         tcx: TyCtxt<'_>,
         true_marker: BlockMarkerId,
@@ -121,14 +116,9 @@ impl BranchInfoBuilder {
                 _ => {
                     // Do not generate mcdc mappings and statements for decisions with too many conditions.
                     let rebase_idx = self.mcdc_branch_spans.len() - decision.conditions_num + 1;
-                    let to_normal_branches = self.mcdc_branch_spans.split_off(rebase_idx);
-                    self.branch_spans.extend(to_normal_branches.into_iter().map(
-                        |MCDCBranchSpan { span, true_marker, false_marker, .. }| BranchSpan {
-                            span,
-                            true_marker,
-                            false_marker,
-                        },
-                    ));
+                    for branch in &mut self.mcdc_branch_spans[rebase_idx..] {
+                        branch.condition_info = None;
+                    }
 
                     // ConditionInfo of this branch shall also be reset.
                     condition_info = None;
@@ -157,7 +147,7 @@ impl BranchInfoBuilder {
             branch_spans,
             mcdc_branch_spans,
             mcdc_decision_spans,
-            ..
+            mcdc_state: _,
         } = self;
 
         if num_block_markers == 0 {
@@ -355,27 +345,30 @@ impl Builder<'_, '_> {
         let true_marker = inject_branch_marker(then_block);
         let false_marker = inject_branch_marker(else_block);
 
-        if let Some(condition_info) =
-            branch_info.fetch_condition_info(self.tcx, true_marker, false_marker)
-        {
+        if branch_info.mcdc_state.is_some() {
+            let condition_info =
+                branch_info.fetch_mcdc_condition_info(self.tcx, true_marker, false_marker);
             branch_info.mcdc_branch_spans.push(MCDCBranchSpan {
                 span: source_info.span,
                 condition_info,
                 true_marker,
                 false_marker,
             });
-        } else {
-            branch_info.branch_spans.push(BranchSpan {
-                span: source_info.span,
-                true_marker,
-                false_marker,
-            });
+            return;
         }
+
+        branch_info.branch_spans.push(BranchSpan {
+            span: source_info.span,
+            true_marker,
+            false_marker,
+        });
     }
 
     pub(crate) fn visit_coverage_branch_operation(&mut self, logical_op: LogicalOp, span: Span) {
-        if let Some(branch_info) = self.coverage_branch_info.as_mut() {
-            branch_info.record_conditions_operation(logical_op, span);
+        if let Some(branch_info) = self.coverage_branch_info.as_mut()
+            && let Some(mcdc_state) = branch_info.mcdc_state.as_mut()
+        {
+            mcdc_state.record_conditions(logical_op, span);
         }
     }
 }

compiler/rustc_mir_transform/src/coverage/mod.rs

@@ -149,13 +149,21 @@ fn create_mappings<'tcx>(
                     true_term: term_for_bcb(true_bcb),
                     false_term: term_for_bcb(false_bcb),
                 },
-                BcbMappingKind::MCDCBranch { true_bcb, false_bcb, condition_info } => {
-                    MappingKind::MCDCBranch {
+                BcbMappingKind::MCDCBranch { true_bcb, false_bcb, condition_info: None } => {
+                    MappingKind::Branch {
                         true_term: term_for_bcb(true_bcb),
                         false_term: term_for_bcb(false_bcb),
-                        mcdc_params: condition_info,
                     }
                 }
+                BcbMappingKind::MCDCBranch {
+                    true_bcb,
+                    false_bcb,
+                    condition_info: Some(mcdc_params),
+                } => MappingKind::MCDCBranch {
+                    true_term: term_for_bcb(true_bcb),
+                    false_term: term_for_bcb(false_bcb),
+                    mcdc_params,
+                },
                 BcbMappingKind::MCDCDecision { bitmap_idx, conditions_num, .. } => {
                     MappingKind::MCDCDecision(DecisionInfo { bitmap_idx, conditions_num })
                 }
@@ -249,7 +257,7 @@ fn inject_mcdc_statements<'tcx>(
     for (true_bcb, false_bcb, condition_id) in
         coverage_spans.all_bcb_mappings().filter_map(|mapping| match mapping.kind {
             BcbMappingKind::MCDCBranch { true_bcb, false_bcb, condition_info } => {
-                Some((true_bcb, false_bcb, condition_info.condition_id))
+                Some((true_bcb, false_bcb, condition_info?.condition_id))
             }
             _ => None,
         })

compiler/rustc_mir_transform/src/coverage/spans.rs

@@ -21,7 +21,9 @@ pub(super) enum BcbMappingKind {
     MCDCBranch {
         true_bcb: BasicCoverageBlock,
         false_bcb: BasicCoverageBlock,
-        condition_info: ConditionInfo,
+        /// If `None`, this actually represents a normal branch mapping inserted
+        /// for code that was too complex for MC/DC.
+        condition_info: Option<ConditionInfo>,
     },
     /// Associates a mcdc decision with its join BCB.
     MCDCDecision { end_bcbs: BTreeSet<BasicCoverageBlock>, bitmap_idx: u32, conditions_num: u16 },
@@ -89,6 +91,12 @@ pub(super) fn generate_coverage_spans(
             hir_info.body_span,
             basic_coverage_blocks,
         ));
+
+        mappings.extend(from_mir::extract_mcdc_mappings(
+            mir_body,
+            hir_info.body_span,
+            basic_coverage_blocks,
+        ));
     }
 
     if mappings.is_empty() {

compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs

@@ -366,15 +366,10 @@ impl SpanFromMir {
     }
 }
 
-pub(super) fn extract_branch_mappings(
+fn resolve_block_markers(
+    branch_info: &mir::coverage::BranchInfo,
     mir_body: &mir::Body<'_>,
-    body_span: Span,
-    basic_coverage_blocks: &CoverageGraph,
-) -> Vec<BcbMapping> {
-    let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else {
-        return vec![];
-    };
-
+) -> IndexVec<BlockMarkerId, Option<BasicBlock>> {
     let mut block_markers = IndexVec::<BlockMarkerId, Option<BasicBlock>>::from_elem_n(
         None,
         branch_info.num_block_markers,
@@ -389,6 +384,57 @@ pub(super) fn extract_branch_mappings(
         }
     }
 
+    block_markers
+}
+
+// FIXME: There is currently a lot of redundancy between
+// `extract_branch_mappings` and `extract_mcdc_mappings`. This is needed so
+// that they can each be modified without interfering with the other, but in
+// the long term we should try to bring them together again when branch coverage
+// and MC/DC coverage support are more mature.
+
+pub(super) fn extract_branch_mappings(
+    mir_body: &mir::Body<'_>,
+    body_span: Span,
+    basic_coverage_blocks: &CoverageGraph,
+) -> Vec<BcbMapping> {
+    let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { return vec![] };
+
+    let block_markers = resolve_block_markers(branch_info, mir_body);
+
+    branch_info
+        .branch_spans
+        .iter()
+        .filter_map(|&BranchSpan { span: raw_span, true_marker, false_marker }| {
+            // For now, ignore any branch span that was introduced by
+            // expansion. This makes things like assert macros less noisy.
+            if !raw_span.ctxt().outer_expn_data().is_root() {
+                return None;
+            }
+            let (span, _) = unexpand_into_body_span_with_visible_macro(raw_span, body_span)?;
+
+            let bcb_from_marker =
+                |marker: BlockMarkerId| basic_coverage_blocks.bcb_from_bb(block_markers[marker]?);
+
+            let true_bcb = bcb_from_marker(true_marker)?;
+            let false_bcb = bcb_from_marker(false_marker)?;
+
+            Some(BcbMapping { kind: BcbMappingKind::Branch { true_bcb, false_bcb }, span })
+        })
+        .collect::<Vec<_>>()
+}
+
+pub(super) fn extract_mcdc_mappings(
+    mir_body: &mir::Body<'_>,
+    body_span: Span,
+    basic_coverage_blocks: &CoverageGraph,
+) -> Vec<BcbMapping> {
+    let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else {
+        return vec![];
+    };
+
+    let block_markers = resolve_block_markers(branch_info, mir_body);
+
     let bcb_from_marker =
         |marker: BlockMarkerId| basic_coverage_blocks.bcb_from_bb(block_markers[marker]?);
 
@@ -406,12 +452,6 @@ pub(super) fn extract_branch_mappings(
             Some((span, true_bcb, false_bcb))
         };
 
-    let branch_filter_map = |&BranchSpan { span: raw_span, true_marker, false_marker }| {
-        check_branch_bcb(raw_span, true_marker, false_marker).map(|(span, true_bcb, false_bcb)| {
-            BcbMapping { kind: BcbMappingKind::Branch { true_bcb, false_bcb }, span }
-        })
-    };
-
     let mcdc_branch_filter_map =
         |&MCDCBranchSpan { span: raw_span, true_marker, false_marker, condition_info }| {
             check_branch_bcb(raw_span, true_marker, false_marker).map(
@@ -446,10 +486,7 @@ pub(super) fn extract_branch_mappings(
         })
     };
 
-    branch_info
-        .branch_spans
-        .iter()
-        .filter_map(branch_filter_map)
+    std::iter::empty()
         .chain(branch_info.mcdc_branch_spans.iter().filter_map(mcdc_branch_filter_map))
         .chain(branch_info.mcdc_decision_spans.iter().filter_map(decision_filter_map))
         .collect::<Vec<_>>()