[llvm-cov] Don't remap existing paths

[greened]

Summary

Ignore path rewrites for files that exist. The avoids testing problems caused by symlinks in the build area. For example:

/path -> /tmp/path

<configure LLVM in /path/to/sources to be built into /path/to/build>

llvm-cov testcase:

// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s

Expands to:

llvm-cov show /tmp/path/to/build/test.cpp.tmp -instr-profile \
  /tmp/path/to/build/test.cpp.tmp.profdata \
  -path-equivalence=/tmp,/tmp/path/to/sources 2>&1 \
  | FileCheck /tmp/path/to/sources/test.cpp

Which results in llvm-cov looking for /tmp/path/to/sources/path/to/sources/test.cpp which doesn't exist.

With this change, /tmp/path/to/sources will not be remapped because /tmp/path/to/sources/test.cpp exists.

In general, this change solves problems with tools that invoke llvm-cov after expanding paths containing symlinks to nested path components.

[greened]

This fixes some compiler-rt tests that break for us due to odd symlinks in /tmp. I haven't figured out how to generate a reasonable test within llvm sources, as any such test uses a binary with a fixed source file path encoded into it. The result is that the test will not fail if LLVM is built in a completely different place than were it was built when the test binary was generated since the test binary path prefix won't patch the -path-equivalence prefix.

I have successfully created a test in compiler-rt fashioned after the existing llvm-cov tests there. This test uses clang to generate a binary during testing so that the path prefixes will match and a failure will result without this patch.

I am happy to contribute the compiler-rt testcase but that goes into a different repository and can't be applied along with this patch. If someone can come up with a good way to write a test for this that lives within the llvm repository, I'm all ears!

[greened]

Corresponding compiler-rt test differential is here:

https://reviews.llvm.org/D53732

[Dor1s]

@greened, thanks for the patch. Could you please share which tests were failing for you?

[greened]

In D53729#1276632, @Dor1s wrote:

@greened, thanks for the patch. Could you please share which tests were failing for you?

Sure.

Profile-aarch64 :: Linux/coverage_ctors.cpp
Profile-aarch64 :: Linux/coverage_dtor.cpp
Profile-aarch64 :: Linux/coverage_test.cpp
Profile-aarch64 :: Linux/instrprof-comdat.test

[vsk]

Even if there's a file at the path we're supposed to remap, shouldn't we still do the remap? Couldn't the file we're intended to read be at the remapped path?

Maybe we could just add 'REQUIRES: build-dir-not-slash-tmp' or something to the affected lit tests.

[Dor1s]

In D53729#1277534, @vsk wrote:

Even if there's a file at the path we're supposed to remap, shouldn't we still do the remap? Couldn't the file we're intended to read be at the remapped path?

Yeah, I'm on the same side as @vsk here. There might be different workflows, but the most obvious behavior of path remapping would be to always remap paths. It would be nice to maintain it rather than change.

I've also seen cases where binaries compile system headers (e.g. from /usr/include) with coverage instrumentation, and if you need to generate a coverage report for those on another machine, you'd better copy all of them rather than hope to have the same headers. For instance, i've seen this problem with wireshark and irssi projects: https://github.com/google/oss-fuzz/issues/1743#issuecomment-416265306

[greened]

Maybe we could just add 'REQUIRES: build-dir-not-slash-tmp' or something to the affected lit tests.

It would be unfortunate to disable tests just because of this issue. I understand your point, though, and remapping always certainly has simpler semantics.

How difficult is it to add new REQUIRES clauses? I've never done it before.

Would it make sense to try to change the tests to not need remapping of /tmp? I don't know how I'd do that, so ideas are welcome.

[vsk]

In D53729#1278901, @greened wrote:

Maybe we could just add 'REQUIRES: build-dir-not-slash-tmp' or something to the affected lit tests.

It would be unfortunate to disable tests just because of this issue. I understand your point, though, and remapping always certainly has simpler semantics.

How difficult is it to add new REQUIRES clauses? I've never done it before.

It's not too bad, test/lit.cfg.py has several examples. You can add a 'feature' by doing config.available_features.add("name-of-feature"). Then you can add REQUIRES: name-of-feature to any test.

Would it make sense to try to change the tests to not need remapping of /tmp? I don't know how I'd do that, so ideas are welcome.

The only I way I see of doing that is to regenerate the .covmapping binary blobs so that they refer to a different path (instructions for doing this are in the tests). This doesn't eliminate the possibility of name conflicts in principle (although in practice it might).

[greened]

We are probably going to reconfigure our machines so this isn't a problem, so these tests should start working for us. However, this took some effort to track down and I wonder if we ought to have a warning or something if lit detects that these tests are check out under /tmp.

[vsk]

Marking the tests are 'unsupported' would be a good way to surface the warning, as the number (and possibly the list?) of unsupported tests is printed out at the end of each test run.

[vsk]

(Clearing my review queue - still think adding an 'unsupported' line would make sense if this remains an issue)

tools/llvm-cov/CodeCoverage.cpp

if (!sys::path::is_separator(NativePath.back()))
       NativePath += sys::path::get_separator();
     return NativePath.c_str();
   };
   std::string RemapFrom = nativeWithTrailing(PathRemapping->first);
   std::string RemapTo = nativeWithTrailing(PathRemapping->second);
 
   // Create a mapping from coverage data file paths to local paths.
   for (StringRef Filename : Coverage.getUniqueSourceFiles()) {
+    // Don't remap paths that exist.
+    llvm::sys::fs::file_status Status;
+    llvm::sys::fs::status(Filename, Status);
+    if (llvm::sys::fs::exists(Status))
+      continue;
+
     SmallString<128> NativeFilename;
     sys::path::native(Filename, NativeFilename);
     if (NativeFilename.startswith(RemapFrom)) {
       RemappedFilenames[Filename] =
           RemapTo + NativeFilename.substr(RemapFrom.size()).str();
     }
   }