Skip to content

Commit

Permalink
Unrolled build for rust-lang#132675
Browse files Browse the repository at this point in the history
Rollup merge of rust-lang#132675 - Zalathar:empty-spans, r=jieyouxu

coverage: Restrict empty-span expansion to only cover `{` and `}`

Coverage instrumentation has some tricky code for converting a coverage-relevant `Span` into a set of start/end line/byte-column coordinates that will be embedded in the CGU's coverage metadata.

A big part of this complexity is special code for handling empty spans, which are expanded into non-empty spans (if possible) because LLVM's coverage reporter does not handle empty spans well.

This PR simplifies that code by restricting it to only apply in two specific situations: when the character after the empty span is `{`, or the character before the empty span is `}`.

(As an added benefit, this means that the expanded spans no longer extend awkwardly beyond the end of a physical line, which was common under the previous implementation.)

Along the way, this PR also removes some unhelpful code for dealing with function source code spread across multiple files. Functions currently can't have coverage spans in multiple files, and if that ever changes (e.g. to properly support expansion regions) then this code will need to be completely overhauled anyway.
  • Loading branch information
rust-timer authored Nov 10, 2024
2 parents f5b6257 + 925dfc8 commit ca31652
Show file tree
Hide file tree
Showing 57 changed files with 320 additions and 305 deletions.
10 changes: 8 additions & 2 deletions compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use rustc_middle::mir::coverage::{CounterId, CovTerm, ExpressionId, SourceRegion};

use crate::coverageinfo::mapgen::LocalFileId;

/// Must match the layout of `LLVMRustCounterKind`.
#[derive(Copy, Clone, Debug)]
#[repr(C)]
Expand Down Expand Up @@ -137,8 +139,12 @@ pub(crate) struct CoverageSpan {
}

impl CoverageSpan {
pub(crate) fn from_source_region(file_id: u32, code_region: &SourceRegion) -> Self {
let &SourceRegion { file_name: _, start_line, start_col, end_line, end_col } = code_region;
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}");
Expand Down
8 changes: 1 addition & 7 deletions compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use rustc_middle::mir::coverage::{
SourceRegion,
};
use rustc_middle::ty::Instance;
use rustc_span::Symbol;
use tracing::{debug, instrument};

use crate::coverageinfo::ffi::{Counter, CounterExpression, ExprKind};
Expand Down Expand Up @@ -180,7 +179,7 @@ impl<'tcx> FunctionCoverageCollector<'tcx> {
}

pub(crate) struct FunctionCoverage<'tcx> {
function_coverage_info: &'tcx FunctionCoverageInfo,
pub(crate) function_coverage_info: &'tcx FunctionCoverageInfo,
is_used: bool,

counters_seen: BitSet<CounterId>,
Expand All @@ -199,11 +198,6 @@ 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<Item = Symbol> + Captures<'_> {
self.function_coverage_info.mappings.iter().map(|mapping| mapping.source_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(
Expand Down
130 changes: 72 additions & 58 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ use rustc_index::IndexVec;
use rustc_middle::mir::coverage::MappingKind;
use rustc_middle::ty::{self, TyCtxt};
use rustc_middle::{bug, mir};
use rustc_span::Symbol;
use rustc_session::RemapFileNameExt;
use rustc_session::config::RemapPathScopeComponents;
use rustc_span::def_id::DefIdSet;
use rustc_span::{Span, Symbol};
use rustc_target::spec::HasTargetSpec;
use tracing::debug;

Expand Down Expand Up @@ -70,8 +72,10 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) {
.map(|(instance, function_coverage)| (instance, function_coverage.into_finished()))
.collect::<Vec<_>>();

let all_file_names =
function_coverage_entries.iter().flat_map(|(_, fn_cov)| fn_cov.all_file_names());
let all_file_names = function_coverage_entries
.iter()
.map(|(_, fn_cov)| fn_cov.function_coverage_info.body_span)
.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.
Expand All @@ -96,7 +100,7 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) {
let is_used = function_coverage.is_used();

let coverage_mapping_buffer =
encode_mappings_for_function(&global_file_table, &function_coverage);
encode_mappings_for_function(tcx, &global_file_table, &function_coverage);

if coverage_mapping_buffer.is_empty() {
if function_coverage.is_used() {
Expand Down Expand Up @@ -164,13 +168,13 @@ impl GlobalFileTable {
Self { raw_file_table }
}

fn global_file_id_for_file_name(&self, file_name: Symbol) -> u32 {
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.
(raw_id + 1) as u32
GlobalFileId::from_usize(raw_id + 1)
}

fn make_filenames_buffer(&self, tcx: TyCtxt<'_>) -> Vec<u8> {
Expand All @@ -196,36 +200,54 @@ impl GlobalFileTable {
}

rustc_index::newtype_index! {
struct LocalFileId {}
/// An index into the CGU's overall list of file paths. The underlying paths
/// will be embedded in the `__llvm_covmap` linker section.
struct GlobalFileId {}
}
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.
pub(crate) struct LocalFileId {}
}

/// Holds a mapping from "local" (per-function) file IDs to "global" (per-CGU)
/// file IDs.
#[derive(Default)]
struct VirtualFileMapping {
local_to_global: IndexVec<LocalFileId, u32>,
global_to_local: FxIndexMap<u32, LocalFileId>,
local_to_global: IndexVec<LocalFileId, GlobalFileId>,
global_to_local: FxIndexMap<GlobalFileId, LocalFileId>,
}

impl VirtualFileMapping {
fn local_id_for_global(&mut self, global_file_id: u32) -> LocalFileId {
fn local_id_for_global(&mut self, global_file_id: GlobalFileId) -> LocalFileId {
*self
.global_to_local
.entry(global_file_id)
.or_insert_with(|| self.local_to_global.push(global_file_id))
}

fn into_vec(self) -> Vec<u32> {
self.local_to_global.raw
// This conversion should be optimized away to ~zero overhead.
// In any case, it's probably not hot enough to worry about.
self.local_to_global.into_iter().map(|global| global.as_u32()).collect()
}
}

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.
///
/// Newly-encountered filenames will be added to the global file table.
fn encode_mappings_for_function(
tcx: TyCtxt<'_>,
global_file_table: &GlobalFileTable,
function_coverage: &FunctionCoverage<'_>,
) -> Vec<u8> {
Expand All @@ -242,53 +264,45 @@ fn encode_mappings_for_function(
let mut mcdc_branch_regions = vec![];
let mut mcdc_decision_regions = vec![];

// Group mappings into runs with the same filename, preserving the order
// yielded by `FunctionCoverage`.
// Prepare file IDs for each filename, and prepare the mapping data so that
// we can pass it through FFI to LLVM.
for (file_name, counter_regions_for_file) in
&counter_regions.group_by(|(_, region)| region.file_name)
{
// Look up the global file ID for this 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 {global_file_id} = '{file_name:?}'");

// For each counter/region pair in this function+file, convert it to a
// form suitable for FFI.
for (mapping_kind, region) in counter_regions_for_file {
debug!("Adding counter {mapping_kind:?} to map for {region:?}");
let span = ffi::CoverageSpan::from_source_region(local_file_id.as_u32(), region);
match mapping_kind {
MappingKind::Code(term) => {
code_regions
.push(ffi::CodeRegion { span, counter: ffi::Counter::from_term(term) });
}
MappingKind::Branch { true_term, false_term } => {
branch_regions.push(ffi::BranchRegion {
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 {
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),
});
}
MappingKind::MCDCDecision(mcdc_decision_params) => {
mcdc_decision_regions.push(ffi::MCDCDecisionRegion {
span,
mcdc_decision_params: ffi::mcdc::DecisionParameters::from(
mcdc_decision_params,
),
});
}
// Currently a function's mappings must all be in the same file as its body span.
let file_name = span_file_name(tcx, function_coverage.function_coverage_info.body_span);

// 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:?}'");

// For each counter/region pair in this function+file, convert it to a
// form suitable for FFI.
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 { span, counter: ffi::Counter::from_term(term) });
}
MappingKind::Branch { true_term, false_term } => {
branch_regions.push(ffi::BranchRegion {
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 {
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),
});
}
MappingKind::MCDCDecision(mcdc_decision_params) => {
mcdc_decision_regions.push(ffi::MCDCDecisionRegion {
span,
mcdc_decision_params: ffi::mcdc::DecisionParameters::from(mcdc_decision_params),
});
}
}
}
Expand Down
11 changes: 4 additions & 7 deletions compiler/rustc_middle/src/mir/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::fmt::{self, Debug, Formatter};

use rustc_index::IndexVec;
use rustc_macros::{HashStable, TyDecodable, TyEncodable, TypeFoldable, TypeVisitable};
use rustc_span::{Span, Symbol};
use rustc_span::Span;

rustc_index::newtype_index! {
/// Used by [`CoverageKind::BlockMarker`] to mark blocks during THIR-to-MIR
Expand Down Expand Up @@ -158,7 +158,6 @@ impl Debug for CoverageKind {
#[derive(Clone, TyEncodable, TyDecodable, Hash, HashStable, PartialEq, Eq, PartialOrd, Ord)]
#[derive(TypeFoldable, TypeVisitable)]
pub struct SourceRegion {
pub file_name: Symbol,
pub start_line: u32,
pub start_col: u32,
pub end_line: u32,
Expand All @@ -167,11 +166,8 @@ pub struct SourceRegion {

impl Debug for SourceRegion {
fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
write!(
fmt,
"{}:{}:{} - {}:{}",
self.file_name, self.start_line, self.start_col, self.end_line, self.end_col
)
let &Self { start_line, start_col, end_line, end_col } = self;
write!(fmt, "{start_line}:{start_col} - {end_line}:{end_col}")
}
}

Expand Down Expand Up @@ -246,6 +242,7 @@ pub struct Mapping {
#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
pub struct FunctionCoverageInfo {
pub function_source_hash: u64,
pub body_span: Span,
pub num_counters: usize,
pub mcdc_bitmap_bits: usize,
pub expressions: IndexVec<ExpressionId, Expression>,
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,8 +596,10 @@ fn write_function_coverage_info(
function_coverage_info: &coverage::FunctionCoverageInfo,
w: &mut dyn io::Write,
) -> io::Result<()> {
let coverage::FunctionCoverageInfo { expressions, mappings, .. } = function_coverage_info;
let coverage::FunctionCoverageInfo { body_span, expressions, mappings, .. } =
function_coverage_info;

writeln!(w, "{INDENT}coverage body span: {body_span:?}")?;
for (id, expression) in expressions.iter_enumerated() {
writeln!(w, "{INDENT}coverage {id:?} => {expression:?};")?;
}
Expand Down
Loading

0 comments on commit ca31652

Please sign in to comment.