This is an archive of the discontinued LLVM Phabricator instance.

[clang] Apply -fcoverage-prefix-map reverse order
ClosedPublic

Authored by gulfem on Apr 19 2023, 4:31 PM.

Details

Summary

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

Diff Detail

Event Timeline

gulfem created this revision.Apr 19 2023, 4:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 4:31 PM
gulfem requested review of this revision.Apr 19 2023, 4:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 4:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

gulfem added a comment.EditedApr 19 2023, 5:06 PM

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

  1. -ffile-prefix-map=../= to remove the relative path from the build directory to the root
  2. -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

gulfem updated this revision to Diff 515560.Apr 20 2023, 7:51 PM

Modified the test case

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

phosek accepted this revision.Apr 21 2023, 11:23 AM

LGTM

This revision is now accepted and ready to land.Apr 21 2023, 11:23 AM
MaskRay requested changes to this revision.Apr 21 2023, 3:25 PM
This revision now requires changes to proceed.Apr 21 2023, 3:25 PM
MaskRay added a comment.EditedApr 21 2023, 3:40 PM

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

MaskRay added a comment.EditedApr 21 2023, 3:59 PM

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

MaskRay added a comment.EditedApr 21 2023, 5:50 PM

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

gulfem updated this revision to Diff 516616.Apr 24 2023, 9:17 PM

Remove the changes from debuginfo, which is handled in D14897.

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.

gulfem retitled this revision from [clang] Follow user-provided order for prefix map to [clang] Apply last -fcoverage-prefix-map option.Apr 24 2023, 9:21 PM
gulfem edited the summary of this revision. (Show Details)
gulfem updated this revision to Diff 516853.Apr 25 2023, 11:20 AM

Updated the text.

gulfem retitled this revision from [clang] Apply last -fcoverage-prefix-map option to [clang] Apply -fcoverage-prefix-map reverse order.Apr 25 2023, 11:21 AM
gulfem edited the summary of this revision. (Show Details)
gulfem edited the summary of this revision. (Show Details)

LGTM

clang/include/clang/Basic/CodeGenOptions.h
211

While adding a comment, clarify what coverage it is? There are multiple coverage instrumentation features in Clang.

clang/lib/CodeGen/CoverageMappingGen.cpp
1638–1639

CoveragePrefixMap can be removed. I just removed DebugPrefixMap as well:)

This revision is now accepted and ready to land.Apr 25 2023, 3:20 PM
gulfem updated this revision to Diff 516989.Apr 25 2023, 5:01 PM

Addressed feedback and rebased.

gulfem updated this revision to Diff 516993.Apr 25 2023, 5:10 PM

Added a comment about reverse traversal.

gulfem marked 2 inline comments as done.Apr 25 2023, 5:12 PM
gulfem added inline comments.
clang/lib/CodeGen/CoverageMappingGen.cpp
1638–1639
gulfem updated this revision to Diff 517012.Apr 25 2023, 6:44 PM
gulfem marked an inline comment as done.

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

gulfem updated this revision to Diff 517375.Apr 26 2023, 4:17 PM

Add text to document that coverage prefix map should be traversed in reverse order.

MaskRay added inline comments.Apr 26 2023, 4:44 PM
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...

gulfem added inline comments.Apr 26 2023, 5:07 PM
clang/include/clang/Basic/CodeGenOptions.h
211

@MaskRay does this sound good to you?

/// Prefix replacement map for source-based code coverage to remap source
/// file paths in coverage mapping.
llvm::SmallVector<std::pair<std::string, std::string>, 0> CoveragePrefixMap;
MaskRay accepted this revision as: MaskRay.Apr 26 2023, 5:18 PM
gulfem updated this revision to Diff 517400.Apr 26 2023, 5:22 PM

Extend the comment about the usage of CoveragePrefixMap.

This revision was landed with ongoing or failed builds.Apr 26 2023, 5:24 PM
This revision was automatically updated to reflect the committed changes.