Page MenuHomePhabricator

[clang] Add -fprofile-prefix-map
ClosedPublic

Authored by keith on Jul 3 2020, 10:28 PM.

Details

Summary

This flag allows you to re-write absolute paths in coverage data analogous to -fdebug-prefix-map. This flag is also implied by -ffile-prefix-map.

Diff Detail

Event Timeline

keith created this revision.Jul 3 2020, 10:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2020, 10:28 PM
Herald added a subscriber: dexonsmith. · View Herald Transcript
keith added a comment.Jul 3 2020, 10:31 PM

Open question: I don't know how all the toolchains fit together, but I noticed that only Clang.cpp handles -fmacro-prefix-map, but Clang.cpp, FreeBSD.cpp, and Gnu.cpp all handle -fdebug-prefix-map. I wasn't sure which pattern I should follow here, so right now this only adds the handling to Clang.cpp, please let me know if that's not appropriate in this case!

keith updated this revision to Diff 275506.Jul 4 2020, 9:57 AM

Fix tests on Windows

MaskRay added a subscriber: MaskRay.EditedJul 6 2020, 6:12 PM

GCC obtained -fdebug-prefix-map first, then r256847 added both -fmacro-prefix-map and -ffile-prefix-map. GCC doesn't have -fcoverage-prefix-map and I saw your https://github.com/apple/swift/pull/32416
However, do we really need one new -f*-prefix-map for every feature?

If you think -fcoverage-prefix-map= is really necessary, you can probably ask whether GCC can adopt a -fcoverage-prefix-map for their gcov (.gcno files). If they do, reviewers may have more confidence to say that we should have the option as well.

keith added a comment.Jul 6 2020, 6:15 PM

I originally implemented this behavior behind -fdebug-prefix-map but there was some pushback, more context: https://lists.llvm.org/pipermail/cfe-dev/2020-June/065668.html

-fdebug-prefix-map does not make sense to me because coverage is not debug info.
I am on the fence whether we need -fcoverage-prefix-map. I created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96092 (Should --coverage respect -ffile-prefix-map?)

clang/include/clang/Driver/Options.td
2606

Drop CC1AsOption

The option does not affect -cc1as (assembly input)

phosek added a subscriber: phosek.Jul 11 2020, 5:20 PM

-fdebug-prefix-map does not make sense to me because coverage is not debug info.
I am on the fence whether we need -fcoverage-prefix-map. I created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96092 (Should --coverage respect -ffile-prefix-map?)

Looks like GCC has [-fprofile-prefix-path](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96092#c2) in the queue which is trying to achieve a similar thing. I'd prefer -fprofile-prefix-map to be consistent with the existing -f*-prefix-map options, both in terms of spelling and usage. I think that -fprofile-prefix-map is better than -fcoverage-prefix-map because it makes it clear that it applies -fprofile-* set of options.

-fdebug-prefix-map does not make sense to me because coverage is not debug info.
I am on the fence whether we need -fcoverage-prefix-map. I created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96092 (Should --coverage respect -ffile-prefix-map?)

Looks like GCC has [-fprofile-prefix-path](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96092#c2) in the queue which is trying to achieve a similar thing. I'd prefer -fprofile-prefix-map to be consistent with the existing -f*-prefix-map options, both in terms of spelling and usage. I think that -fprofile-prefix-map is better than -fcoverage-prefix-map because it makes it clear that it applies -fprofile-* set of options.

-fprofile-prefix-map looks good to me. I made another comment and CCed the author of https://gcc.gnu.org/git/?p=gcc.git&a=commit;h=44b326839d864fc10c459916abcc97f35a9ac3de (which added -fprofile-prefix-path) in that bug.

@keith do you plan on finishing this? I think we should change the name from -fcoverage-prefix-map to -fprofile-prefix-map, but otherwise it might be good to go.

keith added a comment.Aug 18 2020, 7:04 PM

Yes I do, sorry I've been a bit busy, I will try to get to this later this week

keith retitled this revision from clang: Add -fcoverage-prefix-map to clang: Add -fdebug-prefix-map.Sep 8 2020, 4:52 PM
keith edited the summary of this revision. (Show Details)
keith retitled this revision from clang: Add -fdebug-prefix-map to clang: Add -fprofile-prefix-map.
keith updated this revision to Diff 290610.Sep 8 2020, 4:53 PM
keith retitled this revision from clang: Add -fprofile-prefix-map to clang: Add -fcoverage-prefix-map.
keith edited the summary of this revision. (Show Details)

Rename from fcoverage-prefix-map to fprofile-prefix-map

rnk added inline comments.Sep 9 2020, 11:16 AM
clang/include/clang/Driver/Options.td
2604

Any reason for the _QA suffix instead of _EQ?

clang/lib/CodeGen/CoverageMappingGen.cpp
1596

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

clang/test/Profile/profile-prefix-map.c
13

See here, for example, where you must pass -fprofile-prefix-map=%/t/....

keith updated this revision to Diff 290851.Sep 9 2020, 5:27 PM
keith marked 2 inline comments as done.

Small fixes

clang/lib/CodeGen/CoverageMappingGen.cpp
1596

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?

clang/test/Profile/profile-prefix-map.c
13

+1. I do think I still want this test case, but likely another pending the discussion above that validates the relative path behavior.

keith marked 2 inline comments as not done.Sep 9 2020, 5:28 PM
rnk added inline comments.Sep 10 2020, 9:53 AM
clang/lib/CodeGen/CoverageMappingGen.cpp
1596

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.

keith added inline comments.Sep 12 2020, 12:49 PM
clang/lib/CodeGen/CoverageMappingGen.cpp
1596

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.

andrewjcg added inline comments.
clang/lib/CodeGen/CoverageMappingGen.cpp
1596

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

phosek added inline comments.Sep 15 2020, 11:42 AM
clang/lib/CodeGen/CoverageMappingGen.cpp
1596

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.

keith added inline comments.Sep 15 2020, 6:02 PM
clang/lib/CodeGen/CoverageMappingGen.cpp
1596

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.

rnk added a comment.Sep 16 2020, 12:23 PM

Yeah, my goal is for the build system to be able to avoid having to embed PWD into the compiler flags. For example, I believe it is a goal of the LLVM gn build system for the build tree to be relocatable. If the compiler command lines need to contain absolute paths to make the paths in the coverage relative, we won't be able to achieve that goal.

Based on all the comments that have gone before, it sounds like we want a -fprofile-compilation-dir= flag. Once we have that, would this logic work?

  • if profile compilation dir set, absolutize with that as the root
  • if no profile compilation dir, absolutize with CWD
  • apply profile prefix map

Should -no-canonical-prefixes also be involved here? Unclear.

In D83154#2277539, @rnk wrote:

Yeah, my goal is for the build system to be able to avoid having to embed PWD into the compiler flags. For example, I believe it is a goal of the LLVM gn build system for the build tree to be relocatable. If the compiler command lines need to contain absolute paths to make the paths in the coverage relative, we won't be able to achieve that goal.

Based on all the comments that have gone before, it sounds like we want a -fprofile-compilation-dir= flag. Once we have that, would this logic work?

  • if profile compilation dir set, absolutize with that as the root
  • if no profile compilation dir, absolutize with CWD
  • apply profile prefix map

Should -no-canonical-prefixes also be involved here? Unclear.

I originally considered -fprofile-compilation-dir= and perhaps -ffile-compilation-dir= to also handle cases like __FILE__ macro (-ffile-prefix-map and -fmacro-prefix-map suffer from the same issue as -fprofile-prefix-map), but I'm not sure if specifying compilation dir is the right approach. It makes sense for debug info because -fdebug-compilation-dir is used directly as DW_AT_comp_dir and also because it's a convention for source file paths in debug info to be relative to build directory, but it's not the case for paths in coverage mapping or __FILE__.

So what I'm considering now and started prototyping is a -fsource-dirflag (or maybe -fsource-root-dir, I'm open to suggestions for the name) where the semantics would be that if this flag is set, all source paths will be made relative to this directory. There's an open question whether this should also apply to sources outside of that directory, for example system header paths.

The example use case, assuming source is in /path/to/source and build is in /path/to/build, would be: clang -fdebug-compilation-dir . -fsource-dir ../source ../source/dir/file.c. In this case __FILE__ would expand to dir/file.c and that path would be also stored in coverage mapping.

In D83154#2277539, @rnk wrote:

Yeah, my goal is for the build system to be able to avoid having to embed PWD into the compiler flags. For example, I believe it is a goal of the LLVM gn build system for the build tree to be relocatable. If the compiler command lines need to contain absolute paths to make the paths in the coverage relative, we won't be able to achieve that goal.

Based on all the comments that have gone before, it sounds like we want a -fprofile-compilation-dir= flag. Once we have that, would this logic work?

  • if profile compilation dir set, absolutize with that as the root
  • if no profile compilation dir, absolutize with CWD
  • apply profile prefix map

Should -no-canonical-prefixes also be involved here? Unclear.

I originally considered -fprofile-compilation-dir= and perhaps -ffile-compilation-dir= to also handle cases like __FILE__ macro (-ffile-prefix-map and -fmacro-prefix-map suffer from the same issue as -fprofile-prefix-map), but I'm not sure if specifying compilation dir is the right approach. It makes sense for debug info because -fdebug-compilation-dir is used directly as DW_AT_comp_dir and also because it's a convention for source file paths in debug info to be relative to build directory, but it's not the case for paths in coverage mapping or __FILE__.

So what I'm considering now and started prototyping is a -fsource-dirflag (or maybe -fsource-root-dir, I'm open to suggestions for the name) where the semantics would be that if this flag is set, all source paths will be made relative to this directory. There's an open question whether this should also apply to sources outside of that directory, for example system header paths.

The example use case, assuming source is in /path/to/source and build is in /path/to/build, would be: clang -fdebug-compilation-dir . -fsource-dir ../source ../source/dir/file.c. In this case __FILE__ would expand to dir/file.c and that path would be also stored in coverage mapping.

D87928 is the implementation of that idea.

keith added a comment.Sep 21 2020, 5:16 PM

With D87928 do you think we would want this flag as well? The only difference I can think of (besides general UX) is if you need to remap paths outside of your source root, I believe having a *-prefix-map style flag makes that a bit more clear (or potentially possible at all). Ideally there wouldn't be issues around that but in the past in the Apple toolchain the Xcode path was embedded in debug info (as an example), so to get a reproducible build we had to pass -fdebug-prefix-map=/path/to/Xcode.app=SOMETHING. Which -fdebug-compilation-dir . couldn't solve. I'm wondering if we should add both flags for this flexibility, or if we should assume this should not happen?

With D87928 do you think we would want this flag as well? The only difference I can think of (besides general UX) is if you need to remap paths outside of your source root, I believe having a *-prefix-map style flag makes that a bit more clear (or potentially possible at all). Ideally there wouldn't be issues around that but in the past in the Apple toolchain the Xcode path was embedded in debug info (as an example), so to get a reproducible build we had to pass -fdebug-prefix-map=/path/to/Xcode.app=SOMETHING. Which -fdebug-compilation-dir . couldn't solve. I'm wondering if we should add both flags for this flexibility, or if we should assume this should not happen?

That really depends on the use case. For distributed compilation and caching *-prefix-map flags are undesirable because they include absolute path in the compiler invocation, see https://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html for more details. -fdebug-compilation-dir and the proposed -fsource-dir don't have that issue because they use relative paths. However, there might still be uses for *-prefix-map outside of distributed compilation and caching so I'd be fine with including both.

Should we wait for that one to land and then pick this one up? Otherwise any thoughts on the outstanding discussion?

phosek accepted this revision.Nov 15 2020, 3:55 PM

LGTM we may consider implementing -fsource-dir in terms of -fprofile-prefix-map as suggested in D87928 so I think we should go ahead and land this.

This revision is now accepted and ready to land.Nov 15 2020, 3:55 PM

@keith do you plan on landing this?

MaskRay retitled this revision from clang: Add -fcoverage-prefix-map to clang: Add -fprofile-prefix-map.Jan 20 2021, 10:37 AM

Yes sorry, big company reviews month. I will try to land Friday

keith retitled this revision from clang: Add -fprofile-prefix-map to [clang] Add -fprofile-prefix-map.Jan 25 2021, 10:06 AM
keith closed this revision.Jan 25 2021, 10:16 AM

Something went wrong with this being attached to the commit (I believe because I committed from a different machine than I originally wrote this on), landed here https://github.com/llvm/llvm-project/commit/c3324450b204392169d4ec7172cb32f74c03e376 thanks for the review!