llvm-cov HTML Generation
ClosedPublic

Authored by vsk on Mar 18 2016, 1:35 PM.

Details

Summary

Add support to the llvm-cov show command for creating html reports.

This adds a new SourceCoverageView and a new CoveragePrinter specialized for emitting html. I modified two existing tests used for validating textual reports to validate the html.

I used an instrumented llvm-as tool to assemble test/Bitcode/compatibility.ll, and generated a coverage report based on the results. You can get the report here: bitcode-compatibility-coverage.dir.tar.gz.

Based on a patch by Harlan Haskins!

Diff Detail

Repository
rL LLVM
harlanhaskins retitled this revision from to llvm-cov HTML Generation.Mar 18 2016, 1:35 PM
harlanhaskins updated this object.
davidxl added inline comments.Mar 18 2016, 2:03 PM
test/tools/llvm-cov/showLineExecutionCountsHTML.cpp
1 ↗(On Diff #51064)

Please avoid checking in binary profdata when possible. You can convert the profdata into text format by using

llvm-profdata merge -o ... -text <binary_profdata>

After that you can add a llvm-profdata step in the test to generate indexed profile from text profile before feeding to llvm-cov

8 ↗(On Diff #51064)

Can you align the checks properly with the line it is covering? In the current form, it is hard for me to look through it and check correctness.

Removed binary profdata files and updated tests

davidxl accepted this revision.Mar 19 2016, 1:47 PM

lgtm. Wait a couple of days in case other reviewers have more comments.

This revision is now accepted and ready to land.Mar 19 2016, 1:47 PM

Thanks Harlan, LGTM.

harlanhaskins updated this revision to Diff 51615.EditedMar 24 2016, 3:59 PM

Removed colon from HTML function title

It doesn't make much sense when it's in its own, visually distinct box, and
can actually be confusing if demangled.

Any update on the patch? Justin had some made some comments ..

Any update on the patch? Justin had some made some comments ..

Yep, sorry, I've been busy this week. Looking to get to it next week!

Any update?

Justin's recommendations amount to a refactor of much of llvm-cov, and unfortunately I don't have the time right now to overhaul this change. I'll try to get to it soon, but things have ramped up here.

vsk commandeered this revision.Jun 29 2016, 1:32 PM
vsk edited reviewers, added: harlanhaskins; removed: vsk.

Per the list discussion, I'm going to take a shot at cleaning this up.

vsk updated this revision to Diff 62280.Jun 29 2016, 1:41 PM
vsk updated this object.
  • Consolidated the format tests into two files.
  • Rebase onto master. The html formatting logic is now totally separated from the main layout logic (see SourceCoverageView and CoveragePrinter). This should address @bogner's comments about having a pure-virtual rendering interface.
  • Fixed the sub-line region count tooltip display. This previously didn't work at all, and was somewhat worked-around with a CSS kludge ("margin-left: -76px").
This comment was removed by harlanhaskins.
harlanhaskins accepted this revision.Jun 29 2016, 1:47 PM
harlanhaskins requested changes to this revision.Jun 29 2016, 2:31 PM

Looking at the bitcode compatibility coverage example, it looks like those hyperlinks in the index page are absolute links to /tmp/llvm-as.dir/coverage/<file>

This revision now requires changes to proceed.Jun 29 2016, 2:31 PM
vsk added a comment.Jun 29 2016, 3:04 PM

Looking at the bitcode compatibility coverage example, it looks like those hyperlinks in the index page are absolute links to /tmp/llvm-as.dir/coverage/<file>

Fixed in r274173, no changes to this revision are needed. Please check the download link again.

I just ran this on an instrumented Swift code run, and it worked really great! One issue, though; the coverage was output to /home/harlan/swift-coverage, but the hyperlinks pointed to swift-coverage/..., meaning that all the hyperlinks were relative to the parent of the index file.

Other than that, the output looks excellent.

vsk added a comment.Jun 30 2016, 7:48 AM

One issue, though; the coverage was output to /home/harlan/swift-coverage, but the hyperlinks pointed to swift-coverage/..., meaning that all the hyperlinks were relative to the parent of the index file.

Do the generated links not work / is there a use case this breaks? I intended the links to be relative to the report output directory. That makes it possible to copy the directory to other machines while leaving the links intact.

Sorry, I should've provided more details. I ran llvm-cov with -output-dir swift-coverage, which output everything, including the index file, into swift-coverage.

The relative links from the index include a leading swift-coverage/... which make their absolute paths, once resolved, swift-coverage/swift-coverage/...

I attached a sample index file

vsk added a comment.Jun 30 2016, 9:16 AM

Sorry, I should've provided more details. I ran llvm-cov with -output-dir swift-coverage, which output everything, including the index file, into swift-coverage.

The relative links from the index include a leading swift-coverage/... which make their absolute paths, once resolved, swift-coverage/swift-coverage/...

This index file looks correct to me. For example, one entry has LinkText="home/harlan/Swift/swift/lib/IRGen/GenCoverage.cpp" and LinkTarget="file:///Users/vk/Desktop/swift-coverage/coverage/home/harlan/Swift/swift/lib/IRGen/GenCoverage.cpp.html". Does this not work?

Note that the final paths do not contain swift-coverage/swift-coverage/.... Instead, it's swift-coverage/coverage/.... Having the extra 'coverage' component handles a corner case where the user compiles a source file called, e.g "index.html" into their binary.

I don't think that's correct.

If this is my folder structure:

swift-coverage
index.html
coverage
└─ /foo/bar/baz.cpp.html

and the links inside index.html begin with swift-coverage, then I can't click any of those links because they'll resolve to swift-coverage/swift-coverage/foo.

vsk added a comment.Jul 1 2016, 1:32 PM

I don't think that's correct.

If this is my folder structure:

swift-coverage
index.html
coverage
└─ /foo/bar/baz.cpp.html

and the links inside index.html begin with swift-coverage, then I can't click any of those links because they'll resolve to swift-coverage/swift-coverage/foo.

I don't see how that is possible. LinkTarget is set with InToplevel=false and Relative=true. So, FullPath = "coverage" before paths to source files are appended to it. At no point is the output directory included as a prefix or a suffix to FullPath. Can you break on CoveragePrinterHTML::createIndexFile and show it doing the wrong thing?

There are two issues with the current patch on Windows, I am happy to patch these later if you want.

When a project has two files with the same file name but in different drives (e.g. C:\temp\temp.c and D:\temp\temp.c), the llvm-cov.exe tool will only show one of these files. This could be fixed by using the drive letter (or network drive name) as a part of the file output path.

The source file link will not work if the full file path exceeds the maximum path length (260 characters). Could we give a warning in this case?

vsk added a comment.Jul 5 2016, 6:21 PM

There are two issues with the current patch on Windows, I am happy to patch these later if you want.

When a project has two files with the same file name but in different drives (e.g. C:\temp\temp.c and D:\temp\temp.c), the llvm-cov.exe tool will only show one of these files. This could be fixed by using the drive letter (or network drive name) as a part of the file output path.

The source file link will not work if the full file path exceeds the maximum path length (260 characters). Could we give a warning in this case?

Thanks for testing it out!

It seems that all the pending issues we are discussing actually relate to CoveragePrinter::getOutputPath, which is not a part of this patch. We can fix them before or after this patch is committed, and test the changes using "-format text -output-dir ...".

Given that this is a large change, I'd like to make sure it addresses Justin's concerns and that it has an explicit lgtm before moving forward.

In D18278#474741, @vsk wrote:

Thanks for testing it out!

It seems that all the pending issues we are discussing actually relate to CoveragePrinter::getOutputPath, which is not a part of this patch. We can fix them before or after this patch is committed, and test the changes using "-format text -output-dir ...".

Given that this is a large change, I'd like to make sure it addresses Justin's concerns and that it has an explicit lgtm before moving forward.

I agree with you, we could fix these issues after the patch is committed.

The patch in the current state looks good to me.

MatzeB added a subscriber: MatzeB.Jul 6 2016, 1:46 PM

Definitely a nice feature to have (I assume we can replace http://llvm.org/reports/coverage/ with this at some point).

I didn't have time to actually try it out, so no comments on the looks of the generated pages here.

While I like the fact that a stylesheet is hardcoded by default, I can imagine some users wanting to tweak it. Adding an option that generates a <link rel="stylesheet"... instead of outputting the hardcoded stylesheet should be easy.
You could address this in followup patches though.

tools/llvm-cov/HTMLCoverage.inc
1 ↗(On Diff #62280)

Using a .inc file feels slightly unusual for llvm. Why not call this HTMLCoverage.h or alternatively simply copy the contents to the beginning of HTMLCoverageViewHTML.cpp...

tools/llvm-cov/SourceCoverageViewHTML.cpp
57 ↗(On Diff #62280)

Passing the parameters by reference const std::string& or alternatively StringRef would avoid some unnecessary allocating/copying of strings.
The same applies to some other functions here.

vsk marked an inline comment as done.Jul 6 2016, 2:51 PM

Definitely a nice feature to have (I assume we can replace http://llvm.org/reports/coverage/ with this at some point).

We certainly hope to make it competitive :).

I didn't have time to actually try it out, so no comments on the looks of the generated pages here.

The link might have gotten lost above. Here's a sample: http://net.vedantk.com/static/bitcode-compatibility-coverage.dir.tar.gz.

While I like the fact that a stylesheet is hardcoded by default, I can imagine some users wanting to tweak it. Adding an option that generates a <link rel="stylesheet"... instead of outputting the hardcoded stylesheet should be easy.
You could address this in followup patches though.

I'll fix this in a follow-up. I'd also like to add a -demangler option.

tools/llvm-cov/HTMLCoverage.inc
1 ↗(On Diff #62280)

Fair point, I'll move this into the SourceCoverageViewHTML.cpp.

This revision was automatically updated to reflect the committed changes.
harlanhaskins added inline comments.Jul 17 2016, 2:03 PM
llvm/trunk/tools/llvm-cov/SourceCoverageViewHTML.cpp
405

Any chance we can wrap this in an anchor, a la:

<a name="L{{line-number}}"></a>

This would be very useful for linking within the files.