Coverage: Add condition coverage #124402
Coverage: Add condition coverage #124402
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in coverage tests. cc @Zalathar |
RenjiSann
force-pushed
the
renji/branch-on-bool-last-assign
branch
2 times, most recently
from
7a80098
to
290a532
Compare
|
The changes look reasonable to me. However, I'm not very familiar with MIR building and if the change to conditional lowering is desirable, so I would like another compiler reviewer to also take a look at this PR. r? compiler |
|
I plan to take a look at this sometime this week. |
|
@rustbot label +A-code-coverage |
rustbot
added
the
A-code-coverage
|
The main thing that worries me about this change that it adds a big chunk of coverage-specific complexity right in the middle of MIR building, which is something I've generally been trying very hard to avoid. I suspect there is probably a less-disruptive way to get the desired behaviour here, perhaps via one of:
|
|
Also see #124507 for my proposed handling of the coverage level in |
I was concerned by the same thing, that's why I added a big comment to document what is happening. I take your point and share your concern, but I probably lack experience with the compiler codebase to see a better alternative. |
|
r? mir |
RenjiSann
force-pushed
the
renji/branch-on-bool-last-assign
branch
from
290a532
to
c138d40
Compare
|
Rebase on master + splitting tests commits to get before/after diff (diff) |
|
☔ The latest upstream changes (presumably #124507) made this pull request unmergeable. Please resolve the merge conflicts. |
RenjiSann
force-pushed
the
renji/branch-on-bool-last-assign
branch
from
c138d40
to
c06499a
Compare
This comment has been minimized.
This comment has been minimized.
RenjiSann
force-pushed
the
renji/branch-on-bool-last-assign
branch
from
c06499a
to
b95334e
Compare
RenjiSann
force-pushed
the
renji/branch-on-bool-last-assign
branch
from
b95334e
to
bf51676
Compare
|
Rebase on #124399 |
I spent some time trying to find an alternative, and #124644 shows what I was able to come up with. (You were right that it's not easy! I went down several dead-ends before finally arriving at this approach.) |
|
superseded by #124644, which implement the same logic while being less intrusive in the main MIR building code. |
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
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
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
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
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
This MR adds
condition-coverageas defined in #124120 .This is done by removing the "optimization" that prevents the compiler to create a branch for the last operand of a boolean expression that is not an
if-elseblock decision.@Lambdaris
@Zalathar
@ZhuUx