[InstrProfiling] Use !associated metadata for counters, data and values

[phosek]

Summary

C identifier name input sections such as __llvm_prf_* are GC roots so
they cannot be discarded. In LLD, the SHF_LINK_ORDER flag overrides the
C identifier name semantics.

The !associated metadata may be attached to a global object declaration
with a single argument that references another global object, and it
gets lowered to SHF_LINK_ORDER flag. When a function symbol is discarded
by the linker, setting up !associated metadata allows linker to discard
counters, data and values associated with that function symbol.

Note that !associated metadata is only supported by ELF, it does not have
any effect on non-ELF targets.

[davidxl]

maybe adding.a test at profile runtime as well to check GC actually happens?

[phosek]

This is an alternative solution to D64045, but it seems like !associated metadata, which are lowered to SHF_LINK_ORDER are incompatible with section groups which are used for external symbols, so this solution alone isn't sufficient.

[phosek]

I've added a test which is analogous to instrprof-darwin-dead-strip.c which tests the same functionality on Darwin. I've also updated the inliner to properly handle !associated metadata in D80186 which is necessary here since instr-profiling pass runs before the inliner.

[phosek]

@davidxl would it be possible to take a look?

[davidxl]

What is the instrumented size improvement for Clang? How about profile size reduction?

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

40

what is this change for?

41

why adding the weak attribute ?

compiler-rt/test/profile/instrprof-gc-sections.c

4

Also check IR PGO?

73

Explain the expected output here?

[phosek]

In D76802#2061718, @davidxl wrote:

What is the instrumented size improvement for Clang? How about profile size reduction?

I tried building Clang using -fprofile-instr-generate with this change and without it and the size went from 1.7G to 820M, so roughly 50% reduction. I also tried compiling a trivial C file and the generated profile was 155M with this change and 978M without it.

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

40

!associated metadata is lowered to SHF_LINK_ORDER and linker requires that all sections that are merged together have the same flags. Since these symbols are placed into the same section as the compiler generated symbols, and don't set the SHF_LINK_ORDER flag, we get a link-time error:

/usr/bin/ld: __llvm_prf_cnts has both ordered [`__llvm_prf_cnts' in /tmp/instrprof-value-prof-real-2c3254.o] and unordered [`__llvm_prf_cnts' in /src/clang-llvm/llvm-build/debug/lib/clang/11.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfilingPlatformLinux.c.o)] sections

One possible way around this is to define these symbols using inline assembly (since there's no way to set SHF_LINK_ORDER in pure C/C++):

asm(
  ".pushsection " INSTR_PROF_DATA_SECT_NAME ",\"ao\",@progbits,.text\n\t"
  ".popsection\n"
)

Another option is to avoid these symbols altogether, and instead make the __start/__stop symbols weak which means that they'll end up as NULL if those sections are empty, and any loop that iterates over those section contents using those __start/__stop will be no-op. This seemed like a cleaner solution to me, but I'm happy to change if you prefer. One alternative I considered would be to introduce a new attribute which would allow setting the SHF_LINK_ORDER flag from C/C++, but that's going to take a bit more work.

41

See on the left.

compiler-rt/test/profile/instrprof-gc-sections.c

73

I've already tried to do that in Note: comments above.

[phosek]

I've also tested this change in Fuchsia and saw similar improvements: fuchsia.zbi (the boot image) size reduced from 90M to 42M, fvm.sparse.blk (disk image) size reduced from 3.9G to 2.8G.

[phosek]

@davidxl ping?

[davidxl]

(Sorry my last comments was not submitted)

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

33–41

Add a comment here about the need of the weak attribute

[davidxl]

Looks good but can you do help with a clang instrumentation build (IR PGO), and see the binary and profile size change?

[phosek]

In D76802#2077786, @davidxl wrote:

Looks good but can you do help with a clang instrumentation build (IR PGO), and see the binary and profile size change?

With IR PGO, the difference is much smaller: for Clang the size decreased from 141M to 134M, for the generated profile the size decreased from 18M to 17M.

[davidxl]

LGTM (thanks for the test. Will see how this works with very large internal apps.

[thakis]

this broke libprofile tests for i386: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27806 , also https://ci.chromium.org/p/chromium/builders/try/linux_upload_clang/966? (which has more failures, but look like the same cause)

Can you take a look, and revert if it takes a while to fix?

[bsaleil]

Hello,
This patch also broke some of our PowerPC buildbots (e.g. http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/4263).
Can you please push a fix or revert ?
Thanks.

[aeubanks]

This is still breaking 37 tests, even after the first attempted fix forward...
ninja check-profile

[vitalybuka]

Reverted 5a3b380f497b98d824b529a3369370629847a4ea

[phosek]

I apologize about the late response, I didn't get any email from Phabricator for some reason :/

I'll try to reproduce and address the issue on i386, but I don't have access to any PowerPC machine, @bsaleil is there any way to get access to get access to your PowerPC bots?

[phosek]

I looked into these issue:

• http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27806 failure was addressed by rG69c5ff4668cd4d1bfb6b45d71e15c12e3a23ae05
• https://ci.chromium.org/p/chromium/builders/try/linux_upload_clang/966 is unrelated and seems to be related to the use of BFD ld as the linker, I tried various experiments and it seems like BFD ld's handling of SHF_LINK_ORDER is broken, gold and lld handle this fine. Even if I can fix it, it's going to take years before that change gets to all the bots. I don't know how to address this other than requiring lld or gold for profile tests. Would that be reasonable?
• http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/4263 I cannot reproduce this locally, but what I think is happening is that your bot doesn't build lld, but this test requires lld (because we need working SHF_LINK_ORDER support) so when we pass -fuse-ld=lld, it uses the system lld which is likely outdated since the error we're seeing is coming from the bitcode reader. I think that we should either remove https://github.com/llvm/llvm-project/blob/master/compiler-rt/CMakeLists.txt#L539 or introduce a new lit stanza that means "lld is not only available, but was built as part of this build and is up-to-date".

[vitalybuka]

In D76802#2086650, @phosek wrote:

I looked into these issue:

• http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27806 failure was addressed by rG69c5ff4668cd4d1bfb6b45d71e15c12e3a23ae05

69c5ff4668cd4d1bfb6b45d71e15c12e3a23ae05 still fails "check-profile" for me and fails here http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27808

[phosek]

In D76802#2086725, @vitalybuka wrote:

In D76802#2086650, @phosek wrote:

I looked into these issue:

• http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27806 failure was addressed by rG69c5ff4668cd4d1bfb6b45d71e15c12e3a23ae05

69c5ff4668cd4d1bfb6b45d71e15c12e3a23ae05 still fails "check-profile" for me and fails here http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27808

I've updated the patch to handle the _IO_stdin_used symbol in the test. I couldn't reproduce this locally, but it's possible that this is due to different libc version on the bot.

[vitalybuka]

In D76802#2086742, @phosek wrote:

In D76802#2086725, @vitalybuka wrote:

In D76802#2086650, @phosek wrote:

I looked into these issue:

• http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27806 failure was addressed by rG69c5ff4668cd4d1bfb6b45d71e15c12e3a23ae05

69c5ff4668cd4d1bfb6b45d71e15c12e3a23ae05 still fails "check-profile" for me and fails here http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27808

I've updated the patch to handle the _IO_stdin_used symbol in the test. I couldn't reproduce this locally, but it's possible that this is due to different libc version on the bot.

I still can reproduce with the latest patch on my workstation

Exit Code: 1

Command Output (stderr):
--
/usr/bin/ld: __llvm_prf_data has both ordered and unordered sections
/usr/bin/ld: final link failed: bad value
clang-11: error: linker command failed with exit code 1 (use -v to see invocation)

[phosek]

In D76802#2086757, @vitalybuka wrote:

In D76802#2086742, @phosek wrote:

In D76802#2086725, @vitalybuka wrote:

In D76802#2086650, @phosek wrote:

I looked into these issue:

• http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27806 failure was addressed by rG69c5ff4668cd4d1bfb6b45d71e15c12e3a23ae05

69c5ff4668cd4d1bfb6b45d71e15c12e3a23ae05 still fails "check-profile" for me and fails here http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/27808

I've updated the patch to handle the _IO_stdin_used symbol in the test. I couldn't reproduce this locally, but it's possible that this is due to different libc version on the bot.

I still can reproduce with the latest patch on my workstation

Exit Code: 1

Command Output (stderr):
--
/usr/bin/ld: __llvm_prf_data has both ordered and unordered sections
/usr/bin/ld: final link failed: bad value
clang-11: error: linker command failed with exit code 1 (use -v to see invocation)

This is the other issue I mentioned which also affects Chromium bots. The root cause is the lack of support for the extended semantics of SHF_LINK_ORDER that !associated metadata rely on in bfd.ld. I'm trying to figure out what's the best way forward. We could just disable all affected profile tests when bfd.ld is used as the linker, but that's pretty drastic. Another option would be to gate this feature on a backend flag; targets that use gold or lld as their linker could turn this on by default.

[phosek]

In D76802#2086875, @phosek wrote:

In D76802#2086757, @vitalybuka wrote:

I still can reproduce with the latest patch on my workstation

Exit Code: 1

Command Output (stderr):
--
/usr/bin/ld: __llvm_prf_data has both ordered and unordered sections
/usr/bin/ld: final link failed: bad value
clang-11: error: linker command failed with exit code 1 (use -v to see invocation)

This is the other issue I mentioned which also affects Chromium bots. The root cause is the lack of support for the extended semantics of SHF_LINK_ORDER that !associated metadata rely on in bfd.ld. I'm trying to figure out what's the best way forward. We could just disable all affected profile tests when bfd.ld is used as the linker, but that's pretty drastic. Another option would be to gate this feature on a backend flag; targets that use gold or lld as their linker could turn this on by default.

I've found a workaround for the bfd.ld issue. The problem is that __llvm_prf_data input sections have sh_addralign=8 but size is 4mod8. bfd.ld is considering the alignment padding insertions to be "unordered sections" in the input. Inserting artificial padding in the __llvm_prf_data input section seems to work around the issue. @vitalybuka can you test this locally on your machine please?

[aeubanks]

Most failures are fixed, thanks! But I still get one failure with this patch:
LLVM Profile Warning: Unable to merge profile data: source profile file is not compatible.
LLVM Profile Error: Profile Merging of file /usr/local/google/home/aeubanks/repos/llvm-project3/build_cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-gc-sections.c.tmp.pgo.profraw/default_15822682845097088506_0.profraw failed: File exists
LLVM Profile Error: Failed to write file "/usr/local/google/home/aeubanks/repos/llvm-project3/build_cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-gc-sections.c.tmp.pgo.profraw/default_15822682845097088506_0.profraw": File exists
warning: /usr/local/google/home/aeubanks/repos/llvm-project3/build_cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-gc-sections.c.tmp.pgo.profraw/default_15822682845097088506_0.profraw: Unsupported instrumentation profile format version
error: No profiles could be merged.

[phosek]

@davidxl Do you have any preference between the current solution which uses the artificial padding in __llvm_prf_data section to workaround the BFD ld bug, and a backend feature flag that could be set to enable this feature?

[phosek]

In D76802#2089052, @phosek wrote:

@davidxl Do you have any preference between the current solution which uses the artificial padding in __llvm_prf_data section to workaround the BFD ld bug, and a backend feature flag that could be set to enable this feature?

The latter might be preferable because there's another bug in trunk BFD ld which causes to enter infinite loop, see D72904 for more details.

[bsaleil]

In D76802#2086650, @phosek wrote:

• http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/4263 I cannot reproduce this locally, but what I think is happening is that your bot doesn't build lld, but this test requires lld (because we need working SHF_LINK_ORDER support) so when we pass -fuse-ld=lld, it uses the system lld which is likely outdated since the error we're seeing is coming from the bitcode reader. I think that we should either remove https://github.com/llvm/llvm-project/blob/master/compiler-rt/CMakeLists.txt#L539 or introduce a new lit stanza that means "lld is not only available, but was built as part of this build and is up-to-date".

@phosek, we just updated the clang-ppc64le-rhel bot to lld 10.0.0, is that recent enough for SHF_LINK_ORDER support you need ?

[phosek]

@davidxl does this look good to you? This feature is now gated on -counter-associated-metadata flag (I'd welcome suggestions for better name if you have some) because we require recent lld. We might be able to set that flag in the frontend when we know that the latest lld is being used in the future.

[pcc]

In D76802#2200906, @phosek wrote:

@davidxl does this look good to you? This feature is now gated on -counter-associated-metadata flag (I'd welcome suggestions for better name if you have some) because we require recent lld. We might be able to set that flag in the frontend when we know that the latest lld is being used in the future.

Can you find a better way of communicating this infomation than via an llvm::cl flag (e.g. use a function attribute)? The flag won't be compatible with LTO.

[phosek]

In D76802#2200926, @pcc wrote:

In D76802#2200906, @phosek wrote:

@davidxl does this look good to you? This feature is now gated on -counter-associated-metadata flag (I'd welcome suggestions for better name if you have some) because we require recent lld. We might be able to set that flag in the frontend when we know that the latest lld is being used in the future.

Can you find a better way of communicating this infomation than via an llvm::cl flag (e.g. use a function attribute)? The flag won't be compatible with LTO.

Do I understand your suggestion correctly that we would set function attribute when -fprofile-*-generate -fuse-lld is used that would enable the use of metadata in the backend?

[pcc]

In D76802#2201164, @phosek wrote:

In D76802#2200926, @pcc wrote:

In D76802#2200906, @phosek wrote:

@davidxl does this look good to you? This feature is now gated on -counter-associated-metadata flag (I'd welcome suggestions for better name if you have some) because we require recent lld. We might be able to set that flag in the frontend when we know that the latest lld is being used in the future.

Can you find a better way of communicating this infomation than via an llvm::cl flag (e.g. use a function attribute)? The flag won't be compatible with LTO.

Do I understand your suggestion correctly that we would set function attribute when -fprofile-*-generate -fuse-lld is used that would enable the use of metadata in the backend?

Yes, that's what I had in mind.

[phosek]

In D76802#2201170, @pcc wrote:

In D76802#2201164, @phosek wrote:

In D76802#2200926, @pcc wrote:

In D76802#2200906, @phosek wrote:

@davidxl does this look good to you? This feature is now gated on -counter-associated-metadata flag (I'd welcome suggestions for better name if you have some) because we require recent lld. We might be able to set that flag in the frontend when we know that the latest lld is being used in the future.

Can you find a better way of communicating this infomation than via an llvm::cl flag (e.g. use a function attribute)? The flag won't be compatible with LTO.

Do I understand your suggestion correctly that we would set function attribute when -fprofile-*-generate -fuse-lld is used that would enable the use of metadata in the backend?

Yes, that's what I had in mind.

I was thinking that maybe a better approach would be to provide a new flag akin to -fbinutils-version introduced D85474, for example -flld-version. We would then emit the metadata unconditionally. In the backend, we would then check if LLD version is greater than 12 or if binutils version is greater than 2.36, and if not we would avoid using SHF_LINK_ORDER in this case effectively ignoring the metadata. During LTO, LLD would pass its own version to the backend via TargetOptions. WDYT?

[phosek]

In D76802#2514717, @phosek wrote:

In D76802#2201170, @pcc wrote:

In D76802#2201164, @phosek wrote:

In D76802#2200926, @pcc wrote:

In D76802#2200906, @phosek wrote:

@davidxl does this look good to you? This feature is now gated on -counter-associated-metadata flag (I'd welcome suggestions for better name if you have some) because we require recent lld. We might be able to set that flag in the frontend when we know that the latest lld is being used in the future.

Can you find a better way of communicating this infomation than via an llvm::cl flag (e.g. use a function attribute)? The flag won't be compatible with LTO.

Do I understand your suggestion correctly that we would set function attribute when -fprofile-*-generate -fuse-lld is used that would enable the use of metadata in the backend?

Yes, that's what I had in mind.

I was thinking that maybe a better approach would be to provide a new flag akin to -fbinutils-version introduced D85474, for example -flld-version. We would then emit the metadata unconditionally. In the backend, we would then check if LLD version is greater than 12 or if binutils version is greater than 2.36, and if not we would avoid using SHF_LINK_ORDER in this case effectively ignoring the metadata. During LTO, LLD would pass its own version to the backend via TargetOptions. WDYT?

Alternatively, we could assume that LLD version always matches LLVM and avoid -flld-version, but we'll need some way to communicate to the backend that LLD is being used as the linker. @MaskRay do you have any thoughts on this?

[MaskRay]

In D76802#2516296, @phosek wrote:

Alternatively, we could assume that LLD version always matches LLVM and avoid -flld-version, but we'll need some way to communicate to the backend that LLD is being used as the linker. @MaskRay do you have any thoughts on this?

I am happy with the direction here (doing something to decrease binary sizes and help garbage collection).

I think this is probably fine. ELF is stable and has less evolution on binary formats. Some case studies:

• SHF_LINK_ORDER: I think when Clang started to emit this for sanitizers (!associated), Clang required nearly bleeding-edge LLD
• R_X86_64_REX_GOTPCRELX: GCC with ~2015 GNU as will emit R_X86_64_REX_GOTPCRELX by default. LLD had supported it for a few years before I switched Clang default. (There was still few things I did not catch beforehand: (LLD lacked relocation overflow check) + (in an edge-case clang code generation, LLD could wrongly relax the relocation))

I had read through all the previous discussions half a year ago but my memory might be outdated, so apologies in advance if I missed context...

Newer LLD SHF_LINK_ORDER support had landed ~August 2020 which should be in LLD 11.0.0 (some may be tin LLD 10.0.0, I don't check).
If you switch the binary format feature used now, the change will go into Clang 12.0.0, i.e. Clang 12 + LLD 11 should be good.
Clang 12 + LLD 10 is unsupported. This looks quite good to me.
(Note: for PGO+LTO users they need to ensure that LLD is newer than Clang. Newer Clang may emit bitcode files not recognizable by older LLD. The recent event happened just one or two month ago when a new loop related metadata was added.)

One problem is whether we want to make -fuse-ld=lld (currently a pure linker stage option) affect code generation. If I remember it correctly @pcc had concerns with it but from looking at the previous comment seems that he is fine? :)

I remember I have changed some comdat thing and would like to verify the effect of this patch myself.

[phosek]

In D76802#2516388, @MaskRay wrote:

In D76802#2516296, @phosek wrote:

Alternatively, we could assume that LLD version always matches LLVM and avoid -flld-version, but we'll need some way to communicate to the backend that LLD is being used as the linker. @MaskRay do you have any thoughts on this?

I am happy with the direction here (doing something to decrease binary sizes and help garbage collection).

I think this is probably fine. ELF is stable and has less evolution on binary formats. Some case studies:

• SHF_LINK_ORDER: I think when Clang started to emit this for sanitizers (!associated), Clang required nearly bleeding-edge LLD
• R_X86_64_REX_GOTPCRELX: GCC with ~2015 GNU as will emit R_X86_64_REX_GOTPCRELX by default. LLD had supported it for a few years before I switched Clang default. (There was still few things I did not catch beforehand: (LLD lacked relocation overflow check) + (in an edge-case clang code generation, LLD could wrongly relax the relocation))

I had read through all the previous discussions half a year ago but my memory might be outdated, so apologies in advance if I missed context...

Newer LLD SHF_LINK_ORDER support had landed ~August 2020 which should be in LLD 11.0.0 (some may be tin LLD 10.0.0, I don't check).
If you switch the binary format feature used now, the change will go into Clang 12.0.0, i.e. Clang 12 + LLD 11 should be good.
Clang 12 + LLD 10 is unsupported. This looks quite good to me.
(Note: for PGO+LTO users they need to ensure that LLD is newer than Clang. Newer Clang may emit bitcode files not recognizable by older LLD. The recent event happened just one or two month ago when a new loop related metadata was added.)

One problem is whether we want to make -fuse-ld=lld (currently a pure linker stage option) affect code generation. If I remember it correctly @pcc had concerns with it but from looking at the previous comment seems that he is fine? :)

The problem I can see is that -fuse-ld=lld is usually only passed in linker flags (see for example https://source.chromium.org/chromium/chromium/src/+/master:build/config/compiler/BUILD.gn;l=314), but unless LTO is being used, we'd need to know which linker is going to be used during compilation, that is we'd need it to be passed as a compiler flag.

Since binutils 2.36 was released yesterday, we could also just consider emitting SHF_LINKER_ORDER unconditionally which would avoid this issue altogether. The only concern there is if someone tried to use new Clang with older binutils, but it sounds like with sanitizers we've ignored that concern it wasn't a problem, so maybe it won't be a problem here either?

I remember I have changed some comdat thing and would like to verify the effect of this patch myself.

[MaskRay]

The problem I can see is that -fuse-ld=lld is usually only passed in linker flags (see for example https://source.chromium.org/chromium/chromium/src/+/master:build/config/compiler/BUILD.gn;l=314), but unless LTO is being used, we'd need to know which linker is going to be used during compilation, that is we'd need it to be passed as a compiler flag.

Even for LTO, -fuse-ld=lld should have minimum behavior difference when using LLVMgold.so.

Since binutils 2.36 was released yesterday, we could also just consider emitting SHF_LINKER_ORDER unconditionally which would avoid this issue altogether. The only concern there is if someone tried to use new Clang with older binutils, but it sounds like with sanitizers we've ignored that concern it wasn't a problem, so maybe it won't be a problem here either?

It might cause friction to PGO+non-LLD users... I think it might be premature for LLVM 13.0.0. 14.0.0 may be fine.

You may add tests to linkage.ll (see D84723).

With the current path, opt -counter-associated-metadata=1 < linkage.ll -mtriple=x86_64-unknown-linux -instrprof -S | llc -filetype=obj crashes due to an empty metadata node.

[phosek]

In D76802#2520731, @MaskRay wrote:

It might cause friction to PGO+non-LLD users... I think it might be premature for LLVM 13.0.0. 14.0.0 may be fine.

Do you think we should go with the current solution then and change the default to enable -counter-associated-metadata in LLVM 14.0.0?

You may add tests to linkage.ll (see D84723).

With the current path, opt -counter-associated-metadata=1 < linkage.ll -mtriple=x86_64-unknown-linux -instrprof -S | llc -filetype=obj crashes due to an empty metadata node.

I forgot to upload the latest patch which uses sh_link=self instead of sh_link=0, it should be working now.

[MaskRay]

• __llvm_prf_cnts does not need to have a self link SHF_LINK_ORDER. __llvm_prf_data linking to __llvm_prf_cnts suffices. This is on the premise that only __llvm_prf_data may reference __llvm_prf_cnts
• Please update InstrProfiling/icall.ll to test __profvp_ (-vp-static-alloc=true). __llvm_prf_vals should link to __llvm_prf_cnts.
• From the current organization of tests, I think linkage.ll is a better place than a separate associated.ll. The tests there are more comprehensive (needs-comdat and non-needs-comdat cases). You can add the new associated mode to linkage.ll.

-fbinutils-version is now available. You can condition the associated feature on 2.36 now.

[phosek]

In D76802#2524039, @MaskRay wrote:

• __llvm_prf_cnts does not need to have a self link SHF_LINK_ORDER. __llvm_prf_data linking to __llvm_prf_cnts suffices. This is on the premise that only __llvm_prf_data may reference __llvm_prf_cnts

Discussed with @MaskRay over chat, this is necessary because __llvm_prf_cnts is a C identifier.

• Please update InstrProfiling/icall.ll to test __profvp_ (-vp-static-alloc=true). __llvm_prf_vals should link to __llvm_prf_cnts.

Done.

• From the current organization of tests, I think linkage.ll is a better place than a separate associated.ll. The tests there are more comprehensive (needs-comdat and non-needs-comdat cases). You can add the new associated mode to linkage.ll.

Done.

-fbinutils-version is now available. You can condition the associated feature on 2.36 now.

Done.

[phosek]

@MaskRay does this look good to you?

[MaskRay]

Looks quite good.

In D76802#2526382, @phosek wrote:

@MaskRay does this look good to you?

Looks like GNU ld has an infinite loop problem w.r.t self-link SHF_LINK_ORDER: https://sourceware.org/bugzilla/show_bug.cgi?id=27259

The reason is that the C identifier sections are considered as GC roots. Adding SHF_LINK_ORDER is a trick to defeat the GC root semantics.

Hope GNU ld can implement the rule. For now, only -fbinutils-version=none can use this.

About counter-associated-metadata: perhaps we should just call it linkorder-counter. !associated while being metadata, is actually similar to , comdat (it cannot be freely dropped) and its semantics is tightly coupled with SHF_LINK_ORDER. linkorder conveys more information than associated (associated can mean many things).

[MaskRay]

Please also comment (and update the description) that self-link may be used. This is subtle. The reason is that the ELF section names (e.g. __llvm_prf_cnts) are C identifiers and considered GC roots in the absence of the SHF_LINK_ORDER flag.

[phosek]

In D76802#2526483, @MaskRay wrote:

Looks quite good.

In D76802#2526382, @phosek wrote:

@MaskRay does this look good to you?

Looks like GNU ld has an infinite loop problem w.r.t self-link SHF_LINK_ORDER: https://sourceware.org/bugzilla/show_bug.cgi?id=27259

Thanks for checking this.

The reason is that the C identifier sections are considered as GC roots. Adding SHF_LINK_ORDER is a trick to defeat the GC root semantics.

Hope GNU ld can implement the rule. For now, only -fbinutils-version=none can use this.

I've updated the logic to reflect that.

About counter-associated-metadata: perhaps we should just call it linkorder-counter. !associated while being metadata, is actually similar to , comdat (it cannot be freely dropped) and its semantics is tightly coupled with SHF_LINK_ORDER. linkorder conveys more information than associated (associated can mean many things).

Done.

In D76802#2526485, @MaskRay wrote:

Please also comment (and update the description) that self-link may be used. This is subtle. The reason is that the ELF section names (e.g. __llvm_prf_cnts) are C identifiers and considered GC roots in the absence of the SHF_LINK_ORDER flag.

Done.

[phosek]

@MaskRay I saw on https://sourceware.org/bugzilla/show_bug.cgi?id=27259 that the infinite loop fix has been cherry picked into 2.36 branch so I updated the condition to -fbinutils-version=2.36. Is this OK to land?

[MaskRay]

The !associated metadata may be attached to a global object declaration with a single argument that references another global object. This metadata prevents discarding of the global object in linker GC unless the referenced object is also discarded.

"prevents discarding" is not accurate in the context. __llvm_prf_* cannot be discarded because C identifier name input sections are GC roots.

Add "C identifier name input sections are GC roots. In LLD, the SHF_LINK_ORDER flag overrides the C identifier name semantics." in appropriate place.

Furthermore, when a function symbol is discarded by the linker, setting up !associated metadata allows linker to discard counters, data and values associated with that function symbol. This is not possible today because there's metadata to guide the linker. This approach is also used by other instrumentations like sanitizers.

This C identifier name GC currently only works with LLD but does no harm to gold/GNU ld.

Do you think __llvm_prf_* can link to the text section so that the self-link trick is not needed? (https://sourceware.org/bugzilla/show_bug.cgi?id=27259)

[phosek]

In D76802#2534118, @MaskRay wrote:

"prevents discarding" is not accurate in the context. __llvm_prf_* cannot be discarded because C identifier name input sections are GC roots.

Add "C identifier name input sections are GC roots. In LLD, the SHF_LINK_ORDER flag overrides the C identifier name semantics." in appropriate place.

Done, I have revised the change description.

This C identifier name GC currently only works with LLD but does no harm to gold/GNU ld.

Do you think __llvm_prf_* can link to the text section so that the self-link trick is not needed? (https://sourceware.org/bugzilla/show_bug.cgi?id=27259)

I associated the counter with the function symbol which seems to be working and we no longer need the self-link trick.

[MaskRay]

Thanks!

[MaskRay]

compiler-rt/test/profile/instrprof-gc-sections.c

31

This fails on my machine (cc @akuegel). If I use diff -u:

--- /tmp/RelA/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-gc-sections.c.tmp.nocode.syms   2021-02-01 22:14:22.850222346 -0800
+++ /tmp/RelA/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-gc-sections.c.tmp.code.syms     2021-02-01 22:14:22.854222358 -0800
@@ -3,6 +3,5 @@
 __prof_cnts_sect_data
 __prof_data_sect_data
 __prof_nms_sect_data
-__prof_orderfile_sect_data
 __prof_vnodes_sect_data
 lprofDirMode

At the worst we can temporarily disable it (if others report issues as well).

[phosek]

compiler-rt/test/profile/instrprof-gc-sections.c

31

What's your build setup? I ran check-profile on my Linux workstation just now but haven't seen this issue.

[TWeaver]

Hiya,

this patch (probably) broke the build bot at:

http://lab.llvm.org:8011/#/builders/53/builds/1184

[aeubanks]

compiler-rt/test/profile/instrprof-gc-sections.c

31

This also fails on my machine:
$ cmake -GNinja -DLLVM_ENABLE_PROJECTS='clang;compiler-rt;lld' -DLLVM_TARGETS_TO_BUILD=X86 -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_LLD=ON ..
$ ninja check-profile

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

 #define PROF_NAME_STOP INSTR_PROF_SECT_STOP(INSTR_PROF_NAME_COMMON)
 #define PROF_CNTS_START INSTR_PROF_SECT_START(INSTR_PROF_CNTS_COMMON)
 #define PROF_CNTS_STOP INSTR_PROF_SECT_STOP(INSTR_PROF_CNTS_COMMON)
 #define PROF_ORDERFILE_START INSTR_PROF_SECT_START(INSTR_PROF_ORDERFILE_COMMON)
 #define PROF_VNODES_START INSTR_PROF_SECT_START(INSTR_PROF_VNODES_COMMON)
 #define PROF_VNODES_STOP INSTR_PROF_SECT_STOP(INSTR_PROF_VNODES_COMMON)
 
 /* Declare section start and stop symbols for various sections
- * generated by compiler instrumentation.
+ * generated by compiler instrumentation. These symbols are
+ * declared as weak, which means that they'll end up as NULL if
+ * those sections are empty, and any code that iterates over the
+ * content using __llvm_profile_begin_* and __llvm_profile_end_*
+ * functions defined below will be a no-op.
  */
-extern __llvm_profile_data PROF_DATA_START COMPILER_RT_VISIBILITY;
-extern __llvm_profile_data PROF_DATA_STOP COMPILER_RT_VISIBILITY;
-extern uint64_t PROF_CNTS_START COMPILER_RT_VISIBILITY;
-extern uint64_t PROF_CNTS_STOP COMPILER_RT_VISIBILITY;
-extern uint32_t PROF_ORDERFILE_START COMPILER_RT_VISIBILITY;
-extern char PROF_NAME_START COMPILER_RT_VISIBILITY;
-extern char PROF_NAME_STOP COMPILER_RT_VISIBILITY;
-extern ValueProfNode PROF_VNODES_START COMPILER_RT_VISIBILITY;
-extern ValueProfNode PROF_VNODES_STOP COMPILER_RT_VISIBILITY;
-
-/* Add dummy data to ensure the section is always created. */
-__llvm_profile_data
-    __prof_data_sect_data[0] COMPILER_RT_SECTION(INSTR_PROF_DATA_SECT_NAME);
-uint64_t
-    __prof_cnts_sect_data[0] COMPILER_RT_SECTION(INSTR_PROF_CNTS_SECT_NAME);
-uint32_t
-    __prof_orderfile_sect_data[0] COMPILER_RT_SECTION(INSTR_PROF_ORDERFILE_SECT_NAME);
-char __prof_nms_sect_data[0] COMPILER_RT_SECTION(INSTR_PROF_NAME_SECT_NAME);
-ValueProfNode __prof_vnodes_sect_data[0] COMPILER_RT_SECTION(INSTR_PROF_VNODES_SECT_NAME);
+extern __llvm_profile_data PROF_DATA_START COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
+extern __llvm_profile_data PROF_DATA_STOP COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
+extern uint64_t PROF_CNTS_START COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
+extern uint64_t PROF_CNTS_STOP COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
+extern uint32_t PROF_ORDERFILE_START COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
+extern char PROF_NAME_START COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
+extern char PROF_NAME_STOP COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
+extern ValueProfNode PROF_VNODES_START COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
+extern ValueProfNode PROF_VNODES_STOP COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
 
 COMPILER_RT_VISIBILITY const __llvm_profile_data *
 __llvm_profile_begin_data(void) {
   return &PROF_DATA_START;
 }
 COMPILER_RT_VISIBILITY const __llvm_profile_data *
 __llvm_profile_end_data(void) {
   return &PROF_DATA_STOP;

compiler-rt/test/profile/instrprof-gc-sections.c

+// REQUIRES: linux, lld-available
+
+// RUN: %clang_profgen=%t.profraw -fuse-ld=lld -fcoverage-mapping -mllvm -enable-name-compression=false -DCODE=1 -ffunction-sections -fdata-sections -Wl,--gc-sections -o %t %s
+// RUN: %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-profdata show --all-functions %t.profdata | FileCheck %s -check-prefix=PROF
+// RUN: llvm-cov show %t -instr-profile %t.profdata | FileCheck %s -check-prefix=COV
+// RUN: llvm-nm %t | FileCheck %s -check-prefix=NM
+// RUN: llvm-readelf -x __llvm_prf_names %t | FileCheck %s -check-prefix=PRF_NAMES
+// RUN: llvm-readelf -x __llvm_prf_cnts %t | FileCheck %s -check-prefix=PRF_CNTS
+
+// RUN: %clang_lto_profgen=%t.lto.profraw -fuse-ld=lld -fcoverage-mapping -mllvm -enable-name-compression=false -DCODE=1 -ffunction-sections -fdata-sections -Wl,--gc-sections -flto -o %t.lto %s
+// RUN: %run %t.lto
+// RUN: llvm-profdata merge -o %t.lto.profdata %t.lto.profraw
+// RUN: llvm-profdata show --all-functions %t.lto.profdata | FileCheck %s -check-prefix=PROF
+// RUN: llvm-cov show %t.lto -instr-profile %t.lto.profdata | FileCheck %s -check-prefix=COV
+// RUN: llvm-nm %t.lto | FileCheck %s -check-prefix=NM
+// RUN: llvm-readelf -x __llvm_prf_names %t.lto | FileCheck %s -check-prefix=PRF_NAMES
+// RUN: llvm-readelf -x __llvm_prf_cnts %t.lto | FileCheck %s -check-prefix=PRF_CNTS
+
+// Note: We expect foo() and some of the profiling data associated with it to
+// be garbage collected.
+
+// Note: When there is no code in a program, we expect to see the exact same
+// set of external functions provided by the profile runtime.
+
+// RUN: %clang_profgen -fcoverage-mapping -ffunction-sections -fdata-sections -Wl,--gc-sections -shared -o %t.nocode.so %s
+// RUN: llvm-nm -jgU %t.nocode.so | grep -vE "__start_.*|__stop_.*" > %t.nocode.syms
+// RUN: llvm-nm -jgU %t | grep -vE "main|foo|_start|__libc_.*" > %t.code.syms
+// RUN: diff %t.nocode.syms %t.code.syms
+
+// Note: We also check the IR instrumentation and expect foo() to be garbage
+// collected as well.
+
+// RUN: %clang_pgogen=%t.pgo.profraw -fuse-ld=lld -DCODE=1 -ffunction-sections -fdata-sections -Wl,--gc-sections -o %t.pgo %s
+// RUN: %run %t.pgo
+// RUN: llvm-profdata merge -o %t.pgo.profdata %t.pgo.profraw
+// RUN: llvm-profdata show --all-functions %t.pgo.profdata | FileCheck %s -check-prefix=PGO
+// RUN: llvm-nm %t.pgo | FileCheck %s -check-prefix=NM
+
+#ifdef CODE
+
+// COV: [[@LINE+1]]{{ *}}|{{ *}}0|void foo()
+void foo() {}
+
+// COV: [[@LINE+1]]{{ *}}|{{ *}}1|int main
+int main() { return 0; }
+
+#endif // CODE
+
+// NM-NOT: foo
+
+// PROF: Counters:
+// PROF-NEXT:   main:
+// PROF-NEXT:     Hash:
+// PROF-NEXT:     Counters: 1
+// PROF-NEXT:     Function count: 1
+// PROF-NEXT: Instrumentation level: Front-end
+// PROF-NEXT: Functions shown: 1
+// PROF-NEXT: Total functions: 1
+// PROF-NEXT: Maximum function count:
+// PROF-NEXT: Maximum internal block count:
+
+// Note: We don't expect the names of garbage collected functions to disappear
+// from __llvm_prf_names, because collectPGOFuncNameStrings() glues the names
+// together.
+
+// PRF_NAMES: Hex dump of section '__llvm_prf_names':
+// PRF_NAMES-NEXT: {{.*}} 0800666f 6f016d61 696e {{.*$}}
+//                        | | f o  o # m a  i n
+//                        | |___________|
+//                        |             |
+//               UncompressedLen = 8    |
+//                                      |
+//                               CompressedLen = 0
+
+// Note: We expect the profile counters for garbage collected functions to also
+// be garbage collected.
+
+// PRF_CNTS: Hex dump of section '__llvm_prf_cnts':
+// PRF_CNTS-NEXT: {{.*}} 00000000 00000000 {{.*$}}
+
+// PGO: Counters:
+// PGO-NEXT:   main:
+// PGO-NEXT:     Hash:
+// PGO-NEXT:     Counters: 1
+// PGO-NEXT: Instrumentation level: IR
+// PGO-NEXT: Functions shown: 1
+// PGO-NEXT: Total functions: 1
+// PGO-NEXT: Maximum function count:
+// PGO-NEXT: Maximum internal block count:

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

auto *CounterPtr =
                          Constant::getNullValue(CounterTy),
                          getVarName(Inc, getInstrProfCountersVarPrefix()));
   CounterPtr->setVisibility(Visibility);
   CounterPtr->setSection(
       getInstrProfSectionName(IPSK_cnts, TT.getObjectFormat()));
   CounterPtr->setAlignment(Align(8));
   MaybeSetComdat(CounterPtr);
   CounterPtr->setLinkage(Linkage);
+  CounterPtr->setMetadata(LLVMContext::MD_associated,
+                          MDNode::get(Ctx, ValueAsMetadata::get(CounterPtr)));
 
   auto *Int8PtrTy = Type::getInt8PtrTy(Ctx);
   // Allocate statically the array of pointers to value profile nodes for
   // the current function.
   Constant *ValuesPtrExpr = ConstantPointerNull::get(Int8PtrTy);
   if (ValueProfileStaticAlloc && !needsRuntimeRegistrationOfSectionRange(TT)) {
     uint64_t NS = 0;
     for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
       NS += PD.NumValueSites[Kind];
     if (NS) {
       ArrayType *ValuesTy = ArrayType::get(Type::getInt64Ty(Ctx), NS);
 
       auto *ValuesVar =
           new GlobalVariable(*M, ValuesTy, false, Linkage,
                              Constant::getNullValue(ValuesTy),
                              getVarName(Inc, getInstrProfValuesVarPrefix()));
       ValuesVar->setVisibility(Visibility);
       ValuesVar->setSection(
           getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
       ValuesVar->setAlignment(Align(8));
       MaybeSetComdat(ValuesVar);
+      ValuesVar->setMetadata(LLVMContext::MD_associated,
+                             MDNode::get(Ctx, ValueAsMetadata::get(CounterPtr)));
       ValuesPtrExpr =
           ConstantExpr::getBitCast(ValuesVar, Type::getInt8PtrTy(Ctx));
     }
   }
 
   // Create data variable.
   auto *Int16Ty = Type::getInt16Ty(Ctx);
   auto *Int16ArrayTy = ArrayType::get(Int16Ty, IPVK_Last + 1);
#include "llvm/ProfileData/InstrProfData.inc"
   auto *Data = new GlobalVariable(*M, DataTy, false, Linkage,
                                   ConstantStruct::get(DataTy, DataVals),
                                   getVarName(Inc, getInstrProfDataVarPrefix()));
   Data->setVisibility(Visibility);
   Data->setSection(getInstrProfSectionName(IPSK_data, TT.getObjectFormat()));
   Data->setAlignment(Align(INSTR_PROF_DATA_ALIGNMENT));
   MaybeSetComdat(Data);
   Data->setLinkage(Linkage);
+  Data->setMetadata(LLVMContext::MD_associated,
+                    MDNode::get(Ctx, ValueAsMetadata::get(CounterPtr)));
 
   PD.RegionCounters = CounterPtr;
   PD.DataVar = Data;
   ProfileDataMap[NamePtr] = PD;
 
   // Mark the data variable as used so that it isn't stripped out.
   UsedVars.push_back(Data);
   // Now that the linkage set by the FE has been passed to the data and counter

llvm/test/Instrumentation/InstrProfiling/associated.ll

+; RUN: opt < %s -mtriple=x86_64-unknown-linux -instrprof -S | FileCheck %s
+; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=instrprof -S | FileCheck %s
+; RUN: opt < %s -mtriple=x86_64-unknown-linux -pgo-instr-gen -S | FileCheck %s
+; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=pgo-instr-gen -S | FileCheck %s
+
+@__profn_foo = hidden constant [3 x i8] c"foo"
+
+; CHECK: @__profc_foo = hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8, !associated !0
+; CHECK: @__profd_foo = hidden global {{.*}}, section "__llvm_prf_data", align 8, !associated !0
+
+define void @foo() {
+  call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 0, i32 1, i32 0)
+  ret void
+}
+
+declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
+
+; CHECK: !0 = !{[1 x i64]* @__profc_foo}

llvm/test/Transforms/PGOProfile/counter_promo.ll

 ; NONATOMIC_PROMO-NEXT: add {{.*}} %[[PROMO2]], %[[LIVEOUT2]]
 ; NONATOMIC_PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}1)
 ; NONATOMIC_PROMO-NEXT: %[[PROMO3:[a-z0-9.]+]] = load {{.*}} @__profc_foo{{.*}} 2)
 ; NONATOMIC_PROMO-NEXT: add {{.*}} %[[PROMO3]], %[[LIVEOUT3]]
 ; NONATOMIC_PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}2)
 ; ATOMIC_PROMO: atomicrmw add {{.*}} @__profc_foo{{.*}}0), i64 %[[LIVEOUT1]] seq_cst
 ; ATOMIC_PROMO-NEXT: atomicrmw add {{.*}} @__profc_foo{{.*}}1), i64 %[[LIVEOUT2]] seq_cst
 ; ATOMIC_PROMO-NEXT: atomicrmw add {{.*}} @__profc_foo{{.*}}2), i64 %[[LIVEOUT3]] seq_cst
-; PROMO-NOT: @__profc_foo
+; PROMO-NOT: @__profc_foo{{.*}})
 
 
 }
 
 declare void @bar(i32)

llvm/test/Transforms/PGOProfile/counter_promo_mexits.ll

 ; PROMO-NEXT: add 
 ; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}1)
 ; PROMO-NEXT:  %pgocount.promoted{{.*}} = load {{.*}} @__profc_foo{{.*}} 2)
 ; PROMO-NEXT: add 
 ; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}2)
 ; PROMO-NEXT:  %pgocount{{.*}} = load {{.*}} @__profc_foo{{.*}} 4)
 ; PROMO-NEXT: add 
 ; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}4)
-; PROMO-NOT: @__profc_foo
+; PROMO-NOT: @__profc_foo{{.*}})
 
 
 bb15:                                             ; preds = %bb14, %bb4
   tail call void @bar(i32 1)
   ret void
 }
 
 declare void @bar(i32) local_unnamed_addr