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

Fix unreachable coverage generation for inlined functions #98868

Merged
merged 4 commits into from
Jul 22, 2022

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jul 3, 2022

To generate a function coverage we need at least one coverage counter,
so a coverage from unreachable blocks is retained only when some live
counters remain.

The previous implementation incorrectly retained unreachable coverage,
because it didn't account for the fact that those live counters can
belong to another function due to inlining.

Fixes #98833.

@tmiasko tmiasko added A-mir-opt-inlining Area: MIR inlining A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Jul 3, 2022
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 3, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-highfive
Copy link
Collaborator

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2022
@tmiasko
Copy link
Contributor Author

tmiasko commented Jul 3, 2022

r? @tmandry @richkadel

@rust-highfive rust-highfive assigned tmandry and unassigned nagisa Jul 3, 2022
@alex
Copy link
Member

alex commented Jul 3, 2022

Thank you for diving into this -- sorry my test case wasn't especially reduced!

@richkadel
Copy link
Contributor

I'm out of office so I can't look at this in detail, but thanks for taking care of this! I scanned the source code changes and didn't see anything that stood out, except for some double nested loops. Not necessarily bad but just something maybe to do a performance test against if Tyler agrees.

@bors
Copy link
Contributor

bors commented Jul 7, 2022

☔ The latest upstream changes (presumably #99014) made this pull request unmergeable. Please resolve the merge conflicts.

tmiasko added 3 commits July 8, 2022 09:19
To generate a function coverage we need at least one coverage counter,
so a coverage from unreachable blocks is retained only when some live
counters remain.

The previous implementation incorrectly retained unreachable coverage,
because it didn't account for the fact that those live counters can
belong to another function due to inlining.
@tmiasko tmiasko force-pushed the unreachable-coverage branch from c8e80f9 to 62ab4b6 Compare July 8, 2022 07:24
@gshep
Copy link

gshep commented Jul 21, 2022

Hi! Any update on this?

@alex
Copy link
Member

alex commented Jul 21, 2022

It's awaiting a review.

@pnkfelix
Copy link
Member

@rustbot assign @wesleywiser

@rustbot rustbot assigned wesleywiser and unassigned tmandry Jul 21, 2022
@wesleywiser
Copy link
Member

Thanks @tmiasko!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 21, 2022

📌 Commit 9473141 has been approved by wesleywiser

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 Jul 21, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 21, 2022
…leywiser

Fix unreachable coverage generation for inlined functions

To generate a function coverage we need at least one coverage counter,
so a coverage from unreachable blocks is retained only when some live
counters remain.

The previous implementation incorrectly retained unreachable coverage,
because it didn't account for the fact that those live counters can
belong to another function due to inlining.

Fixes rust-lang#98833.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 22, 2022
…leywiser

Fix unreachable coverage generation for inlined functions

To generate a function coverage we need at least one coverage counter,
so a coverage from unreachable blocks is retained only when some live
counters remain.

The previous implementation incorrectly retained unreachable coverage,
because it didn't account for the fact that those live counters can
belong to another function due to inlining.

Fixes rust-lang#98833.
This was referenced Jul 22, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#98174 (Rename `<*{mut,const} T>::as_{const,mut}` to `cast_`)
 - rust-lang#98868 (Fix unreachable coverage generation for inlined functions)
 - rust-lang#99393 (feat: omit suffixes in const generics (e.g. `1_i32`))
 - rust-lang#99423 (Group CSS font rule)
 - rust-lang#99539 (Improve suggestions for returning binding)
 - rust-lang#99579 (Add same warning to Result::expect as Result::unwrap)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6e3dd69 into rust-lang:master Jul 22, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 22, 2022
@tmiasko tmiasko deleted the unreachable-coverage branch July 24, 2022 08:15
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) A-mir-opt-inlining Area: MIR inlining 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.

Recent nightly started ICEing with "No counters provided the source_hash for used function"