[PATCH] D83154: clang: Add -fcoverage-prefix-map

[Keith Smiley via Phabricator via cfe-commits]

keith added inline comments.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1334
+  llvm::SmallString<256> Path(Filename);
+  llvm::sys::fs::make_absolute(Path);
+  llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
----------------
rnk wrote:
> keith wrote:
> > rnk wrote:
> > > Please only make the path absolute if nothing in the prefix map matches. Otherwise, the user must embed the CWD into the prefix map, which is needlessly difficult for the build system. I believe it is also consistent with the way that the debug info prefix map works. It appears to operate on the possibly relative source paths received on the command line (-I...).
> > Are you suggesting that I try to remap them relatively, and if that fails, absolutize it and attempt the remapping again? Or don't absolutize them at all anymore?
> I'd prefer to do the remapping once without any absolutization, and if that fails, preserve the behavior of absolutizing.
> 
> It would be my preference to not absolutize paths at all, since that is what is done for debug info. However, @vsk implemented things this way, and I do understand that this is convenient default behavior for most users: the paths in the coverage are valid anywhere on their system. So, changing this behavior is out of scope for this patch.
I'm not sure how this changed would work for our use case. With bazel the usage of this works something like:

1. Relative paths are passed to the compiler `clang ... foo.c`
2. We would normally do `-fprofile-prefix-map=$PWD=.` to remap them

I think if we made this change, we would either have to:

1. Make the paths we pass absolute, which we couldn't do for reproducibility
2. Add some known prefix to the paths, like `.`, so we could `-fprofile-prefix-map=.=.` just to avoid this absolutizing codepath

So I think without actually removing the absolutizing behavior at the same time, this wouldn't work as we'd hope.

Let me know if I'm mis-understanding your suggestion!

If we did what I said above, I think it would work since relative path remapping would fail, but then prefix mapping the absolute path would work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83154/new/

https://reviews.llvm.org/D83154