coverage: Allow each coverage statement to have multiple code regions #115301

Zalathar · 2023-08-28T02:46:26Z

The original implementation of coverage instrumentation was built around the assumption that a coverage counter/expression would be associated with up to one code region. When it was discovered that multiple regions would sometimes need to share a counter, a workaround was found: for the remaining regions, the instrumentor would create a fresh expression that adds zero to the existing counter/expression.

That got the job done, but resulted in some awkward code, and produces unnecessarily complicated coverage maps in the final binary.

This PR removes that tension by changing StatementKind::Coverage's code region field from Option<CodeRegion> to Vec<CodeRegion>.

The changes on the codegen side are fairly straightforward. As long as each CoverageKind::Counter only injects one llvm.instrprof.increment, the rest of coverage codegen is happy to handle multiple regions mapped to the same counter/expression, with only minor option-to-vec adjustments.

On the instrumentor/mir-transform side, we can get rid of the code that creates extra (x + 0) expressions. Instead we gather all of the code regions associated with a single BCB, and inject them all into one coverage statement.

There are several patches here but they can be divided in to three phases:

Preparatory work
Actually switching over to multiple regions per coverage statement
Cleaning up

So viewing the patches individually may be easier.

rustbot · 2023-08-28T02:46:32Z

r? @davidtwco

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

rustbot · 2023-08-28T02:46:35Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2023-08-28T02:47:52Z

(Cargo.toml changes are part of #114843 and won't remain in the final version of this PR.)

Zalathar · 2023-08-28T02:51:00Z

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

-                    debug!(
-                        "adding counter to coverage_map: instance={:?}, id={:?}, region={:?}",
-                        instance, id, code_region,
-                    );


These debug! invocations have been replaced with #[instrument(...)] tracing on the methods being called.

Zalathar · 2023-08-28T02:54:26Z

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

-                    );
-                    func_coverage.add_counter(id, code_region);
-                }
+                func_coverage.add_counter(id, code_regions);


We now call add_counter unconditionally here, and leave the callee to decide how to handle counters with zero code regions.

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs

tests/coverage-map/status-quo/async.cov-map

davidtwco · 2023-08-28T08:36:25Z

~~There's still lots of discussion and changes happening here, use @rustbot ready when you've got something you're looking to have merged.~~ I mixed this up with one of the other coverage pull requests.

I'll review this once its dependencies land.

There is no need to include a dummy counter reference in the coverage mappings for an unused function.

By encapsulating the coverage spans in a struct, we can change the internal representation without disturbing existing call sites. This will be useful for grouping coverage spans by BCB. This patch includes some changes that were originally in rust-lang#115912, which avoid the need for a particular test to deal with coverage spans at all. (Comments/logs referring to `CoverageSpan` are updated in a subsequent patch.)

The concrete type `CoverageSpan` is no longer used outside of the `spans` module. This is a separate patch to avoid noise in the preceding patch that actually encapsulates coverage spans.

This makes it possible for a `StatementKind::Coverage` to hold more than one code region, but that capability is not yet used.

If a BCB has more than one code region, those extra regions can now all be stored in the same coverage statement, instead of being stored in additional statements.

Now that coverage statements can have multiple code regions attached to them, this code is never used.

When these methods were originally written, I wasn't aware that `newtype_index!` already supports addition with ordinary numbers, without needing to unwrap and re-wrap.

davidtwco

Apologies for the delay in getting to this, looks good to me.

davidtwco · 2023-10-03T10:32:20Z

@bors r+

bors · 2023-10-03T10:32:22Z

📌 Commit 053c4f9 has been approved by davidtwco

It is now in the queue for this repository.

bors · 2023-10-03T18:36:23Z

⌛ Testing commit 053c4f9 with merge 36aab8d...

bors · 2023-10-03T20:34:28Z

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 36aab8d to master...

rust-timer · 2023-10-03T22:34:34Z

Finished benchmarking commit (36aab8d): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.6%]	4
Improvements ✅ (secondary)	-0.4%	[-0.6%, -0.2%]	15
All ❌✅ (primary)	-0.7%	[-0.7%, -0.6%]	4

Max RSS (memory usage)

Results

This 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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 622.003s -> 621.563s (-0.07%)
Artifact size: 272.02 MiB -> 272.01 MiB (-0.00%)

coverage: Move most per-function coverage info into `mir::Body` Currently, all of the coverage information collected by the `InstrumentCoverage` pass is smuggled through MIR in the form of individual `StatementKind::Coverage` statements, which must then be reassembled by coverage codegen. That's awkward for a number of reasons: - While some of the coverage statements do care about their specific position in the MIR control-flow graph, many of them don't, and are just tacked onto the function's first BB as metadata carriers. - MIR inlining can result in coverage statements being duplicated, so coverage codegen has to jump through hoops to avoid emitting duplicate mappings. - MIR optimizations that would delete coverage statements need to carefully copy them into the function's first BB so as not to omit them from coverage reports. - The order in which coverage codegen sees coverage statements is dependent on MIR optimizations/inlining, which can cause unnecessary churn in the emitted coverage mappings. - We don't have a good way to annotate MIR-level functions with extra coverage info that doesn't belong in a statement. --- This PR therefore takes most of the per-function coverage info and stores it in a field in `mir::Body` as `Option<Box<FunctionCoverageInfo>>`. (This adds one pointer to the size of `mir::Body`, even when coverage is not enabled.) Coverage statements still need to be injected into MIR in some cases, but only when they actually affect codegen (counters) or are needed to detect code that has been optimized away as unreachable (counters/expressions). --- By the end of this PR, the information stored in `FunctionCoverageInfo` is: - A hash of the function's source code (needed by LLVM's coverage map format) - The number of coverage counters added by coverage instrumentation - A table of coverage expressions, associating each expression ID with its operator (add or subtract) and its two operands - The list of mappings, associating each covered code region with a counter/expression/zero value --- ~~This is built on top of rust-lang#115301, so I'll rebase and roll a reviewer once that lands.~~ r? `@ghost` `@rustbot` label +A-code-coverage

rustbot assigned davidtwco Aug 28, 2023

rustbot added A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Aug 28, 2023

Zalathar commented Aug 28, 2023

View reviewed changes

Zalathar force-pushed the regions-vec branch 4 times, most recently from 823b02b to 39cfe01 Compare August 28, 2023 07:59

Swatinem reviewed Aug 28, 2023

View reviewed changes

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs Outdated Show resolved Hide resolved

Swatinem reviewed Aug 28, 2023

View reviewed changes

tests/coverage-map/status-quo/async.cov-map Outdated Show resolved Hide resolved

Zalathar force-pushed the regions-vec branch from 39cfe01 to 1695d4f Compare August 28, 2023 08:25

davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2023

Zalathar force-pushed the regions-vec branch 3 times, most recently from 17b3653 to e08eb2e Compare August 29, 2023 03:03

This comment was marked as resolved.

Sign in to view

Zalathar force-pushed the regions-vec branch 2 times, most recently from 594c85c to d280856 Compare August 30, 2023 10:35

This comment was marked as resolved.

Sign in to view

Zalathar force-pushed the regions-vec branch from d280856 to 79d3734 Compare August 31, 2023 01:15

Zalathar force-pushed the regions-vec branch 3 times, most recently from 08cff5b to 36c1667 Compare October 2, 2023 02:06

Zalathar added 7 commits October 3, 2023 13:03

coverage: Mappings for unused functions can all be zero

59a11af

There is no need to include a dummy counter reference in the coverage mappings for an unused function.

coverage: Update comments/logs that referred to CoverageSpan

1355e1f

The concrete type `CoverageSpan` is no longer used outside of the `spans` module. This is a separate patch to avoid noise in the preceding patch that actually encapsulates coverage spans.

coverage: Let each coverage statement hold a vector of code regions

ee9d00f

This makes it possible for a `StatementKind::Coverage` to hold more than one code region, but that capability is not yet used.

coverage: Store each BCB's code regions in one coverage statement

86a66c8

If a BCB has more than one code region, those extra regions can now all be stored in the same coverage statement, instead of being stored in additional statements.

coverage: Remove code for making expression copies of BCB counters

b1cf0c8

Now that coverage statements can have multiple code regions attached to them, this code is never used.

coverage: Remove next_id methods from counter/expression IDs

053c4f9

When these methods were originally written, I wasn't aware that `newtype_index!` already supports addition with ordinary numbers, without needing to unwrap and re-wrap.

Zalathar force-pushed the regions-vec branch from 36c1667 to 053c4f9 Compare October 3, 2023 02:11

davidtwco approved these changes Oct 3, 2023

View reviewed changes

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 Oct 3, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 3, 2023

bors merged commit 36aab8d into rust-lang:master Oct 3, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 3, 2023

Zalathar deleted the regions-vec branch October 4, 2023 01:33

pvdrz mentioned this pull request Oct 4, 2023

Fix divergence from upstream master ferrocene/ferrocene#23

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Allow each coverage statement to have multiple code regions #115301

coverage: Allow each coverage statement to have multiple code regions #115301

Zalathar commented Aug 28, 2023 •

edited

Loading

rustbot commented Aug 28, 2023

rustbot commented Aug 28, 2023

Zalathar commented Aug 28, 2023

Zalathar Aug 28, 2023

Zalathar Aug 28, 2023

davidtwco commented Aug 28, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

davidtwco left a comment

davidtwco commented Oct 3, 2023

bors commented Oct 3, 2023

bors commented Oct 3, 2023

bors commented Oct 3, 2023

rust-timer commented Oct 3, 2023

coverage: Allow each coverage statement to have multiple code regions #115301

coverage: Allow each coverage statement to have multiple code regions #115301

Conversation

Zalathar commented Aug 28, 2023 • edited Loading

rustbot commented Aug 28, 2023

rustbot commented Aug 28, 2023

Zalathar commented Aug 28, 2023

Zalathar Aug 28, 2023

Choose a reason for hiding this comment

Zalathar Aug 28, 2023

Choose a reason for hiding this comment

davidtwco commented Aug 28, 2023 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco commented Oct 3, 2023

bors commented Oct 3, 2023

bors commented Oct 3, 2023

bors commented Oct 3, 2023

rust-timer commented Oct 3, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Zalathar commented Aug 28, 2023 •

edited

Loading

davidtwco commented Aug 28, 2023 •

edited

Loading