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

coverage: Optionally instrument the RHS of lazy logical operators #125756

Merged
merged 3 commits into from
May 31, 2024

Conversation

Zalathar
Copy link
Contributor

(This is an updated version of #124644 and #124402. Fixes #124120.)

When || or && is used outside of a branching context (such as the condition of an if), the rightmost value does not directly influence any branching decision, so branch coverage instrumentation does not treat it as its own true-or-false branch.

That is a correct and useful interpretation of “branch coverage”, but might be undesirable in some contexts, as described at #124120. This PR therefore adds a new coverage level -Zcoverage-options=condition that behaves like branch coverage, but also adds additional branch instrumentation to the right-hand-side of lazy boolean operators.


As discussed at #124120 (comment), this is mainly intended as an intermediate step towards fully-featured MC/DC instrumentation. It's likely that we'll eventually want to remove this coverage level (rather than stabilize it), either because it has been incorporated into MC/DC instrumentation, or because it's getting in the way of future MC/DC work. The main appeal of landing it now is so that work on tracking conditions can proceed concurrently with other MC/DC-related work.

@rustbot label +A-code-coverage

RenjiSann and others added 3 commits May 30, 2024 15:38
When a lazy logical operator (`&&` or `||`) occurs outside of an `if`
condition, it normally doesn't have any associated control-flow branch, so we
don't have an existing way to track whether it was true or false.

This patch adds special code to handle this case, by inserting extra MIR blocks
in a diamond shape after evaluating the RHS. This gives us a place to insert
the appropriate marker statements, which can then be given their own counters.
@rustbot
Copy link
Collaborator

rustbot commented May 30, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
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 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 May 30, 2024
@oli-obk
Copy link
Contributor

oli-obk commented May 31, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 31, 2024

📌 Commit 35a8746 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 May 31, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 31, 2024
coverage: Optionally instrument the RHS of lazy logical operators

(This is an updated version of rust-lang#124644 and rust-lang#124402. Fixes rust-lang#124120.)

When `||` or `&&` is used outside of a branching context (such as the condition of an `if`), the rightmost value does not directly influence any branching decision, so branch coverage instrumentation does not treat it as its own true-or-false branch.

That is a correct and useful interpretation of “branch coverage”, but might be undesirable in some contexts, as described at rust-lang#124120. This PR therefore adds a new coverage level `-Zcoverage-options=condition` that behaves like branch coverage, but also adds additional branch instrumentation to the right-hand-side of lazy boolean operators.

---

As discussed at rust-lang#124120 (comment), this is mainly intended as an intermediate step towards fully-featured MC/DC instrumentation. It's likely that we'll eventually want to remove this coverage level (rather than stabilize it), either because it has been incorporated into MC/DC instrumentation, or because it's getting in the way of future MC/DC work. The main appeal of landing it now is so that work on tracking conditions can proceed concurrently with other MC/DC-related work.

`@rustbot` label +A-code-coverage
jieyouxu added a commit to jieyouxu/rust that referenced this pull request May 31, 2024
coverage: Optionally instrument the RHS of lazy logical operators

(This is an updated version of rust-lang#124644 and rust-lang#124402. Fixes rust-lang#124120.)

When `||` or `&&` is used outside of a branching context (such as the condition of an `if`), the rightmost value does not directly influence any branching decision, so branch coverage instrumentation does not treat it as its own true-or-false branch.

That is a correct and useful interpretation of “branch coverage”, but might be undesirable in some contexts, as described at rust-lang#124120. This PR therefore adds a new coverage level `-Zcoverage-options=condition` that behaves like branch coverage, but also adds additional branch instrumentation to the right-hand-side of lazy boolean operators.

---

As discussed at rust-lang#124120 (comment), this is mainly intended as an intermediate step towards fully-featured MC/DC instrumentation. It's likely that we'll eventually want to remove this coverage level (rather than stabilize it), either because it has been incorporated into MC/DC instrumentation, or because it's getting in the way of future MC/DC work. The main appeal of landing it now is so that work on tracking conditions can proceed concurrently with other MC/DC-related work.

``@rustbot`` label +A-code-coverage
bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#125652 (Revert propagation of drop-live information from Polonius)
 - rust-lang#125730 (Apply `x clippy --fix` and `x fmt` on Rustc)
 - rust-lang#125752 (run-make: enforce `#[must_use]` and arm command wrappers with drop bombs)
 - rust-lang#125756 (coverage: Optionally instrument the RHS of lazy logical operators)
 - rust-lang#125796 (Also InstSimplify `&raw*`)
 - rust-lang#125816 (Don't build the `rust-demangler` binary for coverage tests)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request May 31, 2024
coverage: Optionally instrument the RHS of lazy logical operators

(This is an updated version of rust-lang#124644 and rust-lang#124402. Fixes rust-lang#124120.)

When `||` or `&&` is used outside of a branching context (such as the condition of an `if`), the rightmost value does not directly influence any branching decision, so branch coverage instrumentation does not treat it as its own true-or-false branch.

That is a correct and useful interpretation of “branch coverage”, but might be undesirable in some contexts, as described at rust-lang#124120. This PR therefore adds a new coverage level `-Zcoverage-options=condition` that behaves like branch coverage, but also adds additional branch instrumentation to the right-hand-side of lazy boolean operators.

---

As discussed at rust-lang#124120 (comment), this is mainly intended as an intermediate step towards fully-featured MC/DC instrumentation. It's likely that we'll eventually want to remove this coverage level (rather than stabilize it), either because it has been incorporated into MC/DC instrumentation, or because it's getting in the way of future MC/DC work. The main appeal of landing it now is so that work on tracking conditions can proceed concurrently with other MC/DC-related work.

```@rustbot``` label +A-code-coverage
bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#125652 (Revert propagation of drop-live information from Polonius)
 - rust-lang#125730 (Apply `x clippy --fix` and `x fmt` on Rustc)
 - rust-lang#125756 (coverage: Optionally instrument the RHS of lazy logical operators)
 - rust-lang#125776 (Stop using `translate_args` in the new solver)
 - rust-lang#125796 (Also InstSimplify `&raw*`)
 - rust-lang#125807 (Also resolve the type of constants, even if we already turned it into an error constant)
 - rust-lang#125816 (Don't build the `rust-demangler` binary for coverage tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7667a91 into rust-lang:master May 31, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 31, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2024
Rollup merge of rust-lang#125756 - Zalathar:branch-on-bool, r=oli-obk

coverage: Optionally instrument the RHS of lazy logical operators

(This is an updated version of rust-lang#124644 and rust-lang#124402. Fixes rust-lang#124120.)

When `||` or `&&` is used outside of a branching context (such as the condition of an `if`), the rightmost value does not directly influence any branching decision, so branch coverage instrumentation does not treat it as its own true-or-false branch.

That is a correct and useful interpretation of “branch coverage”, but might be undesirable in some contexts, as described at rust-lang#124120. This PR therefore adds a new coverage level `-Zcoverage-options=condition` that behaves like branch coverage, but also adds additional branch instrumentation to the right-hand-side of lazy boolean operators.

---

As discussed at rust-lang#124120 (comment), this is mainly intended as an intermediate step towards fully-featured MC/DC instrumentation. It's likely that we'll eventually want to remove this coverage level (rather than stabilize it), either because it has been incorporated into MC/DC instrumentation, or because it's getting in the way of future MC/DC work. The main appeal of landing it now is so that work on tracking conditions can proceed concurrently with other MC/DC-related work.

````@rustbot```` label +A-code-coverage
@Zalathar Zalathar deleted the branch-on-bool branch June 1, 2024 01:19
@ZhuUx ZhuUx mentioned this pull request Jun 6, 2024
4 tasks
fmease added a commit to fmease/rust that referenced this pull request Jun 19, 2024
…l, r=nnethercote

MCDC Coverage: instrument last boolean RHS operands from condition coverage

Fresh PR from rust-lang#124652

--

This PR ensures that the top-level boolean expressions that are not part of the control flow are correctly instrumented thanks to condition coverage.

See discussion on rust-lang#124120.
Depends on `@Zalathar` 's condition coverage implementation rust-lang#125756.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup merge of rust-lang#125766 - RenjiSann:fresh-mcdc-branch-on-bool, r=nnethercote

MCDC Coverage: instrument last boolean RHS operands from condition coverage

Fresh PR from rust-lang#124652

--

This PR ensures that the top-level boolean expressions that are not part of the control flow are correctly instrumented thanks to condition coverage.

See discussion on rust-lang#124120.
Depends on `@Zalathar` 's condition coverage implementation rust-lang#125756.
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) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

Optionally extend branch coverage to instrument the last operand of lazy boolean expressions
5 participants