coverage: Initial support for branch coverage instrumentation #122322

Zalathar · 2024-03-11T03:04:02Z

(This is a review-ready version of the changes that were drafted in #118305.)

This PR adds support for branch coverage instrumentation, gated behind the unstable flag value -Zcoverage-options=branch. (Coverage instrumentation must also be enabled with -Cinstrument-coverage.)

During THIR-to-MIR lowering (MIR building), if branch coverage is enabled, we collect additional information about branch conditions and their corresponding then/else blocks. We inject special marker statements into those blocks, so that the InstrumentCoverage MIR pass can reliably identify them even after the initially-built MIR has been simplified and renumbered.

The rest of the changes are mostly just plumbing needed to gather up the information that was collected during MIR building, and include it in the coverage metadata that we embed in the final binary.

Note that llvm-cov show doesn't print branch coverage information in its source views by default; that needs to be explicitly enabled with --show-branches=count or similar.

The current implementation doesn't have any support for instrumenting if let or let-chains. I think it's still useful without that, and adding it would be non-trivial, so I'm happy to leave that for future work.

rustbot · 2024-03-11T03:04:10Z

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-03-11T03:04:12Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in match lowering

cc @Nadrieril

Zalathar · 2024-03-11T03:14:28Z

The changes that are most important in terms of reviewer attention are the alterations to non-coverage-specific code in rustc_mir_build, since they have the most potential to step on other people's toes.

(Whereas the coverage-specific modules are mostly self-contained, so there's less impact on the rest of the compiler.)

Zalathar · 2024-03-11T09:43:05Z

Made some style improvements to coverageinfo.rs, since I wasn't happy with Inversions as a name (diff).

Zalathar · 2024-03-11T09:50:40Z

compiler/rustc_mir_build/src/build/mod.rs

@@ -837,7 +842,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
            self.fn_span,
            self.coroutine,
            None,
-        )
+        );
+        body.coverage_hir_branch_info = self.coverage_branch_info.and_then(|b| b.into_done());


Here I had a choice between two ways to set this field:

Add an extra parameter to Body::new, and None to a half-dozen callers that don't care.

Leave the constructor as-is, and do an explicit field write in the one caller that does care.

I think I prefer the explicit write, as shown here.

nnethercote · 2024-03-12T22:15:30Z

I skimmed through this and it looks reasonable and is well-commented, and I like that you added the test first so the changes to it are clear later on. But I know very little about MIR and so am not a good reviewer for this. According to triagebot.toml the MIR-iest reviewer we have is...

r? @oli-obk

Zalathar · 2024-03-13T03:08:43Z

#122226 has been approved, so once that lands I'll rebase this and make the necessary adjustments.

…hercote coverage: Remove or migrate all unstable values of `-Cinstrument-coverage` (This PR was substantially overhauled from its original version, which migrated all of the existing unstable values intact.) This PR takes the three nightly-only values that are currently accepted by `-Cinstrument-coverage`, completely removes two of them (`except-unused-functions` and `except-unused-generics`), and migrates the third (`branch`) over to a newly-introduced unstable flag `-Zcoverage-options`. I have a few motivations for wanting to do this: - It's unclear whether anyone actually uses the `except-unused-*` values, so this serves as an opportunity to either remove them, or prompt existing users to object to their removal. - After rust-lang#117199, the stable values of `-Cinstrument-coverage` treat it as a boolean-valued flag, so having nightly-only extra values feels out-of-place. - Nightly-only values also require extra ad-hoc code to make sure they aren't accidentally exposed to stable users. - The new system allows multiple different settings to be toggled independently, which isn't possible in the current single-value system. - The new system makes it easier to introduce new behaviour behind an unstable toggle, and then gather nightly-user feedback before possibly making it the default behaviour for all users. - The new system also gives us a convenient place to put relatively-narrow options that won't ever be the default, but that nightly users might still want access to. - It's likely that we will eventually want to give stable users more fine-grained control over coverage instrumentation. The new flag serves as a prototype of what that stable UI might eventually look like. The `branch` option is a placeholder that currently does nothing. It will be used by rust-lang#122322 to opt into branch coverage instrumentation. --- I see `-Zcoverage-options` as something that will exist more-or-less indefinitely, though individual sub-options might come and go as appropriate. I think there will always be some demand for nightly-only toggles, so I don't see `-Zcoverage-options` itself ever being stable, though we might eventually stabilize something similar to it.

Rollup merge of rust-lang#122226 - Zalathar:zcoverage-options, r=nnethercote coverage: Remove or migrate all unstable values of `-Cinstrument-coverage` (This PR was substantially overhauled from its original version, which migrated all of the existing unstable values intact.) This PR takes the three nightly-only values that are currently accepted by `-Cinstrument-coverage`, completely removes two of them (`except-unused-functions` and `except-unused-generics`), and migrates the third (`branch`) over to a newly-introduced unstable flag `-Zcoverage-options`. I have a few motivations for wanting to do this: - It's unclear whether anyone actually uses the `except-unused-*` values, so this serves as an opportunity to either remove them, or prompt existing users to object to their removal. - After rust-lang#117199, the stable values of `-Cinstrument-coverage` treat it as a boolean-valued flag, so having nightly-only extra values feels out-of-place. - Nightly-only values also require extra ad-hoc code to make sure they aren't accidentally exposed to stable users. - The new system allows multiple different settings to be toggled independently, which isn't possible in the current single-value system. - The new system makes it easier to introduce new behaviour behind an unstable toggle, and then gather nightly-user feedback before possibly making it the default behaviour for all users. - The new system also gives us a convenient place to put relatively-narrow options that won't ever be the default, but that nightly users might still want access to. - It's likely that we will eventually want to give stable users more fine-grained control over coverage instrumentation. The new flag serves as a prototype of what that stable UI might eventually look like. The `branch` option is a placeholder that currently does nothing. It will be used by rust-lang#122322 to opt into branch coverage instrumentation. --- I see `-Zcoverage-options` as something that will exist more-or-less indefinitely, though individual sub-options might come and go as appropriate. I think there will always be some demand for nightly-only toggles, so I don't see `-Zcoverage-options` itself ever being stable, though we might eventually stabilize something similar to it.

This will allow MIR building to check whether a function is eligible for coverage instrumentation, and avoid collecting branch coverage info if it is not.

Zalathar · 2024-03-13T09:50:27Z

I've rebased and updated the tests for #122226, ~~but I still need to update the “placeholder” docs for -Zcoverage-options=branch~~.

EDIT: Updates should be complete now.

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs

Zalathar · 2024-03-14T05:38:57Z

Shortened some identifiers, mostly by removing “hir” from them (diff).

oli-obk · 2024-03-14T10:12:01Z

@bors r+ rollup

bors · 2024-03-14T10:12:04Z

📌 Commit 060c7ce has been approved by oli-obk

It is now in the queue for this repository.

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#117118 ([AIX] Remove AixLinker's debuginfo() implementation) - rust-lang#121650 (change std::process to drop supplementary groups based on CAP_SETGID) - rust-lang#121764 (Make incremental sessions identity no longer depend on the crate names provided by source code) - rust-lang#122212 (Copy byval argument to alloca if alignment is insufficient) - rust-lang#122322 (coverage: Initial support for branch coverage instrumentation) - rust-lang#122373 (Fix the conflict problem between the diagnostics fixes of lint `unnecessary_qualification` and `unused_imports`) - rust-lang#122479 (Implement `Duration::as_millis_{f64,f32}`) - rust-lang#122487 (Rename `StmtKind::Local` variant into `StmtKind::Let`) - rust-lang#122498 (Update version of cc crate) - rust-lang#122503 (Make `SubdiagMessageOp` well-formed) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122322 - Zalathar:branch, r=oli-obk coverage: Initial support for branch coverage instrumentation (This is a review-ready version of the changes that were drafted in rust-lang#118305.) This PR adds support for branch coverage instrumentation, gated behind the unstable flag value `-Zcoverage-options=branch`. (Coverage instrumentation must also be enabled with `-Cinstrument-coverage`.) During THIR-to-MIR lowering (MIR building), if branch coverage is enabled, we collect additional information about branch conditions and their corresponding then/else blocks. We inject special marker statements into those blocks, so that the `InstrumentCoverage` MIR pass can reliably identify them even after the initially-built MIR has been simplified and renumbered. The rest of the changes are mostly just plumbing needed to gather up the information that was collected during MIR building, and include it in the coverage metadata that we embed in the final binary. Note that `llvm-cov show` doesn't print branch coverage information in its source views by default; that needs to be explicitly enabled with `--show-branches=count` or similar. --- The current implementation doesn't have any support for instrumenting `if let` or let-chains. I think it's still useful without that, and adding it would be non-trivial, so I'm happy to leave that for future work.

rustbot assigned nnethercote Mar 11, 2024

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. labels Mar 11, 2024

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Mar 11, 2024

This was referenced Mar 11, 2024

coverage: Remove or migrate all unstable values of -Cinstrument-coverage #122226

Merged

Draft of branch coverage support #118305

Closed

Zalathar force-pushed the branch branch from 3777fb6 to 5a390b7 Compare March 11, 2024 09:42

Zalathar commented Mar 11, 2024

View reviewed changes

rustbot assigned oli-obk and unassigned nnethercote Mar 12, 2024

Zalathar added 4 commits March 13, 2024 20:43

coverage: Add branch coverage tests (with branch coverage disabled)

89fab06

Allow rustc_mir_transform to register hook providers

9751098

coverage: Make is_eligible_for_coverage a hook method

73475d0

This will allow MIR building to check whether a function is eligible for coverage instrumentation, and avoid collecting branch coverage info if it is not.

coverage: Add CoverageKind::BlockMarker

c921ab1

Zalathar force-pushed the branch branch from 5a390b7 to fbc0067 Compare March 13, 2024 09:49

RenjiSann reviewed Mar 13, 2024

View reviewed changes

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs Show resolved Hide resolved

Zalathar force-pushed the branch branch 2 times, most recently from 1085c76 to a043c19 Compare March 14, 2024 02:03

Zalathar added 2 commits March 14, 2024 16:27

coverage: Data structures for recording branch info during MIR building

f9cdaeb

coverage: Record branch information during MIR building

c1bec0c

Zalathar force-pushed the branch branch from a043c19 to 4a55292 Compare March 14, 2024 05:37

Zalathar added 3 commits March 14, 2024 17:19

coverage: Include recorded branch info in coverage instrumentation

31d0b50

coverage: Enable branch coverage in the branch coverage tests

5fb1f61

coverage: -Zcoverage-options=branch is no longer a placeholder

060c7ce

Zalathar force-pushed the branch branch from 4a55292 to 060c7ce Compare March 14, 2024 06:19

oli-obk approved these changes Mar 14, 2024

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 Mar 14, 2024

matthiaskrgr mentioned this pull request Mar 14, 2024

Rollup of 10 pull requests #122511

Merged

bors merged commit 54a5a49 into rust-lang:master Mar 15, 2024
11 checks passed

rustbot added this to the 1.78.0 milestone Mar 15, 2024

Zalathar deleted the branch branch March 15, 2024 02:04

Zalathar mentioned this pull request Mar 15, 2024

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

Open

RenjiSann mentioned this pull request Apr 2, 2024

Support MCDC Coverage with LLVM backend. #123358

Closed

Zalathar mentioned this pull request Apr 18, 2024

Known limitations of branch coverage instrumentation #124118

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Initial support for branch coverage instrumentation #122322

coverage: Initial support for branch coverage instrumentation #122322

Zalathar commented Mar 11, 2024 •

edited

Loading

rustbot commented Mar 11, 2024

rustbot commented Mar 11, 2024

Zalathar commented Mar 11, 2024

Zalathar commented Mar 11, 2024

Zalathar Mar 11, 2024

nnethercote commented Mar 12, 2024

Zalathar commented Mar 13, 2024

Zalathar commented Mar 13, 2024 •

edited

Loading

Zalathar commented Mar 14, 2024

oli-obk commented Mar 14, 2024

bors commented Mar 14, 2024

coverage: Initial support for branch coverage instrumentation #122322

coverage: Initial support for branch coverage instrumentation #122322

Conversation

Zalathar commented Mar 11, 2024 • edited Loading

rustbot commented Mar 11, 2024

rustbot commented Mar 11, 2024

Zalathar commented Mar 11, 2024

Zalathar commented Mar 11, 2024

Zalathar Mar 11, 2024

Choose a reason for hiding this comment

nnethercote commented Mar 12, 2024

Zalathar commented Mar 13, 2024

Zalathar commented Mar 13, 2024 • edited Loading

Zalathar commented Mar 14, 2024

oli-obk commented Mar 14, 2024

bors commented Mar 14, 2024

Zalathar commented Mar 11, 2024 •

edited

Loading

Zalathar commented Mar 13, 2024 •

edited

Loading