[PATCH] D151283: [llvm-cov] Support a Hierarchical Structure for HTML Coverage Report Generating

[Yuhao Gu via Phabricator via llvm-commits]

AtomicGu marked 10 inline comments as done.
AtomicGu added inline comments.


================
Comment at: llvm/test/tools/llvm-cov/directory_coverage.test:1
+// RUN: mkdir -p %t && clang -fprofile-instr-generate=%t/default.profraw -fcoverage-mapping -o %t/main.out \
+// RUN:   %S/Inputs/directory_coverage/main.cc \
----------------
phosek wrote:
> 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-cov`as 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
> 
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?


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:710
+
+  std::string S;
+  for (auto I = Components.begin(), E = Components.end();;) {
----------------
phosek wrote:
> Use `llvm::SmallString`.
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`.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:714-715
+    if (++I == E) {
+      S += a("./index.html", Name);
+      S += sys::path::get_separator();
+      break;
----------------
phosek wrote:
> Use `llvm::sys::path::append` appending to a path.
`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.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:737
+
+  std::string LinkTargetStr;
+  if (IsDir) {
----------------
phosek wrote:
> Use `llvm::SmallString` here as well.
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151283/new/

https://reviews.llvm.org/D151283