From 1c4440205607cd18ccfb30abe065bf0f62b5dd18 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 31 Aug 2026 14:46:11 +1000 Subject: [PATCH 1/2] Preliminary cleanup in coverage codegen - Rename `generate_*` functions to `emit_*`, since they emit LLVM globals - Consistently use qualified paths in `counter_for_term` - Remove an unnecessary Clone from `llvm_cov::Regions` --- .../src/coverageinfo/llvm_cov.rs | 2 +- .../src/coverageinfo/mapgen.rs | 14 ++++++------- .../src/coverageinfo/mapgen/covfun.rs | 20 +++++++++---------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/llvm_cov.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/llvm_cov.rs index a58202834cfa5..93d0b09578315 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/llvm_cov.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/llvm_cov.rs @@ -64,7 +64,7 @@ pub(crate) fn write_filenames_to_buffer(filenames: &[impl AsRef]) -> Vec, pub(crate) expansion_regions: Vec, diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 18818fd1a56c0..e9543ebeecf99 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -74,7 +74,7 @@ pub(crate) fn finalize(cx: &mut CodegenCx<'_, '_>) { unused::prepare_covfun_records_for_unused_functions(cx, &mut covfun_records); } - // If there are no covfun records for this CGU, don't generate a covmap record. + // If there are no covfun records for this CGU, don't emit 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 // then the covmap record isn't useful anyway. @@ -89,13 +89,13 @@ pub(crate) fn finalize(cx: &mut CodegenCx<'_, '_>) { GlobalFileTable::build(tcx, covfun_records.iter().flat_map(|c| c.all_source_files())); for covfun in &covfun_records { - covfun::generate_covfun_record(cx, &global_file_table, covfun) + covfun::emit_covfun_record(cx, &global_file_table, covfun); } - // Generate the coverage map header, which contains the filenames used by + // Emit the coverage map header, which contains the filenames used by // this CGU's coverage mappings, and store it in a well-known global. // (This is skipped if we returned early due to having no covfun records.) - generate_covmap_record(cx, covmap_version, &global_file_table.filenames_buffer); + emit_covmap_record(cx, covmap_version, &global_file_table.filenames_buffer); } /// Maps "global" (per-CGU) file ID numbers to their underlying source file paths. @@ -218,10 +218,10 @@ impl VirtualFileMapping { } } -/// Generates the contents of the covmap record for this CGU, which mostly -/// consists of a header and a list of filenames. The record is then stored +/// Generates and emits the covmap record for this CGU, which mostly +/// consists of a header and a list of filenames. The record is emitted /// as a global variable in the `__llvm_covmap` section. -fn generate_covmap_record<'ll>( +fn emit_covmap_record<'ll>( cx: &mut CodegenCx<'ll, '_>, version: CovmapVersion, filenames_buffer: &[u8], diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs index 28985448fd1c1..b83570911c3a0 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs @@ -77,15 +77,15 @@ pub(crate) fn prepare_covfun_record<'tcx>( Some(covfun) } -pub(crate) fn counter_for_term(term: CovTerm) -> ffi::Counter { - use ffi::Counter; +fn counter_for_term(term: CovTerm) -> ffi::Counter { match term { - CovTerm::Zero => Counter::ZERO, - CovTerm::Counter(id) => { - Counter { kind: ffi::CounterKind::CounterValueReference, id: CounterId::as_u32(id) } - } + CovTerm::Zero => ffi::Counter::ZERO, + CovTerm::Counter(id) => ffi::Counter { + kind: ffi::CounterKind::CounterValueReference, + id: CounterId::as_u32(id), + }, CovTerm::Expression(id) => { - Counter { kind: ffi::CounterKind::Expression, id: ExpressionId::as_u32(id) } + ffi::Counter { kind: ffi::CounterKind::Expression, id: ExpressionId::as_u32(id) } } } } @@ -174,10 +174,10 @@ fn fill_region_tables<'tcx>( } } -/// Generates the contents of the covfun record for this function, which -/// contains the function's coverage mapping data. The record is then stored +/// Generates and emits the covfun record for this function, which +/// contains the function's coverage mapping data. The record is emitted /// as a global variable in the `__llvm_covfun` section. -pub(crate) fn generate_covfun_record<'tcx>( +pub(crate) fn emit_covfun_record<'tcx>( cx: &mut CodegenCx<'_, 'tcx>, global_file_table: &GlobalFileTable, covfun: &CovfunRecord<'tcx>, From be8a828aa9b2532c8d726adcafdab72831db9dcf Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 29 Aug 2026 17:15:24 +1000 Subject: [PATCH 2/2] Resolve spans to file-coordinates in a separate step One of the key tasks in coverage codegen is to take the source-code spans that were recorded during MIR instrumentation, and resolve them to physical coordinates in their respective files. In rare cases this resolution can fail, which leads to the awkward possibility that a function might lose _all_ of its mappings for a particular file/expansion. If that happens, we need to avoid emitting a covfun file section containing no regions, because doing so would trigger errors in LLVM. The existing code does handle this edge case, but in a way that won't generalise to multiple files/expansions. Having an explicit intermediate resolution step will make it easier to add support for expansion regions in the future. --- .../src/coverageinfo/mapgen.rs | 30 --- .../src/coverageinfo/mapgen/covfun.rs | 175 ++++++++++++------ 2 files changed, 118 insertions(+), 87 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index e9543ebeecf99..3dac4b3e0167f 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -1,11 +1,9 @@ use std::assert_matches; -use std::sync::Arc; use itertools::Itertools; use rustc_abi::Align; use rustc_codegen_ssa::traits::{BaseTypeCodegenMethods, ConstCodegenMethods}; use rustc_data_structures::fx::FxIndexMap; -use rustc_index::IndexVec; use rustc_middle::ty::TyCtxt; use rustc_span::{FileName, RemapPathScopeComponents, SourceFile, StableSourceFileId}; use tracing::debug; @@ -190,34 +188,6 @@ rustc_index::newtype_index! { struct LocalFileId {} } -/// Holds a mapping from "local" (per-function) file IDs to their corresponding -/// source files. -#[derive(Debug, Default)] -struct VirtualFileMapping { - local_file_table: IndexVec>, -} - -impl VirtualFileMapping { - fn push_file(&mut self, source_file: &Arc) -> LocalFileId { - self.local_file_table.push(Arc::clone(source_file)) - } - - /// Resolves all of the filenames in this local file mapping to a list of - /// global file IDs in its CGU, for inclusion in this function's - /// `__llvm_covfun` record. - /// - /// The global file IDs are returned as `u32` to make FFI easier. - fn resolve_all(&self, global_file_table: &GlobalFileTable) -> Option> { - self.local_file_table - .iter() - .map(|file| try { - let id = global_file_table.get_existing_id(file)?; - GlobalFileId::as_u32(id) - }) - .collect::>>() - } -} - /// Generates and emits the covmap record for this CGU, which mostly /// consists of a header and a list of filenames. The record is emitted /// as a global variable in the `__llvm_covmap` section. diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs index b83570911c3a0..d01ed8765302e 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs @@ -5,13 +5,15 @@ //! [^win]: On Windows the section name is `.lcovfun`. use std::ffi::CString; +use std::iter; use std::sync::Arc; use rustc_abi::Align; use rustc_codegen_ssa::traits::{BaseTypeCodegenMethods as _, ConstCodegenMethods}; +use rustc_index::IndexVec; use rustc_middle::mir::coverage::{ - BasicCoverageBlock, CounterId, CovTerm, CoverageCodegenInfo, CoverageMirInfo, Expression, - ExpressionId, Mapping, MappingKind, Op, + BasicCoverageBlock, CounterId, CovTerm, CoverageCodegenInfo, Expression, ExpressionId, Mapping, + MappingKind, Op, }; use rustc_middle::ty::{Instance, TyCtxt}; use rustc_span::{SourceFile, Span}; @@ -19,7 +21,7 @@ use rustc_target::spec::HasTargetSpec; use tracing::debug; use crate::common::CodegenCx; -use crate::coverageinfo::mapgen::{GlobalFileTable, VirtualFileMapping, spans}; +use crate::coverageinfo::mapgen::{GlobalFileTable, LocalFileId, spans}; use crate::coverageinfo::{ffi, llvm_cov}; use crate::llvm; @@ -34,16 +36,15 @@ pub(crate) struct CovfunRecord<'tcx> { source_hash: u64, is_used: bool, - virtual_file_mapping: VirtualFileMapping, expressions: Vec, - regions: llvm_cov::Regions, + mappings: ResolvedMappings, } impl<'tcx> CovfunRecord<'tcx> { /// Iterator that yields all source files referred to by this function's /// coverage mappings. Used to build the global file table for the CGU. pub(crate) fn all_source_files(&self) -> impl Iterator { - self.virtual_file_mapping.local_file_table.iter().map(Arc::as_ref) + self.mappings.all_source_files() } } @@ -56,24 +57,17 @@ pub(crate) fn prepare_covfun_record<'tcx>( let cg_info = tcx.coverage_codegen_info(instance.def)?; let expressions = prepare_expressions(cg_info); + let mappings = prepare_resolved_mappings(tcx, cg_info, is_used, &mir_info.mappings)?; - let mut covfun = CovfunRecord { + let covfun = CovfunRecord { _instance: instance, mangled_function_name: tcx.symbol_name(instance).name, source_hash: if is_used { mir_info.function_source_hash } else { 0 }, is_used, - virtual_file_mapping: VirtualFileMapping::default(), expressions, - regions: llvm_cov::Regions::default(), + mappings, }; - fill_region_tables(tcx, mir_info, cg_info, &mut covfun); - - if covfun.regions.has_no_regions() { - debug!(?covfun, "function has no mappings to embed; skipping"); - return None; - } - Some(covfun) } @@ -110,16 +104,60 @@ fn prepare_expressions(cg_info: &CoverageCodegenInfo) -> Vec>() } -/// Populates the mapping region tables in the current function's covfun record. -fn fill_region_tables<'tcx>( +/// Intermediate representation of coverage mappings, after all mapping spans +/// have been resolved to file coordinates (or discarded), but before producing +/// a final [`llvm_cov::Regions`]. +/// +/// Having a separate resolution step makes it easier to handle edge cases +/// where a function (or someday an expansion) manages to lose all of its spans, +/// without accidentally emitting invalid covfun records containing empty files. +#[derive(Debug)] +struct ResolvedMappings { + /// Source file for all of the [`spans::Coords`] in these mappings. + source_file: Arc, + + code_mappings: Vec, + branch_mappings: Vec, +} + +impl ResolvedMappings { + fn ensure_nonempty(self) -> Option { + let ResolvedMappings { source_file: _, code_mappings, branch_mappings } = &self; + if code_mappings.is_empty() && branch_mappings.is_empty() { None } else { Some(self) } + } + + fn all_source_files(&self) -> impl Iterator { + // FIXME(Zalathar): When expansion regions are supported, this also needs to yield + // any source files used by descendant expansions. + let ResolvedMappings { source_file, code_mappings: _, branch_mappings: _ } = self; + iter::once(source_file.as_ref()) + } +} + +/// Resolved from [`MappingKind::Code`], and the precursor to [`ffi::CodeRegion`]. +#[derive(Debug)] +struct CodeMapping { + coords: spans::Coords, + counter: ffi::Counter, +} + +/// Resolved from [`MappingKind::Branch`], and the precursor to [`ffi::BranchRegion`]. +#[derive(Debug)] +struct BranchMapping { + coords: spans::Coords, + true_counter: ffi::Counter, + false_counter: ffi::Counter, +} + +fn prepare_resolved_mappings<'tcx>( tcx: TyCtxt<'tcx>, - mir_info: &'tcx CoverageMirInfo, cg_info: &'tcx CoverageCodegenInfo, - covfun: &mut CovfunRecord<'tcx>, -) { + is_used: bool, + mappings: &[Mapping], +) -> Option { // If this function is unused, replace all counters with zero. let counter_for_bcb = |bcb: BasicCoverageBlock| -> ffi::Counter { - let term = if covfun.is_used { + let term = if is_used { cg_info.term_for_bcb[bcb].expect("every BCB in a mapping was given a term") } else { CovTerm::Zero @@ -130,14 +168,9 @@ fn fill_region_tables<'tcx>( // Currently a function's mappings must all be in the same file, so use the // first mapping's span to determine the file. let source_map = tcx.sess.source_map(); - let Some(first_span) = (try { mir_info.mappings.first()?.span }) else { - debug_assert!(false, "function has no mappings: {covfun:?}"); - return; - }; + let first_span = mappings.first()?.span; let source_file = source_map.lookup_source_file(first_span.lo()); - let local_file_id = covfun.virtual_file_mapping.push_file(&source_file); - // In rare cases, _all_ of a function's spans are discarded, and coverage // codegen needs to handle that gracefully to avoid #133606. // It's hard for tests to trigger this organically, so instead we set @@ -147,30 +180,57 @@ fn fill_region_tables<'tcx>( if discard_all { None } else { spans::make_coords(source_map, &source_file, span) } }; + let mut code_mappings = vec![]; + let mut branch_mappings = vec![]; + + for &Mapping { ref kind, span } in mappings { + let Some(coords) = make_coords(span) else { continue }; + match *kind { + MappingKind::Code { bcb } => { + code_mappings.push(CodeMapping { coords, counter: counter_for_bcb(bcb) }) + } + MappingKind::Branch { true_bcb, false_bcb } => branch_mappings.push(BranchMapping { + coords, + true_counter: counter_for_bcb(true_bcb), + false_counter: counter_for_bcb(false_bcb), + }), + } + } + + ResolvedMappings { source_file, code_mappings, branch_mappings }.ensure_nonempty() +} + +/// Populates the mapping region tables for the current function's covfun record. +fn fill_region_tables( + global_file_table: &GlobalFileTable, + mappings: &ResolvedMappings, + virtual_file_mapping: &mut IndexVec, + regions: &mut llvm_cov::Regions, +) { + let ResolvedMappings { source_file, code_mappings, branch_mappings } = mappings; + let Some(global_file_id) = global_file_table.get_existing_id(source_file) else { + debug_assert!(false, "couldn't find an existing global-file-id for {source_file:?}"); + return; + }; + let llvm_cov::Regions { code_regions, expansion_regions: _, // FIXME(Zalathar): Fill out support for expansion regions branch_regions, - } = &mut covfun.regions; + } = regions; - // For each counter/region pair in this function+file, convert it to a - // form suitable for FFI. - for &Mapping { ref kind, span } in &mir_info.mappings { - let Some(coords) = make_coords(span) else { continue }; + // The global file IDs are stored as `u32` to make FFI easier. + // FIXME(Zalathar): Consider giving `newtype_index!` a safe transmute to `&[u32]`. + let local_file_id = virtual_file_mapping.push(global_file_id.as_u32()); + + for &CodeMapping { coords, counter } in code_mappings { let cov_span = coords.make_coverage_span(local_file_id); + code_regions.push(ffi::CodeRegion { cov_span, counter }); + } - match *kind { - MappingKind::Code { bcb } => { - code_regions.push(ffi::CodeRegion { cov_span, counter: counter_for_bcb(bcb) }); - } - MappingKind::Branch { true_bcb, false_bcb } => { - branch_regions.push(ffi::BranchRegion { - cov_span, - true_counter: counter_for_bcb(true_bcb), - false_counter: counter_for_bcb(false_bcb), - }); - } - } + for &BranchMapping { coords, true_counter, false_counter } in branch_mappings { + let cov_span = coords.make_coverage_span(local_file_id); + branch_regions.push(ffi::BranchRegion { cov_span, true_counter, false_counter }); } } @@ -187,24 +247,25 @@ pub(crate) fn emit_covfun_record<'tcx>( mangled_function_name, source_hash, is_used, - ref virtual_file_mapping, ref expressions, - ref regions, + ref mappings, } = covfun; - let Some(local_file_table) = virtual_file_mapping.resolve_all(global_file_table) else { - debug_assert!( - false, - "all local files should be present in the global file table: \ - global_file_table = {global_file_table:?}, \ - virtual_file_mapping = {virtual_file_mapping:?}" - ); + let mut regions = llvm_cov::Regions::default(); + let mut virtual_file_mapping = IndexVec::new(); + fill_region_tables(global_file_table, mappings, &mut virtual_file_mapping, &mut regions); + + if regions.has_no_regions() { + debug_assert!(false, "mappings should have produced at least one region: {mappings:#?}"); return; - }; + } // Encode the function's coverage mappings into a buffer. - let coverage_mapping_buffer = - llvm_cov::write_function_mappings_to_buffer(&local_file_table, expressions, regions); + let coverage_mapping_buffer = llvm_cov::write_function_mappings_to_buffer( + &virtual_file_mapping.raw, + expressions, + ®ions, + ); // A covfun record consists of four target-endian integers, followed by the // encoded mapping data in bytes. Note that the length field is 32 bits.