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 retitled this revision from [llvm-cov] Support a Hierarchical Structure for HTML Report (WORK IN PROGRESS) to [llvm-cov] Support a Hierarchical Structure for `show --format=html` (WORK IN PROGRESS).May 25 2023, 3:11 AM
AtomicGu retitled this revision from [llvm-cov] Support a Hierarchical Structure for `show --format=html` (WORK IN PROGRESS) to [WIP][llvm-cov] Support a Hierarchical Structure for `show --format=html` (WORK IN PROGRESS).May 25 2023, 7:54 PM
gulfem added a comment.Jun 2 2023, 9:53 PM

It can be useful to add an option for hierarchical directory structure like other coverage viewing options (https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-cov/CoverageViewOptions.h#L30) to be able to turn it on/off:

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

Can you please make sure that you only format the lines that you change, and not format the file whole file? This makes it easier for the reviewers to see the actual changes instead of formatting changes? AFAICT, most of the changes in CoverageReport.h and CoverageReport.cpp are formatting changes.

llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
503

When the directory contains a single file, it is added to Files, but all the FileReports for Files are previously generated at line 494. So, there won't be any report generated for a single file in a directory.

AtomicGu added inline comments.Jun 4 2023, 11:00 PM
llvm/tools/llvm-cov/CoverageReport.h
47

Can you please make sure that you only format the lines that you change, and not format the file whole file? This makes it easier for the reviewers to see the actual changes instead of formatting changes? AFAICT, most of the changes in CoverageReport.h and CoverageReport.cpp are formatting changes.

47

Sorry, this is done by clang-format automatically. I will figure this out in the next commit.

llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
503

Thank you! You are right, this is a mistake. I was too careless. I will solve this in the next commit.

AtomicGu published this revision for review.Jun 4 2023, 11:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2023, 11:03 PM
gulfem added inline comments.Jun 5 2023, 6:08 PM
llvm/tools/llvm-cov/SourceCoverageViewHTML.h
47

createTopLevelIndexFile might be a better name.

AtomicGu updated this revision to Diff 529146.Jun 6 2023, 8:18 PM
AtomicGu edited the summary of this revision. (Show Details)
  • [llvm-cov] Finish my new CoveragePrinter.
  • [llvm-cov] Manual test passed
AtomicGu marked 2 inline comments as done.Jun 6 2023, 8:38 PM
AtomicGu added inline comments.
llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
503

The problem has been fixed in the new implementation.

llvm/tools/llvm-cov/SourceCoverageViewHTML.h
47

This function is to create index file in each subdirectory recursivly. I'm not sure the name "TopLevel" is more proper?

AtomicGu edited the summary of this revision. (Show Details)Jun 6 2023, 8:42 PM
AtomicGu marked an inline comment as done.Jun 7 2023, 6:45 PM
AtomicGu added inline comments.
llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
571

This line has changed to LCP = sys::path::parent_path(SourceFiles.front()).size() + 1;

AtomicGu updated this revision to Diff 529607.Jun 8 2023, 8:37 AM
  • [llvm-cov] Add navigation link to the title
gulfem added inline comments.Jun 9 2023, 4:35 PM
llvm/tools/llvm-cov/SourceCoverageView.cpp
81 ↗(On Diff #529607)

Some llvm-cov tests like llvm-cov::branch-c-general.test are failing in the presubmit tests. The reason is that you are changing the default behavior and some tests rely on that behavior. As a transitioning step, I would suggest adding a new option like showDirectoryCoverage into https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-cov/CoverageViewOptions.h#L30, and using the associated class based on that option like:

if (Opts.ShowDirectoryCoverage)
  return std::make_unique<CoveragePrinterHTMLDirectory>(Opts);
else
  return std::make_unique<CoveragePrinterHTML>(Opts);
llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
364

Please remove the whitespace at the end of this line.

652

I would suggest using the words CurrentFiles, CurrentDirs and CurrentTotals.

678–680

This is another formatting change.

687

Make sure that else starts after this curly base as stated in LLVM Coding Standards.
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

750

Use a curly brace for the else part make it uniform.

758

Can you use a long name here? What does buildNavLink mean? Try to use a full name unless the abbreviation is clear.

AtomicGu updated this revision to Diff 531259.Jun 14 2023, 4:35 AM
AtomicGu marked 7 inline comments as done.
  • [llvm-cov] Finish the new CoveragePrinterHTMLDirectory.
  • [llvm-cov] The new CoveragePrinterHTMLDirectory passed manual test.
  • [llvm-cov] Add navigation links to the title of the index file.
  • [llvm-cov] Add additional command line options.
  • [llvm-cov] Accept advices from Gulfem's review.
AtomicGu accepted this revision.Jun 14 2023, 4:35 AM
This revision is now accepted and ready to land.Jun 14 2023, 4:35 AM

Here is a preview coverage report for llvm-cov llvm-profdata lli and llc :

Here is a preview coverage report for llvm-cov llvm-profdata lli and llc :

Provided preview contains wrong relative path to style.css. For example, it should be href='../../../../../style.css' in file coverage/root/GSoC/llvm-project/llvm/index.html.

AtomicGu updated this revision to Diff 532112.Jun 16 2023, 5:53 AM
  • [llvm-cov] Fix links to CSS in index files.
  • [llvm-cov] Improve the title of index files.
AtomicGu updated this revision to Diff 532115.EditedJun 16 2023, 5:57 AM

Reverse formatting changes.

Here is a preview coverage report for llvm-cov llvm-profdata lli and llc :

Provided preview contains wrong relative path to style.css. For example, it should be href='../../../../../style.css' in file coverage/root/GSoC/llvm-project/llvm/index.html.

Thank you. This issue has been fixed. I was testing by python -m http.server remotely and not aware of this.

AtomicGu updated this revision to Diff 532346.Jun 16 2023, 7:10 PM
  • [llvm-cov] Update documents.
AtomicGu retitled this revision from [WIP][llvm-cov] Support a Hierarchical Structure for `show --format=html` (WORK IN PROGRESS) to [WIP][llvm-cov] Support a Hierarchical Structure for HTML Coverage Report Generating.Jun 16 2023, 7:22 PM
AtomicGu edited the summary of this revision. (Show Details)
AtomicGu requested review of this revision.Jul 8 2023, 8:13 PM
AtomicGu updated this revision to Diff 538400.Jul 8 2023, 8:35 PM
  • [llvm-cov] Fix a format issue of git-clang-format.
AtomicGu retitled this revision from [WIP][llvm-cov] Support a Hierarchical Structure for HTML Coverage Report Generating to [llvm-cov] Support a Hierarchical Structure for HTML Coverage Report Generating.Jul 9 2023, 6:09 AM

@davidxl @phosek @gulfem
I removed the WIP tag. Please review this! 😀
The new tests will be added in the comming few days.

phosek added inline comments.Jul 9 2023, 3:07 PM
llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
558–564

This code is duplicated between CoveragePrinterHTML::createIndexFile and CoveragePrinterHTMLDirectory::createIndexFile, could it be extracted into a helper function?

620–645

This code is duplicated between CoveragePrinterHTML::createIndexFile and CoveragePrinterHTMLDirectory::createSubIndexFile, could it be extracted into one or more helper functions?

713–734

This code is duplicated between CoveragePrinterHTML::createIndexFile and CoveragePrinterHTMLDirectory::createSubIndexFile, could it be extracted into one or more helper functions?

AtomicGu marked 2 inline comments as done.Jul 10 2023, 5:17 AM
AtomicGu added inline comments.
llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
558–564

This can be extracted to a helper method of CoveragePrinterHTML.

Personally, I tolerate code repetition for modification flexibility if it's not more than twrice. If it is not welcomed in LLVM community, I will fix it.

AtomicGu added inline comments.Jul 10 2023, 5:52 AM
llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
713–734

These codes are tightly coupled with variables around them. It may needs more boilerplate code than the code extracted. I don't think it will make the code easier to read to extract them into a common helper function.

AtomicGu updated this revision to Diff 538599.Jul 10 2023, 5:55 AM
This comment was removed by AtomicGu.
AtomicGu updated this revision to Diff 538601.Jul 10 2023, 5:58 AM
  • [llvm-cov] Extract some common code to 2 methods.
AtomicGu updated this revision to Diff 539023.Jul 11 2023, 4:44 AM
AtomicGu marked an inline comment as done.
  • [llvm-cov] Add new tests for directory coverage.
AtomicGu edited the summary of this revision. (Show Details)Jul 11 2023, 4:48 AM
phosek added inline comments.Jul 12 2023, 12:56 AM
llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
349

This is a just a suggestion, but I think that emitPrelude might be a better name.

AtomicGu marked an inline comment as done.Jul 12 2023, 10:20 PM
AtomicGu added inline comments.
llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
349

This name has been used by another function in the anonymous namespace in SourceCoverageViewHTML.cpp. So I chose a different name to avoid confusion, although no conflict would be caused if I used emitPrelude since the arguments are different.

phosek added inline comments.Jul 13 2023, 12:18 AM
llvm/test/tools/llvm-cov/directory_coverage.test
1 ↗(On Diff #539023)

Your test cannot assume that clang is available on the host since Clang is not one of LLVM's prerequisites, and we want LLVM to remain compiler agnostic. That's why you're seeing the issue on Windows pre-merge builders: Clang happens to be available on these builders, but it doesn't include the profile runtime, which is perfectly valid because we don't require it.

What we do instead is to either include raw profile in the .proftext textual format and then convert it to .profdata using llvm-profdata or include .profdata directly, plus include coverage mapping information in the textual format as .covmapping, and then run llvm-covas part of the test. We also include instructions for recreating the .profdata and .covmapping files inside the test.

Here are some examples as an inspiration:

https://github.com/llvm/llvm-project/blob/96ae0851c26237378fa1280b0a9ad713e1b72bdb/llvm/test/tools/llvm-cov/coverage-prefix-map.test
https://github.com/llvm/llvm-project/blob/96ae0851c26237378fa1280b0a9ad713e1b72bdb/llvm/test/tools/llvm-cov/multiple-objects.test

llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
349

I see, how about emitReportHeader?

633–634

LLVM's StringMap is more efficient than std::map<StringRef, ...>.

634

We prefer LLVM SmallVector unless there's a specific reason for using std::vector.

723–734

Can you add some comments to explain the logic in this block?

736

Use llvm::SmallString.

740–741

Use llvm::sys::path::append appending to a path.

763

Use llvm::SmallString here as well.

gulfem added a comment.EditedJul 15 2023, 9:08 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."

llvm/docs/CommandGuide/llvm-cov.rst
256 ↗(On Diff #538601)

How about we stop the description before the parenthesis? I think "impossible to read" is a strong statement.

gulfem added inline comments.Jul 15 2023, 9:33 AM
llvm/test/tools/llvm-cov/directory_coverage.test
4 ↗(On Diff #539023)

I would use b0 for this directory name to be consistent overall the test.

AtomicGu marked 10 inline comments as done.Jul 17 2023, 1:29 AM
AtomicGu added inline comments.
llvm/test/tools/llvm-cov/directory_coverage.test
1 ↗(On Diff #539023)

I tried this today. llvm-cov convert-for-testing works well for single source file program. But when I used it for multiple source files, the generated .covmapping file is failed to be load by llvm-cov show:

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

The commands used is same with those in coverage-prefix-map.test:

cp %S/Inputs/directory_coverage -r . /tmp
cd /tmp/directory_coverage
clang -fprofile-instr-generate -mllvm -enable-name-compression=false \
  -fcoverage-mapping -fcoverage-prefix-map=$PWD=. -o main \
  main.cc a0/a1/a2.cc b0/b1_1.cc b0/b1_2.cc c0/c1/c2_1.cc c0/c1/c2_2.cc
LLVM_PROFILE_FILE="main.raw" ./main
llvm-profdata merge main.raw -o main.profdata
llvm-cov convert-for-testing ./main -o main.covmapping
rm main main.raw

llvm-cov report -path-equivalence=.,/tmp/coverage_prefix_map \
  -instr-profile main.profdata main.covmapping

Is this a bug or a designed behavior? Can I just include the program file directly for now?

llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
736

I believe that S is not small. It contains hyperlinks <a href="..."> </a> for each level. So I think it can easily exceed 64 chars or more in large projects.

Also it's the return value. I choose std::string for the return type because the original code uses that. For example, CoveragePrinterHTML::buildLinkToFile returns a std::string.

740–741

S is not a path. It's a html snippet like:

<a href="../../index.html">/root</a>/<a href="./index.html">c0/c1</a>/

Here I just need native separators to show between the <a> tags.

763

I use std::string here because the parameters of the function a(..., ...) is const std::string &. In fact, the two helper function tag and a all uses const std::string &. Do I need to refactor them?

AtomicGu marked 3 inline comments as done.Jul 17 2023, 1:48 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.

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
516

@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
516

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
516

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
516

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 ↗(On Diff #552263)

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

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

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
101
126

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
501

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
126

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 ↗(On Diff #552590)

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 ↗(On Diff #552590)

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.