coverage: Allow each coverage statement to have multiple code regions by Zalathar · Pull Request #115301 · rust-lang/rust

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

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

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.

Zalathar · 2023-09-28T05:11:54Z

(I've been rebasing this for the benefit of other PRs/patches that rely on it.)

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%)

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 from 9086c92 to ef8cfd5 Compare September 22, 2023 12:41

Swatinem approved these changes Sep 22, 2023

View reviewed changes

Zalathar force-pushed the regions-vec branch 7 times, most recently from 504dc99 to cf05f7c Compare September 27, 2023 08:07

Zalathar force-pushed the regions-vec branch 6 times, most recently from fc16cbc to 08cff5b Compare October 1, 2023 11:17

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.

davidtwco approved these changes Oct 3, 2023

View reviewed changes

pvdrz mentioned this pull request Oct 4, 2023

Fix divergence from upstream master ferrocene/ferrocene#23

Merged

Uh oh!

Conversation

Zalathar commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 28, 2023

Uh oh!

rustbot commented Aug 28, 2023

Uh oh!

Zalathar commented Aug 28, 2023

Uh oh!

Zalathar Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

Zalathar Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidtwco commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Zalathar commented Sep 28, 2023

Uh oh!

davidtwco left a comment

Choose a reason for hiding this comment

Uh oh!

davidtwco commented Oct 3, 2023

Uh oh!

bors commented Oct 3, 2023

Uh oh!

bors commented Oct 3, 2023

Uh oh!

bors commented Oct 3, 2023

Uh oh!

rust-timer commented Oct 3, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Zalathar commented Aug 28, 2023 •

edited

Loading

davidtwco commented Aug 28, 2023 •

edited

Loading