Page MenuHomePhabricator

[llvm-cov] Use the native path in the coverage report.
ClosedPublic

Authored by MaggieYi on Aug 26 2016, 8:19 AM.

Details

Summary

This patch is spin-off from https://reviews.llvm.org/D23345.

The coverage reports contain the source or binary file paths. On Windows, the file path might contain the separators of both '/' and '\'.
This patch uses the native path in the coverage reports. For example, on Windows, all '/' are converted to '\'.

Diff Detail

Repository
rL LLVM

Event Timeline

MaggieYi updated this revision to Diff 69375.Aug 26 2016, 8:19 AM
MaggieYi retitled this revision from to [llvm-cov] Use the native path in the coverage report..
MaggieYi updated this object.
MaggieYi added reviewers: vsk, davidxl, bogner.
MaggieYi added a subscriber: llvm-commits.
vsk edited edge metadata.Aug 26 2016, 11:40 AM

This looks like it's in good shape. Just a few comments:

test/tools/llvm-cov/Inputs/native_separators.proftext
8 ↗(On Diff #69375)

Do you mind using just "int main() {}" (without the #include), and re-using doube_dots.proftext? I'm asking because we already have 10 *.proftext files in test/tools/llvm-cov/Inputs, and only 7 of them are unique (md5 test/tools/llvm-cov/Inputs/*.proftext | cut -d'=' -f2 | sort -u | uniq | wc -l).

test/tools/llvm-cov/native_separators.c
10 ↗(On Diff #69375)

Please use the TEXT-INDEX check-prefix here.

20 ↗(On Diff #69375)

This is missing a test for the text source view (i.e, FileCheck -check-prefixes=TEXT -input-file=%t.dir/coverage/tmp/native_separators.c.txt).

tools/llvm-cov/SourceCoverageViewHTML.cpp
312 ↗(On Diff #69375)

I don't think remove_dot_dots should be true. If I have a relative path which looks like: "../x/../y", I wouldn't want that presented as "/x/y" (or even "x/y"). There should be a test for this.

MaggieYi updated this revision to Diff 69517.Aug 28 2016, 12:25 PM
MaggieYi edited edge metadata.

Thanks Vedant, the patch has been updated following your comments.

MaggieYi marked an inline comment as done.Aug 28 2016, 12:35 PM
MaggieYi added inline comments.
test/tools/llvm-cov/native_separators.c
21 ↗(On Diff #69517)

Currently, the source and binary files are only shown in the HTML report, not in the txt report. Should I put the source and binary files in the txt report as well?

tools/llvm-cov/SourceCoverageViewHTML.cpp
312 ↗(On Diff #69517)

I have added an index html test in the double_dots.c test.

I also considered the case you mentioned here, it could happen in the following case:
Assuming:

  1. The “test.cpp”, “test.elf” and “test.profdata” are in the folder: “i:\codecoverage\test1”;
  2. There are two folders “test1” and “test2” in the folder i:\codecoverage.

Running the following commands:

$ cd i:\codecoverage\test1\
$ llvm-cov show –format=html –o output test.elf –instr-profile=test.profdata -filename-equivalence ..\test2\..\test1\test.cpp

Checking the index.html file:

The “codecoverage\test1\test.cpp” is generated by llvm-cov.

I have checked the index.html fie, which is correct. I don’t know how to pass “..\test2\..\test1\test.cpp” to the command line in the llvm testing system. Could you please let me know how to do it?

vsk added inline comments.Aug 29 2016, 10:49 AM
test/tools/llvm-cov/native_separators.c
21 ↗(On Diff #69517)

Ideally I'd like to preserve feature-parity between the html/text views. It would be nice to extend the text view in a follow-up patch, but don't worry about it for now.

tools/llvm-cov/SourceCoverageViewHTML.cpp
312 ↗(On Diff #69517)

Good catch. You should be able to write "../test2/../test1/test.cpp" in a RUN line. IIRC, llvm-lit has logic to parse Unix filenames and re-write them on Windows hosts.

MaggieYi updated this revision to Diff 69626.Aug 29 2016, 3:34 PM

Thanks, the test has been added.

vsk accepted this revision.Aug 29 2016, 3:37 PM
vsk edited edge metadata.

Lgtm, thanks.

This revision is now accepted and ready to land.Aug 29 2016, 3:37 PM
This revision was automatically updated to reflect the committed changes.