Draft of branch coverage support #118305

Zalathar · 2023-11-26T03:05:28Z

EDIT: I've closed this and re-opened a non-draft version at #122322.

This is a work-in-progress snapshot of my implementation of branch coverage.

It works, and produces correct results in many cases. However, it sometimes produces confusing or incorrect results for if expressions that contain the !/&&/|| operators, since the THIR-to-MIR lowering of those is non-trivial.

Zalathar · 2023-11-26T03:05:43Z

@rustbot label +A-code-coverage

Zalathar · 2023-11-26T03:06:00Z

@rustbot author

Zalathar · 2023-11-26T03:35:20Z

tests/coverage/branch.coverage

+   LL|       |    if
+   LL|       |        !
+   LL|     15|        cond
+  ------------------
+  |  Branch (LL:9): [True: 10, False: 5]
+  ------------------


Two problems here:

The span doesn't include !.

The true/false counts are reversed, since they're tracking cond instead of !cond.

Zalathar · 2023-11-26T03:35:51Z

tests/coverage/branch.coverage

+   LL|       |    if
+   LL|     15|        a
+  ------------------
+  |  Branch (LL:9): [True: 12, False: 3]
+  ------------------
+   LL|       |        &&
+   LL|     12|        b
+  ------------------
+  |  Branch (LL:9): [True: 8, False: 4]
+  ------------------


This all looks correct, which is nice.

tests/coverage/branch.coverage

Zalathar · 2023-11-26T03:38:34Z

tests/coverage/branch.coverage

+   LL|       |    if
+   LL|       |        let
+   LL|       |        true
+   LL|       |        =
+   LL|     15|        cond


This currently doesn't get branch coverage at all. I'd like to fix that if possible, but for me it's a lower priority than getting the logical ops correct.

Zalathar · 2023-11-26T03:44:36Z

cc @Swatinem in case you're curious.

The MIR building stuff will have to change once I figure out how to handle the logical ops better.

bors · 2023-11-26T05:43:09Z

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

tests/coverage/bad_counter_ids.coverage

tests/coverage/try_error_result.coverage

Zalathar · 2023-11-26T09:28:20Z

Well, this definitely indicates a need to add macro-expanded spans to the list of things I need to figure out.

The existing coverage code has some functions in spans.rs for dealing with this sort of thing, which I can probably look to for inspiration.

Zalathar · 2023-11-26T11:37:13Z

I tried passing each of the branch spans through function_source_span. That gets rid of the bogus spans, but it also results in a lot of unhelpful noise for assertions.

tests/coverage/branch.coverage

Zalathar · 2023-11-28T09:13:36Z

Now that the || bug is fixed, there are no known correctness issues remaining. The ! mappings are non-ideal, but they're not wrong.

So I'm thinking it might make sense for me to polish the current functionality into a review-ready state, so that people can start playing with branch coverage on nightly and start flushing out any lingering correctness issues.

Zalathar · 2023-11-29T02:18:36Z

I found a way to fix the mappings for !: pass an extra enclosing_not_expr: Option<&Expr<'tcx>> argument to the recursive call.

We also need to propagate it through the ExprKind::Scope case, but the other recursive cases reset it to None.

Make the success arms of `if lhs || rhs` meet up in a separate block Extracted from rust-lang#118305, where this is necessary to avoid introducing a bug when injecting marker statements into the then/else arms. --- In the previous code (rust-lang#111752), the success block of `lhs` would jump directly to the success block of `rhs`. However, `rhs_success_block` could already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well. This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to. --- I think the reason we currently get away with this is that `rhs_success_block` usually doesn't contain anything other than StorageDead statements for locals used by the RHS, and those statements don't seem to cause problems in the LHS success path (which never makes those locals live). But if we start adding meaningful statements for branch coverage (or MC/DC coverage), it's important to keep the LHS and RHS blocks separate.

Rollup merge of rust-lang#121784 - Zalathar:if-or-converge, r=Nadrieril Make the success arms of `if lhs || rhs` meet up in a separate block Extracted from rust-lang#118305, where this is necessary to avoid introducing a bug when injecting marker statements into the then/else arms. --- In the previous code (rust-lang#111752), the success block of `lhs` would jump directly to the success block of `rhs`. However, `rhs_success_block` could already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well. This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to. --- I think the reason we currently get away with this is that `rhs_success_block` usually doesn't contain anything other than StorageDead statements for locals used by the RHS, and those statements don't seem to cause problems in the LHS success path (which never makes those locals live). But if we start adding meaningful statements for branch coverage (or MC/DC coverage), it's important to keep the LHS and RHS blocks separate.

bors · 2024-03-01T21:56:20Z

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

bors · 2024-03-05T00:48:11Z

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

bors · 2024-03-08T01:40:05Z

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

Zalathar · 2024-03-10T11:03:49Z

Switched to a different mechanism for handling ! expressions (diff), which is less intrusive to rustc_mir_build::build::matches, and should be a bit easier to maintain/extend.

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.

…degen

Zalathar · 2024-03-11T02:46:26Z

Now that I've resolved my own concerns, I think it might be time to close this and open a new PR for actual review.

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.

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 added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Nov 26, 2023

rustbot 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 Nov 26, 2023

Zalathar force-pushed the branch branch from d81caf5 to d2a4cb1 Compare November 26, 2023 03:32

Zalathar commented Nov 26, 2023

View reviewed changes

tests/coverage/branch.coverage Outdated Show resolved Hide resolved

Zalathar commented Nov 26, 2023

View reviewed changes

Zalathar force-pushed the branch branch from d2a4cb1 to 0d33b4a Compare November 26, 2023 06:08

Swatinem reviewed Nov 26, 2023

View reviewed changes

tests/coverage/bad_counter_ids.coverage Outdated Show resolved Hide resolved

tests/coverage/try_error_result.coverage Outdated Show resolved Hide resolved

Zalathar force-pushed the branch branch from 0d33b4a to 3af01e8 Compare November 26, 2023 11:35

Zalathar force-pushed the branch branch 2 times, most recently from 996fbe5 to 9000afd Compare November 28, 2023 09:04

Zalathar commented Nov 28, 2023

View reviewed changes

tests/coverage/branch.coverage Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Zalathar force-pushed the branch branch 3 times, most recently from 811cb89 to 934e104 Compare November 29, 2023 02:13

Zalathar force-pushed the branch branch from 49b7641 to e0c4781 Compare March 2, 2024 01:57

Zalathar force-pushed the branch branch from e0c4781 to 5f36f54 Compare March 5, 2024 03:09

Zalathar force-pushed the branch branch 4 times, most recently from e72afef to d40cf6d Compare March 10, 2024 11:01

Zalathar force-pushed the branch branch 3 times, most recently from 9fa37bb to 59eb74a Compare March 11, 2024 02:29

Zalathar added 8 commits March 11, 2024 13:38

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

3284af1

Allow rustc_mir_transform to register hook providers

75368e6

coverage: Make is_eligible_for_coverage a hook method

14b2f5b

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

7422880

coverage: Data structures for recording branch info during MIR building

5cee303

coverage: Record branch information during MIR building

ad8a75b

coverage: Include recorded branch info in coverage instrumentation/co…

cf00cc2

…degen

coverage: Enable branch coverage in the branch coverage tests

67cab6a

Zalathar force-pushed the branch branch from 59eb74a to 67cab6a Compare March 11, 2024 02:42

Zalathar closed this Mar 11, 2024

Zalathar mentioned this pull request Mar 11, 2024

coverage: Initial support for branch coverage instrumentation #122322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft of branch coverage support #118305

Draft of branch coverage support #118305

Zalathar commented Nov 26, 2023 •

edited

Zalathar commented Nov 26, 2023

Zalathar commented Nov 26, 2023

Zalathar Nov 26, 2023

Zalathar Nov 26, 2023

Zalathar Nov 26, 2023

Zalathar commented Nov 26, 2023

bors commented Nov 26, 2023

Zalathar commented Nov 26, 2023

Zalathar commented Nov 26, 2023

Zalathar commented Nov 28, 2023

This comment has been minimized.

Zalathar commented Nov 29, 2023

bors commented Mar 1, 2024

bors commented Mar 5, 2024

bors commented Mar 8, 2024

Zalathar commented Mar 10, 2024 •

edited

Zalathar commented Mar 11, 2024

Draft of branch coverage support #118305

Draft of branch coverage support #118305

Conversation

Zalathar commented Nov 26, 2023 • edited

Zalathar commented Nov 26, 2023

Zalathar commented Nov 26, 2023

Zalathar Nov 26, 2023

Choose a reason for hiding this comment

Zalathar Nov 26, 2023

Choose a reason for hiding this comment

Zalathar Nov 26, 2023

Choose a reason for hiding this comment

Zalathar commented Nov 26, 2023

bors commented Nov 26, 2023

Zalathar commented Nov 26, 2023

Zalathar commented Nov 26, 2023

Zalathar commented Nov 28, 2023

This comment has been minimized.

Zalathar commented Nov 29, 2023

bors commented Mar 1, 2024

bors commented Mar 5, 2024

bors commented Mar 8, 2024

Zalathar commented Mar 10, 2024 • edited

Zalathar commented Mar 11, 2024

Zalathar commented Nov 26, 2023 •

edited

Zalathar commented Mar 10, 2024 •

edited