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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
