From 9461f4296ffcbd75440ec211ead7bff6fb86012c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 29 Nov 2024 14:52:41 +1100 Subject: [PATCH] Revert "Rollup merge of #133418 - Zalathar:spans, r=jieyouxu" This reverts commit adf9b5fcd1de43eaf0a779e10612caee8b47bede, reversing changes made to af1ca153d4aed5ffe22445273aa388a8d3f8f4ae. Reverting due to . --- .../src/coverageinfo/ffi.rs | 36 ++-- .../src/coverageinfo/map_data.rs | 14 +- .../src/coverageinfo/mapgen.rs | 129 +++++++------- .../src/coverageinfo/mapgen/spans.rs | 124 ------------- compiler/rustc_codegen_llvm/src/lib.rs | 1 - compiler/rustc_middle/src/mir/coverage.rs | 18 +- compiler/rustc_middle/src/mir/pretty.rs | 4 +- .../rustc_mir_transform/src/coverage/mod.rs | 166 ++++++++++++++++-- ...ch_match_arms.main.InstrumentCoverage.diff | 12 +- ...ument_coverage.bar.InstrumentCoverage.diff | 2 +- ...ment_coverage.main.InstrumentCoverage.diff | 10 +- ...rage_cleanup.main.CleanupPostBorrowck.diff | 10 +- ...erage_cleanup.main.InstrumentCoverage.diff | 10 +- 13 files changed, 285 insertions(+), 251 deletions(-) delete mode 100644 compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/spans.rs diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index 19d6726002c41..a6e07ea2a60ec 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -1,4 +1,6 @@ -use rustc_middle::mir::coverage::{CounterId, CovTerm, ExpressionId}; +use rustc_middle::mir::coverage::{CounterId, CovTerm, ExpressionId, SourceRegion}; + +use crate::coverageinfo::mapgen::LocalFileId; /// Must match the layout of `LLVMRustCounterKind`. #[derive(Copy, Clone, Debug)] @@ -124,23 +126,37 @@ pub(crate) struct CoverageSpan { /// Local index into the function's local-to-global file ID table. /// The value at that index is itself an index into the coverage filename /// table in the CGU's `__llvm_covmap` section. - pub(crate) file_id: u32, + file_id: u32, /// 1-based starting line of the source code span. - pub(crate) start_line: u32, + start_line: u32, /// 1-based starting column of the source code span. - pub(crate) start_col: u32, + start_col: u32, /// 1-based ending line of the source code span. - pub(crate) end_line: u32, + end_line: u32, /// 1-based ending column of the source code span. High bit must be unset. - pub(crate) end_col: u32, + end_col: u32, +} + +impl CoverageSpan { + pub(crate) fn from_source_region( + local_file_id: LocalFileId, + code_region: &SourceRegion, + ) -> Self { + let file_id = local_file_id.as_u32(); + let &SourceRegion { start_line, start_col, end_line, end_col } = code_region; + // Internally, LLVM uses the high bit of `end_col` to distinguish between + // code regions and gap regions, so it can't be used by the column number. + assert!(end_col & (1u32 << 31) == 0, "high bit of `end_col` must be unset: {end_col:#X}"); + Self { file_id, start_line, start_col, end_line, end_col } + } } /// Must match the layout of `LLVMRustCoverageCodeRegion`. #[derive(Clone, Debug)] #[repr(C)] pub(crate) struct CodeRegion { - pub(crate) cov_span: CoverageSpan, + pub(crate) span: CoverageSpan, pub(crate) counter: Counter, } @@ -148,7 +164,7 @@ pub(crate) struct CodeRegion { #[derive(Clone, Debug)] #[repr(C)] pub(crate) struct BranchRegion { - pub(crate) cov_span: CoverageSpan, + pub(crate) span: CoverageSpan, pub(crate) true_counter: Counter, pub(crate) false_counter: Counter, } @@ -157,7 +173,7 @@ pub(crate) struct BranchRegion { #[derive(Clone, Debug)] #[repr(C)] pub(crate) struct MCDCBranchRegion { - pub(crate) cov_span: CoverageSpan, + pub(crate) span: CoverageSpan, pub(crate) true_counter: Counter, pub(crate) false_counter: Counter, pub(crate) mcdc_branch_params: mcdc::BranchParameters, @@ -167,6 +183,6 @@ pub(crate) struct MCDCBranchRegion { #[derive(Clone, Debug)] #[repr(C)] pub(crate) struct MCDCDecisionRegion { - pub(crate) cov_span: CoverageSpan, + pub(crate) span: CoverageSpan, pub(crate) mcdc_decision_params: mcdc::DecisionParameters, } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 95746b88cedbf..e3c0df278836e 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -3,9 +3,9 @@ use rustc_data_structures::fx::FxIndexSet; use rustc_index::bit_set::BitSet; use rustc_middle::mir::coverage::{ CounterId, CovTerm, Expression, ExpressionId, FunctionCoverageInfo, Mapping, MappingKind, Op, + SourceRegion, }; use rustc_middle::ty::Instance; -use rustc_span::Span; use tracing::{debug, instrument}; use crate::coverageinfo::ffi::{Counter, CounterExpression, ExprKind}; @@ -220,16 +220,16 @@ impl<'tcx> FunctionCoverage<'tcx> { }) } - /// Yields all this function's coverage mappings, after simplifying away - /// unused counters and counter expressions. - pub(crate) fn mapping_spans( + /// 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 + Captures<'_> { + ) -> impl Iterator + ExactSizeIterator { self.function_coverage_info.mappings.iter().map(move |mapping| { - let &Mapping { ref kind, span } = mapping; + let Mapping { kind, source_region } = mapping; let kind = kind.map_terms(|term| if self.is_zero_term(term) { CovTerm::Zero } else { term }); - (kind, span) + (kind, source_region) }) } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index ed881418cb003..b582dd967a706 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -1,14 +1,12 @@ -mod spans; - use std::ffi::CString; -use std::sync::Arc; +use std::iter; use itertools::Itertools as _; use rustc_abi::Align; use rustc_codegen_ssa::traits::{ BaseTypeCodegenMethods, ConstCodegenMethods, StaticCodegenMethods, }; -use rustc_data_structures::fx::{FxHashSet, FxIndexMap}; +use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet}; use rustc_hir::def_id::{DefId, LocalDefId}; use rustc_index::IndexVec; use rustc_middle::mir::coverage::MappingKind; @@ -17,7 +15,7 @@ use rustc_middle::{bug, mir}; use rustc_session::RemapFileNameExt; use rustc_session::config::RemapPathScopeComponents; use rustc_span::def_id::DefIdSet; -use rustc_span::{SourceFile, StableSourceFileId}; +use rustc_span::{Span, Symbol}; use rustc_target::spec::HasTargetSpec; use tracing::debug; @@ -74,11 +72,11 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { .map(|(instance, function_coverage)| (instance, function_coverage.into_finished())) .collect::>(); - let all_files = function_coverage_entries + let all_file_names = function_coverage_entries .iter() .map(|(_, fn_cov)| fn_cov.function_coverage_info.body_span) - .map(|span| tcx.sess.source_map().lookup_source_file(span.lo())); - let global_file_table = GlobalFileTable::new(all_files); + .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); @@ -105,8 +103,15 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { encode_mappings_for_function(tcx, &global_file_table, &function_coverage); if coverage_mapping_buffer.is_empty() { - debug!("function has no mappings to embed; skipping"); - continue; + if function_coverage.is_used() { + bug!( + "A used function should have had coverage mapping data but did not: {}", + mangled_function_name + ); + } else { + debug!("unused function had no coverage mapping data: {}", mangled_function_name); + continue; + } } if !is_used { @@ -143,34 +148,29 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { } } -/// Maps "global" (per-CGU) file ID numbers to their underlying source files. +/// Maps "global" (per-CGU) file ID numbers to their underlying filenames. struct GlobalFileTable { - /// This "raw" table doesn't include the working dir, so a file's + /// 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: FxIndexMap>, + raw_file_table: FxIndexSet, } impl GlobalFileTable { - fn new(all_files: impl IntoIterator>) -> Self { - // Collect all of the files into a set. Files usually come in contiguous - // runs, so we can dedup adjacent ones to save work. - let mut raw_file_table = all_files - .into_iter() - .dedup_by(|a, b| a.stable_id == b.stable_id) - .map(|f| (f.stable_id, f)) - .collect::>>(); + 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 underlying filenames. - raw_file_table.sort_unstable_by(|_, a, _, b| { - Ord::cmp(&a.name, &b.name).then_with(|| Ord::cmp(&a.stable_id, &b.stable_id)) - }); + // 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(&self, file: &SourceFile) -> GlobalFileId { - let raw_id = self.raw_file_table.get_index_of(&file.stable_id).unwrap_or_else(|| { - bug!("file not found in prepared global file table: {:?}", file.name); + 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}"); }); // 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. @@ -178,27 +178,24 @@ impl GlobalFileTable { } fn make_filenames_buffer(&self, tcx: TyCtxt<'_>) -> Vec { - let mut table = Vec::with_capacity(self.raw_file_table.len() + 1); - // 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. - table.push( - tcx.sess - .opts - .working_dir - .for_scope(tcx.sess, RemapPathScopeComponents::MACRO) - .to_string_lossy(), - ); - - // Add the regular entries after the base directory. - table.extend(self.raw_file_table.values().map(|file| { - file.name.for_scope(tcx.sess, RemapPathScopeComponents::MACRO).to_string_lossy() - })); - - llvm_cov::write_filenames_to_buffer(table.iter().map(|f| f.as_ref())) + use rustc_session::RemapFileNameExt; + use rustc_session::config::RemapPathScopeComponents; + let working_dir: &str = &tcx + .sess + .opts + .working_dir + .for_scope(tcx.sess, RemapPathScopeComponents::MACRO) + .to_string_lossy(); + + // Insert the working dir at index 0, before the other filenames. + let filenames = + iter::once(working_dir).chain(self.raw_file_table.iter().map(Symbol::as_str)); + llvm_cov::write_filenames_to_buffer(filenames) } } @@ -211,7 +208,7 @@ rustc_index::newtype_index! { /// An index into a function's list of global file IDs. That underlying list /// of local-to-global mappings will be embedded in the function's record in /// the `__llvm_covfun` linker section. - struct LocalFileId {} + pub(crate) struct LocalFileId {} } /// Holds a mapping from "local" (per-function) file IDs to "global" (per-CGU) @@ -237,6 +234,13 @@ impl VirtualFileMapping { } } +fn span_file_name(tcx: TyCtxt<'_>, span: Span) -> Symbol { + let source_file = tcx.sess.source_map().lookup_source_file(span.lo()); + let name = + source_file.name.for_scope(tcx.sess, RemapPathScopeComponents::MACRO).to_string_lossy(); + Symbol::intern(&name) +} + /// 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. @@ -247,13 +251,11 @@ fn encode_mappings_for_function( global_file_table: &GlobalFileTable, function_coverage: &FunctionCoverage<'_>, ) -> Vec { - let mapping_spans = function_coverage.mapping_spans(); - if mapping_spans.is_empty() { + let counter_regions = function_coverage.counter_regions(); + if counter_regions.is_empty() { return Vec::new(); } - let fn_cov_info = function_coverage.function_coverage_info; - let expressions = function_coverage.counter_expressions().collect::>(); let mut virtual_file_mapping = VirtualFileMapping::default(); @@ -263,39 +265,34 @@ fn encode_mappings_for_function( let mut mcdc_decision_regions = vec![]; // Currently a function's mappings must all be in the same file as its body span. - let source_map = tcx.sess.source_map(); - let source_file = source_map.lookup_source_file(fn_cov_info.body_span.lo()); + let file_name = span_file_name(tcx, function_coverage.function_coverage_info.body_span); - // Look up the global file ID for that file. - let global_file_id = global_file_table.global_file_id_for_file(&source_file); + // Look up the global file ID for that filename. + 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 = virtual_file_mapping.local_id_for_global(global_file_id); + debug!(" file id: {local_file_id:?} => {global_file_id:?} = '{file_name:?}'"); - let make_cov_span = |span| { - spans::make_coverage_span(local_file_id, source_map, fn_cov_info, &source_file, span) - }; - - // For each coverage mapping span in this function+file, convert it to a + // For each counter/region pair in this function+file, convert it to a // form suitable for FFI. - for (mapping_kind, span) in mapping_spans { - debug!("Adding counter {mapping_kind:?} to map for {span:?}"); - let Some(cov_span) = make_cov_span(span) else { continue }; + 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 { MappingKind::Code(term) => { - code_regions - .push(ffi::CodeRegion { cov_span, counter: ffi::Counter::from_term(term) }); + code_regions.push(ffi::CodeRegion { span, counter: ffi::Counter::from_term(term) }); } MappingKind::Branch { true_term, false_term } => { branch_regions.push(ffi::BranchRegion { - cov_span, + span, true_counter: ffi::Counter::from_term(true_term), false_counter: ffi::Counter::from_term(false_term), }); } MappingKind::MCDCBranch { true_term, false_term, mcdc_params } => { mcdc_branch_regions.push(ffi::MCDCBranchRegion { - cov_span, + span, true_counter: ffi::Counter::from_term(true_term), false_counter: ffi::Counter::from_term(false_term), mcdc_branch_params: ffi::mcdc::BranchParameters::from(mcdc_params), @@ -303,7 +300,7 @@ fn encode_mappings_for_function( } MappingKind::MCDCDecision(mcdc_decision_params) => { mcdc_decision_regions.push(ffi::MCDCDecisionRegion { - cov_span, + span, mcdc_decision_params: ffi::mcdc::DecisionParameters::from(mcdc_decision_params), }); } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/spans.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/spans.rs deleted file mode 100644 index 4a7721879fd0d..0000000000000 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/spans.rs +++ /dev/null @@ -1,124 +0,0 @@ -use rustc_middle::mir::coverage::FunctionCoverageInfo; -use rustc_span::source_map::SourceMap; -use rustc_span::{BytePos, Pos, SourceFile, Span}; -use tracing::debug; - -use crate::coverageinfo::ffi; -use crate::coverageinfo::mapgen::LocalFileId; - -/// Converts the span into its start line and column, and end line and column. -/// -/// Line numbers and column numbers are 1-based. Unlike most column numbers emitted by -/// the compiler, these column numbers are denoted in **bytes**, because that's what -/// LLVM's `llvm-cov` tool expects to see in coverage maps. -/// -/// Returns `None` if the conversion failed for some reason. This shouldn't happen, -/// but it's hard to rule out entirely (especially in the presence of complex macros -/// 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_coverage_span( - file_id: LocalFileId, - source_map: &SourceMap, - fn_cov_info: &FunctionCoverageInfo, - file: &SourceFile, - span: Span, -) -> Option { - let span = ensure_non_empty_span(source_map, fn_cov_info, span)?; - - let lo = span.lo(); - let hi = span.hi(); - - // Column numbers need to be in bytes, so we can't use the more convenient - // `SourceMap` methods for looking up file coordinates. - let line_and_byte_column = |pos: BytePos| -> Option<(usize, usize)> { - let rpos = file.relative_position(pos); - let line_index = file.lookup_line(rpos)?; - let line_start = file.lines()[line_index]; - // Line numbers and column numbers are 1-based, so add 1 to each. - Some((line_index + 1, (rpos - line_start).to_usize() + 1)) - }; - - let (mut start_line, start_col) = line_and_byte_column(lo)?; - let (mut end_line, end_col) = line_and_byte_column(hi)?; - - // Apply an offset so that code in doctests has correct line numbers. - // FIXME(#79417): Currently we have no way to offset doctest _columns_. - start_line = source_map.doctest_offset_line(&file.name, start_line); - end_line = source_map.doctest_offset_line(&file.name, end_line); - - check_coverage_span(ffi::CoverageSpan { - file_id: file_id.as_u32(), - start_line: start_line as u32, - start_col: start_col as u32, - end_line: end_line as u32, - end_col: end_col as u32, - }) -} - -fn ensure_non_empty_span( - source_map: &SourceMap, - fn_cov_info: &FunctionCoverageInfo, - span: Span, -) -> Option { - if !span.is_empty() { - return Some(span); - } - - let lo = span.lo(); - let hi = span.hi(); - - // The span is empty, so try to expand it to cover an adjacent '{' or '}', - // but only within the bounds of the body span. - let try_next = hi < fn_cov_info.body_span.hi(); - let try_prev = fn_cov_info.body_span.lo() < lo; - if !(try_next || try_prev) { - return None; - } - - source_map - .span_to_source(span, |src, start, end| try { - // We're only checking for specific ASCII characters, so we don't - // have to worry about multi-byte code points. - if try_next && src.as_bytes()[end] == b'{' { - Some(span.with_hi(hi + BytePos(1))) - } else if try_prev && src.as_bytes()[start - 1] == b'}' { - Some(span.with_lo(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 -/// way that makes them improperly ordered. -fn check_coverage_span(cov_span: ffi::CoverageSpan) -> Option { - let ffi::CoverageSpan { file_id: _, start_line, start_col, end_line, end_col } = cov_span; - - // Line/column coordinates are supposed to be 1-based. If we ever emit - // coordinates of 0, `llvm-cov` might misinterpret them. - let all_nonzero = [start_line, start_col, end_line, end_col].into_iter().all(|x| x != 0); - // Coverage mappings use the high bit of `end_col` to indicate that a - // region is actually a "gap" region, so make sure it's unset. - let end_col_has_high_bit_unset = (end_col & (1 << 31)) == 0; - // If a region is improperly ordered (end < start), `llvm-cov` will exit - // with a fatal error, which is inconvenient for users and hard to debug. - let is_ordered = (start_line, start_col) <= (end_line, end_col); - - if all_nonzero && end_col_has_high_bit_unset && is_ordered { - Some(cov_span) - } else { - debug!( - ?cov_span, - ?all_nonzero, - ?end_col_has_high_bit_unset, - ?is_ordered, - "Skipping source region that would be misinterpreted or rejected by LLVM" - ); - // If this happens in a debug build, ICE to make it easier to notice. - debug_assert!(false, "Improper source region: {cov_span:?}"); - None - } -} diff --git a/compiler/rustc_codegen_llvm/src/lib.rs b/compiler/rustc_codegen_llvm/src/lib.rs index 9f398107fc610..3dfb86d422dd2 100644 --- a/compiler/rustc_codegen_llvm/src/lib.rs +++ b/compiler/rustc_codegen_llvm/src/lib.rs @@ -17,7 +17,6 @@ #![feature(iter_intersperse)] #![feature(let_chains)] #![feature(rustdoc_internals)] -#![feature(try_blocks)] #![warn(unreachable_pub)] // tidy-alphabetical-end diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 33f9a2bdca6b4..11a4e7f89a7c7 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -155,6 +155,22 @@ impl Debug for CoverageKind { } } +#[derive(Clone, TyEncodable, TyDecodable, Hash, HashStable, PartialEq, Eq, PartialOrd, Ord)] +#[derive(TypeFoldable, TypeVisitable)] +pub struct SourceRegion { + pub start_line: u32, + pub start_col: u32, + pub end_line: u32, + pub end_col: u32, +} + +impl Debug for SourceRegion { + fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result { + let &Self { start_line, start_col, end_line, end_col } = self; + write!(fmt, "{start_line}:{start_col} - {end_line}:{end_col}") + } +} + #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, HashStable)] #[derive(TyEncodable, TyDecodable, TypeFoldable, TypeVisitable)] pub enum Op { @@ -216,7 +232,7 @@ impl MappingKind { #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub struct Mapping { pub kind: MappingKind, - pub span: Span, + pub source_region: SourceRegion, } /// Stores per-function coverage information attached to a `mir::Body`, diff --git a/compiler/rustc_middle/src/mir/pretty.rs b/compiler/rustc_middle/src/mir/pretty.rs index ece468947c25d..2bfcd0a62274d 100644 --- a/compiler/rustc_middle/src/mir/pretty.rs +++ b/compiler/rustc_middle/src/mir/pretty.rs @@ -603,8 +603,8 @@ fn write_function_coverage_info( for (id, expression) in expressions.iter_enumerated() { writeln!(w, "{INDENT}coverage {id:?} => {expression:?};")?; } - for coverage::Mapping { kind, span } in mappings { - writeln!(w, "{INDENT}coverage {kind:?} => {span:?};")?; + for coverage::Mapping { kind, source_region } in mappings { + writeln!(w, "{INDENT}coverage {kind:?} => {source_region:?};")?; } writeln!(w)?; diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 02fd289b6ef6d..e8b3d80be0212 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -13,15 +13,16 @@ use rustc_hir::intravisit::{Visitor, walk_expr}; use rustc_middle::hir::map::Map; use rustc_middle::hir::nested_filter; use rustc_middle::mir::coverage::{ - CoverageKind, DecisionInfo, FunctionCoverageInfo, Mapping, MappingKind, + CoverageKind, DecisionInfo, FunctionCoverageInfo, Mapping, MappingKind, SourceRegion, }; use rustc_middle::mir::{ self, BasicBlock, BasicBlockData, SourceInfo, Statement, StatementKind, Terminator, TerminatorKind, }; use rustc_middle::ty::TyCtxt; -use rustc_span::Span; use rustc_span::def_id::LocalDefId; +use rustc_span::source_map::SourceMap; +use rustc_span::{BytePos, Pos, SourceFile, Span}; use tracing::{debug, debug_span, trace}; use crate::coverage::counters::{CounterIncrementSite, CoverageCounters}; @@ -96,7 +97,7 @@ fn instrument_function_for_coverage<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mut mir: let coverage_counters = CoverageCounters::make_bcb_counters(&basic_coverage_blocks, &bcbs_with_counter_mappings); - let mappings = create_mappings(&extracted_mappings, &coverage_counters); + let mappings = create_mappings(tcx, &hir_info, &extracted_mappings, &coverage_counters); if mappings.is_empty() { // No spans could be converted into valid mappings, so skip this function. debug!("no spans could be converted into valid mappings; skipping"); @@ -135,12 +136,18 @@ fn instrument_function_for_coverage<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mut mir: /// /// Precondition: All BCBs corresponding to those spans have been given /// coverage counters. -fn create_mappings( +fn create_mappings<'tcx>( + tcx: TyCtxt<'tcx>, + hir_info: &ExtractedHirInfo, extracted_mappings: &ExtractedMappings, coverage_counters: &CoverageCounters, ) -> Vec { + let source_map = tcx.sess.source_map(); + let file = source_map.lookup_source_file(hir_info.body_span.lo()); + let term_for_bcb = |bcb| coverage_counters.term_for_bcb(bcb).expect("all BCBs with spans were given counters"); + let region_for_span = |span: Span| make_source_region(source_map, hir_info, &file, span); // Fully destructure the mappings struct to make sure we don't miss any kinds. let ExtractedMappings { @@ -153,20 +160,22 @@ fn create_mappings( } = extracted_mappings; let mut mappings = Vec::new(); - mappings.extend(code_mappings.iter().map( + mappings.extend(code_mappings.iter().filter_map( // Ordinary code mappings are the simplest kind. |&mappings::CodeMapping { span, bcb }| { + let source_region = region_for_span(span)?; let kind = MappingKind::Code(term_for_bcb(bcb)); - Mapping { kind, span } + Some(Mapping { kind, source_region }) }, )); - mappings.extend(branch_pairs.iter().map( + mappings.extend(branch_pairs.iter().filter_map( |&mappings::BranchPair { span, true_bcb, false_bcb }| { let true_term = term_for_bcb(true_bcb); let false_term = term_for_bcb(false_bcb); let kind = MappingKind::Branch { true_term, false_term }; - Mapping { kind, span } + let source_region = region_for_span(span)?; + Some(Mapping { kind, source_region }) }, )); @@ -174,7 +183,7 @@ fn create_mappings( |bcb| coverage_counters.term_for_bcb(bcb).expect("all BCBs with spans were given counters"); // MCDC branch mappings are appended with their decisions in case decisions were ignored. - mappings.extend(mcdc_degraded_branches.iter().map( + mappings.extend(mcdc_degraded_branches.iter().filter_map( |&mappings::MCDCBranch { span, true_bcb, @@ -183,9 +192,10 @@ fn create_mappings( true_index: _, false_index: _, }| { + let source_region = region_for_span(span)?; let true_term = term_for_bcb(true_bcb); let false_term = term_for_bcb(false_bcb); - Mapping { kind: MappingKind::Branch { true_term, false_term }, span } + Some(Mapping { kind: MappingKind::Branch { true_term, false_term }, source_region }) }, )); @@ -193,7 +203,7 @@ fn create_mappings( let num_conditions = branches.len() as u16; let conditions = branches .into_iter() - .map( + .filter_map( |&mappings::MCDCBranch { span, true_bcb, @@ -202,29 +212,31 @@ fn create_mappings( true_index: _, false_index: _, }| { + let source_region = region_for_span(span)?; let true_term = term_for_bcb(true_bcb); let false_term = term_for_bcb(false_bcb); - Mapping { + Some(Mapping { kind: MappingKind::MCDCBranch { true_term, false_term, mcdc_params: condition_info, }, - span, - } + source_region, + }) }, ) .collect::>(); - if conditions.len() == num_conditions as usize { + if conditions.len() == num_conditions as usize + && let Some(source_region) = region_for_span(decision.span) + { // LLVM requires end index for counter mapping regions. let kind = MappingKind::MCDCDecision(DecisionInfo { bitmap_idx: (decision.bitmap_idx + decision.num_test_vectors) as u32, num_conditions, }); mappings.extend( - std::iter::once(Mapping { kind, span: decision.span }) - .chain(conditions.into_iter()), + std::iter::once(Mapping { kind, source_region }).chain(conditions.into_iter()), ); } else { mappings.extend(conditions.into_iter().map(|mapping| { @@ -233,7 +245,10 @@ fn create_mappings( else { unreachable!("all mappings here are MCDCBranch as shown above"); }; - Mapping { kind: MappingKind::Branch { true_term, false_term }, span: mapping.span } + Mapping { + kind: MappingKind::Branch { true_term, false_term }, + source_region: mapping.source_region, + } })) } } @@ -376,6 +391,121 @@ fn inject_statement(mir_body: &mut mir::Body<'_>, counter_kind: CoverageKind, bb data.statements.insert(0, statement); } +fn ensure_non_empty_span( + source_map: &SourceMap, + hir_info: &ExtractedHirInfo, + span: Span, +) -> Option { + if !span.is_empty() { + return Some(span); + } + + let lo = span.lo(); + let hi = span.hi(); + + // The span is empty, so try to expand it to cover an adjacent '{' or '}', + // but only within the bounds of the body span. + let try_next = hi < hir_info.body_span.hi(); + let try_prev = hir_info.body_span.lo() < lo; + if !(try_next || try_prev) { + return None; + } + + source_map + .span_to_source(span, |src, start, end| try { + // We're only checking for specific ASCII characters, so we don't + // have to worry about multi-byte code points. + if try_next && src.as_bytes()[end] == b'{' { + Some(span.with_hi(hi + BytePos(1))) + } else if try_prev && src.as_bytes()[start - 1] == b'}' { + Some(span.with_lo(lo - BytePos(1))) + } else { + None + } + }) + .ok()? +} + +/// Converts the span into its start line and column, and end line and column. +/// +/// Line numbers and column numbers are 1-based. Unlike most column numbers emitted by +/// the compiler, these column numbers are denoted in **bytes**, because that's what +/// LLVM's `llvm-cov` tool expects to see in coverage maps. +/// +/// Returns `None` if the conversion failed for some reason. This shouldn't happen, +/// but it's hard to rule out entirely (especially in the presence of complex macros +/// 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. +fn make_source_region( + source_map: &SourceMap, + hir_info: &ExtractedHirInfo, + file: &SourceFile, + span: Span, +) -> Option { + let span = ensure_non_empty_span(source_map, hir_info, span)?; + + let lo = span.lo(); + let hi = span.hi(); + + // Column numbers need to be in bytes, so we can't use the more convenient + // `SourceMap` methods for looking up file coordinates. + let line_and_byte_column = |pos: BytePos| -> Option<(usize, usize)> { + let rpos = file.relative_position(pos); + let line_index = file.lookup_line(rpos)?; + let line_start = file.lines()[line_index]; + // Line numbers and column numbers are 1-based, so add 1 to each. + Some((line_index + 1, (rpos - line_start).to_usize() + 1)) + }; + + let (mut start_line, start_col) = line_and_byte_column(lo)?; + let (mut end_line, end_col) = line_and_byte_column(hi)?; + + // Apply an offset so that code in doctests has correct line numbers. + // FIXME(#79417): Currently we have no way to offset doctest _columns_. + start_line = source_map.doctest_offset_line(&file.name, start_line); + end_line = source_map.doctest_offset_line(&file.name, end_line); + + check_source_region(SourceRegion { + start_line: start_line as u32, + start_col: start_col as u32, + end_line: end_line as u32, + end_col: end_col as u32, + }) +} + +/// 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 +/// way that makes them improperly ordered. +fn check_source_region(source_region: SourceRegion) -> Option { + let SourceRegion { start_line, start_col, end_line, end_col } = source_region; + + // Line/column coordinates are supposed to be 1-based. If we ever emit + // coordinates of 0, `llvm-cov` might misinterpret them. + let all_nonzero = [start_line, start_col, end_line, end_col].into_iter().all(|x| x != 0); + // Coverage mappings use the high bit of `end_col` to indicate that a + // region is actually a "gap" region, so make sure it's unset. + let end_col_has_high_bit_unset = (end_col & (1 << 31)) == 0; + // If a region is improperly ordered (end < start), `llvm-cov` will exit + // with a fatal error, which is inconvenient for users and hard to debug. + let is_ordered = (start_line, start_col) <= (end_line, end_col); + + if all_nonzero && end_col_has_high_bit_unset && is_ordered { + Some(source_region) + } else { + debug!( + ?source_region, + ?all_nonzero, + ?end_col_has_high_bit_unset, + ?is_ordered, + "Skipping source region that would be misinterpreted or rejected by LLVM" + ); + // If this happens in a debug build, ICE to make it easier to notice. + debug_assert!(false, "Improper source region: {source_region:?}"); + None + } +} + /// Function information extracted from HIR by the coverage instrumentor. #[derive(Debug)] struct ExtractedHirInfo { diff --git a/tests/mir-opt/coverage/branch_match_arms.main.InstrumentCoverage.diff b/tests/mir-opt/coverage/branch_match_arms.main.InstrumentCoverage.diff index 39d52ba698a43..cbb11d50f7974 100644 --- a/tests/mir-opt/coverage/branch_match_arms.main.InstrumentCoverage.diff +++ b/tests/mir-opt/coverage/branch_match_arms.main.InstrumentCoverage.diff @@ -33,12 +33,12 @@ + coverage ExpressionId(3) => Expression { lhs: Counter(3), op: Add, rhs: Counter(2) }; + coverage ExpressionId(4) => Expression { lhs: Expression(3), op: Add, rhs: Counter(1) }; + coverage ExpressionId(5) => Expression { lhs: Expression(4), op: Add, rhs: Expression(2) }; -+ coverage Code(Counter(0)) => $DIR/branch_match_arms.rs:14:1: 15:21 (#0); -+ coverage Code(Counter(3)) => $DIR/branch_match_arms.rs:16:17: 16:33 (#0); -+ coverage Code(Counter(2)) => $DIR/branch_match_arms.rs:17:17: 17:33 (#0); -+ coverage Code(Counter(1)) => $DIR/branch_match_arms.rs:18:17: 18:33 (#0); -+ coverage Code(Expression(2)) => $DIR/branch_match_arms.rs:19:17: 19:33 (#0); -+ coverage Code(Expression(5)) => $DIR/branch_match_arms.rs:21:2: 21:2 (#0); ++ coverage Code(Counter(0)) => 14:1 - 15:21; ++ coverage Code(Counter(3)) => 16:17 - 16:33; ++ coverage Code(Counter(2)) => 17:17 - 17:33; ++ coverage Code(Counter(1)) => 18:17 - 18:33; ++ coverage Code(Expression(2)) => 19:17 - 19:33; ++ coverage Code(Expression(5)) => 21:1 - 21:2; + bb0: { + Coverage::CounterIncrement(0); diff --git a/tests/mir-opt/coverage/instrument_coverage.bar.InstrumentCoverage.diff b/tests/mir-opt/coverage/instrument_coverage.bar.InstrumentCoverage.diff index 148ff86354b57..2efb1fd0a17a3 100644 --- a/tests/mir-opt/coverage/instrument_coverage.bar.InstrumentCoverage.diff +++ b/tests/mir-opt/coverage/instrument_coverage.bar.InstrumentCoverage.diff @@ -5,7 +5,7 @@ let mut _0: bool; + coverage body span: $DIR/instrument_coverage.rs:19:18: 21:2 (#0) -+ coverage Code(Counter(0)) => $DIR/instrument_coverage.rs:19:1: 21:2 (#0); ++ coverage Code(Counter(0)) => 19:1 - 21:2; + bb0: { + Coverage::CounterIncrement(0); diff --git a/tests/mir-opt/coverage/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/coverage/instrument_coverage.main.InstrumentCoverage.diff index b480d1ac13a76..a179824d6c736 100644 --- a/tests/mir-opt/coverage/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/coverage/instrument_coverage.main.InstrumentCoverage.diff @@ -9,11 +9,11 @@ + coverage body span: $DIR/instrument_coverage.rs:10:11: 16:2 (#0) + coverage ExpressionId(0) => Expression { lhs: Counter(0), op: Add, rhs: Counter(1) }; -+ coverage Code(Counter(0)) => $DIR/instrument_coverage.rs:10:1: 10:11 (#0); -+ coverage Code(Expression(0)) => $DIR/instrument_coverage.rs:12:12: 12:17 (#0); -+ coverage Code(Counter(0)) => $DIR/instrument_coverage.rs:13:13: 13:18 (#0); -+ coverage Code(Counter(1)) => $DIR/instrument_coverage.rs:14:10: 14:10 (#0); -+ coverage Code(Counter(0)) => $DIR/instrument_coverage.rs:16:2: 16:2 (#0); ++ coverage Code(Counter(0)) => 10:1 - 10:11; ++ coverage Code(Expression(0)) => 12:12 - 12:17; ++ coverage Code(Counter(0)) => 13:13 - 13:18; ++ coverage Code(Counter(1)) => 14:9 - 14:10; ++ coverage Code(Counter(0)) => 16:1 - 16:2; + bb0: { + Coverage::CounterIncrement(0); diff --git a/tests/mir-opt/coverage/instrument_coverage_cleanup.main.CleanupPostBorrowck.diff b/tests/mir-opt/coverage/instrument_coverage_cleanup.main.CleanupPostBorrowck.diff index 2c7ec6e85eb0a..082539369f707 100644 --- a/tests/mir-opt/coverage/instrument_coverage_cleanup.main.CleanupPostBorrowck.diff +++ b/tests/mir-opt/coverage/instrument_coverage_cleanup.main.CleanupPostBorrowck.diff @@ -9,11 +9,11 @@ coverage body span: $DIR/instrument_coverage_cleanup.rs:13:11: 15:2 (#0) coverage ExpressionId(0) => Expression { lhs: Counter(0), op: Subtract, rhs: Counter(1) }; - coverage Code(Counter(0)) => $DIR/instrument_coverage_cleanup.rs:13:1: 14:36 (#0); - coverage Code(Expression(0)) => $DIR/instrument_coverage_cleanup.rs:14:37: 14:39 (#0); - coverage Code(Counter(1)) => $DIR/instrument_coverage_cleanup.rs:14:39: 14:39 (#0); - coverage Code(Counter(0)) => $DIR/instrument_coverage_cleanup.rs:15:2: 15:2 (#0); - coverage Branch { true_term: Expression(0), false_term: Counter(1) } => $DIR/instrument_coverage_cleanup.rs:14:8: 14:36 (#0); + coverage Code(Counter(0)) => 13:1 - 14:36; + coverage Code(Expression(0)) => 14:37 - 14:39; + coverage Code(Counter(1)) => 14:38 - 14:39; + coverage Code(Counter(0)) => 15:1 - 15:2; + coverage Branch { true_term: Expression(0), false_term: Counter(1) } => 14:8 - 14:36; bb0: { Coverage::CounterIncrement(0); diff --git a/tests/mir-opt/coverage/instrument_coverage_cleanup.main.InstrumentCoverage.diff b/tests/mir-opt/coverage/instrument_coverage_cleanup.main.InstrumentCoverage.diff index c08265eb0e953..8635818c6a7a1 100644 --- a/tests/mir-opt/coverage/instrument_coverage_cleanup.main.InstrumentCoverage.diff +++ b/tests/mir-opt/coverage/instrument_coverage_cleanup.main.InstrumentCoverage.diff @@ -9,11 +9,11 @@ + coverage body span: $DIR/instrument_coverage_cleanup.rs:13:11: 15:2 (#0) + coverage ExpressionId(0) => Expression { lhs: Counter(0), op: Subtract, rhs: Counter(1) }; -+ coverage Code(Counter(0)) => $DIR/instrument_coverage_cleanup.rs:13:1: 14:36 (#0); -+ coverage Code(Expression(0)) => $DIR/instrument_coverage_cleanup.rs:14:37: 14:39 (#0); -+ coverage Code(Counter(1)) => $DIR/instrument_coverage_cleanup.rs:14:39: 14:39 (#0); -+ coverage Code(Counter(0)) => $DIR/instrument_coverage_cleanup.rs:15:2: 15:2 (#0); -+ coverage Branch { true_term: Expression(0), false_term: Counter(1) } => $DIR/instrument_coverage_cleanup.rs:14:8: 14:36 (#0); ++ coverage Code(Counter(0)) => 13:1 - 14:36; ++ coverage Code(Expression(0)) => 14:37 - 14:39; ++ coverage Code(Counter(1)) => 14:38 - 14:39; ++ coverage Code(Counter(0)) => 15:1 - 15:2; ++ coverage Branch { true_term: Expression(0), false_term: Counter(1) } => 14:8 - 14:36; + bb0: { + Coverage::CounterIncrement(0);