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

[LLVM-COV] Fix branch coverage merging across function instantiations #49738

Closed
evodius96 opened this issue May 18, 2021 · 10 comments
Closed

[LLVM-COV] Fix branch coverage merging across function instantiations #49738

evodius96 opened this issue May 18, 2021 · 10 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@evodius96
Copy link
Contributor

Bugzilla Link 50394
Resolution FIXED
Resolved on Jun 17, 2021 10:46
Version 12.0
OS All
Blocks #48661
CC @tstellar
Fixed by commit(s) eccb925 b89942c

Extended Description

When I made the upstream commit for branch coverage support in code-coverage in January, I made an incorrect assumption that branch coverage numbers (covered branch and total branches) should be accumulated across function instantiations. This is not what is done for lines and regions, where the maximum line/region coverage found in a function instantiation across an instantiation group is returned. For example, if a function template definition has 2 total branches, the branch coverage would be reported as an accumulated total across all instantiations (8 total branches across 4 instantiations).

The correct assumption for the FunctionCoverageSummary::get(const InstantiationGroup &Group, ...) routine is that the summary it returns should agree with the function definition in the source code on lines, regions, and branches. So we should do the same thing for branch coverage as we do for line and region coverage. If a function template definition has 2 total branches, the summary should also reflect branch coverage for 2 total branches.

Code Review for fix:
https://reviews.llvm.org/D102193

Main branch commit for fix:
https://reviews.llvm.org/rGeccb925147d5f262a3e74cc050d0665dd4e6d8db

This fix needs to be applied to the 12.x release branch.

@tstellar
Copy link
Collaborator

Hi Todd,

What is your opinion on backporting this?

https://reviews.llvm.org/rGeccb925147d5f262a3e74cc050d0665dd4e6d8db

@tstellar
Copy link
Collaborator

Merged: b89942c

@tstellar
Copy link
Collaborator

Merged: b89942c

Ignore this, the automation was confused by the revert.

@llvmbot
Copy link
Member

llvmbot commented Jun 1, 2021

I'm OK with backporting this to the 12.x release branch

@tstellar
Copy link
Collaborator

tstellar commented Jun 4, 2021

What is the final version of the fix that needs to be backported?

@evodius96
Copy link
Contributor Author

Hi Tom,

The final version of the fix is what I originally reported and tried to merge to the branch before. There haven't been any changes since then. Let me know if that's not what you are referring to.

https://reviews.llvm.org/rGeccb925147d5f262a3e74cc050d0665dd4e6d8db

@tstellar
Copy link
Collaborator

tstellar commented Jun 4, 2021

Merged: b89942c

@evodius96
Copy link
Contributor Author

Hi Tom -- I don't yet see this merge on the 12.x branch. The commit SHA looks to be the original commit we did that was later reverted. Not sure if something got mixed up here. Thanks!

@tstellar
Copy link
Collaborator

Do I just need to unrevert b89942c to fix this bug?

@evodius96
Copy link
Contributor Author

Yes, unreverting it should do the trick. Thanks!

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

3 participants