This is an archive of the discontinued LLVM Phabricator instance.

[llvm-cov] Support directory layout in coverage reports
ClosedPublic

Authored by AtomicGu on May 23 2023, 8:42 PM.

Details

Summary

This is a GSoC 2023 project (discourse link).

llvm-cov currently generates a single top-level index HTML file, which causes rendering scalability issues in large projects. This patch adds support for hierarchical directory structure into the HTML reports to solve scalability issues by introducing the following changes:

  • Added a new command line option --show-directory-coverage for llvm-cov show. It works both for --format=html and --format=text.
  • Two new classes: CoveragePrinterHTMLDirectory and CoveragePrinterTextDirectory was added to support the new option.
  • A tool class DirectoryCoverageReport was added to support the two classes above.
  • Updated the document.
  • Added a new regression test for --show-directory-coverage.

Here is a coverage report preview of LLVM:

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
AtomicGu updated this revision to Diff 540910.Jul 17 2023, 1:51 AM
  • [llvm-cov] Take advices from phosek and gulfem.
AtomicGu updated this revision to Diff 541411.Jul 18 2023, 2:32 AM
  • [llvm-cov] --show-directory_coverage for text.

I refactored the HTML printer and extract the recursion part into a common template method pattern class named "DirectoryCoverageReport".

Manual test is passed (both on linux and windows).

AtomicGu retitled this revision from [llvm-cov] Support a Hierarchical Structure for HTML Coverage Report Generating to [llvm-cov] Support a Hierarchical Structure for Coverage Report Generating.Jul 18 2023, 2:41 AM
AtomicGu edited the summary of this revision. (Show Details)
gulfem added a comment.EditedJul 18 2023, 8:18 AM

Can we simplify the commit message? We do not need all the details in the commit message as we have them in the Discourse discussion. I would suggest something like: "llvm-cov currently generates a single top-level index HTML file, which causes rendering scalability issues in large projects. This patch adds support for hierarchical directory structure into the HTML reports to solve scalability issues."

Do you mean I should squash the commits into one single commit? I thought Phabricator would do this for me as I always use arc diff with origin/main. I'm not sure what should I do.

Current commit message looks like this:

[llvm-cov] Support a Hierarchical Structure for HTML Coverage Report Generating
   
> This is a GSoC23 project ([discourse link](https://discourse.llvm.org/t/coverage-support-a-hierarchical-directory-structure-in-generated-coverage-html-reports/68239/7)).

Clang supports [source-based coverage](https://clang.llvm.org/docs/SourceBasedCodeCoverage.html) that shows which lines of code are covered by the executed tests. It uses [llvm-profdata](https://llvm.org/docs/CommandGuide/llvm-profdata.html) and [llvm-cov](https://llvm.org/docs/CommandGuide/llvm-cov.html) tools to generate coverage reports. llvm-cov currently generates a single top-level index HTML file. For example, a single top-level directory [code coverage report](https://lab.llvm.org/coverage/coverage-reports/index.html) for LLVM repo is published on a coverage bot. Top-level indexing causes rendering scalability issues in large projects, such as [Fuchsia](https://fuchsia.dev). The goal of this project is to generate a hierarchical directory structure in generated coverage html reports to match the directory structure and solve scalability issues. Chromium uses its own [post-processing tools](https://source.chromium.org/chromium/chromium/tools/build/+/main:recipes/recipe_modules/code_coverage/resources/generate_coverage_metadata.py) to show a per-directory hierarchical structure for [coverage results](https://analysis.chromium.org/coverage/p/chromium). Similarly, [Lcov](https://llvm.org/reports/coverage/index.html), which is a graphical front-end [Gcov](https://gcc.gnu.org/onlinedocs/gcc/Gcov.html), provides a one-level directory structure to display coverage results.

Changes to the code:

- Added a new CoveragePrinterHTMLDirectory by inheriting the original CoveragePrinterHTML.
- Added a new command line option `--show-directory-coverage` to select the new CoveragePrinterHTMLDirectory.
- Updated the coverage report generating script of LLVM itself to use the new option.
- Updated the document.
- Added a new regression test for `--show-directory-coverage`.

Currently the new option only works with `--format=html`. There is a plan to support `--format=text` too in the future.

I'm suggesting removing the first paragraph:

[llvm-cov] Support directory layout in coverage reports 
    
This is a GSoC 2023 project ([discourse link](https://discourse.llvm.org/t/coverage-support-a-hierarchical-directory-structure-in-generated-coverage-html-reports/68239)).
 
llvm-cov currently generates a single top-level index HTML file, which causes rendering scalability issues in large projects. This patch adds support for hierarchical directory structure into the HTML reports to solve scalability issues by introducing the following changes:
 - Added a new CoveragePrinterHTMLDirectory by inheriting the original CoveragePrinterHTML.
 - Added a new command line option `--show-directory-coverage` to select the new CoveragePrinterHTMLDirectory.
 - Updated the coverage report generating script of LLVM itself to use the new option.
 - Updated the document.
 - Added a new regression test for `--show-directory-coverage`.
 
 Currently the new option only works with `--format=html`. There is a plan to support `--format=text` too in the future.
AtomicGu updated this revision to Diff 541575.Jul 18 2023, 8:47 AM
  • [llvm-cov] Add regression tests.

llvm-cov convert-for-testing not works for me. When I use it for program with multiple source files, the generated .covmapping file can not be read back by llvm-cov:

error: Failed to load coverage: 'main.covmapping': Malformed coverage data

So, I just include two program files (one for Linux and the other one for Windows) directly.

AtomicGu retitled this revision from [llvm-cov] Support a Hierarchical Structure for Coverage Report Generating to [llvm-cov] Support directory layout in coverage reports .Jul 18 2023, 8:49 AM
AtomicGu edited the summary of this revision. (Show Details)

@gulfem OK, I have updated the title and summary. I hope this will do.

AtomicGu updated this revision to Diff 541867.Jul 19 2023, 12:51 AM
AtomicGu retitled this revision from [llvm-cov] Support directory layout in coverage reports to [llvm-cov] Support directory layout in coverage reports.
  • [llvm-cov] Add regression tests.

I think they can pass the pre-merge checks now.

phosek added inline comments.Jul 19 2023, 12:52 AM
llvm/test/tools/llvm-cov/directory_coverage.test
1 โ†—(On Diff #539023)

That's not expected, I'd try to investigate this further. Did you build all the tools yourself from the latest revision? Did you enable zlib?

We definitely don't want to include the program file directly since that's going to be in the Git history forever and will take extra space.

AtomicGu marked 2 inline comments as done.Jul 19 2023, 1:30 AM
AtomicGu added inline comments.
llvm/test/tools/llvm-cov/directory_coverage.test
1 โ†—(On Diff #539023)

I pulled the newest commit and tried again. The problem still exists.

I checked the CMakeCache.txt. LLVM_ENABLE_ZLIB is ON.

Here are the source code, program, profdata and covmapping files:

Commands to reproduce the issue:

llvm-cov report main.covmapping --instr-profile main.profdata
gulfem added inline comments.Jul 19 2023, 11:31 AM
llvm/test/tools/llvm-cov/directory_coverage.test
1 โ†—(On Diff #539023)

I was able to reproduce it locally. AFAICT, the error is thrown in the following line:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp#L720

The issue seems about the filenames. I did not have much time to dig into that yet, but we might be uncovering an issue about convert-for-testing flag in llvm-cov.

@gulfem OK, I have updated the title and summary. I hope this will do.

Thanks, that looks better.

AtomicGu updated this revision to Diff 542338.Jul 20 2023, 12:35 AM
AtomicGu marked an inline comment as done.
  • [llvm-cov] Add regression tests.
AtomicGu updated this revision to Diff 550187.Aug 14 2023, 9:04 PM
  • [llvm-cov] Use .covmapping files for tests.
AtomicGu marked an inline comment as done.Aug 14 2023, 9:18 PM

@gulfem @phosek Hi. Do I need to update the Release Notes to indicate the new llvm-cov functionality?

AtomicGu updated this revision to Diff 550603.Aug 15 2023, 9:39 PM
This comment was removed by AtomicGu.
AtomicGu updated this revision to Diff 550604.Aug 15 2023, 9:45 PM

Update differential based on D156611.

AtomicGu added inline comments.Aug 18 2023, 8:11 PM
llvm/tools/llvm-cov/CoverageReport.cpp
574

@gulfem @phosek Any advises for this problem?

AtomicGu updated this revision to Diff 551743.EditedAug 19 2023, 5:46 AM

Rebase to D156611.

AtomicGu updated this revision to Diff 551800.Aug 19 2023, 7:31 PM

Linux tests passed.

AtomicGu updated this revision to Diff 551823.Aug 20 2023, 6:03 AM
  • Pass windows tests and fix COFF support of convert-for-testing.
  • Disable name compression of directory_coverage.linux.test
AtomicGu marked an inline comment as not done.Aug 20 2023, 7:56 PM

@phosek @gulfem All tests have been passed. The patch is ready to land, unless you may want to check this problem: https://reviews.llvm.org/D151283#inline-1531509 .

gulfem added inline comments.Aug 21 2023, 8:22 PM
llvm/tools/llvm-cov/CoverageReport.cpp
574

Can you use sys::path::native to convert the path to native path? I looked at native_separators.c test:
https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-cov/native_separators.c

It's a Windows test, but its *.covmapping file input does not include backslashes for the filenames because it converts the path read to native path:
https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-cov/Inputs/native_separators.covmapping

So, what I was thinking is that you can have two separate tests directory_coverage.linux.test and directory_coverage.win.test, but they use the same inputs for *.profdata and *.covmapping files. Do you think that would work? If not, having two separate files with their own inputs should be fine.

AtomicGu marked an inline comment as done.Aug 21 2023, 11:03 PM
AtomicGu added inline comments.
llvm/tools/llvm-cov/CoverageReport.cpp
574

Actually there's another difference between the two tests: the linux test uses absolute path in the .covmapping file but the windows test uses relative path.

The key problem here is: do we want to support generating reports for inputs of different platforms? I think native_separators.c answers "yes", so I will follow it.

phosek added inline comments.Aug 21 2023, 11:40 PM
llvm/tools/llvm-cov/CoverageReport.cpp
574

Yes, I think so, we don't currently restrict handling of inputs to a particular platform so this would be a regression. I agree that using sys::path::native to convert all paths to native format is likely the right approach.

AtomicGu updated this revision to Diff 552247.Aug 22 2023, 12:56 AM
AtomicGu marked an inline comment as done.
  • Use same input for linux and windows test.
AtomicGu updated this revision to Diff 552263.Aug 22 2023, 1:36 AM
  • Use sys::path::native to suport showing on different platforms.
gulfem accepted this revision.Aug 22 2023, 1:39 PM
This revision is now accepted and ready to land.Aug 22 2023, 1:39 PM
phosek added inline comments.Aug 22 2023, 4:03 PM
llvm/docs/CommandGuide/llvm-cov.rst
254โ€“256

I think this would be a better summary of what this flag does.

llvm/tools/llvm-cov/CoverageReport.cpp
559

Rather than using an assert which would be compiled out when assertions are disabled, I'd consider returning an Error in this case.

llvm/tools/llvm-cov/CoverageReport.h
105
130

Is there a reason for using an operator here rather than a regular method?

AtomicGu marked 5 inline comments as done.Aug 22 2023, 8:28 PM
AtomicGu added inline comments.
llvm/tools/llvm-cov/CoverageReport.cpp
559

This assertion is to prevent coding errors. In a correctly written program, it cannot fail. All its callers, prepareSubDirectoryReports and prepareDirectoryReports, won't pass an empty Files to it. The only exception is that the prepareDirectoryReports is used incorrectly. So, I think returning an` Error` is unnecessary and might cause runtime overhead, potentially delaying the discovery of bugs.

llvm/tools/llvm-cov/CoverageReport.h
130

No. I was thinking this class would be used like functions, but so far it's not. I'm going to rename it to generateSubDirectoryReport.

AtomicGu updated this revision to Diff 552589.Aug 22 2023, 8:54 PM
AtomicGu marked 2 inline comments as done.
  • Rename operator()() and revise comments.

@phosek @gulfem The tests finished. I think we can land the patch now. One test failed because of timeout, but it has nothing to do with this patch.

AtomicGu updated this revision to Diff 552590.Aug 22 2023, 9:36 PM

Retry per-merge checks.

Acturally, I have got the commit access from Chris (๐Ÿ˜Š). But I'm not sure how to do that. Accroding to the documentation: https://llvm.org/docs/Contributing.html#commit-from-git . Should I use git push to commit? How to squash all the commits into a single one and set the commit message correctly? Does arc have some automatic commands?

phosek accepted this revision.Aug 23 2023, 3:21 PM
phosek added inline comments.
llvm/utils/prepare-code-coverage-artifact.py
70

Would it be possible to drop this change from this patch and do it as a separate change? That way, if the coverage bot breaks for some reason, we can only revert a much smaller change rather than having to revert this entire change.

AtomicGu marked an inline comment as done.Aug 23 2023, 9:10 PM
AtomicGu added inline comments.
llvm/utils/prepare-code-coverage-artifact.py
70

Nice advice. So thoughtful of you! I will do this.

AtomicGu updated this revision to Diff 552986.Aug 23 2023, 9:57 PM
AtomicGu marked an inline comment as done.
  • Revert llvm/utils/prepare-code-coverage-artifact.py
AtomicGu edited the summary of this revision. (Show Details)Aug 23 2023, 10:15 PM
This revision was landed with ongoing or failed builds.Aug 23 2023, 10:46 PM
This revision was automatically updated to reflect the committed changes.