phosek 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); ---------------- 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. 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
