coverage: Move most per-function coverage info into `mir::Body` #116046

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

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

coverage: Simplify the injection of coverage statements This is a follow-up to rust-lang#116046 that I left out of that PR because I didn't want to make it any larger. After the various changes we've made to how coverage data is stored and transferred, the old code structure for injecting coverage statements into MIR is built around a lot of constraints that don't exist any more. We can simplify it by replacing it with a handful of loops over the BCB node/edge counters and the BCB spans. --- `@rustbot` label +A-code-coverage

Rollup merge of rust-lang#116917 - Zalathar:injection, r=cjgillot coverage: Simplify the injection of coverage statements This is a follow-up to rust-lang#116046 that I left out of that PR because I didn't want to make it any larger. After the various changes we've made to how coverage data is stored and transferred, the old code structure for injecting coverage statements into MIR is built around a lot of constraints that don't exist any more. We can simplify it by replacing it with a handful of loops over the BCB node/edge counters and the BCB spans. --- `@rustbot` label +A-code-coverage

These two tasks historically needed to be interleaved, but after various recent changes (including rust-lang#116046 and rust-lang#116917) they can now be fully separated.

coverage: Prepare mappings separately from injecting statements These two tasks historically needed to be interleaved, but after various recent changes (including rust-lang#116046 and rust-lang#116917) they can now be fully separated. --- `@rustbot` label +A-code-coverage

These two tasks historically needed to be interleaved, but after various recent changes (including rust-lang#116046 and rust-lang#116917) they can now be fully separated.

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

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
12 checks passed

rustbot added this to the 1.75.0 milestone Oct 18, 2023

Zalathar deleted the fn-cov-info branch October 18, 2023 22:16

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Move most per-function coverage info into `mir::Body` #116046

coverage: Move most per-function coverage info into `mir::Body` #116046

Zalathar commented Sep 22, 2023 •

edited

Zalathar commented Sep 23, 2023

Zalathar commented Sep 23, 2023 •

edited

bors commented Sep 26, 2023

bors commented Oct 18, 2023

rust-log-analyzer commented Oct 18, 2023

bors commented Oct 18, 2023

cjgillot commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 18, 2023

rust-timer commented Oct 18, 2023

coverage: Move most per-function coverage info into mir::Body #116046

coverage: Move most per-function coverage info into mir::Body #116046

Conversation

Zalathar commented Sep 22, 2023 • edited

Zalathar commented Sep 23, 2023

Zalathar commented Sep 23, 2023 • edited

bors commented Sep 26, 2023

bors commented Oct 18, 2023

rust-log-analyzer commented Oct 18, 2023

bors commented Oct 18, 2023

cjgillot commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 18, 2023

rust-timer commented Oct 18, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

coverage: Move most per-function coverage info into `mir::Body` #116046

coverage: Move most per-function coverage info into `mir::Body` #116046

Zalathar commented Sep 22, 2023 •

edited

Zalathar commented Sep 23, 2023 •

edited