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

Tue Sep 15 18:02:57 PDT 2020

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);
----------------
phosek wrote:
> andrewjcg wrote:
> > keith wrote:
> > > 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.
> > FWIW, there's a similar issue with doing the absolutizing for the Buck build tool, which by default sets `-fdebug-compilation-dir=.` for all compilations, then expects to use `-fdebug-prefix-map` (or `-ffile-prefix-map`) to fixup relative paths of sandboxed header symlink trees to point to their *real* paths (e.g. something like `clang -c -fdebug-compilation-dir=. -Iheader-tree-sandbox -ffile-prefix-map=header-tree-sandbox=original_dir`) (e.g. https://github.com/facebook/buck/blob/master/src/com/facebook/buck/cxx/toolchain/PrefixMapDebugPathSanitizer.java#L134).
> > 
> > So, in this case, *not* absolutizing would help here, but it kind of feels that the real issue is that there's not an equivalent `-fprofile-compilation-dir` to opt-out of the absolutizing of profile paths (which is orthogonal to this change)...
> FWIW this is something we've already discussed on the list and I started prototyping that feature, I'm hoping to upload the change for review in the next few days.
It's probably worth going back to the mailing list for some of these discussions. I think everyone was happy with the idea of one of the `-*compilation-dir` flags, but we decided to only add this flag for now https://lists.llvm.org/pipermail/cfe-dev/2020-June/066014.html

Also bazel "gets around" excluding PWD from command lines by hiding it in nested flags https://github.com/bazelbuild/bazel/blob/e3c8eb48dc05a666c68ae18e996f5d37cac5a607/tools/osx/crosstool/wrapped_clang.cc#L219-L221 which is why this case was good enough for me as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83154