coverage: Give the instrumentor its own counter type, separate from MIR #114791
Conversation
|
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
rustbot
added
the
A-code-coverage
|
This is one step in my larger coverage refactoring ambitions described at rust-lang/compiler-team#645. |
Zalathar
force-pushed
the
bcb-counter
branch
5 times, most recently
from
221d4d3 to
8b93430
Compare
This splits off `BcbCounter` from MIR's `CoverageKind`, allowing the two types to evolve in different directions as necessary.
This shows one small benefit of separating `BcbCounter` from `CoverageKind`. The function source hash will be the same for all counters within a function, so instead of passing it through `CoverageCounters` and storing it in every counter, we can just supply it during the final conversion to `CoverageKind`.
| source_map.span_to_diagnostic_string(span), | ||
| source_map.span_to_diagnostic_string(body_span) | ||
| ); | ||
|
|
There was a problem hiding this comment.
You can use #[instrument(level="debug", skip(source_map))] attribute macro.
There was a problem hiding this comment.
Note that this isn't new code; I'm just moving it here as-is so that it's out of the way of other changes to inject_coverage_span_counters (which is the only caller).
| /// Tracks which BCBs have a counter associated with some incoming edge. | ||
| /// Only used by debug assertions, to verify that BCBs with incoming edge | ||
| /// counters do not have their own physical counters (expressions are allowed). | ||
| bcb_has_incoming_edge_counters: BitSet<BasicCoverageBlock>, | ||
| /// 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>, | ||
| /// These are always [`BcbCounter::Expression`]. |
There was a problem hiding this comment.
If they always are the same variant, should they get a specific struct?
There was a problem hiding this comment.
Yes they should, and I've tried to make that change in the past, but it ends up being pretty fiddly because of how the existing code interacts with counters/expressions.
For this PR I wanted to keep things mostly simple and mechanical, and treat BcbCounter as a drop-in replacement for CoverageKind, without disturbing other code too much.
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (0510a15): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 637.88s -> 636.195s (-0.26%) |
Within the MIR representation of coverage data,
CoverageKindis an important part ofStatementKind::Coverage, but theInstrumentCoveragepass also uses it heavily as an internal data structure. This means that any change toCoverageKindalso needs to update all of the internal parts ofInstrumentCoveragethat manipulate it directly, making the MIR representation difficult to modify.This change fixes that by giving the instrumentor its own
BcbCountertype for internal use, which is then converted to aCoverageKindwhen injecting coverage information into MIR.The main change is mostly mechanical, because the initial
BcbCounteris drop-in compatible withCoverageKind, minus the unnecessaryCoverageKind::Unreachablevariant.I've then removed the
function_source_hashfield fromBcbCounter::Counter, as a small example of how the two types can now usefully differ from each other. Every counter in a MIR-level function should have the same source hash, so we can supply the hash during the conversion toCoverageKind::Counterinstead.Background: BCB stands for “basic coverage block”, which is a node in the simplified control-flow graph used by coverage instrumentation. The instrumentor pass uses the function's actual MIR control-flow graph to build a simplified BCB graph, then assigns coverage counters and counter expressions to various nodes/edges in that simplified graph, and then finally injects corresponding coverage information into the underlying MIR.