[llvm-cov] Add an option which maps the location of source directories on another machine to your local copies

[seaneveson]

Summary

This patch adds the -path-equivalence option (example: llvm-cov show -path-equivalence=/origin/path,/local/path) which maps the source code path from one machine to another when using llvm-cov show. This is similar to the -filename-equivalence option, but doesn't require you to specify all the source files on the command line.

This allows you to generate the coverage data on one machine (e.g. in a CI system), and then use llvm-cov on another machine where you have the same code base on a different path.

[seaneveson]

Add missing file.

[vsk]

Thanks for the patch, this should address:
https://bugs.llvm.org/show_bug.cgi?id=30901

tools/llvm-cov/CodeCoverage.cpp

137 ↗

(On Diff #109964)

Please use Optional<std::pair<...>>, this will make it easier to see whether/not there is a remapping.

520 ↗

(On Diff #109964)

The path remap option is a strict improvement over -filename-equivalence. Could you delete the old option, delete CompareFilenamesOnly & any resulting dead code, and update any affected tests to use path remap?

614 ↗

(On Diff #109964)

I don't think that either of these paths need to go through native(). That should happen right before inserting into RemappedFilenames.

616 ↗

(On Diff #109964)

The field can just be initialized to None in the CodeCoverageTool constructor.

822 ↗

(On Diff #109964)

You should only need to call native() on the string Filename is mapped to.

830 ↗

(On Diff #109964)

Why should -path-equivalence be ignored if source files are specified? IMO this should be supported so that users can see coverage for a restricted set of files.

[seaneveson]

Update based on review comments.

[seaneveson]

Thanks for your feedback, I think I've addressed all your comments (or replied to them).

test/tools/llvm-cov/scan-directory.test

11 ↗

(On Diff #110207)

I'm not sure what the expected output should be now. The actual output is:

error: a: Missing source file
error: b: Missing source file
error: c: Missing source file
error: d: Missing source file
error: e: Missing source file
error: f: Missing source file
D:\Test\x\a\b\c.tmp
D:\Test\x\a\d.tmp

tools/llvm-cov/CodeCoverage.cpp

822 ↗

(On Diff #109964)

I think you do have to call native on both paths and the Filename to cover the case where the coverage data was generated on linux and llvm-cov is being run on windows (like in the lit tests).

Since the RemapFrom path and the Filename path are being compared it is important they are both native. Otherwise the RemapFrom path could be anything the user typed in, and we can't be sure about Filename because the data could come from another platform.

Given that the Filename path is then already native, it is quicker and easier to make RemapTo native and then just append the NativeFilename.

[vsk]

This is looking good, there's just one real issue left with removeUnmappedInputs().

test/tools/llvm-cov/scan-directory.test

11 ↗

(On Diff #110207)

I think "remapped-from" should be "%t" (all of the 'source files' live within that directory, see the first RUN line). The "remapped-to" field can be whatever you'd like.

tools/llvm-cov/CodeCoverage.cpp

127 ↗

(On Diff #110207)

Could you also remove the -filename-equivalance entry from docs/CommandGuide/llvm-cov.rst, and add in a short sentence or two about -path-equivalence?

355 ↗

(On Diff #110207)

This function must ensure that if a file outside of the coverage mapping is passed to llvm-cov (outside of -filename-equivalence mode), it does not get displayed. With your change, the !CompareFilenamesOnly branch is still correct (the condition would need to be changed to !PathRemapped). It's the other branch which is now wrong: you'll need to do a reverse mapping on the source files, and check that coverage mapping contains it.

822 ↗

(On Diff #109964)

Ok, sgtm.

[seaneveson]

• Address review comments
• Move the creation of the mapping into its own method, and call that inside load(), so it works properly for report as well as show

[vsk]

tools/llvm-cov/CodeCoverage.cpp

179 ↗

(On Diff #110576)

I don't think we need the PathRemapping check here (you can just keep the else branch). Otherwise, using relative paths in path equivalence mode won't work.

368 ↗

(On Diff #110576)

nit, extra 'to'

379 ↗

(On Diff #110576)

I can see why 'remap-from' would be empty (the user wants to add a path prefix). It would also make sense if 'remap-to' were empty (the user wants to strip out a path prefix).

Maybe the emptiness check should be in nativeWithTrailing?

405 ↗

(On Diff #110576)

I don't see the need for this side table. It should be possible to do this without using any extra space:

std::remove_if(SourceFiles.begin(), SourceFiles.end(), [&](const std::string &filename) {
  if (filename.startswith(PathRemapping.second))
    inv_mapped_path = path::join(PathRemapping.first, full_path.drop_first(PathRemapping.second.size()))
  else
    inv_mapped_path = filename
  return !std::binary_search(CoveredFiles.begin(), CoveredFiles.end(), inv_mapped_path);
});

This might fit nicely in the previous call to remove_if().

[seaneveson]

Address some review comments and tidy up removeUnmappedInputs.

[vsk]

It might be nice to find a simpler way to remove unmapped inputs, but that can certainly wait. LGTM, thanks again for working on this.

tools/llvm-cov/CodeCoverage.cpp

389 ↗

(On Diff #110704)

nit, 'input'

[seaneveson]

Fix typo and rebase.

test/tools/llvm-cov/Inputs/path_equivalence.proftext

+main
+# Func Hash:
+0
+# Num Counters:
+1
+# Counter Values:
+1
+

test/tools/llvm-cov/path_equivalence.c

+// RUN: llvm-profdata merge %S/Inputs/path_equivalence.proftext -o %t.profdata
+// RUN: llvm-cov show %S/Inputs/path_equivalence.covmapping -instr-profile=%t.profdata -path-equivalence=/tmp,%S | FileCheck %s
+
+int main() {} // CHECK: [[@LINE]]|      1|int main() {}
+
+// RUN: llvm-cov show %S/Inputs/path_equivalence.covmapping -instr-profile=%t.profdata -path-equivalence=/tmp,%S --filename-equivalence %s | FileCheck --check-prefix IGNORED_REMAP %s
+// IGNORED_REMAP: warning: path-equivalence was ignored as source files were given