From 339556eb026e6d1a0a18313f9c3cfe0ab87fe738 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 4 May 2025 14:09:52 +1000 Subject: coverage: Enlarge empty spans during MIR instrumentation, not codegen This allows us to assume that coverage spans will only be discarded during codegen in very unusual situations. --- .../src/coverageinfo/mapgen/spans.rs | 28 ++++------------------ 1 file changed, 4 insertions(+), 24 deletions(-) (limited to 'compiler/rustc_codegen_llvm/src') diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/spans.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/spans.rs index 39a59560c9d..574463be7ff 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/spans.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/spans.rs @@ -39,7 +39,10 @@ impl Coords { /// or other expansions), and if it does happen then skipping a span or function is /// better than an ICE or `llvm-cov` failure that the user might have no way to avoid. pub(crate) fn make_coords(source_map: &SourceMap, file: &SourceFile, span: Span) -> Option { - let span = ensure_non_empty_span(source_map, span)?; + if span.is_empty() { + debug_assert!(false, "can't make coords from empty span: {span:?}"); + return None; + } let lo = span.lo(); let hi = span.hi(); @@ -70,29 +73,6 @@ pub(crate) fn make_coords(source_map: &SourceMap, file: &SourceFile, span: Span) }) } -fn ensure_non_empty_span(source_map: &SourceMap, span: Span) -> Option { - if !span.is_empty() { - return Some(span); - } - - // The span is empty, so try to enlarge it to cover an adjacent '{' or '}'. - source_map - .span_to_source(span, |src, start, end| try { - // Adjusting span endpoints by `BytePos(1)` is normally a bug, - // but in this case we have specifically checked that the character - // we're skipping over is one of two specific ASCII characters, so - // adjusting by exactly 1 byte is correct. - if src.as_bytes().get(end).copied() == Some(b'{') { - Some(span.with_hi(span.hi() + BytePos(1))) - } else if start > 0 && src.as_bytes()[start - 1] == b'}' { - Some(span.with_lo(span.lo() - BytePos(1))) - } else { - None - } - }) - .ok()? -} - /// If `llvm-cov` sees a source region that is improperly ordered (end < start), /// it will immediately exit with a fatal error. To prevent that from happening, /// discard regions that are improperly ordered, or might be interpreted in a -- cgit 1.4.1-3-g733a5 From 8cd8b23b9e4b1f05373831644b005b21a13a9c69 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 9 May 2025 21:27:27 +1000 Subject: coverage: Hoist `counter_for_bcb` out of its loop Having this helper function in the loop was confusing, because it doesn't rely on anything that changes between loop iterations. --- .../src/coverageinfo/mapgen/covfun.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'compiler/rustc_codegen_llvm/src') diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs index 7bdbc685952..6c2ea71799a 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs @@ -104,6 +104,16 @@ fn fill_region_tables<'tcx>( ids_info: &'tcx CoverageIdsInfo, covfun: &mut CovfunRecord<'tcx>, ) { + // If this function is unused, replace all counters with zero. + let counter_for_bcb = |bcb: BasicCoverageBlock| -> ffi::Counter { + let term = if covfun.is_used { + ids_info.term_for_bcb[bcb].expect("every BCB in a mapping was given a term") + } else { + CovTerm::Zero + }; + ffi::Counter::from_term(term) + }; + // 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(); @@ -135,16 +145,6 @@ fn fill_region_tables<'tcx>( // For each counter/region pair in this function+file, convert it to a // form suitable for FFI. for &Mapping { ref kind, span } in &fn_cov_info.mappings { - // If this function is unused, replace all counters with zero. - let counter_for_bcb = |bcb: BasicCoverageBlock| -> ffi::Counter { - let term = if covfun.is_used { - ids_info.term_for_bcb[bcb].expect("every BCB in a mapping was given a term") - } else { - CovTerm::Zero - }; - ffi::Counter::from_term(term) - }; - let Some(coords) = make_coords(span) else { continue }; let cov_span = coords.make_coverage_span(local_file_id); -- cgit 1.4.1-3-g733a5 From 078144fdfa7ae627d43cd919d660a71bef1e4658 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 9 May 2025 20:57:17 +1000 Subject: coverage: Detect unused local file IDs to avoid an LLVM assertion This case can't actually happen yet (other than via a testing flag), because currently all of a function's spans must belong to the same file and expansion. But this will be an important edge case when adding expansion region support. --- .../rustc_codegen_llvm/src/coverageinfo/ffi.rs | 26 +++++++++++++++++----- .../src/coverageinfo/mapgen/covfun.rs | 24 ++++++++++++++++++++ compiler/rustc_interface/src/tests.rs | 3 ++- compiler/rustc_session/src/config.rs | 5 +++++ compiler/rustc_session/src/options.rs | 1 + compiler/rustc_session/src/session.rs | 5 +++++ tests/coverage/unused-local-file.coverage | 7 ++++++ tests/coverage/unused-local-file.rs | 22 ++++++++++++++++++ 8 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 tests/coverage/unused-local-file.coverage create mode 100644 tests/coverage/unused-local-file.rs (limited to 'compiler/rustc_codegen_llvm/src') diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index f6000e72840..c207df2fb0b 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -155,6 +155,20 @@ pub(crate) struct Regions { impl Regions { /// Returns true if none of this structure's tables contain any regions. pub(crate) fn has_no_regions(&self) -> bool { + // Every region has a span, so if there are no spans then there are no regions. + self.all_cov_spans().next().is_none() + } + + pub(crate) fn all_cov_spans(&self) -> impl Iterator { + macro_rules! iter_cov_spans { + ( $( $regions:expr ),* $(,)? ) => { + std::iter::empty() + $( + .chain( $regions.iter().map(|region| ®ion.cov_span) ) + )* + } + } + let Self { code_regions, expansion_regions, @@ -163,11 +177,13 @@ impl Regions { mcdc_decision_regions, } = self; - code_regions.is_empty() - && expansion_regions.is_empty() - && branch_regions.is_empty() - && mcdc_branch_regions.is_empty() - && mcdc_decision_regions.is_empty() + iter_cov_spans!( + code_regions, + expansion_regions, + branch_regions, + mcdc_branch_regions, + mcdc_decision_regions, + ) } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs index 6c2ea71799a..d3a815fabe7 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs @@ -11,6 +11,7 @@ use rustc_abi::Align; use rustc_codegen_ssa::traits::{ BaseTypeCodegenMethods as _, ConstCodegenMethods, StaticCodegenMethods, }; +use rustc_index::IndexVec; use rustc_middle::mir::coverage::{ BasicCoverageBlock, CovTerm, CoverageIdsInfo, Expression, FunctionCoverageInfo, Mapping, MappingKind, Op, @@ -125,6 +126,12 @@ fn fill_region_tables<'tcx>( let local_file_id = covfun.virtual_file_mapping.push_file(&source_file); + // If this testing flag is set, add an extra unused entry to the local + // file table, to help test the code for detecting unused file IDs. + if tcx.sess.coverage_inject_unused_local_file() { + 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 @@ -177,6 +184,19 @@ fn fill_region_tables<'tcx>( } } +/// LLVM requires all local file IDs to have at least one mapping region. +/// If that's not the case, skip this function, to avoid an assertion failure +/// (or worse) in LLVM. +fn check_local_file_table(covfun: &CovfunRecord<'_>) -> bool { + let mut local_file_id_seen = + IndexVec::::from_elem_n(false, covfun.virtual_file_mapping.local_file_table.len()); + for cov_span in covfun.regions.all_cov_spans() { + local_file_id_seen[cov_span.file_id] = true; + } + + local_file_id_seen.into_iter().all(|seen| seen) +} + /// Generates the contents of the covfun record for this function, which /// contains the function's coverage mapping data. The record is then stored /// as a global variable in the `__llvm_covfun` section. @@ -185,6 +205,10 @@ pub(crate) fn generate_covfun_record<'tcx>( global_file_table: &GlobalFileTable, covfun: &CovfunRecord<'tcx>, ) { + if !check_local_file_table(covfun) { + return; + } + let &CovfunRecord { mangled_function_name, source_hash, diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs index 0ceda220134..8bcd0453624 100644 --- a/compiler/rustc_interface/src/tests.rs +++ b/compiler/rustc_interface/src/tests.rs @@ -776,7 +776,8 @@ fn test_unstable_options_tracking_hash() { CoverageOptions { level: CoverageLevel::Mcdc, no_mir_spans: true, - discard_all_spans_in_codegen: true + discard_all_spans_in_codegen: true, + inject_unused_local_file: true, } ); tracked!(crate_attr, vec!["abc".to_string()]); diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index 60e1b465ba9..144aeb5c369 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -195,6 +195,11 @@ pub struct CoverageOptions { /// regression tests for #133606, because we don't have an easy way to /// reproduce it from actual source code. pub discard_all_spans_in_codegen: bool, + + /// `-Zcoverage-options=inject-unused-local-file`: During codegen, add an + /// extra dummy entry to each function's local file table, to exercise the + /// code that checks for local file IDs with no mapping regions. + pub inject_unused_local_file: bool, } /// Controls whether branch coverage or MC/DC coverage is enabled. diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index b95ebfbe89f..1f783c59c79 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -1413,6 +1413,7 @@ pub mod parse { "mcdc" => slot.level = CoverageLevel::Mcdc, "no-mir-spans" => slot.no_mir_spans = true, "discard-all-spans-in-codegen" => slot.discard_all_spans_in_codegen = true, + "inject-unused-local-file" => slot.inject_unused_local_file = true, _ => return false, } } diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index 010ae42c280..34ac37d6378 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -371,6 +371,11 @@ impl Session { self.opts.unstable_opts.coverage_options.discard_all_spans_in_codegen } + /// True if testing flag `-Zcoverage-options=inject-unused-local-file` was passed. + pub fn coverage_inject_unused_local_file(&self) -> bool { + self.opts.unstable_opts.coverage_options.inject_unused_local_file + } + pub fn is_sanitizer_cfi_enabled(&self) -> bool { self.opts.unstable_opts.sanitizer.contains(SanitizerSet::CFI) } diff --git a/tests/coverage/unused-local-file.coverage b/tests/coverage/unused-local-file.coverage new file mode 100644 index 00000000000..8f5a32f6d70 --- /dev/null +++ b/tests/coverage/unused-local-file.coverage @@ -0,0 +1,7 @@ + LL| |//@ edition: 2021 + LL| | + LL| |// Force this function to be generated in its home crate, so that it ends up + LL| |// with normal coverage metadata. + LL| |#[inline(never)] + LL| 1|pub fn external_function() {} + diff --git a/tests/coverage/unused-local-file.rs b/tests/coverage/unused-local-file.rs new file mode 100644 index 00000000000..cf43c62d703 --- /dev/null +++ b/tests/coverage/unused-local-file.rs @@ -0,0 +1,22 @@ +//! If we give LLVM a local file table for a function, but some of the entries +//! in that table have no associated mapping regions, then an assertion failure +//! will occur in LLVM. We therefore need to detect and skip any function that +//! would trigger that assertion. +//! +//! To test that this case is handled, even before adding code that could allow +//! it to happen organically (for expansion region support), we use a special +//! testing-only flag to force it to occur. + +//@ edition: 2024 +//@ compile-flags: -Zcoverage-options=inject-unused-local-file + +// The `llvm-cov` tool will complain if the test binary ends up having no +// coverage metadata at all. To prevent that, we also link to instrumented +// code in an auxiliary crate that doesn't have the special flag set. + +//@ aux-build: discard_all_helper.rs +extern crate discard_all_helper; + +fn main() { + discard_all_helper::external_function(); +} -- cgit 1.4.1-3-g733a5