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

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

In D76802#2516388 <https://reviews.llvm.org/D76802#2516388>, @MaskRay wrote:

> In D76802#2516296 <https://reviews.llvm.org/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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76802/new/

https://reviews.llvm.org/D76802