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] Revert D85036 #48641

Closed
nikic opened this issue Feb 20, 2021 · 8 comments
Closed

[llvm-cov] Revert D85036 #48641

nikic opened this issue Feb 20, 2021 · 8 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@nikic
Copy link
Contributor

nikic commented Feb 20, 2021

Bugzilla Link 49297
Resolution FIXED
Resolved on Feb 23, 2021 15:59
Version trunk
OS Linux
Blocks #48246
CC @rnk,@tstellar,@vedantk,@ZequanWu

Extended Description

https://reviews.llvm.org/D85036 has broken coverage generation in Rust, and based on the comments on the review, in Swift as well. There already is a consensus that the change is not correct and should be reverted. https://reviews.llvm.org/D97101 has been opened to address the original motivation on the clang side instead. I have verified that a simple revert does fix coverage generation in Rust.

@rnk
Copy link
Collaborator

rnk commented Feb 22, 2021

I gave Zequan some guidance suggesting that he shouldn't revert. My possibly-incomplete understanding at the time was that reverting that change will create a regression for Clang code coverage, even though it fixes Rust coverage. I felt that it was better to fix clang first, rather than reintroducing the issue. I think I was worried that reintroducing the coverage bug would jeopardize our efforts to continuously release clang in Chromium, and it was likely to be the kind of issue that wouldn't show up in release testing and that users would instead discover it weeks later. It's possible that I accidentally I put my priorities ahead of the community priorities, but I was hoping we'd get a clang fix in a matter of weeks and get this all wrapped up quickly. However, I can't claim to have a deep understanding of the coverage mapping format. If there is a good reason to expedite the llvm-cov change revert, we can do that first too.

@vedantk
Copy link
Collaborator

vedantk commented Feb 22, 2021

On the swift side, it's not urgent that D85036 be reverted. After it landed, we discovered and addressed a deeper issue in the swift implementation (apple/swift@cfd38c4).

@tstellar
Copy link
Collaborator

Thanks for the explanations. Given that the bugs fixed by D85036 were present in the release/11.x branch, I think it's best to revert this in release/12.x to avoid introducing a regression.

Once https://reviews.llvm.org/D97101 lands in main, we can look into cherry-picking it to release/12.x for the 12.0.1 release.

@tstellar
Copy link
Collaborator

Merged: a92ceea

@tstellar
Copy link
Collaborator

Reverting this patch breaks some of the llvm-cov tests in the branch:

https://github.com/llvm/llvm-project/runs/1957551005

Can someone take a look at this?

@ZequanWu
Copy link
Contributor

Reverting this patch breaks some of the llvm-cov tests in the branch:

For the three branch tests, it was because branch coverage landed after that change. The statistics it currently reports may not be correct due to the bug. I think, you can just update the statistics. I don't know about the symbolizer test failure.

@nikic
Copy link
Contributor Author

nikic commented Feb 23, 2021

Here's a diff for updating the statistics in the tests: https://gist.github.com/nikic/d8fb6c12b20b60843d87b77dbefa2b48

@tstellar
Copy link
Collaborator

Reverting this patch breaks some of the llvm-cov tests in the branch:

For the three branch tests, it was because branch coverage landed after that
change. The statistics it currently reports may not be correct due to the
bug. I think, you can just update the statistics. I don't know about the
symbolizer test failure.

The symbolizer failure is unrelated and has been already fixed.

@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

5 participants