!! (WIP) Only CoverageKind::Mappings can have code regions

Zalathar · Zalathar · commit 00aa76ab092d · 2023-09-07T21:36:16.000+10:00
diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs
@@ -72,39 +72,34 @@ impl<'tcx> FunctionCoverage<'tcx> {
         self.is_used
     }
 
-    /// Sets the function source hash value. If called multiple times for the same function, all
-    /// calls should have the same hash value.
-    pub fn set_function_source_hash(&mut self, source_hash: u64) {
+    /// Notes that a counter increment has been encountered with the given
+    /// counter ID and source hash.
+    ///
+    /// If called multiple times for the same function, all calls should have
+    /// the same hash value.
+    #[instrument(level = "debug", skip(self))]
+    pub(crate) fn counter_seen(&mut self, id: CounterId, source_hash: u64) {
+        self.counters_seen.insert(id);
+
         if self.source_hash == 0 {
             self.source_hash = source_hash;
         } else {
             debug_assert_eq!(source_hash, self.source_hash);
         }
     }
 
-    /// Adds code regions to be counted by an injected counter intrinsic.
-    #[instrument(level = "debug", skip(self))]
-    pub(crate) fn add_counter(&mut self, id: CounterId, code_regions: &[CodeRegion]) {
-        if !self.counters_seen.insert(id) {
-            return;
-        }
-        self.add_mappings(CovTerm::Counter(id), code_regions);
-    }
-
-    /// Adds information about a coverage expression, along with zero or more
-    /// code regions mapped to that expression.
+    /// Adds information about a coverage expression.
     ///
     /// Both counters and "counter expressions" (or simply, "expressions") can be operands in other
     /// expressions. These are tracked as separate variants of `CovTerm`, so there is no ambiguity
     /// between operands that are counter IDs and operands that are expression IDs.
     #[instrument(level = "debug", skip(self))]
-    pub(crate) fn add_counter_expression(
+    pub(crate) fn add_expression_data(
         &mut self,
         expression_id: ExpressionId,
         lhs: CovTerm,
         op: Op,
         rhs: CovTerm,
-        code_regions: &[CodeRegion],
     ) {
         debug_assert!(
             expression_id.as_usize() < self.expressions.len(),
@@ -121,23 +116,17 @@ impl<'tcx> FunctionCoverage<'tcx> {
             None => *slot = Some(expression),
             // If this expression ID slot has already been filled, it should
             // contain identical information.
-            Some(ref previous_expression) => {
-                assert_eq!(
-                    previous_expression, &expression,
-                    "add_counter_expression: expression for id changed"
-                );
-                return;
-            }
+            Some(ref previous_expression) => assert_eq!(
+                previous_expression, &expression,
+                "add_counter_expression: expression for id changed"
+            ),
         }
-
-        self.add_mappings(CovTerm::Expression(expression_id), code_regions);
     }
 
-    /// Adds regions that will be marked as "unreachable", with a constant "zero counter".
+    /// Adds a region that will be marked as "unreachable", with a constant "zero counter".
     #[instrument(level = "debug", skip(self))]
-    pub(crate) fn add_unreachable_regions(&mut self, code_regions: &[CodeRegion]) {
-        assert!(!code_regions.is_empty(), "unreachable regions always have code regions");
-        self.add_mappings(CovTerm::Zero, code_regions);
+    pub(crate) fn add_unreachable_region(&mut self, code_region: &CodeRegion) {
+        self.add_mappings(CovTerm::Zero, std::slice::from_ref(code_region));
     }
 
     pub(crate) fn add_mappings(&mut self, kind: CovTerm, code_regions: &[CodeRegion]) {
diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
@@ -110,15 +110,14 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
             .entry(instance)
             .or_insert_with(|| FunctionCoverage::new(bx.tcx(), instance));
 
-        let Coverage { kind, code_regions } = coverage;
+        let Coverage { kind } = coverage;
         match *kind {
             CoverageKind::Counter { function_source_hash, id } => {
                 debug!(
                     "ensuring function source hash is set for instance={:?}; function_source_hash={}",
                     instance, function_source_hash,
                 );
-                func_coverage.set_function_source_hash(function_source_hash);
-                func_coverage.add_counter(id, code_regions);
+                func_coverage.counter_seen(id, function_source_hash);
                 // We need to explicitly drop the `RefMut` before calling into `instrprof_increment`,
                 // as that needs an exclusive borrow.
                 drop(coverage_map);
@@ -136,12 +135,9 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
                 bx.instrprof_increment(fn_name, hash, num_counters, index);
             }
             CoverageKind::Expression { id, lhs, op, rhs } => {
-                func_coverage.add_counter_expression(id, lhs, op, rhs, code_regions);
+                func_coverage.add_expression_data(id, lhs, op, rhs);
             }
-            CoverageKind::Unreachable => {
-                func_coverage.add_unreachable_regions(code_regions);
-            }
-            CoverageKind::Mappings { kind } => {
+            CoverageKind::Mappings { kind, ref code_regions } => {
                 func_coverage.add_mappings(kind, code_regions);
             }
         }
@@ -211,8 +207,7 @@ fn add_unused_function_coverage<'tcx>(
 
     let mut function_coverage = FunctionCoverage::unused(tcx, instance);
     for &code_region in tcx.covered_code_regions(def_id) {
-        let code_region = std::slice::from_ref(code_region);
-        function_coverage.add_unreachable_regions(code_region);
+        function_coverage.add_unreachable_region(code_region);
     }
 
     if let Some(coverage_context) = cx.coverage_context() {
diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs
@@ -75,9 +75,9 @@ pub enum CoverageKind {
         op: Op,
         rhs: CovTerm,
     },
-    Unreachable,
     Mappings {
         kind: CovTerm,
+        code_regions: Vec<CodeRegion>,
     },
 }
 
@@ -97,8 +97,11 @@ impl Debug for CoverageKind {
                 },
                 rhs,
             ),
-            Unreachable => write!(fmt, "Unreachable"),
-            Mappings { kind } => fmt.debug_struct("Mappings").field("kind", kind).finish(),
+            Mappings { kind, code_regions } => fmt
+                .debug_struct("Mappings")
+                .field("kind", kind)
+                .field("code_regions", code_regions)
+                .finish(),
         }
     }
 }
diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs
@@ -1473,12 +1473,8 @@ impl Debug for Statement<'_> {
             AscribeUserType(box (ref place, ref c_ty), ref variance) => {
                 write!(fmt, "AscribeUserType({place:?}, {variance:?}, {c_ty:?})")
             }
-            Coverage(box self::Coverage { ref kind, ref code_regions }) => {
-                if code_regions.is_empty() {
-                    write!(fmt, "Coverage::{kind:?}")
-                } else {
-                    write!(fmt, "Coverage::{kind:?} for {code_regions:?}")
-                }
+            Coverage(box self::Coverage { ref kind }) => {
+                write!(fmt, "Coverage::{kind:?}")
             }
             Intrinsic(box ref intrinsic) => write!(fmt, "{intrinsic}"),
             ConstEvalCounter => write!(fmt, "ConstEvalCounter"),
diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs
@@ -5,7 +5,7 @@
 
 use super::{BasicBlock, Constant, Local, SwitchTargets, UserTypeProjection};
 
-use crate::mir::coverage::{CodeRegion, CoverageKind};
+use crate::mir::coverage::CoverageKind;
 use crate::traits::Reveal;
 use crate::ty::adjustment::PointerCoercion;
 use crate::ty::GenericArgsRef;
@@ -523,7 +523,6 @@ pub enum FakeReadCause {
 #[derive(TypeFoldable, TypeVisitable)]
 pub struct Coverage {
     pub kind: CoverageKind,
-    pub code_regions: Vec<CodeRegion>,
 }
 
 #[derive(Clone, Debug, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)]
diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs
@@ -316,15 +316,13 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
 
             inject_statement(
                 self.mir_body,
-                CoverageKind::Mappings { kind: counter_kind.as_term() },
+                CoverageKind::Mappings { kind: counter_kind.as_term(), code_regions },
                 mir::START_BLOCK,
-                code_regions,
             );
             inject_statement(
                 self.mir_body,
                 self.make_mir_coverage_kind(&counter_kind),
                 self.bcb_leader_bb(bcb),
-                Vec::new(),
             );
         }
     }
@@ -407,7 +405,6 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
                         self.mir_body,
                         self.make_mir_coverage_kind(&counter_kind),
                         inject_to_bb,
-                        Vec::new(),
                     );
                 }
                 BcbCounter::Expression { .. } => inject_intermediate_expression(
@@ -473,18 +470,13 @@ fn inject_edge_counter_basic_block(
     new_bb
 }
 
-fn inject_statement(
-    mir_body: &mut mir::Body<'_>,
-    counter_kind: CoverageKind,
-    bb: BasicBlock,
-    code_regions: Vec<CodeRegion>,
-) {
-    debug!("  injecting statement {counter_kind:?} for {bb:?} at code regions: {code_regions:?}");
+fn inject_statement(mir_body: &mut mir::Body<'_>, counter_kind: CoverageKind, bb: BasicBlock) {
+    debug!("  injecting statement {counter_kind:?} for {bb:?}");
     let data = &mut mir_body[bb];
     let source_info = data.terminator().source_info;
     let statement = Statement {
         source_info,
-        kind: StatementKind::Coverage(Box::new(Coverage { kind: counter_kind, code_regions })),
+        kind: StatementKind::Coverage(Box::new(Coverage { kind: counter_kind })),
     };
     data.statements.insert(0, statement);
 }
@@ -498,10 +490,7 @@ fn inject_intermediate_expression(mir_body: &mut mir::Body<'_>, expression: Cove
     let source_info = data.terminator().source_info;
     let statement = Statement {
         source_info,
-        kind: StatementKind::Coverage(Box::new(Coverage {
-            kind: expression,
-            code_regions: Vec::new(),
-        })),
+        kind: StatementKind::Coverage(Box::new(Coverage { kind: expression })),
     };
     data.statements.push(statement);
 }
diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs
@@ -68,8 +68,7 @@ impl CoverageVisitor {
                 self.update_from_term(lhs);
                 self.update_from_term(rhs);
             }
-            CoverageKind::Unreachable => {}
-            CoverageKind::Mappings { kind } => self.update_from_term(kind),
+            CoverageKind::Mappings { kind, .. } => self.update_from_term(kind),
         }
     }
 }
@@ -94,8 +93,11 @@ fn coverageinfo<'tcx>(tcx: TyCtxt<'tcx>, instance_def: ty::InstanceDef<'tcx>) ->
 fn covered_code_regions(tcx: TyCtxt<'_>, def_id: DefId) -> Vec<&CodeRegion> {
     let body = mir_body(tcx, def_id);
     all_coverage_in_mir_body(body)
-        // Coverage statements have a list of code regions (possibly empty).
-        .flat_map(|coverage| coverage.code_regions.as_slice())
+        // Only `Mappings` statements have a list of code regions.
+        .flat_map(|coverage| match coverage.kind {
+            CoverageKind::Mappings { ref code_regions, .. } => code_regions.as_slice(),
+            _ => &[],
+        })
         .collect()
 }
 
diff --git a/compiler/rustc_mir_transform/src/simplify.rs b/compiler/rustc_mir_transform/src/simplify.rs
@@ -28,9 +28,8 @@
 //! return.
 
 use crate::MirPass;
-use rustc_data_structures::fx::{FxHashSet, FxIndexSet};
+use rustc_data_structures::fx::FxIndexSet;
 use rustc_index::{Idx, IndexSlice, IndexVec};
-use rustc_middle::mir::coverage::*;
 use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor};
 use rustc_middle::mir::*;
 use rustc_middle::ty::TyCtxt;
@@ -336,15 +335,14 @@ pub fn remove_duplicate_unreachable_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut B
     }
 }
 
-pub fn remove_dead_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
+pub fn remove_dead_blocks<'tcx>(_tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
     let reachable = traversal::reachable_as_bitset(body);
     let num_blocks = body.basic_blocks.len();
     if num_blocks == reachable.count() {
         return;
     }
 
     let basic_blocks = body.basic_blocks.as_mut();
-    let source_scopes = &body.source_scopes;
     let mut replacements: Vec<_> = (0..num_blocks).map(BasicBlock::new).collect();
     let mut used_blocks = 0;
     for alive_index in reachable.iter() {
@@ -358,10 +356,6 @@ pub fn remove_dead_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
         used_blocks += 1;
     }
 
-    if tcx.sess.instrument_coverage() {
-        save_unreachable_coverage(basic_blocks, source_scopes, used_blocks);
-    }
-
     basic_blocks.raw.truncate(used_blocks);
 
     for block in basic_blocks {
@@ -371,93 +365,6 @@ pub fn remove_dead_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
     }
 }
 
-/// Some MIR transforms can determine at compile time that a sequences of
-/// statements will never be executed, so they can be dropped from the MIR.
-/// For example, an `if` or `else` block that is guaranteed to never be executed
-/// because its condition can be evaluated at compile time, such as by const
-/// evaluation: `if false { ... }`.
-///
-/// Those statements are bypassed by redirecting paths in the CFG around the
-/// `dead blocks`; but with `-C instrument-coverage`, the dead blocks usually
-/// include `Coverage` statements representing the Rust source code regions to
-/// be counted at runtime. Without these `Coverage` statements, the regions are
-/// lost, and the Rust source code will show no coverage information.
-///
-/// What we want to show in a coverage report is the dead code with coverage
-/// counts of `0`. To do this, we need to save the code regions, by injecting
-/// `Unreachable` coverage statements. These are non-executable statements whose
-/// code regions are still recorded in the coverage map, representing regions
-/// with `0` executions.
-///
-/// If there are no live `Counter` `Coverage` statements remaining, we remove
-/// `Coverage` statements along with the dead blocks. Since at least one
-/// counter per function is required by LLVM (and necessary, to add the
-/// `function_hash` to the counter's call to the LLVM intrinsic
-/// `instrprof.increment()`).
-///
-/// The `generator::StateTransform` MIR pass and MIR inlining can create
-/// atypical conditions, where all live `Counter`s are dropped from the MIR.
-///
-/// With MIR inlining we can have coverage counters belonging to different
-/// instances in a single body, so the strategy described above is applied to
-/// coverage counters from each instance individually.
-fn save_unreachable_coverage(
-    basic_blocks: &mut IndexSlice<BasicBlock, BasicBlockData<'_>>,
-    source_scopes: &IndexSlice<SourceScope, SourceScopeData<'_>>,
-    first_dead_block: usize,
-) {
-    // Identify instances that still have some live coverage counters left.
-    let mut live = FxHashSet::default();
-    for basic_block in &basic_blocks.raw[0..first_dead_block] {
-        for statement in &basic_block.statements {
-            let StatementKind::Coverage(coverage) = &statement.kind else { continue };
-            let CoverageKind::Counter { .. } = coverage.kind else { continue };
-            let instance = statement.source_info.scope.inlined_instance(source_scopes);
-            live.insert(instance);
-        }
-    }
-
-    for block in &mut basic_blocks.raw[..first_dead_block] {
-        for statement in &mut block.statements {
-            let StatementKind::Coverage(_) = &statement.kind else { continue };
-            let instance = statement.source_info.scope.inlined_instance(source_scopes);
-            if !live.contains(&instance) {
-                statement.make_nop();
-            }
-        }
-    }
-
-    if live.is_empty() {
-        return;
-    }
-
-    // Retain coverage for instances that still have some live counters left.
-    let mut retained_coverage = Vec::new();
-    for dead_block in &basic_blocks.raw[first_dead_block..] {
-        for statement in &dead_block.statements {
-            let StatementKind::Coverage(coverage) = &statement.kind else { continue };
-            if coverage.code_regions.is_empty() {
-                continue;
-            };
-            let instance = statement.source_info.scope.inlined_instance(source_scopes);
-            if live.contains(&instance) {
-                retained_coverage.push((statement.source_info, coverage.code_regions.clone()));
-            }
-        }
-    }
-
-    let start_block = &mut basic_blocks[START_BLOCK];
-    start_block.statements.extend(retained_coverage.into_iter().map(
-        |(source_info, code_regions)| Statement {
-            source_info,
-            kind: StatementKind::Coverage(Box::new(Coverage {
-                kind: CoverageKind::Unreachable,
-                code_regions,
-            })),
-        },
-    ));
-}
-
 pub enum SimplifyLocals {
     BeforeConstProp,
     Final,
diff --git a/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff
diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff

Original file line number	Diff line number	Diff line change
`@@ -75,9 +75,9 @@ pub enum CoverageKind {`
`75`	`75`	`op: Op,`
`76`	`76`	`rhs: CovTerm,`
`77`	`77`	`},`
`78`		`- Unreachable,`
`79`	`78`	`Mappings {`
`80`	`79`	`kind: CovTerm,`
	`80`	`+ code_regions: Vec<CodeRegion>,`
`81`	`81`	`},`
`82`	`82`	`}`
`83`	`83`
`@@ -97,8 +97,11 @@ impl Debug for CoverageKind {`
`97`	`97`	`},`
`98`	`98`	`rhs,`
`99`	`99`	`),`
`100`		`- Unreachable => write!(fmt, "Unreachable"),`
`101`		`- Mappings { kind } => fmt.debug_struct("Mappings").field("kind", kind).finish(),`
	`100`	`+ Mappings { kind, code_regions } => fmt`
	`101`	`+ .debug_struct("Mappings")`
	`102`	`+ .field("kind", kind)`
	`103`	`+ .field("code_regions", code_regions)`
	`104`	`+ .finish(),`
`102`	`105`	`}`
`103`	`106`	`}`
`104`	`107`	`}`
Original file line number	Diff line number	Diff line change
`@@ -68,8 +68,7 @@ impl CoverageVisitor {`
`68`	`68`	`self.update_from_term(lhs);`
`69`	`69`	`self.update_from_term(rhs);`
`70`	`70`	`}`
`71`		`- CoverageKind::Unreachable => {}`
`72`		`- CoverageKind::Mappings { kind } => self.update_from_term(kind),`
	`71`	`+ CoverageKind::Mappings { kind, .. } => self.update_from_term(kind),`
`73`	`72`	`}`
`74`	`73`	`}`
`75`	`74`	`}`
`@@ -94,8 +93,11 @@ fn coverageinfo<'tcx>(tcx: TyCtxt<'tcx>, instance_def: ty::InstanceDef<'tcx>) ->`
`94`	`93`	`fn covered_code_regions(tcx: TyCtxt<'_>, def_id: DefId) -> Vec<&CodeRegion> {`
`95`	`94`	`let body = mir_body(tcx, def_id);`
`96`	`95`	`all_coverage_in_mir_body(body)`
`97`		`- // Coverage statements have a list of code regions (possibly empty).`
`98`		`- .flat_map(\|coverage\| coverage.code_regions.as_slice())`
	`96`	+ // Only `Mappings` statements have a list of code regions.
	`97`	`+ .flat_map(\|coverage\| match coverage.kind {`
	`98`	`+ CoverageKind::Mappings { ref code_regions, .. } => code_regions.as_slice(),`
	`99`	`+ _ => &[],`
	`100`	`+ })`
`99`	`101`	`.collect()`
`100`	`102`	`}`
`101`	`103`