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

Merged
merged 3 commits into from
Aug 13, 2023

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Aug 2, 2023

When deciding how to instrument the underlying MIR for coverage, the InstrumentCoverage pass builds a simplified “Basic Counter Block” graph, and then allocates coverage counters/expressions to various nodes/edges in the BCB graph as necessary. Those counters/expressions are then injected into the function's MIR.

The awkward thing here is that the code for doing this needs &mut access to the graph, in order to associate coverage info with individual nodes, even though it isn't making any structural changes to the graph itself. That makes it harder to understand and modify the instrumentation code.

In addition, the graph alone can't hold all the information that is needed. There ends up being an extra vector of “intermediate expressions” that needs to be passed around separately anyway.


This PR simplifies things by instead storing all of that temporary coverage information in a number of side-tables inside CoverageCounters.

This makes it easier to see all of the information produced by the make-counters step, and how it is used by the inject-into-mir step.


Looking at the combined changes is possible, but I recommend reviewing the commits individually, because the big changes are mostly independent of each other (despite being conceptually related).

@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2023

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 2, 2023
@Zalathar
Copy link
Contributor Author

Zalathar commented Aug 2, 2023

This is one step in my larger coverage refactoring ambitions described at rust-lang/compiler-team#645.

@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@Zalathar
Copy link
Contributor Author

Zalathar commented Aug 2, 2023

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Aug 2, 2023
@Zalathar Zalathar changed the title coverage: Accumulate counter info externally, not directly in the BCB graph coverage: Store BCB counter info externally, not directly in the BCB graph Aug 2, 2023
@Zalathar Zalathar force-pushed the external-counters branch 2 times, most recently from ae30452 to b7488e7 Compare August 3, 2023 01:44
Comment on lines +26 to +28
/// Coverage counters/expressions that are associated with the control-flow
/// edge between two BCBs.
bcb_edge_counters: FxHashMap<(BasicCoverageBlock, BasicCoverageBlock), CoverageKind>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the old code, each BCB node had its own hashmap of incoming edge counters, indexed by the node that those edges come from.

I've changed that to a single hashmap whose keys are pairs of BCBs, representing an edge.

For most of the code, that change makes no big difference. But it does mean that it's no longer possible to quickly check whether a BCB node has any incoming edge counters, which is something that some debug assertions were relying on. To solve that, I've also added a separate bit set that tracks which BCBs have at least one incoming edge counter.

Comment on lines +134 to +138
fn set_bcb_counter(
&mut self,
bcb: BasicCoverageBlock,
counter_kind: CoverageKind,
) -> Result<Operand, Error> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is mostly a direct copy of the old BasicCoverageBlockData::set_counter, which was moved here from graph.rs.

Comment on lines +157 to +162
fn set_bcb_edge_counter(
&mut self,
from_bcb: BasicCoverageBlock,
to_bcb: BasicCoverageBlock,
counter_kind: CoverageKind,
) -> Result<Operand, Error> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is mostly a direct copy of the old BasicCoverageBlockData::set_edge_counter, which was moved here from graph.rs.

Comment on lines +346 to 354
for ((from_bcb, target_bcb), counter_kind) in
self.coverage_counters.drain_bcb_edge_counters()
{
bcb_counters_without_direct_coverage_spans.push((
Some(from_bcb),
target_bcb,
counter_kind,
));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because edge counters are now stored in a single hashmap (instead of one per BCB node), iterating over all of them is now its own top-level loop, rather than being nested in the per-BCB loop.

@Zalathar Zalathar force-pushed the external-counters branch from b7488e7 to cd8955a Compare August 4, 2023 01:03
@Zalathar Zalathar force-pushed the external-counters branch 3 times, most recently from 19ec657 to 96450ed Compare August 12, 2023 02:26
This avoids confusion with data structures that actually hold BCB counter
information.
This avoids the need to pass around a separate vector to accumulate into, and
avoids the need to create a fake empty vector when failure occurs.
Storing coverage counter information in `CoverageCounters` has a few advantages
over storing it directly inside BCB graph nodes:

- The graph doesn't need to be mutable when making the counters, making it
easier to see that the graph itself is not modified during this step.

- All of the counter data is clearly visible in one place.

- It becomes possible to use a representation that doesn't correspond 1:1 to
graph nodes, e.g. storing all the edge counters in a single hashmap instead of
several.
@@ -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.

@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 13, 2023

📌 Commit 5ca30c4 has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2023
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#94667 (Add `Iterator::map_windows`)
 - rust-lang#114069 (Allow using external builds of the compiler-rt profile lib)
 - rust-lang#114354 (coverage: Store BCB counter info externally, not directly in the BCB graph)
 - rust-lang#114625 (CI: use smaller machines in PR runs)
 - rust-lang#114777 (Migrate GUI colors test to original CSS color format)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2442c9b into rust-lang:master Aug 13, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 13, 2023
@bors
Copy link
Contributor

bors commented Aug 13, 2023

⌛ Testing commit 5ca30c4 with merge 1b198b3...

@Zalathar Zalathar deleted the external-counters branch August 13, 2023 23:39
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 12, 2023
Having to keep passing in a graph reference was a holdover from when the graph
was partly mutated during traversal. As of rust-lang#114354 that is no longer necessary,
so we can simplify the traversal code by storing a graph reference as a field
in `TraverseCoverageGraphWithLoops`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants