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.

Sign up for GitHub

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: Store BCB counter info externally, not directly in the BCB graph #114354

Merged
merged 3 commits into from
Aug 13, 2023
Merged

coverage: Store BCB counter info externally, not directly in the BCB graph #114354

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c74db79
Rename helper struct `BcbCounters` to `MakeBcbCounters`
Zalathar Jun 29, 2023
5302c9d
Accumulate intermediate expressions into `CoverageCounters`
Zalathar Jun 29, 2023
5ca30c4
Store BCB counters externally, not directly in the BCB graph
Zalathar Jun 29, 2023
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
73 changes: 23 additions & 50 deletions compiler/rustc_mir_transform/src/coverage/counters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ pub(super) struct CoverageCounters {
function_source_hash: u64,
next_counter_id: CounterId,
next_expression_id: ExpressionId,

/// Expression nodes that are not directly associated with any particular
/// BCB/edge, but are needed as operands to more complex expressions.
/// These are always `CoverageKind::Expression`.
pub(super) intermediate_expressions: Vec<CoverageKind>,

pub debug_counters: DebugCounters,
}

Expand All @@ -27,6 +33,9 @@ impl CoverageCounters {
function_source_hash,
next_counter_id: CounterId::START,
next_expression_id: ExpressionId::START,

intermediate_expressions: Vec::new(),

debug_counters: DebugCounters::new(),
}
}
Expand All @@ -38,13 +47,13 @@ impl CoverageCounters {
}

/// Makes `CoverageKind` `Counter`s and `Expressions` for the `BasicCoverageBlock`s directly or
/// indirectly associated with `CoverageSpans`, and returns additional `Expression`s
/// indirectly associated with `CoverageSpans`, and accumulates additional `Expression`s
/// representing intermediate values.
pub fn make_bcb_counters(
&mut self,
basic_coverage_blocks: &mut CoverageGraph,
coverage_spans: &[CoverageSpan],
) -> Result<Vec<CoverageKind>, Error> {
) -> Result<(), Error> {
MakeBcbCounters::new(self, basic_coverage_blocks).make_bcb_counters(coverage_spans)
}

Expand Down Expand Up @@ -134,13 +143,9 @@ impl<'a> MakeBcbCounters<'a> {
/// Returns any non-code-span expressions created to represent intermediate values (such as to
/// add two counters so the result can be subtracted from another counter), or an Error with
/// message for subsequent debugging.
fn make_bcb_counters(
&mut self,
coverage_spans: &[CoverageSpan],
) -> Result<Vec<CoverageKind>, Error> {
fn make_bcb_counters(&mut self, coverage_spans: &[CoverageSpan]) -> Result<(), Error> {
debug!("make_bcb_counters(): adding a counter or expression to each BasicCoverageBlock");
let num_bcbs = self.basic_coverage_blocks.num_nodes();
let mut collect_intermediate_expressions = Vec::with_capacity(num_bcbs);

let mut bcbs_with_coverage = BitSet::new_empty(num_bcbs);
for covspan in coverage_spans {
Expand All @@ -161,16 +166,10 @@ impl<'a> MakeBcbCounters<'a> {
while let Some(bcb) = traversal.next(self.basic_coverage_blocks) {
if bcbs_with_coverage.contains(bcb) {
debug!("{:?} has at least one `CoverageSpan`. Get or make its counter", bcb);
let branching_counter_operand =
self.get_or_make_counter_operand(bcb, &mut collect_intermediate_expressions)?;
let branching_counter_operand = self.get_or_make_counter_operand(bcb)?;

if self.bcb_needs_branch_counters(bcb) {
self.make_branch_counters(
&mut traversal,
bcb,
branching_counter_operand,
&mut collect_intermediate_expressions,
)?;
self.make_branch_counters(&mut traversal, bcb, branching_counter_operand)?;
}
} else {
debug!(
Expand All @@ -182,7 +181,7 @@ impl<'a> MakeBcbCounters<'a> {
}

if traversal.is_complete() {
Ok(collect_intermediate_expressions)
Ok(())
} else {
Error::from_string(format!(
"`TraverseCoverageGraphWithLoops` missed some `BasicCoverageBlock`s: {:?}",
Expand All @@ -196,7 +195,6 @@ impl<'a> MakeBcbCounters<'a> {
traversal: &mut TraverseCoverageGraphWithLoops,
branching_bcb: BasicCoverageBlock,
branching_counter_operand: Operand,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
) -> Result<(), Error> {
let branches = self.bcb_branches(branching_bcb);
debug!(
Expand Down Expand Up @@ -232,17 +230,10 @@ impl<'a> MakeBcbCounters<'a> {
counter",
branch, branching_bcb
);
self.get_or_make_counter_operand(
branch.target_bcb,
collect_intermediate_expressions,
)?
self.get_or_make_counter_operand(branch.target_bcb)?
} else {
debug!(" {:?} has multiple incoming edges, so adding an edge counter", branch);
self.get_or_make_edge_counter_operand(
branching_bcb,
branch.target_bcb,
collect_intermediate_expressions,
)?
self.get_or_make_edge_counter_operand(branching_bcb, branch.target_bcb)?
};
if let Some(sumup_counter_operand) =
some_sumup_counter_operand.replace(branch_counter_operand)
Expand All @@ -258,7 +249,7 @@ impl<'a> MakeBcbCounters<'a> {
self.format_counter(&intermediate_expression)
);
let intermediate_expression_operand = intermediate_expression.as_operand();
collect_intermediate_expressions.push(intermediate_expression);
self.coverage_counters.intermediate_expressions.push(intermediate_expression);
some_sumup_counter_operand.replace(intermediate_expression_operand);
}
}
Expand Down Expand Up @@ -290,18 +281,13 @@ impl<'a> MakeBcbCounters<'a> {
Ok(())
}

fn get_or_make_counter_operand(
&mut self,
bcb: BasicCoverageBlock,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
) -> Result<Operand, Error> {
self.recursive_get_or_make_counter_operand(bcb, collect_intermediate_expressions, 1)
fn get_or_make_counter_operand(&mut self, bcb: BasicCoverageBlock) -> Result<Operand, Error> {
self.recursive_get_or_make_counter_operand(bcb, 1)
}

fn recursive_get_or_make_counter_operand(
&mut self,
bcb: BasicCoverageBlock,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
debug_indent_level: usize,
) -> Result<Operand, Error> {
// If the BCB already has a counter, return it.
Expand Down Expand Up @@ -354,15 +340,13 @@ impl<'a> MakeBcbCounters<'a> {
let first_edge_counter_operand = self.recursive_get_or_make_edge_counter_operand(
predecessors.next().unwrap(),
bcb,
collect_intermediate_expressions,
debug_indent_level + 1,
)?;
let mut some_sumup_edge_counter_operand = None;
for predecessor in predecessors {
let edge_counter_operand = self.recursive_get_or_make_edge_counter_operand(
predecessor,
bcb,
collect_intermediate_expressions,
debug_indent_level + 1,
)?;
if let Some(sumup_edge_counter_operand) =
Expand All @@ -380,7 +364,7 @@ impl<'a> MakeBcbCounters<'a> {
self.format_counter(&intermediate_expression)
);
let intermediate_expression_operand = intermediate_expression.as_operand();
collect_intermediate_expressions.push(intermediate_expression);
self.coverage_counters.intermediate_expressions.push(intermediate_expression);
some_sumup_edge_counter_operand.replace(intermediate_expression_operand);
}
}
Expand All @@ -403,32 +387,21 @@ impl<'a> MakeBcbCounters<'a> {
&mut self,
from_bcb: BasicCoverageBlock,
to_bcb: BasicCoverageBlock,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
) -> Result<Operand, Error> {
self.recursive_get_or_make_edge_counter_operand(
from_bcb,
to_bcb,
collect_intermediate_expressions,
1,
)
self.recursive_get_or_make_edge_counter_operand(from_bcb, to_bcb, 1)
}

fn recursive_get_or_make_edge_counter_operand(
&mut self,
from_bcb: BasicCoverageBlock,
to_bcb: BasicCoverageBlock,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
debug_indent_level: usize,
) -> Result<Operand, Error> {
// If the source BCB has only one successor (assumed to be the given target), an edge
// counter is unnecessary. Just get or make a counter for the source BCB.
let successors = self.bcb_successors(from_bcb).iter();
if successors.len() == 1 {
return self.recursive_get_or_make_counter_operand(
from_bcb,
collect_intermediate_expressions,
debug_indent_level + 1,
);
return self.recursive_get_or_make_counter_operand(from_bcb, debug_indent_level + 1);
}

// If the edge already has a counter, return it.
Expand Down
81 changes: 38 additions & 43 deletions compiler/rustc_mir_transform/src/coverage/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,52 +199,47 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
// `BasicCoverageBlock`s not already associated with a `CoverageSpan`.
//
// Intermediate expressions (used to compute other `Expression` values), which have no
// direct associate to any `BasicCoverageBlock`, are returned in the method `Result`.
let intermediate_expressions_or_error = self
// direct association with any `BasicCoverageBlock`, are accumulated inside `coverage_counters`.
let result = self
.coverage_counters
.make_bcb_counters(&mut self.basic_coverage_blocks, &coverage_spans);

let (result, intermediate_expressions) = match intermediate_expressions_or_error {
Ok(intermediate_expressions) => {
// If debugging, add any intermediate expressions (which are not associated with any
// BCB) to the `debug_used_expressions` map.
if debug_used_expressions.is_enabled() {
for intermediate_expression in &intermediate_expressions {
debug_used_expressions.add_expression_operands(intermediate_expression);
}
if let Ok(()) = result {
// If debugging, add any intermediate expressions (which are not associated with any
// BCB) to the `debug_used_expressions` map.
if debug_used_expressions.is_enabled() {
for intermediate_expression in &self.coverage_counters.intermediate_expressions {
debug_used_expressions.add_expression_operands(intermediate_expression);
}

////////////////////////////////////////////////////
// Remove the counter or edge counter from of each `CoverageSpan`s associated
// `BasicCoverageBlock`, and inject a `Coverage` statement into the MIR.
//
// `Coverage` statements injected from `CoverageSpan`s will include the code regions
// (source code start and end positions) to be counted by the associated counter.
//
// These `CoverageSpan`-associated counters are removed from their associated
// `BasicCoverageBlock`s so that the only remaining counters in the `CoverageGraph`
// are indirect counters (to be injected next, without associated code regions).
self.inject_coverage_span_counters(
coverage_spans,
&mut graphviz_data,
&mut debug_used_expressions,
);

////////////////////////////////////////////////////
// For any remaining `BasicCoverageBlock` counters (that were not associated with
// any `CoverageSpan`), inject `Coverage` statements (_without_ code region `Span`s)
// to ensure `BasicCoverageBlock` counters that other `Expression`s may depend on
// are in fact counted, even though they don't directly contribute to counting
// their own independent code region's coverage.
self.inject_indirect_counters(&mut graphviz_data, &mut debug_used_expressions);

// Intermediate expressions will be injected as the final step, after generating
// debug output, if any.
////////////////////////////////////////////////////

(Ok(()), intermediate_expressions)
}
Err(e) => (Err(e), Vec::new()),

////////////////////////////////////////////////////
// Remove the counter or edge counter from of each `CoverageSpan`s associated
// `BasicCoverageBlock`, and inject a `Coverage` statement into the MIR.
//
// `Coverage` statements injected from `CoverageSpan`s will include the code regions
// (source code start and end positions) to be counted by the associated counter.
//
// These `CoverageSpan`-associated counters are removed from their associated
// `BasicCoverageBlock`s so that the only remaining counters in the `CoverageGraph`
// are indirect counters (to be injected next, without associated code regions).
self.inject_coverage_span_counters(
coverage_spans,
&mut graphviz_data,
&mut debug_used_expressions,
);

////////////////////////////////////////////////////
// For any remaining `BasicCoverageBlock` counters (that were not associated with
// any `CoverageSpan`), inject `Coverage` statements (_without_ code region `Span`s)
// to ensure `BasicCoverageBlock` counters that other `Expression`s may depend on
// are in fact counted, even though they don't directly contribute to counting
// their own independent code region's coverage.
self.inject_indirect_counters(&mut graphviz_data, &mut debug_used_expressions);

// Intermediate expressions will be injected as the final step, after generating
// debug output, if any.
////////////////////////////////////////////////////
};

if graphviz_data.is_enabled() {
Expand All @@ -257,7 +252,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
&self.basic_coverage_blocks,
&self.coverage_counters.debug_counters,
&graphviz_data,
&intermediate_expressions,
&self.coverage_counters.intermediate_expressions,
&debug_used_expressions,
);
}
Expand All @@ -273,7 +268,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {

////////////////////////////////////////////////////
// Finally, inject the intermediate expressions collected along the way.
for intermediate_expression in intermediate_expressions {
for intermediate_expression in self.coverage_counters.intermediate_expressions.drain(..) {
inject_intermediate_expression(self.mir_body, intermediate_expression);
}
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/coverage/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,10 +676,10 @@ fn test_make_bcb_counters() {
}
}
let mut coverage_counters = counters::CoverageCounters::new(0);
let intermediate_expressions = coverage_counters
let () = coverage_counters
Copy link
Member

Choose a reason for hiding this comment

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

nit: this let () = isn't needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I remember doing this on purpose to verify that the success type was (), but in hindsight it doesn't really add anything.

I have some more planned changes that touch this file, so I'll get rid of the let () = in one of those.

.make_bcb_counters(&mut basic_coverage_blocks, &coverage_spans)
.expect("should be Ok");
assert_eq!(intermediate_expressions.len(), 0);
assert_eq!(coverage_counters.intermediate_expressions.len(), 0);

let_bcb!(1);
assert_eq!(
Expand Down