From 371883a05acf04be9fb8d3c0766990ba56cd22e3 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 6 Oct 2023 12:51:48 +1100 Subject: coverage: Split `FunctionCoverage` into distinct collector/finished phases This gives us a clearly-defined place to run code after the instance's MIR has been traversed by codegen, but before we emit its `__llvm_covfun` record. --- .../src/coverageinfo/map_data.rs | 48 ++++++++++++++-------- 1 file changed, 30 insertions(+), 18 deletions(-) (limited to '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 index 84319b4ba2d..302e00b06ed 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -10,7 +10,7 @@ use rustc_middle::ty::Instance; /// Holds all of the coverage mapping data associated with a function instance, /// collected during traversal of `Coverage` statements in the function's MIR. #[derive(Debug)] -pub struct FunctionCoverage<'tcx> { +pub struct FunctionCoverageCollector<'tcx> { /// Coverage info that was attached to this function by the instrumentor. function_coverage_info: &'tcx FunctionCoverageInfo, is_used: bool, @@ -26,7 +26,7 @@ pub struct FunctionCoverage<'tcx> { expressions_seen: BitSet, } -impl<'tcx> FunctionCoverage<'tcx> { +impl<'tcx> FunctionCoverageCollector<'tcx> { /// Creates a new set of coverage data for a used (called) function. pub fn new( instance: Instance<'tcx>, @@ -76,11 +76,6 @@ impl<'tcx> FunctionCoverage<'tcx> { } } - /// Returns true for a used (called) function, and false for an unused function. - pub fn is_used(&self) -> bool { - self.is_used - } - /// Marks a counter ID as having been seen in a counter-increment statement. #[instrument(level = "debug", skip(self))] pub(crate) fn mark_counter_id_seen(&mut self, id: CounterId) { @@ -165,6 +160,28 @@ impl<'tcx> FunctionCoverage<'tcx> { ZeroExpressions(zero_expressions) } + pub(crate) fn into_finished(self) -> FunctionCoverage<'tcx> { + let zero_expressions = self.identify_zero_expressions(); + let FunctionCoverageCollector { function_coverage_info, is_used, counters_seen, .. } = self; + + FunctionCoverage { function_coverage_info, is_used, counters_seen, zero_expressions } + } +} + +pub(crate) struct FunctionCoverage<'tcx> { + function_coverage_info: &'tcx FunctionCoverageInfo, + is_used: bool, + + counters_seen: BitSet, + zero_expressions: ZeroExpressions, +} + +impl<'tcx> FunctionCoverage<'tcx> { + /// Returns true for a used (called) function, and false for an unused function. + pub(crate) fn is_used(&self) -> bool { + self.is_used + } + /// 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 fn source_hash(&self) -> u64 { @@ -177,29 +194,27 @@ impl<'tcx> FunctionCoverage<'tcx> { pub fn get_expressions_and_counter_regions( &self, ) -> (Vec, impl Iterator) { - let zero_expressions = self.identify_zero_expressions(); - - let counter_expressions = self.counter_expressions(&zero_expressions); + let counter_expressions = self.counter_expressions(); // Expression IDs are indices into `self.expressions`, and on the LLVM // side they will be treated as indices into `counter_expressions`, so // the two vectors should correspond 1:1. assert_eq!(self.function_coverage_info.expressions.len(), counter_expressions.len()); - let counter_regions = self.counter_regions(zero_expressions); + let counter_regions = self.counter_regions(); (counter_expressions, counter_regions) } /// Convert this function's coverage expression data into a form that can be /// passed through FFI to LLVM. - fn counter_expressions(&self, zero_expressions: &ZeroExpressions) -> Vec { + fn counter_expressions(&self) -> Vec { // 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`.) let counter_from_operand = |operand: CovTerm| match operand { - CovTerm::Expression(id) if zero_expressions.contains(id) => Counter::ZERO, + CovTerm::Expression(id) if self.zero_expressions.contains(id) => Counter::ZERO, _ => Counter::from_term(operand), }; @@ -219,10 +234,7 @@ impl<'tcx> FunctionCoverage<'tcx> { /// Converts this function's coverage mappings into an intermediate form /// that will be used by `mapgen` when preparing for FFI. - fn counter_regions( - &self, - zero_expressions: ZeroExpressions, - ) -> impl Iterator { + fn counter_regions(&self) -> impl Iterator { // Historically, mappings were stored directly in counter/expression // statements in MIR, and MIR optimizations would sometimes remove them. // That's mostly no longer true, so now we detect cases where that would @@ -230,7 +242,7 @@ impl<'tcx> FunctionCoverage<'tcx> { let counter_for_term = move |term: CovTerm| { let force_to_zero = match term { CovTerm::Counter(id) => !self.counters_seen.contains(id), - CovTerm::Expression(id) => zero_expressions.contains(id), + CovTerm::Expression(id) => self.zero_expressions.contains(id), CovTerm::Zero => false, }; if force_to_zero { Counter::ZERO } else { Counter::from_term(term) } -- cgit 1.4.1-3-g733a5 From 86b55cccffa6f8c49747335a43abb04376d1e06f Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 6 Oct 2023 22:46:04 +1100 Subject: coverage: Fetch expressions and mappings separately The combined `get_expressions_and_counter_regions` method was an artifact of having to prepare the expressions and mappings at the same time, to avoid ownership/lifetime problems with temporary data used by both. Now that we have an explicit transition from `FunctionCoverageCollector` to the final `FunctionCoverage`, we can prepare any shared data during that step and store it in the final struct. --- .../src/coverageinfo/map_data.rs | 36 +++++++--------------- .../rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 6 ++-- 2 files changed, 14 insertions(+), 28 deletions(-) (limited to '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 index 302e00b06ed..2914f310f03 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -1,5 +1,6 @@ use crate::coverageinfo::ffi::{Counter, CounterExpression, ExprKind}; +use rustc_data_structures::captures::Captures; use rustc_data_structures::fx::FxIndexSet; use rustc_index::bit_set::BitSet; use rustc_middle::mir::coverage::{ @@ -188,26 +189,11 @@ impl<'tcx> FunctionCoverage<'tcx> { if self.is_used { self.function_coverage_info.function_source_hash } else { 0 } } - /// Generate an array of CounterExpressions, and an iterator over all `Counter`s and their - /// associated `Regions` (from which the LLVM-specific `CoverageMapGenerator` will create - /// `CounterMappingRegion`s. - pub fn get_expressions_and_counter_regions( - &self, - ) -> (Vec, impl Iterator) { - let counter_expressions = self.counter_expressions(); - // Expression IDs are indices into `self.expressions`, and on the LLVM - // side they will be treated as indices into `counter_expressions`, so - // the two vectors should correspond 1:1. - assert_eq!(self.function_coverage_info.expressions.len(), counter_expressions.len()); - - let counter_regions = self.counter_regions(); - - (counter_expressions, counter_regions) - } - /// Convert this function's coverage expression data into a form that can be /// passed through FFI to LLVM. - fn counter_expressions(&self) -> Vec { + 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. @@ -218,23 +204,23 @@ impl<'tcx> FunctionCoverage<'tcx> { _ => Counter::from_term(operand), }; - self.function_coverage_info - .expressions - .iter() - .map(|&Expression { lhs, op, rhs }| CounterExpression { + self.function_coverage_info.expressions.iter().map(move |&Expression { lhs, op, rhs }| { + CounterExpression { lhs: counter_from_operand(lhs), kind: match op { Op::Add => ExprKind::Add, Op::Subtract => ExprKind::Subtract, }, rhs: counter_from_operand(rhs), - }) - .collect::>() + } + }) } /// Converts this function's coverage mappings into an intermediate form /// that will be used by `mapgen` when preparing for FFI. - fn counter_regions(&self) -> impl Iterator { + pub(crate) fn counter_regions( + &self, + ) -> impl Iterator + ExactSizeIterator { // Historically, mappings were stored directly in counter/expression // statements in MIR, and MIR optimizations would sometimes remove them. // That's mostly no longer true, so now we detect cases where that would diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 5b2dae2c0a4..965be3f05ee 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -185,13 +185,13 @@ fn encode_mappings_for_function( global_file_table: &mut GlobalFileTable, function_coverage: &FunctionCoverage<'_>, ) -> Vec { - let (expressions, counter_regions) = function_coverage.get_expressions_and_counter_regions(); - - let mut counter_regions = counter_regions.collect::>(); + let mut counter_regions = function_coverage.counter_regions().collect::>(); if counter_regions.is_empty() { return Vec::new(); } + let expressions = function_coverage.counter_expressions().collect::>(); + let mut virtual_file_mapping = IndexVec::::new(); let mut mapping_regions = Vec::with_capacity(counter_regions.len()); -- cgit 1.4.1-3-g733a5 From e985ae5a459e7bc4ac68a926f2560621aea30a6c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 3 Oct 2023 21:40:50 +1100 Subject: coverage: Build the global file table ahead of time --- Cargo.lock | 1 + compiler/rustc_codegen_llvm/Cargo.toml | 1 + .../src/coverageinfo/map_data.rs | 6 ++ .../rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 68 ++++++++++++++-------- 4 files changed, 51 insertions(+), 25 deletions(-) (limited to 'compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs') diff --git a/Cargo.lock b/Cargo.lock index 7aa243ad8b5..7a80f172145 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3596,6 +3596,7 @@ version = "0.0.0" dependencies = [ "bitflags 1.3.2", "cstr", + "itertools", "libc", "measureme", "object", diff --git a/compiler/rustc_codegen_llvm/Cargo.toml b/compiler/rustc_codegen_llvm/Cargo.toml index be09820d08d..e864337e5dc 100644 --- a/compiler/rustc_codegen_llvm/Cargo.toml +++ b/compiler/rustc_codegen_llvm/Cargo.toml @@ -9,6 +9,7 @@ test = false [dependencies] bitflags = "1.0" cstr = "0.2" +itertools = "0.10.5" libc = "0.2" measureme = "10.0.0" object = { version = "0.32.0", default-features = false, features = [ diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 2914f310f03..93a8a4b1d5e 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -7,6 +7,7 @@ use rustc_middle::mir::coverage::{ CodeRegion, CounterId, CovTerm, Expression, ExpressionId, FunctionCoverageInfo, Mapping, Op, }; use rustc_middle::ty::Instance; +use rustc_span::Symbol; /// Holds all of the coverage mapping data associated with a function instance, /// collected during traversal of `Coverage` statements in the function's MIR. @@ -189,6 +190,11 @@ impl<'tcx> FunctionCoverage<'tcx> { if self.is_used { self.function_coverage_info.function_source_hash } else { 0 } } + /// Returns an iterator over all filenames used by this function's mappings. + pub(crate) fn all_file_names(&self) -> impl Iterator + Captures<'_> { + self.function_coverage_info.mappings.iter().map(|mapping| mapping.code_region.file_name) + } + /// Convert this function's coverage expression data into a form that can be /// passed through FFI to LLVM. pub(crate) fn counter_expressions( diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 965be3f05ee..48cef459476 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -4,6 +4,7 @@ use crate::coverageinfo::ffi::CounterMappingRegion; use crate::coverageinfo::map_data::{FunctionCoverage, FunctionCoverageCollector}; use crate::llvm; +use itertools::Itertools as _; use rustc_codegen_ssa::traits::{BaseTypeMethods, ConstMethods}; use rustc_data_structures::fx::FxIndexSet; use rustc_hir::def::DefKind; @@ -57,12 +58,18 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { return; } - let mut global_file_table = GlobalFileTable::new(tcx); + let function_coverage_entries = function_coverage_map + .into_iter() + .map(|(instance, function_coverage)| (instance, function_coverage.into_finished())) + .collect::>(); + + let all_file_names = + function_coverage_entries.iter().flat_map(|(_, fn_cov)| fn_cov.all_file_names()); + let global_file_table = GlobalFileTable::new(all_file_names); // Encode coverage mappings and generate function records let mut function_data = Vec::new(); - for (instance, function_coverage) in function_coverage_map { - let function_coverage = function_coverage.into_finished(); + for (instance, function_coverage) in function_coverage_entries { debug!("Generate function coverage for {}, {:?}", cx.codegen_unit.name(), instance); let mangled_function_name = tcx.symbol_name(instance).name; @@ -70,7 +77,7 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { let is_used = function_coverage.is_used(); let coverage_mapping_buffer = - encode_mappings_for_function(&mut global_file_table, &function_coverage); + encode_mappings_for_function(&global_file_table, &function_coverage); if coverage_mapping_buffer.is_empty() { if function_coverage.is_used() { @@ -88,7 +95,7 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { } // Encode all filenames referenced by counters/expressions in this module - let filenames_buffer = global_file_table.into_filenames_buffer(); + let filenames_buffer = global_file_table.make_filenames_buffer(tcx); let filenames_size = filenames_buffer.len(); let filenames_val = cx.const_bytes(&filenames_buffer); @@ -139,37 +146,48 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { coverageinfo::save_cov_data_to_mod(cx, cov_data_val); } +/// Maps "global" (per-CGU) file ID numbers to their underlying filenames. struct GlobalFileTable { - global_file_table: FxIndexSet, + /// This "raw" table doesn't include the working dir, so a filename's + /// global ID is its index in this set **plus one**. + raw_file_table: FxIndexSet, } impl GlobalFileTable { - fn new(tcx: TyCtxt<'_>) -> Self { - let mut global_file_table = FxIndexSet::default(); + 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 global_file_id_for_file_name(&self, file_name: Symbol) -> u32 { + 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}"); + }); + // 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. + (raw_id + 1) as u32 + } + + fn make_filenames_buffer(&self, tcx: TyCtxt<'_>) -> Vec { // LLVM Coverage Mapping Format version 6 (zero-based encoded as 5) // requires setting the first filename to the compilation directory. // Since rustc generates coverage maps with relative paths, the // compilation directory can be combined with the relative paths // to get absolute paths, if needed. use rustc_session::RemapFileNameExt; - let working_dir = - Symbol::intern(&tcx.sess.opts.working_dir.for_codegen(&tcx.sess).to_string_lossy()); - global_file_table.insert(working_dir); - Self { global_file_table } - } - - fn global_file_id_for_file_name(&mut self, file_name: Symbol) -> u32 { - let (global_file_id, _) = self.global_file_table.insert_full(file_name); - global_file_id as u32 - } - - fn into_filenames_buffer(self) -> Vec { - // This method takes `self` so that the caller can't accidentally - // modify the original file table after encoding it into a buffer. + let working_dir: &str = &tcx.sess.opts.working_dir.for_codegen(&tcx.sess).to_string_lossy(); llvm::build_byte_buffer(|buffer| { coverageinfo::write_filenames_section_to_buffer( - self.global_file_table.iter().map(Symbol::as_str), + // Insert the working dir at index 0, before the other filenames. + std::iter::once(working_dir).chain(self.raw_file_table.iter().map(Symbol::as_str)), buffer, ); }) @@ -182,7 +200,7 @@ impl GlobalFileTable { /// /// Newly-encountered filenames will be added to the global file table. fn encode_mappings_for_function( - global_file_table: &mut GlobalFileTable, + global_file_table: &GlobalFileTable, function_coverage: &FunctionCoverage<'_>, ) -> Vec { let mut counter_regions = function_coverage.counter_regions().collect::>(); @@ -203,7 +221,7 @@ fn encode_mappings_for_function( for counter_regions_for_file in counter_regions.group_by(|(_, a), (_, b)| a.file_name == b.file_name) { - // Look up (or allocate) the global file ID for this filename. + // Look up the global file ID for this filename. let file_name = counter_regions_for_file[0].1.file_name; let global_file_id = global_file_table.global_file_id_for_file_name(file_name); -- cgit 1.4.1-3-g733a5