From 525ac15b66473e63782bff4194e06f665003bd4e Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 1 Sep 2023 23:27:45 +1000 Subject: [PATCH 1/6] coverage: Convert `CoverageMapGenerator` to `GlobalFileTable` This struct was only being used to hold the global file table, and one of its methods didn't even use the table. Changing its methods to ordinary functions makes it easier to see where the table is mutated. --- .../src/coverageinfo/mapgen.rs | 172 ++++++++++-------- 1 file changed, 96 insertions(+), 76 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 97a99e5105675..5e897241cbbb4 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -55,7 +55,7 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { return; } - let mut mapgen = CoverageMapGenerator::new(tcx); + let mut global_file_table = GlobalFileTable::new(tcx); // Encode coverage mappings and generate function records let mut function_data = Vec::new(); @@ -68,7 +68,12 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { function_coverage.get_expressions_and_counter_regions(); let coverage_mapping_buffer = llvm::build_byte_buffer(|coverage_mapping_buffer| { - mapgen.write_coverage_mapping(expressions, counter_regions, coverage_mapping_buffer); + write_coverage_mapping( + &mut global_file_table, + expressions, + counter_regions, + coverage_mapping_buffer, + ); }); if coverage_mapping_buffer.is_empty() { @@ -87,19 +92,14 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { } // Encode all filenames referenced by counters/expressions in this module - let filenames_buffer = llvm::build_byte_buffer(|filenames_buffer| { - coverageinfo::write_filenames_section_to_buffer( - mapgen.filenames.iter().map(Symbol::as_str), - filenames_buffer, - ); - }); + let filenames_buffer = global_file_table.into_filenames_buffer(); let filenames_size = filenames_buffer.len(); let filenames_val = cx.const_bytes(&filenames_buffer); let filenames_ref = coverageinfo::hash_bytes(&filenames_buffer); // Generate the LLVM IR representation of the coverage map and store it in a well-known global - let cov_data_val = mapgen.generate_coverage_map(cx, version, filenames_size, filenames_val); + let cov_data_val = generate_coverage_map(cx, version, filenames_size, filenames_val); let covfun_section_name = coverageinfo::covfun_section_name(cx); for (mangled_function_name, source_hash, is_used, coverage_mapping_buffer) in function_data { @@ -118,13 +118,13 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { coverageinfo::save_cov_data_to_mod(cx, cov_data_val); } -struct CoverageMapGenerator { - filenames: FxIndexSet, +struct GlobalFileTable { + global_file_table: FxIndexSet, } -impl CoverageMapGenerator { +impl GlobalFileTable { fn new(tcx: TyCtxt<'_>) -> Self { - let mut filenames = FxIndexSet::default(); + let mut global_file_table = FxIndexSet::default(); // 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 @@ -133,48 +133,67 @@ impl CoverageMapGenerator { let working_dir = Symbol::intern( &tcx.sess.opts.working_dir.remapped_path_if_available().to_string_lossy(), ); - filenames.insert(working_dir); - Self { filenames } + global_file_table.insert(working_dir); + Self { global_file_table } } - /// Using the `expressions` and `counter_regions` collected for the current function, generate - /// the `mapping_regions` and `virtual_file_mapping`, and capture any new filenames. Then use - /// LLVM APIs to encode the `virtual_file_mapping`, `expressions`, and `mapping_regions` into - /// the given `coverage_mapping` byte buffer, compliant with the LLVM Coverage Mapping format. - fn write_coverage_mapping<'a>( - &mut self, - expressions: Vec, - counter_regions: impl Iterator, - coverage_mapping_buffer: &RustString, - ) { - let mut counter_regions = counter_regions.collect::>(); - if counter_regions.is_empty() { - return; - } + 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. + + llvm::build_byte_buffer(|buffer| { + coverageinfo::write_filenames_section_to_buffer( + self.global_file_table.iter().map(Symbol::as_str), + buffer, + ); + }) + } +} + +/// Using the `expressions` and `counter_regions` collected for the current function, generate +/// the `mapping_regions` and `virtual_file_mapping`, and capture any new filenames. Then use +/// LLVM APIs to encode the `virtual_file_mapping`, `expressions`, and `mapping_regions` into +/// the given `coverage_mapping` byte buffer, compliant with the LLVM Coverage Mapping format. +fn write_coverage_mapping<'a>( + global_file_table: &mut GlobalFileTable, + expressions: Vec, + counter_regions: impl Iterator, + coverage_mapping_buffer: &RustString, +) { + let mut counter_regions = counter_regions.collect::>(); + if counter_regions.is_empty() { + return; + } - let mut virtual_file_mapping = Vec::new(); - let mut mapping_regions = Vec::new(); - let mut current_file_name = None; - let mut current_file_id = 0; - - // Convert the list of (Counter, CodeRegion) pairs to an array of `CounterMappingRegion`, sorted - // by filename and position. Capture any new files to compute the `CounterMappingRegion`s - // `file_id` (indexing files referenced by the current function), and construct the - // function-specific `virtual_file_mapping` from `file_id` to its index in the module's - // `filenames` array. - counter_regions.sort_unstable_by_key(|(_counter, region)| *region); - for (counter, region) in counter_regions { - let CodeRegion { file_name, start_line, start_col, end_line, end_col } = *region; - let same_file = current_file_name.is_some_and(|p| p == file_name); - if !same_file { - if current_file_name.is_some() { - current_file_id += 1; - } - current_file_name = Some(file_name); - debug!(" file_id: {} = '{:?}'", current_file_id, file_name); - let (filenames_index, _) = self.filenames.insert_full(file_name); - virtual_file_mapping.push(filenames_index as u32); + let mut virtual_file_mapping = Vec::new(); + let mut mapping_regions = Vec::new(); + let mut current_file_name = None; + let mut current_file_id = 0; + + // Convert the list of (Counter, CodeRegion) pairs to an array of `CounterMappingRegion`, sorted + // by filename and position. Capture any new files to compute the `CounterMappingRegion`s + // `file_id` (indexing files referenced by the current function), and construct the + // function-specific `virtual_file_mapping` from `file_id` to its index in the module's + // `filenames` array. + counter_regions.sort_unstable_by_key(|(_counter, region)| *region); + for (counter, region) in counter_regions { + let CodeRegion { file_name, start_line, start_col, end_line, end_col } = *region; + let same_file = current_file_name.is_some_and(|p| p == file_name); + if !same_file { + if current_file_name.is_some() { + current_file_id += 1; } + current_file_name = Some(file_name); + debug!(" file_id: {} = '{:?}'", current_file_id, file_name); + let global_file_id = global_file_table.global_file_id_for_file_name(file_name); + virtual_file_mapping.push(global_file_id); + } + { debug!("Adding counter {:?} to map for {:?}", counter, region); mapping_regions.push(CounterMappingRegion::code_region( counter, @@ -185,7 +204,9 @@ impl CoverageMapGenerator { end_col, )); } + } + { // Encode and append the current function's coverage mapping data coverageinfo::write_mapping_to_buffer( virtual_file_mapping, @@ -194,33 +215,32 @@ impl CoverageMapGenerator { coverage_mapping_buffer, ); } +} - /// Construct coverage map header and the array of function records, and combine them into the - /// coverage map. Save the coverage map data into the LLVM IR as a static global using a - /// specific, well-known section and name. - fn generate_coverage_map<'ll>( - self, - cx: &CodegenCx<'ll, '_>, - version: u32, - filenames_size: usize, - filenames_val: &'ll llvm::Value, - ) -> &'ll llvm::Value { - debug!("cov map: filenames_size = {}, 0-based version = {}", filenames_size, version); - - // Create the coverage data header (Note, fields 0 and 2 are now always zero, - // as of `llvm::coverage::CovMapVersion::Version4`.) - let zero_was_n_records_val = cx.const_u32(0); - let filenames_size_val = cx.const_u32(filenames_size as u32); - let zero_was_coverage_size_val = cx.const_u32(0); - let version_val = cx.const_u32(version); - let cov_data_header_val = cx.const_struct( - &[zero_was_n_records_val, filenames_size_val, zero_was_coverage_size_val, version_val], - /*packed=*/ false, - ); +/// Construct coverage map header and the array of function records, and combine them into the +/// coverage map. Save the coverage map data into the LLVM IR as a static global using a +/// specific, well-known section and name. +fn generate_coverage_map<'ll>( + cx: &CodegenCx<'ll, '_>, + version: u32, + filenames_size: usize, + filenames_val: &'ll llvm::Value, +) -> &'ll llvm::Value { + debug!("cov map: filenames_size = {}, 0-based version = {}", filenames_size, version); + + // Create the coverage data header (Note, fields 0 and 2 are now always zero, + // as of `llvm::coverage::CovMapVersion::Version4`.) + let zero_was_n_records_val = cx.const_u32(0); + let filenames_size_val = cx.const_u32(filenames_size as u32); + let zero_was_coverage_size_val = cx.const_u32(0); + let version_val = cx.const_u32(version); + let cov_data_header_val = cx.const_struct( + &[zero_was_n_records_val, filenames_size_val, zero_was_coverage_size_val, version_val], + /*packed=*/ false, + ); - // Create the complete LLVM coverage data value to add to the LLVM IR - cx.const_struct(&[cov_data_header_val, filenames_val], /*packed=*/ false) - } + // Create the complete LLVM coverage data value to add to the LLVM IR + cx.const_struct(&[cov_data_header_val, filenames_val], /*packed=*/ false) } /// Construct a function record and combine it with the function's coverage mapping data. From 4f88aa0fbd71b67be4a4910d312015ddc2a654ff Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 30 Aug 2023 20:08:13 +1000 Subject: [PATCH 2/6] coverage: Use a stable sort when grouping mapped regions by file If two or more mappings cover exactly the same region, their relative order will now be preserved from `get_expressions_and_counter_regions`, rather than being disturbed by implementation details of an unstable sort. The current order is: counter mappings, expression mappings, zero mappings. (LLVM will also perform its own stable sort on these mappings, but that sort only compares file ID, start location, and `RegionKind`.) --- compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 5e897241cbbb4..1ad4b249947e6 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -180,7 +180,7 @@ fn write_coverage_mapping<'a>( // `file_id` (indexing files referenced by the current function), and construct the // function-specific `virtual_file_mapping` from `file_id` to its index in the module's // `filenames` array. - counter_regions.sort_unstable_by_key(|(_counter, region)| *region); + counter_regions.sort_by_key(|(_counter, region)| *region); for (counter, region) in counter_regions { let CodeRegion { file_name, start_line, start_col, end_line, end_col } = *region; let same_file = current_file_name.is_some_and(|p| p == file_name); From fbbb543ced054daf718e6442247778713bde7008 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 3 Sep 2023 16:22:06 +1000 Subject: [PATCH 3/6] coverage: Reserve capacity for all of a function's mapping regions We already know in advance how many entries will be pushed onto this vector. --- compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 1ad4b249947e6..b9025eff82c86 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -171,7 +171,7 @@ fn write_coverage_mapping<'a>( } let mut virtual_file_mapping = Vec::new(); - let mut mapping_regions = Vec::new(); + let mut mapping_regions = Vec::with_capacity(counter_regions.len()); let mut current_file_name = None; let mut current_file_id = 0; From 99da8a83c295b26b2211c90c0d6e4566b39b2c10 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 3 Sep 2023 16:07:50 +1000 Subject: [PATCH 4/6] coverage: Push down creation of the mappings payload buffer Instead of writing coverage mappings into a supplied `&RustString`, this function can just create the buffer itself and return the resulting vector of bytes. --- .../src/coverageinfo/mapgen.rs | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index b9025eff82c86..cbf245d054b95 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -7,7 +7,6 @@ use rustc_codegen_ssa::traits::ConstMethods; use rustc_data_structures::fx::FxIndexSet; use rustc_hir::def::DefKind; use rustc_hir::def_id::DefId; -use rustc_llvm::RustString; use rustc_middle::bug; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::mir::coverage::CodeRegion; @@ -67,14 +66,8 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { let (expressions, counter_regions) = function_coverage.get_expressions_and_counter_regions(); - let coverage_mapping_buffer = llvm::build_byte_buffer(|coverage_mapping_buffer| { - write_coverage_mapping( - &mut global_file_table, - expressions, - counter_regions, - coverage_mapping_buffer, - ); - }); + let coverage_mapping_buffer = + encode_mappings_for_function(&mut global_file_table, expressions, counter_regions); if coverage_mapping_buffer.is_empty() { if function_coverage.is_used() { @@ -155,19 +148,19 @@ impl GlobalFileTable { } } -/// Using the `expressions` and `counter_regions` collected for the current function, generate -/// the `mapping_regions` and `virtual_file_mapping`, and capture any new filenames. Then use -/// LLVM APIs to encode the `virtual_file_mapping`, `expressions`, and `mapping_regions` into -/// the given `coverage_mapping` byte buffer, compliant with the LLVM Coverage Mapping format. -fn write_coverage_mapping<'a>( +/// Using the expressions and counter regions collected for a single function, +/// generate the variable-sized payload of its corresponding `__llvm_covfun` +/// entry. The payload is returned as a vector of bytes. +/// +/// Newly-encountered filenames will be added to the global file table. +fn encode_mappings_for_function<'a>( global_file_table: &mut GlobalFileTable, expressions: Vec, counter_regions: impl Iterator, - coverage_mapping_buffer: &RustString, -) { +) -> Vec { let mut counter_regions = counter_regions.collect::>(); if counter_regions.is_empty() { - return; + return Vec::new(); } let mut virtual_file_mapping = Vec::new(); @@ -206,15 +199,15 @@ fn write_coverage_mapping<'a>( } } - { - // Encode and append the current function's coverage mapping data + // Encode the function's coverage mappings into a buffer. + llvm::build_byte_buffer(|buffer| { coverageinfo::write_mapping_to_buffer( virtual_file_mapping, expressions, mapping_regions, - coverage_mapping_buffer, + buffer, ); - } + }) } /// Construct coverage map header and the array of function records, and combine them into the From 1f56fa96573eec02f53cc9113eaa4557756517b6 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 4 Sep 2023 11:40:13 +1000 Subject: [PATCH 5/6] coverage: Push down the call to `get_expressions_and_counter_regions` These expressions and counter regions are only needed by the function that encodes a function's coverage mappings payload. --- .../rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index cbf245d054b95..a8380c6dcb967 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -1,6 +1,7 @@ use crate::common::CodegenCx; use crate::coverageinfo; -use crate::coverageinfo::ffi::{Counter, CounterExpression, CounterMappingRegion}; +use crate::coverageinfo::ffi::CounterMappingRegion; +use crate::coverageinfo::map_data::FunctionCoverage; use crate::llvm; use rustc_codegen_ssa::traits::ConstMethods; @@ -63,11 +64,9 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { let mangled_function_name = tcx.symbol_name(instance).name; let source_hash = function_coverage.source_hash(); let is_used = function_coverage.is_used(); - let (expressions, counter_regions) = - function_coverage.get_expressions_and_counter_regions(); let coverage_mapping_buffer = - encode_mappings_for_function(&mut global_file_table, expressions, counter_regions); + encode_mappings_for_function(&mut global_file_table, &function_coverage); if coverage_mapping_buffer.is_empty() { if function_coverage.is_used() { @@ -153,11 +152,12 @@ impl GlobalFileTable { /// entry. The payload is returned as a vector of bytes. /// /// Newly-encountered filenames will be added to the global file table. -fn encode_mappings_for_function<'a>( +fn encode_mappings_for_function( global_file_table: &mut GlobalFileTable, - expressions: Vec, - counter_regions: impl Iterator, + function_coverage: &FunctionCoverage<'_>, ) -> Vec { + let (expressions, counter_regions) = function_coverage.get_expressions_and_counter_regions(); + let mut counter_regions = counter_regions.collect::>(); if counter_regions.is_empty() { return Vec::new(); From 6e968b1e454d87870e8afff1a60ed706bad7377d Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 3 Sep 2023 15:52:49 +1000 Subject: [PATCH 6/6] coverage: Simplify grouping of mappings by file This removes an ad-hoc implementation of `group_by`. --- .../src/coverageinfo/mapgen.rs | 50 +++++++++---------- compiler/rustc_codegen_llvm/src/lib.rs | 1 + 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index a8380c6dcb967..bda287d782c85 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -8,6 +8,7 @@ use rustc_codegen_ssa::traits::ConstMethods; use rustc_data_structures::fx::FxIndexSet; use rustc_hir::def::DefKind; use rustc_hir::def_id::DefId; +use rustc_index::IndexVec; use rustc_middle::bug; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::mir::coverage::CodeRegion; @@ -163,34 +164,33 @@ fn encode_mappings_for_function( return Vec::new(); } - let mut virtual_file_mapping = Vec::new(); + let mut virtual_file_mapping = IndexVec::::new(); let mut mapping_regions = Vec::with_capacity(counter_regions.len()); - let mut current_file_name = None; - let mut current_file_id = 0; - - // Convert the list of (Counter, CodeRegion) pairs to an array of `CounterMappingRegion`, sorted - // by filename and position. Capture any new files to compute the `CounterMappingRegion`s - // `file_id` (indexing files referenced by the current function), and construct the - // function-specific `virtual_file_mapping` from `file_id` to its index in the module's - // `filenames` array. + + // Sort the list of (counter, region) mapping pairs by region, so that they + // can be grouped by filename. Prepare file IDs for each filename, and + // prepare the mapping data so that we can pass it through FFI to LLVM. counter_regions.sort_by_key(|(_counter, region)| *region); - for (counter, region) in counter_regions { - let CodeRegion { file_name, start_line, start_col, end_line, end_col } = *region; - let same_file = current_file_name.is_some_and(|p| p == file_name); - if !same_file { - if current_file_name.is_some() { - current_file_id += 1; - } - current_file_name = Some(file_name); - debug!(" file_id: {} = '{:?}'", current_file_id, file_name); - let global_file_id = global_file_table.global_file_id_for_file_name(file_name); - virtual_file_mapping.push(global_file_id); - } - { - debug!("Adding counter {:?} to map for {:?}", counter, region); + 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. + 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); + + // Associate that global file ID with a local file ID for this function. + let local_file_id: u32 = virtual_file_mapping.push(global_file_id); + debug!(" file id: local {local_file_id} => global {global_file_id} = '{file_name:?}'"); + + // For each counter/region pair in this function+file, convert it to a + // form suitable for FFI. + for &(counter, region) in counter_regions_for_file { + let CodeRegion { file_name: _, start_line, start_col, end_line, end_col } = *region; + + debug!("Adding counter {counter:?} to map for {region:?}"); mapping_regions.push(CounterMappingRegion::code_region( counter, - current_file_id, + local_file_id, start_line, start_col, end_line, @@ -202,7 +202,7 @@ fn encode_mappings_for_function( // Encode the function's coverage mappings into a buffer. llvm::build_byte_buffer(|buffer| { coverageinfo::write_mapping_to_buffer( - virtual_file_mapping, + virtual_file_mapping.raw, expressions, mapping_regions, buffer, diff --git a/compiler/rustc_codegen_llvm/src/lib.rs b/compiler/rustc_codegen_llvm/src/lib.rs index d283299ac46b4..ac199624e347b 100644 --- a/compiler/rustc_codegen_llvm/src/lib.rs +++ b/compiler/rustc_codegen_llvm/src/lib.rs @@ -10,6 +10,7 @@ #![feature(iter_intersperse)] #![feature(let_chains)] #![feature(never_type)] +#![feature(slice_group_by)] #![feature(impl_trait_in_assoc_type)] #![recursion_limit = "256"] #![allow(rustc::potential_query_instability)]