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