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

Work around missing code coverage data causing llvm-cov failures #93144

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Jan 21, 2022

If we do not add code coverage instrumentation to the Body of a
function, then when we go to generate the function record for it, we
won't write any data and this later causes llvm-cov to fail when
processing data for the entire coverage report.

I've identified two main cases where we do not currently add code
coverage instrumentation to the Body of a function:

  1. If the function has a single BasicBlock and it ends with a
    TerminatorKind::Unreachable.

  2. If the function is created using a proc macro of some kind.

For case 1, this is typically not important as this most often occurs as
a result of function definitions that take or return uninhabited
types. These kinds of functions, by definition, cannot even be called so
they logically should not be counted in code coverage statistics.

For case 2, I haven't looked into this very much but I've noticed while
testing this patch that (other than functions which are covered by case

  1. the skipped function coverage debug message is occasionally triggered
    in large crate graphs by functions generated from a proc macro. This may
    have something to do with weird spans being generated by the proc macro
    but this is just a guess.

I think it's reasonable to land this change since currently, we fail to
generate any results from llvm-cov when a function has no coverage
instrumentation applied to it. With this change, we get coverage data
for all functions other than the two cases discussed above.

Fixes #93054 which occurs because of uncallable functions which shouldn't
have code coverage anyway.

I will open an issue for missing code coverage of proc macro generated
functions and leave a link here once I have a more minimal repro.

r? @tmandry
cc @richkadel

@wesleywiser wesleywiser added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jan 21, 2022
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 21, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2022
@wesleywiser
Copy link
Member Author

Just FYI, I spent a good chunk of today trying a different approach to resolve this issue which was to identify functions that are uncallable and filter them out before we call define_unused_fn on them. That change ended up being much more complex, didn't do anything for proc macro generated functions and caused an incremental compilation ICE that I wasn't able to track down.

Given those factors, this seemed like the better approach to me as we shouldn't be generating invalid (empty) coverage_mapping_buffer values anyway which is what leads to llvm-cov failing with the "Truncated coverage data" error.

@wesleywiser wesleywiser force-pushed the uninhabited_type_code_cov2 branch from 4835793 to 02bdfa4 Compare January 21, 2022 17:35
@wesleywiser wesleywiser force-pushed the uninhabited_type_code_cov2 branch 2 times, most recently from ddae6a7 to 7eb56cb Compare January 21, 2022 19:02
@rust-log-analyzer

This comment has been minimized.

If we do not add code coverage instrumentation to the `Body` of a
function, then when we go to generate the function record for it, we
won't write any data and this later causes llvm-cov to fail when
processing data for the entire coverage report.

I've identified two main cases where we do not currently add code
coverage instrumentation to the `Body` of a function:

  1. If the function has a single `BasicBlock` and it ends with a
     `TerminatorKind::Unreachable`.

  2. If the function is created using a proc macro of some kind.

For case 1, this typically not important as this most often occurs as
the result of function definitions that take or return uninhabited
types. These kinds of functions, by definition, cannot even be called so
they logically should not be counted in code coverage statistics.

For case 2, I haven't looked into this very much but I've noticed while
testing this patch that (other than functions which are covered by case
1) the skipped function coverage debug message is occasionally triggered
in large crate graphs by functions generated from a proc macro. This may
have something to do with weird spans being generated by the proc macro
but this is just a guess.

I think it's reasonable to land this change since currently, we fail to
generate *any* results from llvm-cov when a function has no coverage
instrumentation applied to it. With this change, we get coverage data
for all functions other than the two cases discussed above.
@wesleywiser wesleywiser force-pushed the uninhabited_type_code_cov2 branch from 7eb56cb to 1a0278e Compare January 21, 2022 19:39
Copy link
Contributor

@richkadel richkadel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I guess we still have the side effect of adding the unused function stubs without including them in coverage; but as I said, the process for computing the counter_regions is somewhat complex, and not easily cacheable, so I think this should work for now.

I'll file and issue to see if I can improve it so we can avoid adding the function.

Thanks Wesley!

@tmandry
Copy link
Member

tmandry commented Jan 24, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 24, 2022

📌 Commit 1a0278e has been approved by tmandry

@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 Jan 24, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 25, 2022
…ov2, r=tmandry

Work around missing code coverage data causing llvm-cov failures

If we do not add code coverage instrumentation to the `Body` of a
function, then when we go to generate the function record for it, we
won't write any data and this later causes llvm-cov to fail when
processing data for the entire coverage report.

I've identified two main cases where we do not currently add code
coverage instrumentation to the `Body` of a function:

  1. If the function has a single `BasicBlock` and it ends with a
     `TerminatorKind::Unreachable`.

  2. If the function is created using a proc macro of some kind.

For case 1, this is typically not important as this most often occurs as
a result of function definitions that take or return uninhabited
types. These kinds of functions, by definition, cannot even be called so
they logically should not be counted in code coverage statistics.

For case 2, I haven't looked into this very much but I've noticed while
testing this patch that (other than functions which are covered by case
1) the skipped function coverage debug message is occasionally triggered
in large crate graphs by functions generated from a proc macro. This may
have something to do with weird spans being generated by the proc macro
but this is just a guess.

I think it's reasonable to land this change since currently, we fail to
generate *any* results from llvm-cov when a function has no coverage
instrumentation applied to it. With this change, we get coverage data
for all functions other than the two cases discussed above.

Fixes rust-lang#93054 which occurs because of uncallable functions which shouldn't
have code coverage anyway.

I will open an issue for missing code coverage of proc macro generated
functions and leave a link here once I have a more minimal repro.

r? `@tmandry`
cc `@richkadel`
This was referenced Jan 25, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2022
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#88794 (Add a `try_clone()` function to `OwnedFd`.)
 - rust-lang#93064 (Properly track `DepNode`s in trait evaluation provisional cache)
 - rust-lang#93118 (Move param count error emission to end of `check_argument_types`)
 - rust-lang#93144 (Work around missing code coverage data causing llvm-cov failures)
 - rust-lang#93169 (Fix inconsistency of local blanket impls)
 - rust-lang#93175 (Implement stable overlap check considering negative traits)
 - rust-lang#93251 (rustdoc settings: use radio buttons for theme)
 - rust-lang#93269 (Use error-on-mismatch policy for PAuth module flags.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8dddc86 into rust-lang:master Jan 25, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 25, 2022
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-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.

LLVM coverage regression in nightly-2022-01-15
7 participants