Auto merge of #122860 - Zalathar:unused, r=<try>

coverage: Re-enable `UnreachablePropagation` for coverage builds

This is a sequence of 3 related changes:
- Clean up the existing code that scans for unused functions
- Detect functions that were instrumented for coverage, but have had all their coverage statements removed by later MIR transforms (e.g. `UnreachablePropagation`)
- Re-enable `UnreachablePropagation` in coverage builds

Because we now detect functions that have lost their coverage statements, and treat them as unused, we don't need to worry about `UnreachablePropagation` removing all of those statements. This is demonstrated by `tests/coverage/unreachable.rs`.

Fixes #116171.

Author: bors

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@@ -6,9 +6,8 @@ use crate::llvm;
 
 use itertools::Itertools as _;
 use rustc_codegen_ssa::traits::{BaseTypeMethods, ConstMethods};
-use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
-use rustc_hir::def::DefKind;
-use rustc_hir::def_id::DefId;
+use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet};
+use rustc_hir::def_id::{DefId, LocalDefId};
 use rustc_index::IndexVec;
 use rustc_middle::bug;
 use rustc_middle::mir;
@@ -335,16 +334,9 @@ fn save_function_record(
     );
 }
 
-/// When finalizing the coverage map, `FunctionCoverage` only has the `CodeRegion`s and counters for
-/// the functions that went through codegen; such as public functions and "used" functions
-/// (functions referenced by other "used" or public items). Any other functions considered unused,
-/// or "Unreachable", were still parsed and processed through the MIR stage, but were not
-/// codegenned. (Note that `-Clink-dead-code` can force some unused code to be codegenned, but
-/// that flag is known to cause other errors, when combined with `-C instrument-coverage`; and
-/// `-Clink-dead-code` will not generate code for unused generic functions.)
-///
-/// We can find the unused functions (including generic functions) by the set difference of all MIR
-/// `DefId`s (`tcx` query `mir_keys`) minus the codegenned `DefId`s (`codegenned_and_inlined_items`).
+/// Each CGU will normally only emit coverage metadata for the functions that it actually generates.
+/// But since we don't want unused functions to disappear from coverage reports, we also scan for
+/// functions that were instrumented but are not participating in codegen.
 ///
 /// These unused functions don't need to be codegenned, but we do need to add them to the function
 /// coverage map (in a single designated CGU) so that we still emit coverage mappings for them.
@@ -354,75 +346,111 @@ fn add_unused_functions(cx: &CodegenCx<'_, '_>) {
     assert!(cx.codegen_unit.is_code_coverage_dead_code_cgu());
 
     let tcx = cx.tcx;
+    let usage = prepare_usage_sets(tcx);
+
+    let is_unused_fn = |def_id: &&LocalDefId| -> bool {
+        let def_id = def_id.to_def_id();
+
+        // To be eligible for "unused function" mappings, a definition must:
+        // - Be function-like
+        // - Not participate directly in codegen (or have lost all its coverage statements)
+        // - Not have any coverage statements inlined into codegenned functions
+        tcx.def_kind(def_id).is_fn_like()
+            && (!usage.all_mono_items.contains(&def_id)
+                || usage.missing_own_coverage.contains(&def_id))
+            && !usage.used_via_inlining.contains(&def_id)
+    };
 
-    let eligible_def_ids = tcx.mir_keys(()).iter().filter_map(|local_def_id| {
-        let def_id = local_def_id.to_def_id();
-        let kind = tcx.def_kind(def_id);
-        // `mir_keys` will give us `DefId`s for all kinds of things, not
-        // just "functions", like consts, statics, etc. Filter those out.
-        if !matches!(kind, DefKind::Fn | DefKind::AssocFn | DefKind::Closure) {
-            return None;
-        }
+    // Scan for unused functions that were instrumented for coverage.
+    for &def_id in tcx.mir_keys(()).iter().filter(is_unused_fn) {
+        // Get the coverage info from MIR, skipping functions that were never instrumented.
+        let body = tcx.optimized_mir(def_id);
+        let Some(function_coverage_info) = body.function_coverage_info.as_deref() else { continue };
 
         // FIXME(79651): Consider trying to filter out dummy instantiations of
         // unused generic functions from library crates, because they can produce
         // "unused instantiation" in coverage reports even when they are actually
         // used by some downstream crate in the same binary.
 
-        Some(local_def_id.to_def_id())
-    });
-
-    let codegenned_def_ids = codegenned_and_inlined_items(tcx);
-
-    // For each `DefId` that should have coverage instrumentation but wasn't
-    // codegenned, add it to the function coverage map as an unused function.
-    for def_id in eligible_def_ids.filter(|id| !codegenned_def_ids.contains(id)) {
-        // Skip any function that didn't have coverage data added to it by the
-        // coverage instrumentor.
-        let body = tcx.instance_mir(ty::InstanceDef::Item(def_id));
-        let Some(function_coverage_info) = body.function_coverage_info.as_deref() else {
-            continue;
-        };
-
         debug!("generating unused fn: {def_id:?}");
-        let instance = declare_unused_fn(tcx, def_id);
-        add_unused_function_coverage(cx, instance, function_coverage_info);
+        add_unused_function_coverage(cx, def_id, function_coverage_info);
     }
 }
 
-/// All items participating in code generation together with (instrumented)
-/// items inlined into them.
-fn codegenned_and_inlined_items(tcx: TyCtxt<'_>) -> DefIdSet {
-    let (items, cgus) = tcx.collect_and_partition_mono_items(());
-    let mut visited = DefIdSet::default();
-    let mut result = items.clone();
-
-    for cgu in cgus {
-        for item in cgu.items().keys() {
-            if let mir::mono::MonoItem::Fn(ref instance) = item {
-                let did = instance.def_id();
-                if !visited.insert(did) {
-                    continue;
-                }
-                let body = tcx.instance_mir(instance.def);
-                for block in body.basic_blocks.iter() {
-                    for statement in &block.statements {
-                        let mir::StatementKind::Coverage(_) = statement.kind else { continue };
-                        let scope = statement.source_info.scope;
-                        if let Some(inlined) = scope.inlined_instance(&body.source_scopes) {
-                            result.insert(inlined.def_id());
-                        }
-                    }
-                }
+struct UsageSets<'tcx> {
+    all_mono_items: &'tcx DefIdSet,
+    used_via_inlining: FxHashSet<DefId>,
+    missing_own_coverage: FxHashSet<DefId>,
+}
+
+/// Prepare sets of definitions that are relevant to deciding whether something
+/// is an "unused function" for coverage purposes.
+fn prepare_usage_sets<'tcx>(tcx: TyCtxt<'tcx>) -> UsageSets<'tcx> {
+    let (all_mono_items, cgus) = tcx.collect_and_partition_mono_items(());
+
+    // Obtain a MIR body for each function participating in codegen, via an
+    // arbitrary instance.
+    let def_and_mir_for_all_mono_fns = cgus
+        .iter()
+        .flat_map(|cgu| cgu.items().keys())
+        .filter_map(|item| match item {
+            mir::mono::MonoItem::Fn(instance) => Some(instance),
+            mir::mono::MonoItem::Static(_) | mir::mono::MonoItem::GlobalAsm(_) => None,
+        })
+        .filter({
+            // We only need one arbitrary instance per definition.
+            let mut seen = FxHashSet::default();
+            move |instance| seen.insert(instance.def_id())
+        })
+        .map(|instance| {
+            // We don't care about the instance, just its underlying MIR.
+            let body = tcx.instance_mir(instance.def);
+            (instance.def_id(), body)
+        });
+
+    // Functions whose coverage statments were found inlined into other functions.
+    let mut used_via_inlining = FxHashSet::default();
+    // Functions that were instrumented, but had all of their coverage statements
+    // removed by later MIR transforms (e.g. UnreachablePropagation).
+    let mut missing_own_coverage = FxHashSet::default();
+
+    for (def_id, body) in def_and_mir_for_all_mono_fns {
+        let mut saw_own_coverage = false;
+
+        // Inspect every coverage statement in the function's MIR.
+        for stmt in body
+            .basic_blocks
+            .iter()
+            .flat_map(|block| &block.statements)
+            .filter(|stmt| matches!(stmt.kind, mir::StatementKind::Coverage(_)))
+        {
+            if let Some(inlined) = stmt.source_info.scope.inlined_instance(&body.source_scopes) {
+                // This coverage statement was inlined from another function.
+                used_via_inlining.insert(inlined.def_id());
+            } else {
+                // Non-inlined coverage statements belong to the enclosing function.
+                saw_own_coverage = true;
             }
         }
+
+        if !saw_own_coverage && body.function_coverage_info.is_some() {
+            missing_own_coverage.insert(def_id);
+        }
     }
 
-    result
+    UsageSets { all_mono_items, used_via_inlining, missing_own_coverage }
 }
 
-fn declare_unused_fn<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> ty::Instance<'tcx> {
-    ty::Instance::new(
+fn add_unused_function_coverage<'tcx>(
+    cx: &CodegenCx<'_, 'tcx>,
+    def_id: LocalDefId,
+    function_coverage_info: &'tcx mir::coverage::FunctionCoverageInfo,
+) {
+    let tcx = cx.tcx;
+    let def_id = def_id.to_def_id();
+
+    // Make a dummy instance that fills in all generics with placeholders.
+    let instance = ty::Instance::new(
         def_id,
         ty::GenericArgs::for_item(tcx, def_id, |param, _| {
             if let ty::GenericParamDefKind::Lifetime = param.kind {
@@ -431,14 +459,8 @@ fn declare_unused_fn<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> ty::Instance<'tc
                 tcx.mk_param_from_def(param)
             }
         }),
-    )
-}
+    );
 
-fn add_unused_function_coverage<'tcx>(
-    cx: &CodegenCx<'_, 'tcx>,
-    instance: ty::Instance<'tcx>,
-    function_coverage_info: &'tcx mir::coverage::FunctionCoverageInfo,
-) {
     // An unused function's mappings will automatically be rewritten to map to
     // zero, because none of its counters/expressions are marked as seen.
     let function_coverage = FunctionCoverageCollector::unused(instance, function_coverage_info);

compiler/rustc_mir_transform/src/unreachable_prop.rs

@@ -14,11 +14,7 @@ pub struct UnreachablePropagation;
 impl MirPass<'_> for UnreachablePropagation {
     fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
         // Enable only under -Zmir-opt-level=2 as this can make programs less debuggable.
-
-        // FIXME(#116171) Coverage gets confused by MIR passes that can remove all
-        // coverage statements from an instrumented function. This pass can be
-        // re-enabled when coverage codegen is robust against that happening.
-        sess.mir_opt_level() >= 2 && !sess.instrument_coverage()
+        sess.mir_opt_level() >= 2
     }
 
     fn run_pass<'tcx>(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {

tests/coverage/unreachable.cov-map

@@ -14,11 +14,11 @@ Number of expressions: 0
 Number of file 0 mappings: 1
 - Code(Counter(0)) at (prev + 17, 1) to (start + 1, 37)
 
-Function name: unreachable::unreachable_intrinsic
-Raw bytes (9): 0x[01, 01, 00, 01, 01, 16, 01, 01, 2c]
+Function name: unreachable::unreachable_intrinsic (unused)
+Raw bytes (9): 0x[01, 01, 00, 01, 00, 16, 01, 01, 2c]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 0
 Number of file 0 mappings: 1
-- Code(Counter(0)) at (prev + 22, 1) to (start + 1, 44)
+- Code(Zero) at (prev + 22, 1) to (start + 1, 44)