Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage: Add enums to accommodate other kinds of coverage mappings #119842

Merged
merged 3 commits into from
Jan 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use rustc_middle::mir::coverage::{CounterId, CovTerm, ExpressionId};
use rustc_middle::mir::coverage::{CodeRegion, CounterId, CovTerm, ExpressionId, MappingKind};

/// Must match the layout of `LLVMRustCounterKind`.
#[derive(Copy, Clone, Debug)]
Expand Down Expand Up @@ -149,6 +149,24 @@ pub struct CounterMappingRegion {
}

impl CounterMappingRegion {
pub(crate) fn from_mapping(
mapping_kind: &MappingKind,
local_file_id: u32,
code_region: &CodeRegion,
) -> Self {
let &CodeRegion { file_name: _, start_line, start_col, end_line, end_col } = code_region;
match *mapping_kind {
MappingKind::Code(term) => Self::code_region(
Counter::from_term(term),
local_file_id,
start_line,
start_col,
end_line,
end_col,
),
}
}

pub(crate) fn code_region(
counter: Counter,
file_id: u32,
Expand Down
26 changes: 14 additions & 12 deletions compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ use rustc_data_structures::captures::Captures;
use rustc_data_structures::fx::FxIndexSet;
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::coverage::{
CodeRegion, CounterId, CovTerm, Expression, ExpressionId, FunctionCoverageInfo, Mapping, Op,
CodeRegion, CounterId, CovTerm, Expression, ExpressionId, FunctionCoverageInfo, Mapping,
MappingKind, Op,
};
use rustc_middle::ty::Instance;
use rustc_span::Symbol;
Expand Down Expand Up @@ -64,8 +65,8 @@ impl<'tcx> FunctionCoverageCollector<'tcx> {
// For each expression ID that is directly used by one or more mappings,
// mark it as not-yet-seen. This indicates that we expect to see a
// corresponding `ExpressionUsed` statement during MIR traversal.
for Mapping { term, .. } in &function_coverage_info.mappings {
if let &CovTerm::Expression(id) = term {
for term in function_coverage_info.mappings.iter().flat_map(|m| m.kind.terms()) {
if let CovTerm::Expression(id) = term {
expressions_seen.remove(id);
}
}
Expand Down Expand Up @@ -221,20 +222,21 @@ impl<'tcx> FunctionCoverage<'tcx> {
/// that will be used by `mapgen` when preparing for FFI.
pub(crate) fn counter_regions(
&self,
) -> impl Iterator<Item = (Counter, &CodeRegion)> + ExactSizeIterator {
) -> impl Iterator<Item = (MappingKind, &CodeRegion)> + ExactSizeIterator {
self.function_coverage_info.mappings.iter().map(move |mapping| {
let &Mapping { term, ref code_region } = mapping;
let counter = self.counter_for_term(term);
(counter, code_region)
let Mapping { kind, code_region } = mapping;
let kind =
kind.map_terms(|term| if self.is_zero_term(term) { CovTerm::Zero } else { term });
(kind, code_region)
})
}

fn counter_for_term(&self, term: CovTerm) -> Counter {
if is_zero_term(&self.counters_seen, &self.zero_expressions, term) {
Counter::ZERO
} else {
Counter::from_term(term)
}
if self.is_zero_term(term) { Counter::ZERO } else { Counter::from_term(term) }
}

fn is_zero_term(&self, term: CovTerm) -> bool {
is_zero_term(&self.counters_seen, &self.zero_expressions, term)
}
}

Expand Down
18 changes: 6 additions & 12 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use rustc_hir::def_id::DefId;
use rustc_index::IndexVec;
use rustc_middle::bug;
use rustc_middle::mir;
use rustc_middle::mir::coverage::CodeRegion;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::def_id::DefIdSet;
use rustc_span::Symbol;
Expand Down Expand Up @@ -237,7 +236,7 @@ fn encode_mappings_for_function(
// 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(|(_counter, region)| region.file_name)
&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);
Expand All @@ -248,17 +247,12 @@ fn encode_mappings_for_function(

// 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,
for (mapping_kind, region) in counter_regions_for_file {
debug!("Adding counter {mapping_kind:?} to map for {region:?}");
mapping_regions.push(CounterMappingRegion::from_mapping(
&mapping_kind,
local_file_id.as_u32(),
start_line,
start_col,
end_line,
end_col,
region,
));
}
}
Expand Down
34 changes: 26 additions & 8 deletions compiler/rustc_middle/src/mir/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,18 +158,36 @@ pub struct Expression {
pub rhs: CovTerm,
}

#[derive(Clone, Debug)]
#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
pub enum MappingKind {
/// Associates a normal region of code with a counter/expression/zero.
Code(CovTerm),
}

impl MappingKind {
/// Iterator over all coverage terms in this mapping kind.
pub fn terms(&self) -> impl Iterator<Item = CovTerm> {
let one = |a| std::iter::once(a);
match *self {
Self::Code(term) => one(term),
}
Comment on lines +171 to +174
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work with variants that have multiple terms unless all of them are slices and you use https://doc.rust-lang.org/std/slice/fn.from_ref.html here (in which case you could proabably return a slice instead of an iterator)

But also not important, you can refactor to make it work when you add more variants

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the variants supported by LLVM have exactly zero, one, or two terms.

So my plan is to eventually change this definition to something like:

let one = |a| [Some(a), None].into_iter().flatten()

That way the individual arms that use one won't have to change.

(But I won't bother doing this until I actually add a variant that requires 0 or 2 terms.)

}

/// Returns a copy of this mapping kind, in which all coverage terms have
/// been replaced with ones returned by the given function.
pub fn map_terms(&self, map_fn: impl Fn(CovTerm) -> CovTerm) -> Self {
match *self {
Self::Code(term) => Self::Code(map_fn(term)),
}
}
}

#[derive(Clone, Debug)]
#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
pub struct Mapping {
pub kind: MappingKind,
pub code_region: CodeRegion,

/// Indicates whether this mapping uses a counter value, expression value,
/// or zero value.
///
/// FIXME: When we add support for mapping kinds other than `Code`
/// (e.g. branch regions, expansion regions), replace this with a dedicated
/// mapping-kind enum.
pub term: CovTerm,
}

/// Stores per-function coverage information attached to a `mir::Body`,
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -493,8 +493,8 @@ fn write_function_coverage_info(
for (id, expression) in expressions.iter_enumerated() {
writeln!(w, "{INDENT}coverage {id:?} => {expression:?};")?;
}
for coverage::Mapping { term, code_region } in mappings {
writeln!(w, "{INDENT}coverage {term:?} => {code_region:?};")?;
for coverage::Mapping { kind, code_region } in mappings {
writeln!(w, "{INDENT}coverage {kind:?} => {code_region:?};")?;
}
writeln!(w)?;

Expand Down
10 changes: 6 additions & 4 deletions compiler/rustc_mir_transform/src/coverage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ mod tests;

use self::counters::{BcbCounter, CoverageCounters};
use self::graph::{BasicCoverageBlock, CoverageGraph};
use self::spans::{BcbMapping, CoverageSpans};
use self::spans::{BcbMapping, BcbMappingKind, CoverageSpans};

use crate::MirPass;

Expand Down Expand Up @@ -150,10 +150,12 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {

coverage_spans
.all_bcb_mappings()
.filter_map(|&BcbMapping { bcb, span }| {
let term = term_for_bcb(bcb);
.filter_map(|&BcbMapping { kind: bcb_mapping_kind, span }| {
let kind = match bcb_mapping_kind {
BcbMappingKind::Code(bcb) => MappingKind::Code(term_for_bcb(bcb)),
};
let code_region = make_code_region(source_map, file_name, span, body_span)?;
Some(Mapping { term, code_region })
Some(Mapping { kind, code_region })
})
.collect::<Vec<_>>()
}
Expand Down
17 changes: 14 additions & 3 deletions compiler/rustc_mir_transform/src/coverage/spans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,15 @@ use crate::coverage::ExtractedHirInfo;

mod from_mir;

#[derive(Clone, Copy, Debug)]
pub(super) enum BcbMappingKind {
/// Associates an ordinary executable code span with its corresponding BCB.
Code(BasicCoverageBlock),
}

#[derive(Debug)]
pub(super) struct BcbMapping {
pub(super) bcb: BasicCoverageBlock,
pub(super) kind: BcbMappingKind,
pub(super) span: Span,
}

Expand Down Expand Up @@ -38,7 +44,7 @@ impl CoverageSpans {
);
mappings.extend(coverage_spans.into_iter().map(|CoverageSpan { bcb, span, .. }| {
// Each span produced by the generator represents an ordinary code region.
BcbMapping { bcb, span }
BcbMapping { kind: BcbMappingKind::Code(bcb), span }
}));

if mappings.is_empty() {
Expand All @@ -47,8 +53,13 @@ impl CoverageSpans {

// Identify which BCBs have one or more mappings.
let mut bcb_has_mappings = BitSet::new_empty(basic_coverage_blocks.num_nodes());
for &BcbMapping { bcb, span: _ } in &mappings {
let mut insert = |bcb| {
bcb_has_mappings.insert(bcb);
};
for &BcbMapping { kind, span: _ } in &mappings {
match kind {
BcbMappingKind::Code(bcb) => insert(bcb),
}
}

Some(Self { bcb_has_mappings, mappings })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
fn bar() -> bool {
let mut _0: bool;

+ coverage Counter(0) => /the/src/instrument_coverage.rs:21:1 - 23:2;
+ coverage Code(Counter(0)) => /the/src/instrument_coverage.rs:21:1 - 23:2;
+
bb0: {
+ Coverage::CounterIncrement(0);
Expand Down
10 changes: 5 additions & 5 deletions tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@

+ coverage ExpressionId(0) => Expression { lhs: Counter(0), op: Add, rhs: Counter(1) };
+ coverage ExpressionId(1) => Expression { lhs: Expression(0), op: Subtract, rhs: Counter(1) };
+ coverage Counter(0) => /the/src/instrument_coverage.rs:12:1 - 12:11;
+ coverage Expression(0) => /the/src/instrument_coverage.rs:13:5 - 14:17;
+ coverage Expression(1) => /the/src/instrument_coverage.rs:15:13 - 15:18;
+ coverage Expression(1) => /the/src/instrument_coverage.rs:18:1 - 18:2;
+ coverage Counter(1) => /the/src/instrument_coverage.rs:16:10 - 16:11;
+ coverage Code(Counter(0)) => /the/src/instrument_coverage.rs:12:1 - 12:11;
+ coverage Code(Expression(0)) => /the/src/instrument_coverage.rs:13:5 - 14:17;
+ coverage Code(Expression(1)) => /the/src/instrument_coverage.rs:15:13 - 15:18;
+ coverage Code(Counter(1)) => /the/src/instrument_coverage.rs:16:10 - 16:11;
+ coverage Code(Expression(1)) => /the/src/instrument_coverage.rs:18:1 - 18:2;
+
bb0: {
+ Coverage::CounterIncrement(0);
Expand Down
Loading