MCDC Coverage: instrument last boolean RHS operands from condition coverage #124652
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
rustbot
added
S-waiting-on-review
RenjiSann
changed the title
RenjiSann
commented
May 3, 2024
Comment on lines
21
to
82
| LL| 4| let x = a || b && c; | ||
| ^2 ^1 | ||
| LL| 4| black_box(x); |
There was a problem hiding this comment.
It took me some times to understand why this would not create an MCDC record for the first 2 conditions.
Actually, it's just because of operator's priority. The AST looks like this
||
/ \
a &&
/ \
b c
Therefore, before the change,
- We would instrument
||'s LHS throughthen_else_break, which would not produce a record because there is only 1 condition (a) - We would repeat the same for
&&, its LHS being justb.
This emphasizes another weak point of the previous implementation :
let x = a && (b && ( c && (d && (e))));This decision wasn't recorded at all by MCDC.
There was a problem hiding this comment.
Good catch. The existing MC/DC tests are enough to demonstrate that the feature exists and functions, but aren't very thorough about testing a variety of code patterns.
(Honestly, prior to reviewing #123409 I had no idea that a && b && c normally gets parsed as (a && b) && c; I would have assumed the opposite.)
This comment has been minimized.
This comment has been minimized.
RenjiSann
force-pushed
the
mcdc-register-lazy-rhs
branch
from
c3628f1
to
75b98f5
Compare
|
Added a |
This comment has been minimized.
This comment has been minimized.
Condition coverage extends branch coverage to treat the specific case of last operands of boolean decisions not involved in control flow. This is ultimately made for MCDC to be exhaustive on all boolean expressions. This patch adds a call to `visit_branch_coverage_operation` to track the top-level operand of the said decisions, and changes `visit_coverage_standalone_condition` so MCDC branch registration is called when enabled on these _last RHS_ cases.
RenjiSann
force-pushed
the
mcdc-register-lazy-rhs
branch
from
75b98f5
to
cb3b528
Compare
|
Superseeded by #125766 |
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #124120.
Depends on @Zalathar 's condition coverage implementation #124644.