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.
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.
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 (https://github.com/apple/swift/commit/cfd38c42a1d524fa6fb879190f3a48c6d9412770).
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.
Merged: a92ceea91116e7b95d23eff634507fa2cff86ef2
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?
> 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.
Here's a diff for updating the statistics in the tests: https://gist.github.com/nikic/d8fb6c12b20b60843d87b77dbefa2b48
(In reply to Zequan Wu from comment #6) > > 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.