Page MenuHomePhabricator

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

Authored by seaneveson on Aug 7 2017, 2:48 AM.

Details

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.

Diff Detail

Event Timeline

seaneveson created this revision.Aug 7 2017, 2:48 AM
seaneveson updated this revision to Diff 109964.Aug 7 2017, 2:54 AM

Add missing file.

vsk edited edge metadata.Aug 7 2017, 1:23 PM

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

tools/llvm-cov/CodeCoverage.cpp
138

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

558–559

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?

647

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

649

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

852

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

860

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 updated this revision to Diff 110207.Aug 8 2017, 8:07 AM

Update based on review comments.

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

test/tools/llvm-cov/scan-directory.test
11–12

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
852

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.

seaneveson marked 4 inline comments as done.Aug 8 2017, 8:30 AM
vsk added a comment.Aug 8 2017, 9:31 AM

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

test/tools/llvm-cov/scan-directory.test
11–12

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
131–132

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?

394

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.

852

Ok, sgtm.

  • 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 added inline comments.Aug 10 2017, 9:43 AM
tools/llvm-cov/CodeCoverage.cpp
179

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

nit, extra 'to'

379

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

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 marked 8 inline comments as done.

Address some review comments and tidy up removeUnmappedInputs.

vsk accepted this revision.Aug 11 2017, 7:52 AM

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
393

nit, 'input'

This revision is now accepted and ready to land.Aug 11 2017, 7:52 AM

Fix typo and rebase.

seaneveson closed this revision.Aug 14 2017, 3:21 AM