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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jun 19, 2024
Merged

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

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

@RenjiSann RenjiSann mentioned this pull request May 30, 2024
Closed
@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!

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

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

@ZhuUx ZhuUx mentioned this pull request Jun 6, 2024
Open
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

@RenjiSann
coverage: Make MCDC take in account last RHS of condition-coverage
6c7c824 
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
tests(coverage): Bless mcdc_non_control_flow tests
e15adef
@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
@fmease fmease mentioned this pull request Jun 19, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
@bors
Auto merge of rust-lang#126679 - fmease:rollup-njrv2py, r=fmease
3186d17 
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
@rust-timer
Unrolled build for rust-lang#125766
6c1a6d3 
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
This was referenced Jul 2, 2024
Merged
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 9, 2024
@bors
Auto merge of rust-lang#127234 - ZhuUx:inlined-expr, r=davidtwco,Zala…
6be96e3 
…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
Reviewers

@Zalathar Zalathar Zalathar left review comments

@nnethercote nnethercote nnethercote left review comments

Assignees

@nnethercote nnethercote

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
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.
7 participants
@RenjiSann @rustbot @fee1-dead @nnethercote @Zalathar @bors @fmease