This patch changes handling multiple -fcoverage-prefix-map options to
match GCC's behavior. GCC applies prefix remappings that are provided
in reverse order (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109591).
Details
- Reviewers
phosek keith MaskRay - Group Reviewers
debug-info - Commits
- rGd7fa92126cc3: [clang] Apply -fcoverage-prefix-map reverse order
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Can you add example options in the description and say how this patch is going to change it?
I think std::map is intended so that for -f*-prefix-map values, if both a/b=... and a/b/c=... match an input path, the longest wins, not the latest wins.
We discovered the issue in the context of supporting source-based code coverage in the Pigweed project.
The project provides two -ffile-prefix-map paths that can impact coverage:
https://cs.opensource.google/pigweed/pigweed/+/main:pw_build/BUILD.gn;l=251-256
- -ffile-prefix-map=../= to remove the relative path from the build directory to the root
- -ffile-prefix-map==out/ to prepend out/ to any unmatched files
If the user-specified order is applied, 1) should be applied, but in this case 2) is applied because the std::map iteration order is not the same of the insertion order. Applying 2) appends out to beginning of file paths in coverage mapping records.
The question that we need to answer is whether following the user-specified order for -f*-prefix-map is the expected behavior. If that's the case, Clang does not follow that.
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L1653
I compared Clang and GCC behavior.
1) Following user-specified prefix mapping order
GCC uses a linked list to store the prefix mappings, so it applies the prefix mappings based on the user-provided order.
https://github.com/gcc-mirror/gcc/blob/master/gcc/file-prefix-map.cc#L27
Whereas, Clang currently uses an std::map, which does NOT follow user-provided order. So, Clang and GCC behavior do not match on that.
2) Applying multiple prefix mappings
Both Clang and GCC apply the first matching prefix mapping, and does not apply the following prefix mappings. So, they are consistent in that.
https://github.com/gcc-mirror/gcc/blob/master/gcc/file-prefix-map.cc#L88
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L1653
Sorry, request changes as I think I see conflicting needs and I think this needs more discussions. Cc debug-info
We have DebugPrefixMap and CoveragePrefixMap, and I think their order behavior should be the same.
D49466 added DebugPrefixMap to Clang which prefers a long prefix.
D132390 is a MC version of DebugPrefixMap that prefers a long prefix.
The current order of CoveragePrefixMap is definitely weird as it prefers a shorter prefix.
This patch is going to change the order to the command line option order, but it will be different from DebugPrefixMap.
Can CoveragePrefixMap be made to prefer a long prefix as DebugPrefixMap does?
-ffile-prefix-map==out/ to prepend out/ to any unmatched files
This operation feels a bit strange to me...
Created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109591 (Multiple -fdebug-prefix-map= prefixes match, which one wins?) to discuss GCC's behavior.
I wasn't aware that debug-prefix-map prefers longest prefix. I agree that ideally coverage-prefix-map, debug-prefix-map and macro-prefix-map should be consistent across all LLVM tools and ideally also across compilers.
I'm creating a patch to change DebugPrefixMap to respect the command line option order...
Created D148975 to follow GCC's behavior (the last applicable option wins).
In this patch, you may drop the DebugPrefixMap (clang/include/clang/Basic/CodeGenOptions.h) change (a no-op without other changes to DebugPrefixMap ).
Thanks for you guidance on this @MaskRay, and I removed the changes to debuginfo in the new revision.
clang/lib/CodeGen/CoverageMappingGen.cpp | ||
---|---|---|
1638 | I'm marking this Done as you removed it in https://github.com/llvm/llvm-project/commit/d3e121780ac2724969a030415f3092170f3c7589. |
Added more info into HelpText and reversed prefix map in the first place to incorporate coworker feedback.
GCC uses a linked list to store the prefix mappings in reverse order:
https://github.com/gcc-mirror/gcc/blob/master/gcc/file-prefix-map.cc#L27
clang/include/clang/Basic/CodeGenOptions.h | ||
---|---|---|
211 | The declaration should not repeat the comment at the use site. I am asking for what coverage features this CoveragePrefixMap is used for... |
While adding a comment, clarify what coverage it is? There are multiple coverage instrumentation features in Clang.