Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft of branch coverage support #118305

Closed
wants to merge 8 commits into from
Closed

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Nov 26, 2023

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.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 26, 2023
@Zalathar
Copy link
Contributor Author

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Nov 26, 2023
@Zalathar
Copy link
Contributor Author

@rustbot author

@rustbot 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
Comment on lines 21 to 16
LL| | if
LL| | !
LL| 15| cond
------------------
| Branch (LL:9): [True: 10, False: 5]
------------------
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two problems here:

  • The span doesn't include !.
  • The true/false counts are reversed, since they're tracking cond instead of !cond.

Comment on lines 37 to 64
LL| | if
LL| 15| a
------------------
| Branch (LL:9): [True: 12, False: 3]
------------------
LL| | &&
LL| 12| b
------------------
| Branch (LL:9): [True: 8, False: 4]
------------------
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks correct, which is nice.

Comment on lines 77 to 135
LL| | if
LL| | let
LL| | true
LL| | =
LL| 15| cond
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Contributor

bors commented Nov 26, 2023

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

tests/coverage/bad_counter_ids.coverage Outdated Show resolved Hide resolved
tests/coverage/try_error_result.coverage Outdated Show resolved Hide resolved
@Zalathar
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

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

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.

@rust-log-analyzer

This comment has been minimized.

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

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2024
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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2024
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.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2024
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
Copy link
Contributor

bors commented Mar 1, 2024

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

@bors
Copy link
Contributor

bors commented Mar 5, 2024

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

@bors
Copy link
Contributor

bors commented Mar 8, 2024

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

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

Zalathar commented Mar 10, 2024

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.

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

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.

@Zalathar Zalathar closed this Mar 11, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 14, 2024
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.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants