Rollup merge of rust-lang#127758 - Zalathar:expression-used, r=oli-obk

coverage: Restrict `ExpressionUsed` simplification to `Code` mappings

In the future, branch and MC/DC mappings might have expressions that don't correspond to any single point in the control-flow graph. That makes it trickier to keep track of which expressions should expect an `ExpressionUsed` node.

We therefore sidestep that complexity by only performing `ExpressionUsed` simplification for expressions associated directly with ordinary `Code` mappings.

(This simplification step is inherited from the original coverage implementation, which only supported `Code` mappings anyway, so there's no particular reason to extend it to other kinds of mappings unless we specifically choose to.)

Relevant to:
- rust-lang#124154
- rust-lang#126677
- rust-lang#124278

`@rustbot` label +A-code-coverage

Author: matthiaskrgr

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs

@@ -66,8 +66,15 @@ impl<'tcx> FunctionCoverageCollector<'tcx> {
         // For each expression ID that is directly used by one or more mappings,
         // mark it as not-yet-seen. This indicates that we expect to see a
         // corresponding `ExpressionUsed` statement during MIR traversal.
-        for term in function_coverage_info.mappings.iter().flat_map(|m| m.kind.terms()) {
-            if let CovTerm::Expression(id) = term {
+        for mapping in function_coverage_info.mappings.iter() {
+            // Currently we only worry about ordinary code mappings.
+            // For branch and MC/DC mappings, expressions might not correspond
+            // to any particular point in the control-flow graph.
+            // (Keep this in sync with the injection of `ExpressionUsed`
+            // statements in the `InstrumentCoverage` MIR pass.)
+            if let MappingKind::Code(term) = mapping.kind
+                && let CovTerm::Expression(id) = term
+            {
                 expressions_seen.remove(id);
             }
         }

compiler/rustc_middle/src/mir/coverage.rs

@@ -220,19 +220,6 @@ pub enum MappingKind {
 }
 
 impl MappingKind {
-    /// Iterator over all coverage terms in this mapping kind.
-    pub fn terms(&self) -> impl Iterator<Item = CovTerm> {
-        let zero = || None.into_iter().chain(None);
-        let one = |a| Some(a).into_iter().chain(None);
-        let two = |a, b| Some(a).into_iter().chain(Some(b));
-        match *self {
-            Self::Code(term) => one(term),
-            Self::Branch { true_term, false_term } => two(true_term, false_term),
-            Self::MCDCBranch { true_term, false_term, .. } => two(true_term, false_term),
-            Self::MCDCDecision(_) => zero(),
-        }
-    }
-
     /// Returns a copy of this mapping kind, in which all coverage terms have
     /// been replaced with ones returned by the given function.
     pub fn map_terms(&self, map_fn: impl Fn(CovTerm) -> CovTerm) -> Self {

compiler/rustc_mir_transform/src/coverage/mappings.rs

@@ -56,6 +56,10 @@ pub(super) struct MCDCDecision {
 
 #[derive(Default)]
 pub(super) struct ExtractedMappings {
+    /// Store our own copy of [`CoverageGraph::num_nodes`], so that we don't
+    /// need access to the whole graph when allocating per-BCB data. This is
+    /// only public so that other code can still use exhaustive destructuring.
+    pub(super) num_bcbs: usize,
     pub(super) code_mappings: Vec<CodeMapping>,
     pub(super) branch_pairs: Vec<BranchPair>,
     pub(super) mcdc_bitmap_bytes: u32,
@@ -106,6 +110,7 @@ pub(super) fn extract_all_mapping_info_from_mir<'tcx>(
     );
 
     ExtractedMappings {
+        num_bcbs: basic_coverage_blocks.num_nodes(),
         code_mappings,
         branch_pairs,
         mcdc_bitmap_bytes,
@@ -115,12 +120,10 @@ pub(super) fn extract_all_mapping_info_from_mir<'tcx>(
 }
 
 impl ExtractedMappings {
-    pub(super) fn all_bcbs_with_counter_mappings(
-        &self,
-        basic_coverage_blocks: &CoverageGraph, // Only used for allocating a correctly-sized set
-    ) -> BitSet<BasicCoverageBlock> {
+    pub(super) fn all_bcbs_with_counter_mappings(&self) -> BitSet<BasicCoverageBlock> {
         // Fully destructure self to make sure we don't miss any fields that have mappings.
         let Self {
+            num_bcbs,
             code_mappings,
             branch_pairs,
             mcdc_bitmap_bytes: _,
@@ -129,7 +132,7 @@ impl ExtractedMappings {
         } = self;
 
         // Identify which BCBs have one or more mappings.
-        let mut bcbs_with_counter_mappings = BitSet::new_empty(basic_coverage_blocks.num_nodes());
+        let mut bcbs_with_counter_mappings = BitSet::new_empty(*num_bcbs);
         let mut insert = |bcb| {
             bcbs_with_counter_mappings.insert(bcb);
         };
@@ -156,6 +159,15 @@ impl ExtractedMappings {
 
         bcbs_with_counter_mappings
     }
+
+    /// Returns the set of BCBs that have one or more `Code` mappings.
+    pub(super) fn bcbs_with_ordinary_code_mappings(&self) -> BitSet<BasicCoverageBlock> {
+        let mut bcbs = BitSet::new_empty(self.num_bcbs);
+        for &CodeMapping { span: _, bcb } in &self.code_mappings {
+            bcbs.insert(bcb);
+        }
+        bcbs
+    }
 }
 
 fn resolve_block_markers(

compiler/rustc_mir_transform/src/coverage/mod.rs

@@ -25,7 +25,7 @@ use rustc_span::source_map::SourceMap;
 use rustc_span::{BytePos, Pos, RelativeBytePos, Span, Symbol};
 
 use crate::coverage::counters::{CounterIncrementSite, CoverageCounters};
-use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph};
+use crate::coverage::graph::CoverageGraph;
 use crate::coverage::mappings::ExtractedMappings;
 use crate::MirPass;
 
@@ -88,8 +88,7 @@ fn instrument_function_for_coverage<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mut mir:
     // every coverage span has a `Counter` or `Expression` assigned to its `BasicCoverageBlock`
     // and all `Expression` dependencies (operands) are also generated, for any other
     // `BasicCoverageBlock`s not already associated with a coverage span.
-    let bcbs_with_counter_mappings =
-        extracted_mappings.all_bcbs_with_counter_mappings(&basic_coverage_blocks);
+    let bcbs_with_counter_mappings = extracted_mappings.all_bcbs_with_counter_mappings();
     if bcbs_with_counter_mappings.is_empty() {
         // No relevant spans were found in MIR, so skip instrumenting this function.
         return;
@@ -109,7 +108,7 @@ fn instrument_function_for_coverage<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mut mir:
     inject_coverage_statements(
         mir_body,
         &basic_coverage_blocks,
-        bcb_has_counter_mappings,
+        &extracted_mappings,
         &coverage_counters,
     );
 
@@ -163,6 +162,7 @@ fn create_mappings<'tcx>(
 
     // Fully destructure the mappings struct to make sure we don't miss any kinds.
     let ExtractedMappings {
+        num_bcbs: _,
         code_mappings,
         branch_pairs,
         mcdc_bitmap_bytes: _,
@@ -219,7 +219,7 @@ fn create_mappings<'tcx>(
 fn inject_coverage_statements<'tcx>(
     mir_body: &mut mir::Body<'tcx>,
     basic_coverage_blocks: &CoverageGraph,
-    bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool,
+    extracted_mappings: &ExtractedMappings,
     coverage_counters: &CoverageCounters,
 ) {
     // Inject counter-increment statements into MIR.
@@ -252,11 +252,16 @@ fn inject_coverage_statements<'tcx>(
     // can check whether the injected statement survived MIR optimization.
     // (BCB edges can't have spans, so we only need to process BCB nodes here.)
     //
+    // We only do this for ordinary `Code` mappings, because branch and MC/DC
+    // mappings might have expressions that don't correspond to any single
+    // point in the control-flow graph.
+    //
     // See the code in `rustc_codegen_llvm::coverageinfo::map_data` that deals
     // with "expressions seen" and "zero terms".
+    let eligible_bcbs = extracted_mappings.bcbs_with_ordinary_code_mappings();
     for (bcb, expression_id) in coverage_counters
         .bcb_nodes_with_coverage_expressions()
-        .filter(|&(bcb, _)| bcb_has_coverage_spans(bcb))
+        .filter(|&(bcb, _)| eligible_bcbs.contains(bcb))
     {
         inject_statement(
             mir_body,