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

MCDC Coverage: instrument last boolean RHS operands from condition coverage #125766

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

RenjiSann
Copy link
Contributor

Fresh PR from #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 #124120.
Depends on @Zalathar 's condition coverage implementation #125756.

@rustbot
Copy link
Collaborator

rustbot commented May 30, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
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. labels May 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 30, 2024

Some changes occurred in coverage tests.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

@fee1-dead
Copy link
Member

r? compiler

@rustbot rustbot assigned nnethercote and unassigned fee1-dead May 30, 2024
@nnethercote
Copy link
Contributor

I only looked at the last three commit, because the first three are from #125756.

This seems ok to me but I know very little about the coverage code. @Zalathar, could you also review, and if you are happy then I will give r+. Thanks!

@RenjiSann RenjiSann force-pushed the fresh-mcdc-branch-on-bool branch from 80c1cbf to 10d6546 Compare May 31, 2024 08:06
@Zalathar
Copy link
Contributor

Zalathar commented Jun 3, 2024

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jun 3, 2024
@RenjiSann RenjiSann force-pushed the fresh-mcdc-branch-on-bool branch from 10d6546 to cb22039 Compare June 3, 2024 09:40
@Zalathar
Copy link
Contributor

Zalathar commented Jun 4, 2024

Other than some tiny nitpicks, the main changes seem reasonable.

When the nits are addressed (fixed or justified), this should be good to go.

@RenjiSann RenjiSann force-pushed the fresh-mcdc-branch-on-bool branch from cb22039 to b0a0496 Compare June 5, 2024 13:00
@RenjiSann
Copy link
Contributor Author

All nitpicks are addressed. This is good to go :)

@ZhuUx ZhuUx mentioned this pull request Jun 6, 2024
4 tasks
@RenjiSann
Copy link
Contributor Author

Hi, @Zalathar
Can you merge this, if it's OK for you ?

@Zalathar
Copy link
Contributor

I'm happy with this, with or without addressing my tiny nitpick.

r+ from me, but I don't actually have permissions to add this to the queue on my own.

@Zalathar
Copy link
Contributor

(Sorry for the delay!)

@nnethercote
Copy link
Contributor

r=me once the expr_span nit has been addressed.

@bors delegate=RenjiSann

@bors
Copy link
Contributor

bors commented Jun 18, 2024

✌️ @RenjiSann, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

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 RenjiSann force-pushed the fresh-mcdc-branch-on-bool branch from b0a0496 to e15adef Compare June 19, 2024 07:46
@RenjiSann
Copy link
Contributor Author

Done 👍

@bors r=@nnethercote

@bors
Copy link
Contributor

bors commented Jun 19, 2024

📌 Commit e15adef has been approved by nnethercote

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 Jun 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#125447 (Allow constraining opaque types during subtyping in the trait system)
 - rust-lang#125766 (MCDC Coverage: instrument last boolean RHS operands from condition coverage)
 - rust-lang#125880 (Remove `src/tools/rust-demangler`)
 - rust-lang#126154 (StorageLive: refresh storage (instead of UB) when local is already live)
 - rust-lang#126572 (override user defined channel when using precompiled rustc)
 - rust-lang#126662 (Unconditionally warn on usage of `wasm32-wasi`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a7cf6ec into rust-lang:master Jun 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 19, 2024
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.
@bors
Copy link
Contributor

bors commented Jun 19, 2024

⌛ Testing commit e15adef with merge 3186d17...

@fmease
Copy link
Member

fmease commented Jun 19, 2024

@bors retry r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 9, 2024
…thar

[Coverage][MCDC] Group mcdc tests and fix panic when generating mcdc code for inlined expressions.

### Changes

1. Group all mcdc tests to one directory.
2. Since mcdc instruments different mappings for boolean expressions with normal branch coverage as rust-lang#125766 introduces, it would be better also trace branch coverage results in mcdc tests.
3. So far rustc does not call `CoverageInfoBuilderMethods::init_coverage` for inlined functions. As a result, it could panic if it tries to instrument mcdc statements for inlined functions due to uninitialized cond bitmaps. We can reproduce this issue by current nightly rustc and [the test](https://github.com/rust-lang/rust/pull/127234/files#diff-c81af6bf4869aa42f5c7334e3e86344475de362f673f54ce439ec75fcb5ac3e5) with flag `--release`. This patch fixes it.
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-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

7 participants