coverage: Move most per-function coverage info into `mir::Body` by Zalathar · Pull Request #116046 · rust-lang/rust

Zalathar · 2023-09-22T01:33:39Z

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 #115301, so I'll rebase and roll a reviewer once that lands.~~
r? @ghost
@rustbot label +A-code-coverage

Zalathar · 2023-09-23T00:30:05Z

When I moved expression data into FunctionCoverageInfo, I was hoping to be able to get rid of CoverageKind::Expression, leaving counter-increments as the only kind of coverage statement.

To my surprise, I found that this can't be done without regressing the mappings: even though an expression statement no longer carries op/operand data, it still witnesses the fact that its enclosing BB/BCB wasn't removed by later MIR transforms.

(CoverageKind::Counter also witnesses non-removal in the same way, in addition to lowering to llvm.instrprof.increment.)

So when coverage codegen generates the final mappings to embed in the binary, it can zero out any mappings whose BB/BCB was removed by MIR transforms, while leaving all other mappings intact.

(This zeroing is also currently load-bearing for unused functions: instead of explicitly zeroing out all of the mappings, we rely on the normal-case zeroing code to observe that no counters/expressions were ever seen for an unused function's mappings.)

Zalathar · 2023-09-23T03:00:56Z

I've also renamed the remaining variants of CoverageKind (diff), since this PR alters/narrows their roles.

EDIT: Also updated the docs for StatementKind::Coverage.

When coverage instrumentation and MIR opts are both enabled, coverage relies on two assumptions: - MIR opts that would delete `StatementKind::Coverage` statements instead move them into bb0 and change them to `CoverageKind::Unreachable`. - MIR opts won't delete all `CoverageKind::Counter` statements from an instrumented function. Most MIR opts naturally satisfy the second assumption, because they won't remove coverage statements from bb0, but `UnreachablePropagation` can do so if it finds that bb0 is unreachable. If this happens, LLVM thinks the function is uncovered, and it vanishes from coverage reports. A proper solution won't be possible until after per-function coverage info lands in rust-lang#116046, but for now we can avoid the problem by turning off this particular pass when coverage instrumentation is enabled.

When coverage instrumentation and MIR opts are both enabled, coverage relies on two assumptions: - MIR opts that would delete `StatementKind::Coverage` statements instead move them into bb0 and change them to `CoverageKind::Unreachable`. - MIR opts won't delete all `CoverageKind::Counter` statements from an instrumented function. Most MIR opts naturally satisfy the second assumption, because they won't remove coverage statements from bb0, but `UnreachablePropagation` can do so if it finds that bb0 is unreachable. If this happens, LLVM thinks the function isn't instrumented, and it vanishes from coverage reports. A proper solution won't be possible until after per-function coverage info lands in rust-lang#116046, but for now we can avoid the problem by turning off this particular pass when coverage instrumentation is enabled.

bors · 2023-09-26T20:00:54Z

☔ The latest upstream changes (presumably #116154) made this pull request unmergeable. Please resolve the merge conflicts.

Skip MIR pass `UnreachablePropagation` when coverage is enabled When coverage instrumentation and MIR opts are both enabled, coverage relies on two assumptions: - MIR opts that would delete `StatementKind::Coverage` statements instead move them into bb0 and change them to `CoverageKind::Unreachable`. - MIR opts won't delete all `CoverageKind::Counter` statements from an instrumented function. Most MIR opts naturally satisfy the second assumption, because they won't remove coverage statements from bb0, but `UnreachablePropagation` can do so if it finds that bb0 is unreachable. If this happens, LLVM thinks the function isn't instrumented, and it vanishes from coverage reports. A proper solution won't be possible until after per-function coverage info lands in rust-lang#116046, but for now we can avoid the problem by turning off this particular pass when coverage instrumentation is enabled. --- cc `@cjgillot` since I found this while investigating coverage problems encountered by rust-lang#113970 `@rustbot` label +A-code-coverage +A-mir-opt

Even though expression details are now stored in the info structure, we still need to inject `ExpressionUsed` statements into MIR, because if one is missing during codegen then we know that it was optimized out and we can remap all of its associated code regions to zero.

This new description reflects the changes made in this PR, and should hopefully be more useful to non-coverage developers who need to care about coverage statements.

davidtwco · 2023-10-18T13:04:39Z

Apologies for not getting to this quicker, I'll re-assign to cjgillot because they've already started reviewing.

r? @cjgillot

cjgillot · 2023-10-18T16:15:26Z

@bors r+

bors · 2023-10-18T16:15:28Z

📌 Commit 33da097 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-10-18T17:14:48Z

⌛ Testing commit 33da097 with merge 00d5227...

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

rust-log-analyzer · 2023-10-18T18:23:24Z

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-10-18T18:24:00Z

💔 Test failed - checks-actions

cjgillot · 2023-10-18T18:33:42Z

@bors retry failed to download from https://crates.io/api/v1/crates/flate2/1.0.27/download

bors · 2023-10-18T18:48:37Z

⌛ Testing commit 33da097 with merge cc705b8...

bors · 2023-10-18T20:40:04Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing cc705b8 to master...

rust-timer · 2023-10-18T22:32:47Z

Finished benchmarking commit (cc705b8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	3.1%	[3.1%, 3.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.0%	[-4.0%, -4.0%]	1
All ❌✅ (primary)	-	-	0

Cycles

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.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.8%, 0.8%]	1

Binary size

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.1%	[0.0%, 0.2%]	53
Regressions ❌ (secondary)	0.1%	[0.0%, 0.3%]	39
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.2%]	53

Bootstrap: 628.593s -> 628.235s (-0.06%)
Artifact size: 303.98 MiB -> 304.03 MiB (0.02%)

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. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Sep 22, 2023

Zalathar force-pushed the fn-cov-info branch 2 times, most recently from 646abfa to adce72b Compare September 22, 2023 12:41

Swatinem approved these changes Sep 22, 2023

View reviewed changes

Swatinem mentioned this pull request Sep 22, 2023

Move Coverage Regions / Expressions to a side table #115170

Closed

Zalathar force-pushed the fn-cov-info branch 2 times, most recently from 4b27f9d to d990af0 Compare September 23, 2023 02:58

Zalathar mentioned this pull request Sep 23, 2023

Add support for branch coverage in source-based coverage #79649

Open

Zalathar force-pushed the fn-cov-info branch 3 times, most recently from a3befaf to 3e0a088 Compare September 25, 2023 01:15

This was referenced Sep 25, 2023

Investigate why #113970 changes async2.rs coverage maps on some targets #116134

Open

Replace switch to unreachable by assume statements #113970

Merged

Zalathar force-pushed the fn-cov-info branch from 3e0a088 to 7899bc3 Compare September 26, 2023 02:15

This was referenced Sep 26, 2023

Skip MIR pass UnreachablePropagation when coverage is enabled #116166

Merged

Make coverage robust against MIR optimizations removing all counter-increment statements #116171

Closed

Zalathar force-pushed the fn-cov-info branch from 7899bc3 to 2d3a240 Compare September 27, 2023 02:10

Zalathar force-pushed the fn-cov-info branch 3 times, most recently from ed54edf to d3933ed Compare September 28, 2023 05:17

Zalathar added 3 commits October 18, 2023 23:44

coverage: Update docs for StatementKind::Coverage

753caf2

This new description reflects the changes made in this PR, and should hopefully be more useful to non-coverage developers who need to care about coverage statements.

coverage: Explicitly note that counter/expression IDs are function-local

33da097

Zalathar force-pushed the fn-cov-info branch from 2c2c9c4 to 33da097 Compare October 18, 2023 12:44

rustbot assigned cjgillot and unassigned davidtwco Oct 18, 2023

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 18, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 18, 2023

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 18, 2023

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

bors merged commit cc705b8 into rust-lang:master Oct 18, 2023

This was referenced Oct 19, 2023

coverage: Simplify the injection of coverage statements #116917

Merged

coverage: Emit mappings for unused functions without generating stubs #116922

Merged

Zalathar mentioned this pull request Oct 30, 2023

Update some outdated descriptions of coverage instrumentation rust-lang/rustc-dev-guide#1809

Merged

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

Zalathar mentioned this pull request Dec 30, 2023

coverage: Prepare mappings separately from injecting statements #119438

Merged

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Zalathar commented Sep 23, 2023

Uh oh!

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

Uh oh!

Uh oh!

bors commented Sep 26, 2023

Uh oh!

davidtwco commented Oct 18, 2023

Uh oh!

cjgillot commented Oct 18, 2023

Uh oh!

bors commented Oct 18, 2023

Uh oh!

bors commented Oct 18, 2023

Uh oh!

rust-log-analyzer commented Oct 18, 2023

Uh oh!

bors commented Oct 18, 2023

Uh oh!

cjgillot commented Oct 18, 2023

Uh oh!

bors commented Oct 18, 2023

Uh oh!

bors commented Oct 18, 2023

Uh oh!

rust-timer commented Oct 18, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Zalathar commented Sep 22, 2023 •

edited

Loading

Zalathar commented Sep 23, 2023 •

edited

Loading