From fe412af4fce43aa5222b8940e068da3a19188e4b Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 15 Dec 2024 00:34:06 +1100 Subject: [PATCH 1/6] coverage: Use `is_eligible_for_coverage` to filter unused functions The checks in `is_eligible_for_coverage` include `is_fn_like`, but will also exclude various function-like things that cannot possibly have coverage instrumentation. --- .../rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 4f2af73252751..c57946cc31721 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -271,16 +271,15 @@ fn add_unused_functions(cx: &CodegenCx<'_, '_>) { 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 + // Usage sets expect `DefId`, so convert from `LocalDefId`. + let d: DefId = LocalDefId::to_def_id(def_id); + // To be potentially eligible for "unused function" mappings, a definition must: + // - Be eligible for coverage instrumentation // - 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) + tcx.is_eligible_for_coverage(def_id) + && (!usage.all_mono_items.contains(&d) || usage.missing_own_coverage.contains(&d)) + && !usage.used_via_inlining.contains(&d) }; // Scan for unused functions that were instrumented for coverage. From 154fae1e8d825955ce83ae4c7d21a03db406e56c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 13 Dec 2024 22:40:45 +1100 Subject: [PATCH 2/6] coverage: Build the global file table on the fly --- .../src/coverageinfo/mapgen.rs | 58 +++++++++---------- .../src/coverageinfo/mapgen/covfun.rs | 4 +- tests/coverage/unused_mod.cov-map | 8 +-- 3 files changed, 33 insertions(+), 37 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index c57946cc31721..0c7ad300a8b12 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -1,6 +1,6 @@ use std::iter; -use itertools::Itertools as _; +use itertools::Itertools; use rustc_abi::Align; use rustc_codegen_ssa::traits::{ BaseTypeCodegenMethods, ConstCodegenMethods, StaticCodegenMethods, @@ -8,8 +8,8 @@ use rustc_codegen_ssa::traits::{ use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet}; use rustc_hir::def_id::{DefId, LocalDefId}; use rustc_index::IndexVec; +use rustc_middle::mir; use rustc_middle::ty::{self, TyCtxt}; -use rustc_middle::{bug, mir}; use rustc_session::RemapFileNameExt; use rustc_session::config::RemapPathScopeComponents; use rustc_span::def_id::DefIdSet; @@ -67,25 +67,21 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { return; } - let all_file_names = function_coverage_map - .iter() - .map(|(_, fn_cov)| fn_cov.function_coverage_info.body_span) - .map(|span| span_file_name(tcx, span)); - let global_file_table = GlobalFileTable::new(all_file_names); - - // Encode all filenames referenced by coverage mappings in this CGU. - let filenames_buffer = global_file_table.make_filenames_buffer(tcx); - // The `llvm-cov` tool uses this hash to associate each covfun record with - // its corresponding filenames table, since the final binary will typically - // contain multiple covmap records from different compilation units. - let filenames_hash = llvm_cov::hash_bytes(&filenames_buffer); - - let mut unused_function_names = Vec::new(); + // The order of entries in this global file table needs to be deterministic, + // and ideally should also be independent of the details of stable-hashing, + // because coverage tests snapshots (`.cov-map`) can observe the order and + // would need to be re-blessed if it changes. As long as those requirements + // are satisfied, the order can be arbitrary. + let mut global_file_table = GlobalFileTable::new(); let covfun_records = function_coverage_map .into_iter() + // Sort by symbol name, so that the global file table is built in an + // order that doesn't depend on the stable-hash-based order in which + // instances were visited during codegen. + .sorted_by_cached_key(|&(instance, _)| tcx.symbol_name(instance).name) .filter_map(|(instance, function_coverage)| { - prepare_covfun_record(tcx, &global_file_table, instance, &function_coverage) + prepare_covfun_record(tcx, &mut global_file_table, instance, &function_coverage) }) .collect::>(); @@ -98,6 +94,15 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { return; } + // Encode all filenames referenced by coverage mappings in this CGU. + let filenames_buffer = global_file_table.make_filenames_buffer(tcx); + // The `llvm-cov` tool uses this hash to associate each covfun record with + // its corresponding filenames table, since the final binary will typically + // contain multiple covmap records from different compilation units. + let filenames_hash = llvm_cov::hash_bytes(&filenames_buffer); + + let mut unused_function_names = vec![]; + for covfun in &covfun_records { unused_function_names.extend(covfun.mangled_function_name_if_unused()); @@ -137,22 +142,13 @@ struct GlobalFileTable { } impl GlobalFileTable { - fn new(all_file_names: impl IntoIterator) -> Self { - // Collect all of the filenames into a set. Filenames usually come in - // contiguous runs, so we can dedup adjacent ones to save work. - let mut raw_file_table = all_file_names.into_iter().dedup().collect::>(); - - // Sort the file table by its actual string values, not the arbitrary - // ordering of its symbols. - raw_file_table.sort_unstable_by(|a, b| a.as_str().cmp(b.as_str())); - - Self { raw_file_table } + fn new() -> Self { + Self { raw_file_table: FxIndexSet::default() } } - fn global_file_id_for_file_name(&self, file_name: Symbol) -> GlobalFileId { - let raw_id = self.raw_file_table.get_index_of(&file_name).unwrap_or_else(|| { - bug!("file name not found in prepared global file table: {file_name}"); - }); + fn global_file_id_for_file_name(&mut self, file_name: Symbol) -> GlobalFileId { + // Ensure the given file has a table entry, and get its index. + let (raw_id, _) = self.raw_file_table.insert_full(file_name); // The raw file table doesn't include an entry for the working dir // (which has ID 0), so add 1 to get the correct ID. GlobalFileId::from_usize(raw_id + 1) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs index 33e7a0f2f201b..6a151d3c2b009 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs @@ -45,7 +45,7 @@ impl<'tcx> CovfunRecord<'tcx> { pub(crate) fn prepare_covfun_record<'tcx>( tcx: TyCtxt<'tcx>, - global_file_table: &GlobalFileTable, + global_file_table: &mut GlobalFileTable, instance: Instance<'tcx>, function_coverage: &FunctionCoverage<'tcx>, ) -> Option> { @@ -75,7 +75,7 @@ pub(crate) fn prepare_covfun_record<'tcx>( /// Populates the mapping region tables in the current function's covfun record. fn fill_region_tables<'tcx>( tcx: TyCtxt<'tcx>, - global_file_table: &GlobalFileTable, + global_file_table: &mut GlobalFileTable, function_coverage: &FunctionCoverage<'tcx>, covfun: &mut CovfunRecord<'tcx>, ) { diff --git a/tests/coverage/unused_mod.cov-map b/tests/coverage/unused_mod.cov-map index af10906fa3c54..5e8b69fcdba9f 100644 --- a/tests/coverage/unused_mod.cov-map +++ b/tests/coverage/unused_mod.cov-map @@ -1,16 +1,16 @@ Function name: unused_mod::main -Raw bytes (9): 0x[01, 02, 00, 01, 01, 04, 01, 02, 02] +Raw bytes (9): 0x[01, 01, 00, 01, 01, 04, 01, 02, 02] Number of files: 1 -- file 0 => global file 2 +- file 0 => global file 1 Number of expressions: 0 Number of file 0 mappings: 1 - Code(Counter(0)) at (prev + 4, 1) to (start + 2, 2) Highest counter ID seen: c0 Function name: unused_mod::unused_module::never_called_function (unused) -Raw bytes (9): 0x[01, 01, 00, 01, 00, 02, 01, 02, 02] +Raw bytes (9): 0x[01, 02, 00, 01, 00, 02, 01, 02, 02] Number of files: 1 -- file 0 => global file 1 +- file 0 => global file 2 Number of expressions: 0 Number of file 0 mappings: 1 - Code(Zero) at (prev + 2, 1) to (start + 2, 2) From 252276a53d05f49523e6eac992b5f49e9815db0c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 12 Dec 2024 21:36:20 +1100 Subject: [PATCH 3/6] coverage: Pull expression conversion out of `map_data.rs` --- .../src/coverageinfo/map_data.rs | 32 +------------ .../src/coverageinfo/mapgen/covfun.rs | 45 ++++++++++++++++++- 2 files changed, 44 insertions(+), 33 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index c5d1ebdfe7c5f..5b05f0867b9f4 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -1,11 +1,7 @@ -use rustc_data_structures::captures::Captures; use rustc_middle::mir::coverage::{ - CovTerm, CoverageIdsInfo, Expression, FunctionCoverageInfo, Mapping, MappingKind, Op, - SourceRegion, + CovTerm, CoverageIdsInfo, FunctionCoverageInfo, Mapping, MappingKind, SourceRegion, }; -use crate::coverageinfo::ffi::{Counter, CounterExpression, ExprKind}; - pub(crate) struct FunctionCoverage<'tcx> { pub(crate) function_coverage_info: &'tcx FunctionCoverageInfo, /// If `None`, the corresponding function is unused. @@ -35,28 +31,6 @@ impl<'tcx> FunctionCoverage<'tcx> { if self.is_used() { self.function_coverage_info.function_source_hash } else { 0 } } - /// Convert this function's coverage expression data into a form that can be - /// passed through FFI to LLVM. - pub(crate) fn counter_expressions( - &self, - ) -> impl Iterator + ExactSizeIterator + Captures<'_> { - // We know that LLVM will optimize out any unused expressions before - // producing the final coverage map, so there's no need to do the same - // thing on the Rust side unless we're confident we can do much better. - // (See `CounterExpressionsMinimizer` in `CoverageMappingWriter.cpp`.) - - self.function_coverage_info.expressions.iter().map(move |&Expression { lhs, op, rhs }| { - CounterExpression { - lhs: self.counter_for_term(lhs), - kind: match op { - Op::Add => ExprKind::Add, - Op::Subtract => ExprKind::Subtract, - }, - rhs: self.counter_for_term(rhs), - } - }) - } - /// Converts this function's coverage mappings into an intermediate form /// that will be used by `mapgen` when preparing for FFI. pub(crate) fn counter_regions( @@ -70,10 +44,6 @@ impl<'tcx> FunctionCoverage<'tcx> { }) } - fn counter_for_term(&self, term: CovTerm) -> Counter { - if self.is_zero_term(term) { Counter::ZERO } else { Counter::from_term(term) } - } - fn is_zero_term(&self, term: CovTerm) -> bool { match self.ids_info { Some(ids_info) => ids_info.is_zero_term(term), diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs index 6a151d3c2b009..e71c15e785655 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs @@ -11,7 +11,9 @@ use rustc_codegen_ssa::traits::{ BaseTypeCodegenMethods, ConstCodegenMethods, StaticCodegenMethods, }; use rustc_middle::bug; -use rustc_middle::mir::coverage::MappingKind; +use rustc_middle::mir::coverage::{ + CoverageIdsInfo, Expression, FunctionCoverageInfo, MappingKind, Op, +}; use rustc_middle::ty::{Instance, TyCtxt}; use rustc_target::spec::HasTargetSpec; use tracing::debug; @@ -49,12 +51,17 @@ pub(crate) fn prepare_covfun_record<'tcx>( instance: Instance<'tcx>, function_coverage: &FunctionCoverage<'tcx>, ) -> Option> { + let fn_cov_info = tcx.instance_mir(instance.def).function_coverage_info.as_deref()?; + let ids_info = tcx.coverage_ids_info(instance.def); + + let expressions = prepare_expressions(fn_cov_info, ids_info, function_coverage.is_used()); + let mut covfun = CovfunRecord { mangled_function_name: tcx.symbol_name(instance).name, source_hash: function_coverage.source_hash(), is_used: function_coverage.is_used(), virtual_file_mapping: VirtualFileMapping::default(), - expressions: function_coverage.counter_expressions().collect::>(), + expressions, regions: ffi::Regions::default(), }; @@ -72,6 +79,40 @@ pub(crate) fn prepare_covfun_record<'tcx>( Some(covfun) } +/// Convert the function's coverage-counter expressions into a form suitable for FFI. +fn prepare_expressions( + fn_cov_info: &FunctionCoverageInfo, + ids_info: &CoverageIdsInfo, + is_used: bool, +) -> Vec { + // If any counters or expressions were removed by MIR opts, replace their + // terms with zero. + let counter_for_term = |term| { + if !is_used || ids_info.is_zero_term(term) { + ffi::Counter::ZERO + } else { + ffi::Counter::from_term(term) + } + }; + + // We know that LLVM will optimize out any unused expressions before + // producing the final coverage map, so there's no need to do the same + // thing on the Rust side unless we're confident we can do much better. + // (See `CounterExpressionsMinimizer` in `CoverageMappingWriter.cpp`.) + fn_cov_info + .expressions + .iter() + .map(move |&Expression { lhs, op, rhs }| ffi::CounterExpression { + lhs: counter_for_term(lhs), + kind: match op { + Op::Add => ffi::ExprKind::Add, + Op::Subtract => ffi::ExprKind::Subtract, + }, + rhs: counter_for_term(rhs), + }) + .collect::>() +} + /// Populates the mapping region tables in the current function's covfun record. fn fill_region_tables<'tcx>( tcx: TyCtxt<'tcx>, From 527f8127bbde4d189ee292dee0d6070550ec0ba6 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 12 Dec 2024 21:46:34 +1100 Subject: [PATCH 4/6] coverage: Pull region conversion out of `map_data.rs` --- .../src/coverageinfo/map_data.rs | 25 +----------------- .../src/coverageinfo/mapgen/covfun.rs | 26 +++++++++---------- 2 files changed, 14 insertions(+), 37 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 5b05f0867b9f4..e1b2a1b87bb96 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -1,6 +1,4 @@ -use rustc_middle::mir::coverage::{ - CovTerm, CoverageIdsInfo, FunctionCoverageInfo, Mapping, MappingKind, SourceRegion, -}; +use rustc_middle::mir::coverage::{CoverageIdsInfo, FunctionCoverageInfo}; pub(crate) struct FunctionCoverage<'tcx> { pub(crate) function_coverage_info: &'tcx FunctionCoverageInfo, @@ -30,25 +28,4 @@ impl<'tcx> FunctionCoverage<'tcx> { pub(crate) fn source_hash(&self) -> u64 { if self.is_used() { self.function_coverage_info.function_source_hash } else { 0 } } - - /// Converts this function's coverage mappings into an intermediate form - /// that will be used by `mapgen` when preparing for FFI. - pub(crate) fn counter_regions( - &self, - ) -> impl Iterator + ExactSizeIterator { - self.function_coverage_info.mappings.iter().map(move |mapping| { - let Mapping { kind, source_region } = mapping; - let kind = - kind.map_terms(|term| if self.is_zero_term(term) { CovTerm::Zero } else { term }); - (kind, source_region) - }) - } - - fn is_zero_term(&self, term: CovTerm) -> bool { - match self.ids_info { - Some(ids_info) => ids_info.is_zero_term(term), - // This function is unused, so all coverage counters/expressions are zero. - None => true, - } - } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs index e71c15e785655..fa0af8415e746 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs @@ -12,7 +12,7 @@ use rustc_codegen_ssa::traits::{ }; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CoverageIdsInfo, Expression, FunctionCoverageInfo, MappingKind, Op, + CovTerm, CoverageIdsInfo, Expression, FunctionCoverageInfo, Mapping, MappingKind, Op, }; use rustc_middle::ty::{Instance, TyCtxt}; use rustc_target::spec::HasTargetSpec; @@ -65,7 +65,7 @@ pub(crate) fn prepare_covfun_record<'tcx>( regions: ffi::Regions::default(), }; - fill_region_tables(tcx, global_file_table, function_coverage, &mut covfun); + fill_region_tables(tcx, global_file_table, fn_cov_info, ids_info, &mut covfun); if covfun.regions.has_no_regions() { if covfun.is_used { @@ -117,16 +117,12 @@ fn prepare_expressions( fn fill_region_tables<'tcx>( tcx: TyCtxt<'tcx>, global_file_table: &mut GlobalFileTable, - function_coverage: &FunctionCoverage<'tcx>, + fn_cov_info: &'tcx FunctionCoverageInfo, + ids_info: &'tcx CoverageIdsInfo, covfun: &mut CovfunRecord<'tcx>, ) { - let counter_regions = function_coverage.counter_regions(); - if counter_regions.is_empty() { - return; - } - // Currently a function's mappings must all be in the same file as its body span. - let file_name = span_file_name(tcx, function_coverage.function_coverage_info.body_span); + let file_name = span_file_name(tcx, fn_cov_info.body_span); // Look up the global file ID for that filename. let global_file_id = global_file_table.global_file_id_for_file_name(file_name); @@ -140,10 +136,14 @@ fn fill_region_tables<'tcx>( // For each counter/region pair in this function+file, convert it to a // form suitable for FFI. - for (mapping_kind, region) in counter_regions { - debug!("Adding counter {mapping_kind:?} to map for {region:?}"); - let span = ffi::CoverageSpan::from_source_region(local_file_id, region); - match mapping_kind { + let is_zero_term = |term| !covfun.is_used || ids_info.is_zero_term(term); + for Mapping { kind, ref source_region } in &fn_cov_info.mappings { + // If the mapping refers to counters/expressions that were removed by + // MIR opts, replace those occurrences with zero. + let kind = kind.map_terms(|term| if is_zero_term(term) { CovTerm::Zero } else { term }); + + let span = ffi::CoverageSpan::from_source_region(local_file_id, source_region); + match kind { MappingKind::Code(term) => { code_regions.push(ffi::CodeRegion { span, counter: ffi::Counter::from_term(term) }); } From d34c365eb0c40d907daf42fff42b7b6ebdc314ab Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 14 Dec 2024 23:14:19 +1100 Subject: [PATCH 5/6] coverage: Pull function source hash out of `map_data.rs` --- compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs | 7 +------ compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 3 ++- .../rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs | 9 ++++----- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index e1b2a1b87bb96..261a014c3d1c7 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -1,6 +1,7 @@ use rustc_middle::mir::coverage::{CoverageIdsInfo, FunctionCoverageInfo}; pub(crate) struct FunctionCoverage<'tcx> { + #[expect(unused)] // This whole file gets deleted later in the same PR. pub(crate) function_coverage_info: &'tcx FunctionCoverageInfo, /// If `None`, the corresponding function is unused. ids_info: Option<&'tcx CoverageIdsInfo>, @@ -22,10 +23,4 @@ impl<'tcx> FunctionCoverage<'tcx> { pub(crate) fn is_used(&self) -> bool { self.ids_info.is_some() } - - /// Return the source hash, generated from the HIR node structure, and used to indicate whether - /// or not the source code structure changed between different compilations. - pub(crate) fn source_hash(&self) -> u64 { - if self.is_used() { self.function_coverage_info.function_source_hash } else { 0 } - } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 0c7ad300a8b12..9bc3396704456 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -81,7 +81,8 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { // instances were visited during codegen. .sorted_by_cached_key(|&(instance, _)| tcx.symbol_name(instance).name) .filter_map(|(instance, function_coverage)| { - prepare_covfun_record(tcx, &mut global_file_table, instance, &function_coverage) + let is_used = function_coverage.is_used(); + prepare_covfun_record(tcx, &mut global_file_table, instance, is_used) }) .collect::>(); diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs index fa0af8415e746..8e853f057bedf 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs @@ -19,7 +19,6 @@ use rustc_target::spec::HasTargetSpec; use tracing::debug; use crate::common::CodegenCx; -use crate::coverageinfo::map_data::FunctionCoverage; use crate::coverageinfo::mapgen::{GlobalFileTable, VirtualFileMapping, span_file_name}; use crate::coverageinfo::{ffi, llvm_cov}; use crate::llvm; @@ -49,17 +48,17 @@ pub(crate) fn prepare_covfun_record<'tcx>( tcx: TyCtxt<'tcx>, global_file_table: &mut GlobalFileTable, instance: Instance<'tcx>, - function_coverage: &FunctionCoverage<'tcx>, + is_used: bool, ) -> Option> { let fn_cov_info = tcx.instance_mir(instance.def).function_coverage_info.as_deref()?; let ids_info = tcx.coverage_ids_info(instance.def); - let expressions = prepare_expressions(fn_cov_info, ids_info, function_coverage.is_used()); + let expressions = prepare_expressions(fn_cov_info, ids_info, is_used); let mut covfun = CovfunRecord { mangled_function_name: tcx.symbol_name(instance).name, - source_hash: function_coverage.source_hash(), - is_used: function_coverage.is_used(), + source_hash: if is_used { fn_cov_info.function_source_hash } else { 0 }, + is_used, virtual_file_mapping: VirtualFileMapping::default(), expressions, regions: ffi::Regions::default(), From 541d4e85d90b4c0416014c3620d3f3c9771bd476 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 14 Dec 2024 22:48:12 +1100 Subject: [PATCH 6/6] coverage: Track used functions in a set instead of a map This patch dismantles what was left of `FunctionCoverage` in `map_data.rs`, replaces `function_coverage_map` with a set, and overhauls how we prepare covfun records for unused functions. --- .../src/coverageinfo/map_data.rs | 26 ------ .../src/coverageinfo/mapgen.rs | 82 ++++++++----------- .../src/coverageinfo/mod.rs | 19 +---- 3 files changed, 37 insertions(+), 90 deletions(-) delete mode 100644 compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs deleted file mode 100644 index 261a014c3d1c7..0000000000000 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ /dev/null @@ -1,26 +0,0 @@ -use rustc_middle::mir::coverage::{CoverageIdsInfo, FunctionCoverageInfo}; - -pub(crate) struct FunctionCoverage<'tcx> { - #[expect(unused)] // This whole file gets deleted later in the same PR. - pub(crate) function_coverage_info: &'tcx FunctionCoverageInfo, - /// If `None`, the corresponding function is unused. - ids_info: Option<&'tcx CoverageIdsInfo>, -} - -impl<'tcx> FunctionCoverage<'tcx> { - pub(crate) fn new_used( - function_coverage_info: &'tcx FunctionCoverageInfo, - ids_info: &'tcx CoverageIdsInfo, - ) -> Self { - Self { function_coverage_info, ids_info: Some(ids_info) } - } - - pub(crate) fn new_unused(function_coverage_info: &'tcx FunctionCoverageInfo) -> Self { - Self { function_coverage_info, ids_info: None } - } - - /// Returns true for a used (called) function, and false for an unused function. - pub(crate) fn is_used(&self) -> bool { - self.ids_info.is_some() - } -} diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 9bc3396704456..ca33428620052 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -18,7 +18,6 @@ use tracing::debug; use crate::common::CodegenCx; use crate::coverageinfo::llvm_cov; -use crate::coverageinfo::map_data::FunctionCoverage; use crate::coverageinfo::mapgen::covfun::prepare_covfun_record; use crate::llvm; @@ -49,23 +48,11 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { debug!("Generating coverage map for CodegenUnit: `{}`", cx.codegen_unit.name()); - // In order to show that unused functions have coverage counts of zero (0), LLVM requires the - // functions exist. Generate synthetic functions with a (required) single counter, and add the - // MIR `Coverage` code regions to the `function_coverage_map`, before calling - // `ctx.take_function_coverage_map()`. - if cx.codegen_unit.is_code_coverage_dead_code_cgu() { - add_unused_functions(cx); - } - // FIXME(#132395): Can this be none even when coverage is enabled? - let function_coverage_map = match cx.coverage_cx { - Some(ref cx) => cx.take_function_coverage_map(), + let instances_used = match cx.coverage_cx { + Some(ref cx) => cx.instances_used.borrow(), None => return, }; - if function_coverage_map.is_empty() { - // This CGU has no functions with coverage instrumentation. - return; - } // The order of entries in this global file table needs to be deterministic, // and ideally should also be independent of the details of stable-hashing, @@ -74,18 +61,27 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { // are satisfied, the order can be arbitrary. let mut global_file_table = GlobalFileTable::new(); - let covfun_records = function_coverage_map - .into_iter() + let mut covfun_records = instances_used + .iter() + .copied() // Sort by symbol name, so that the global file table is built in an // order that doesn't depend on the stable-hash-based order in which // instances were visited during codegen. - .sorted_by_cached_key(|&(instance, _)| tcx.symbol_name(instance).name) - .filter_map(|(instance, function_coverage)| { - let is_used = function_coverage.is_used(); - prepare_covfun_record(tcx, &mut global_file_table, instance, is_used) - }) + .sorted_by_cached_key(|&instance| tcx.symbol_name(instance).name) + .filter_map(|instance| prepare_covfun_record(tcx, &mut global_file_table, instance, true)) .collect::>(); + // In a single designated CGU, also prepare covfun records for functions + // in this crate that were instrumented for coverage, but are unused. + if cx.codegen_unit.is_code_coverage_dead_code_cgu() { + let mut unused_instances = gather_unused_function_instances(cx); + // Sort the unused instances by symbol name, for the same reason as the used ones. + unused_instances.sort_by_cached_key(|&instance| tcx.symbol_name(instance).name); + covfun_records.extend(unused_instances.into_iter().filter_map(|instance| { + prepare_covfun_record(tcx, &mut global_file_table, instance, false) + })); + } + // If there are no covfun records for this CGU, don't generate a covmap record. // Emitting a covmap record without any covfun records causes `llvm-cov` to // fail when generating coverage reports, and if there are no covfun records @@ -261,7 +257,7 @@ fn generate_covmap_record<'ll>(cx: &CodegenCx<'ll, '_>, version: u32, filenames_ /// coverage map (in a single designated CGU) so that we still emit coverage mappings for them. /// We also end up adding their symbol names to a special global array that LLVM will include in /// its embedded coverage data. -fn add_unused_functions(cx: &CodegenCx<'_, '_>) { +fn gather_unused_function_instances<'tcx>(cx: &CodegenCx<'_, 'tcx>) -> Vec> { assert!(cx.codegen_unit.is_code_coverage_dead_code_cgu()); let tcx = cx.tcx; @@ -279,20 +275,17 @@ fn add_unused_functions(cx: &CodegenCx<'_, '_>) { && !usage.used_via_inlining.contains(&d) }; - // Scan for unused functions that were instrumented for coverage. - for def_id in tcx.mir_keys(()).iter().copied().filter(|&def_id| is_unused_fn(def_id)) { - // 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. - // 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. - - debug!("generating unused fn: {def_id:?}"); - add_unused_function_coverage(cx, def_id, function_coverage_info); - } + tcx.mir_keys(()) + .iter() + .copied() + .filter(|&def_id| is_unused_fn(def_id)) + .map(|def_id| make_dummy_instance(tcx, def_id)) + .collect::>() } struct UsageSets<'tcx> { @@ -357,16 +350,11 @@ fn prepare_usage_sets<'tcx>(tcx: TyCtxt<'tcx>) -> UsageSets<'tcx> { UsageSets { all_mono_items, used_via_inlining, missing_own_coverage } } -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(); +fn make_dummy_instance<'tcx>(tcx: TyCtxt<'tcx>, local_def_id: LocalDefId) -> ty::Instance<'tcx> { + let def_id = local_def_id.to_def_id(); // Make a dummy instance that fills in all generics with placeholders. - let instance = ty::Instance::new( + ty::Instance::new( def_id, ty::GenericArgs::for_item(tcx, def_id, |param, _| { if let ty::GenericParamDefKind::Lifetime = param.kind { @@ -375,9 +363,5 @@ fn add_unused_function_coverage<'tcx>( tcx.mk_param_from_def(param) } }), - ); - - // An unused function's mappings will all be rewritten to map to zero. - let function_coverage = FunctionCoverage::new_unused(function_coverage_info); - cx.coverage_cx().function_coverage_map.borrow_mut().insert(instance, function_coverage); + ) } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 82b6677e2038f..7311cd9d230c6 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -5,7 +5,7 @@ use rustc_abi::Size; use rustc_codegen_ssa::traits::{ BuilderMethods, ConstCodegenMethods, CoverageInfoBuilderMethods, MiscCodegenMethods, }; -use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; +use rustc_data_structures::fx::{FxHashMap, FxIndexSet}; use rustc_middle::mir::coverage::CoverageKind; use rustc_middle::ty::Instance; use rustc_middle::ty::layout::HasTyCtxt; @@ -13,18 +13,16 @@ use tracing::{debug, instrument}; use crate::builder::Builder; use crate::common::CodegenCx; -use crate::coverageinfo::map_data::FunctionCoverage; use crate::llvm; pub(crate) mod ffi; mod llvm_cov; -pub(crate) mod map_data; mod mapgen; /// Extra per-CGU context/state needed for coverage instrumentation. pub(crate) struct CguCoverageContext<'ll, 'tcx> { /// Coverage data for each instrumented function identified by DefId. - pub(crate) function_coverage_map: RefCell, FunctionCoverage<'tcx>>>, + pub(crate) instances_used: RefCell>>, pub(crate) pgo_func_name_var_map: RefCell, &'ll llvm::Value>>, pub(crate) mcdc_condition_bitmap_map: RefCell, Vec<&'ll llvm::Value>>>, @@ -34,17 +32,13 @@ pub(crate) struct CguCoverageContext<'ll, 'tcx> { impl<'ll, 'tcx> CguCoverageContext<'ll, 'tcx> { pub(crate) fn new() -> Self { Self { - function_coverage_map: Default::default(), + instances_used: RefCell::>::default(), pgo_func_name_var_map: Default::default(), mcdc_condition_bitmap_map: Default::default(), covfun_section_name: Default::default(), } } - fn take_function_coverage_map(&self) -> FxIndexMap, FunctionCoverage<'tcx>> { - self.function_coverage_map.replace(FxIndexMap::default()) - } - /// LLVM use a temp value to record evaluated mcdc test vector of each decision, which is /// called condition bitmap. In order to handle nested decisions, several condition bitmaps can /// be allocated for a function body. These values are named `mcdc.addr.{i}` and are a 32-bit @@ -157,12 +151,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { // Mark the instance as used in this CGU, for coverage purposes. // This includes functions that were not partitioned into this CGU, // but were MIR-inlined into one of this CGU's functions. - coverage_cx.function_coverage_map.borrow_mut().entry(instance).or_insert_with(|| { - FunctionCoverage::new_used( - function_coverage_info, - bx.tcx.coverage_ids_info(instance.def), - ) - }); + coverage_cx.instances_used.borrow_mut().insert(instance); match *kind { CoverageKind::SpanMarker | CoverageKind::BlockMarker { .. } => unreachable!(