1. Use || to merge counters that refer to the same regions
2. Use || to merge counters from raw profiles

Fixed the issue that causes a crash in InstCombinePHI.cpp. Phi instructions need to be inserted at the beginning of basic blocks, and profile increments need to be inserted after phis for conditional operators.

Remove hyphen between "single counter" for consistency

Ensure the correct traversal order for loops in CodeGenPGO to fix the profile test failures (Profile/cxx-hash-v2.cpp and Profile/cxx-rangefor.cpp)

I added a few nit-picks. Other than that, looks good to me.

In D156611#4555830, @phosek wrote:

This is a breaking change since it modifies the .covmapping format in a backwards incompatible way. For .profdata format we always try to avoid breaking changes and provide backwards compatibility, but I don't know if the same holds for the .covmapping format which is used only for testing. @davidxl do you have any thoughts on that?

In D156611#4581842, @AtomicGu wrote:

@gulfem The pre-merge checks have been passed. How to commit this review? I think I don't have the commit access.

• Extend the prototype by implementing the rest of the control-flow statements
• Rename the prototype to single byte counters to be consistent with the single byte coverage used in PGO

Please remove the following in the commit message:

Reuse the CoverageSize field of the first file record to store the section size after Version4.

In D156468#4574430, @thakis wrote:

Your fix is in now, so that's ok, but there was a 2-day window between things being known-bad and the fix landing. This causes needless work (see e.g. https://bugs.chromium.org/p/chromium/issues/detail?id=1471633). Reverting is usually much faster than fixing, so please default to revert-first to keep trunk green (unless you have a fix that's ready to land as quickly as the revert).

In D156611#4555830, @phosek wrote:

This is a breaking change since it modifies the .covmapping format in a backwards incompatible way. For .profdata format we always try to avoid breaking changes and provide backwards compatibility, but I don't know if the same holds for the .covmapping format which is used only for testing. @davidxl do you have any thoughts on that?

We started seeing a segmentation fault after this patch in our mac builders.

[12554/40876](1) RUST host_x64/netstack3_core_instrumented_lib_test host_x64/exe.unstripped/netstack3_core_instrumented_lib_test
FAILED: host_x64/netstack3_core_instrumented_lib_test host_x64/exe.unstripped/netstack3_core_instrumented_lib_test 
          PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
          Stack dump:
           #0 0x0000000107d536d8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x1057e16d8)
           #1 0x0000000107d515c9 llvm::sys::RunSignalHandlers() (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x1057df5c9)
           #2 0x0000000107d53d66 SignalHandler(int) (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x1057e1d66)
           #3 0x00007fff70b215fd (/usr/lib/system/libsystem_platform.dylib+0x7fff673fc5fd)
           #4 0x0000000000000000 
           #5 0x00000001077bf1d2 llvm::object::ArchiveMemberHeader::getSize() const (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x10524d1d2)
           #6 0x00000001077c09cf llvm::object::Archive::Child::getName() const (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x10524e9cf)
           #7 0x00000001077c149e llvm::object::Archive::Child::getMemoryBufferRef() const (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x10524f49e)
           #8 0x0000000104e9cf7b lld::macho::ArchiveFile::addLazySymbols() (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x10292af7b)
           #9 0x0000000104e835c6 addFile(llvm::StringRef, LoadType, bool, bool, bool, bool) (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x1029115c6)
          #10 0x0000000104e80852 lld::macho::link(llvm::ArrayRef<char const*>, llvm::raw_ostream&, llvm::raw_ostream&, bool, bool) (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x10290e852)
          #11 0x000000010537f33f lld::unsafeLldMain(llvm::ArrayRef<char const*>, llvm::raw_ostream&, llvm::raw_ostream&, llvm::ArrayRef<lld::DriverDef>, bool) (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x102e0d33f)
          #12 0x000000010263af99 lld_main(int, char**, llvm::ToolContext const&) (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x1000c8f99)
          #13 0x00000001028474bb findTool(int, char**, char const*) (/opt/s/w/ir/x/w/fuchsia/out/not-default/../../prebuilt/third_party/clang/mac-x64/bin/ld64.lld+0x1002d54bb)
          #14 0x00007fff70928cc9 (/usr/lib/system/libdyld.dylib+0x7fff67203cc9)
          #15 0x000000000000009f 
          clang++: error: unable to execute command: Segmentation fault: 11
          clang++: error: linker command failed due to signal (use -v to see invocation)

In D151283#4511131, @AtomicGu wrote:

@gulfem OK, I have updated the title and summary. I hope this will do.

Thanks, that looks better.

In D151283#4505153, @AtomicGu wrote:

In D151283#4503401, @gulfem wrote:

Can we simplify the commit message? We do not need all the details in the commit message as we have them in the Discourse discussion. I would suggest something like: "llvm-cov currently generates a single top-level index HTML file, which causes rendering scalability issues in large projects. This patch adds support for hierarchical directory structure into the HTML reports to solve scalability issues."

Do you mean I should squash the commits into one single commit? I thought Phabricator would do this for me as I always use arc diff with origin/main. I'm not sure what should I do.

Can we simplify the commit message? We do not need all the details in the commit message as we have them in the Discourse discussion. I would suggest something like: "llvm-cov currently generates a single top-level index HTML file, which causes rendering scalability issues in large projects. This patch adds support for hierarchical directory structure into the HTML reports to solve scalability issues."

Created a duplicate review by mistake.

In D151319#4373115, @melver wrote:

In D151319#4372921, @gulfem wrote:

We started seeing the following failure in our Clang Toolchain builders:

[1576/1617](6) Linking CXX shared library /b/s/w/ir/x/w/staging/llvm_build/lib/clang/17/lib/x86_64-unknown-fuchsia/libclang_rt.hwasan.so
FAILED: /b/s/w/ir/x/w/staging/llvm_build/lib/clang/17/lib/x86_64-unknown-fuchsia/libclang_rt.hwasan.so
ld.lld: error: undefined symbol: __sanitizer::RenderFrame(__sanitizer::InternalScopedString*, char const*, int, unsigned long, __sanitizer::AddressInfo const*, bool, char const*)

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8780110002348293249/+/u/clang/build/stdout

Here is our CMake commands:
https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8780133481341449745/+/u/clang/configure/execution_details

Thanks for the report. This should be fixed now: https://reviews.llvm.org/rG150470a055d5

We started seeing the following failure in our Clang Toolchain builders:

[1576/1617](6) Linking CXX shared library /b/s/w/ir/x/w/staging/llvm_build/lib/clang/17/lib/x86_64-unknown-fuchsia/libclang_rt.hwasan.so
FAILED: /b/s/w/ir/x/w/staging/llvm_build/lib/clang/17/lib/x86_64-unknown-fuchsia/libclang_rt.hwasan.so
ld.lld: error: undefined symbol: __sanitizer::RenderFrame(__sanitizer::InternalScopedString*, char const*, int, unsigned long, __sanitizer::AddressInfo const*, bool, char const*)

Changed the commit message

I did not have a change to finish reviewing this patch, but it breaks binary-id.c and binary-id-lookup.c tests that you can see in the pre-merge checks.

LLVM Profile Error: Failed to write file "/var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/profile/Profile-x86_64/Linux/Output/binary-id.c.tmp.profraw": File exists
error: /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/profile/Profile-x86_64/Linux/Output/binary-id.c.tmp.profraw: invalid instrumentation profile data (file header is corrupt)

https://reviews.llvm.org/harbormaster/unit/view/6779087/.

Extend the comment about the usage of CoveragePrefixMap.

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

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

Added a comment about reverse traversal.

Addressed feedback and rebased.

Updated the text.

In D148757#4289152, @MaskRay wrote:

In D148757#4289076, @MaskRay wrote:

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

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

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.

Modified the test case

In D148757#4282002, @MaskRay wrote:

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.

In D144083#4218819, @sgraenitz wrote:

This was due to my change to use a target flag to represent the thumb bit. Obviously, these are target-specific and ObjectLinkingLayer has to account for that. Relanding now with the fix.

Thanks, and the issue is fixed in your reland.

We started seeing a segmentation fault while running ExecutionEngine tests on Mac.

******************** TEST 'LLVM :: ExecutionEngine/MCJIT/2003-01-04-ArgumentBug.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /opt/s/w/ir/x/w/staging/llvm_build/bin/lli -jit-kind=mcjit /opt/s/w/ir/x/w/llvm-llvm-project/llvm/test/ExecutionEngine/MCJIT/2003-01-04-ArgumentBug.ll > /dev/null
: 'RUN: at line 2';   /opt/s/w/ir/x/w/staging/llvm_build/bin/lli /opt/s/w/ir/x/w/llvm-llvm-project/llvm/test/ExecutionEngine/MCJIT/2003-01-04-ArgumentBug.ll > /dev/null
--
Exit Code: 139

In D145646#4213975, @MaskRay wrote:

In D145646#4213547, @gulfem wrote:

We started seeing a test failure in avr-ld.c.

/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/avr-ld.c:48:11: error: 
// LINKP: {{".*ld.*"}} {{.*}} "--defsym=__DATA_REGION_ORIGIN__=0x800100" "-plugin" {{.*}} "-plugin-opt=mcpu=atmega328"
          ^
<stdin>:1:1: note: scanning from here
Fuchsia clang version 17.0.0 (https://llvm.googlesource.com/llvm-project 0d37efdbc599e61ce2a0418723a66d6b45aea8d7)
^
<stdin>:6:239: note: possible intended match here

https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8785950851645447937/overview

I am testing a change and will fix it in one minute.

We started seeing a test failure in avr-ld.c.

/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/avr-ld.c:48:11: error: 
// LINKP: {{".*ld.*"}} {{.*}} "--defsym=__DATA_REGION_ORIGIN__=0x800100" "-plugin" {{.*}} "-plugin-opt=mcpu=atmega328"
          ^
<stdin>:1:1: note: scanning from here
Fuchsia clang version 17.0.0 (https://llvm.googlesource.com/llvm-project 0d37efdbc599e61ce2a0418723a66d6b45aea8d7)
^
<stdin>:6:239: note: possible intended match here

Your commit message mentioned: "Fuchsia is not yet supported."
Is there anything that stops us generating MC/DC coverage in Fuchsia or is it just not tested yet?

Overall, it looks good to me. I added a few minor things.
Please add --show-mcdc to the llvm-cov command guide.
https://github.com/llvm/llvm-project/blob/main/llvm/docs/CommandGuide/llvm-cov.rst

Can you please add a test with DEBUGINFOD_CACHE_POLICY environment variable?

Please remove the dot at the end of the commit message.

Addressed reviewer feedback.

Corrected Petr's name.

In D142216#4078574, @Amir wrote:

Was fixed in 7557b83aa543e1e3f0c820ecbb14721d4e80a809