[PATCH] D98686: [lld:elf] Weaken the requirement for a computed binding to be STB_LOCAL

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D98686#2630618 <https://reviews.llvm.org/D98686#2630618>, @lanza wrote:

> clang++-13 invocation
>
>   clang++ Cat.cpp CatUser.cpp -o libA.so -shared -std=c++20 -O3 -fuse-ld=lld \
>                         -fwhole-program-vtables -flto=thin \
>                         -Wl,--lto-whole-program-visibility \
>                         -Wl,--version-script,version.exp \
>                         -Wl,--opt-remarks-format,yaml -Wl,--opt-remarks-filename,libA.yaml -fpic
>
> `_ZTV3Cat` occurs 4 times here -- twice for each file due to split LTO. Two instances have `isDefined() == true` and two have `isDefined() == false`. My change instead tests `!isLazy()` which they are both non-lazy.

I see what you mean. With the change devirtualization can happen.

  % fld.lld @response.txt -mllvm -pass-remarks=. --opt-remarks-format yaml --opt-remarks-filename xx.yaml
  <unknown>:0:0: single-impl: devirtualized a call to _ZNK3Cat9makeNoiseEv
  <unknown>:0:0: single-impl: devirtualized a call to _ZNK3Cat9makeNoiseEv
  <unknown>:0:0: _ZNK3Cat9makeNoiseEv inlined into _Z14doThingWithCatP6Animal with (cost=0, threshold=375)
  <unknown>:0:0: _ZNK3Cat9makeNoiseEv inlined into _Z14doThingWithCatP6Animal with (cost=0, threshold=375)
  <unknown>:0:0: _Z14doThingWithCatP6Animal inlined into _Z17useDoThingWithCatv with (cost=0, threshold=375)

The problem is due to duplicate entries in the IR symbol table.

I did notice that llvm-nm/lld can dump duplicate entries (note: `llvm-lto2 dump-symtab` merges duplicate entries).
For example, with module level inline asm, the undefined symbols are placed in the end.
If a referenced symbol is defined in the IR, you may notice two entries.
It would not matter if IR symbol table can make sure defined symbols are always ordered before undefined ones. (That is still brittle.)

In this case, before the undefine symbol precedes the define one. The duplication is more harmful.

  % fllvm-nm a.o
  ---------------- T _Z14doThingWithCatP6Animal
  ---------------- T _ZNK3Cat9makeNoiseEv
                   U _ZNK3Cat9makeNoiseEv
  ---------------- D _ZTI3Cat
                   U _ZTI3Cat
  ---------------- W _ZTI6Animal
  ---------------- D _ZTS3Cat
  ---------------- W _ZTS6Animal
                   U _ZTV3Cat
  ---------------- D _ZTV3Cat
                   U _ZTVN10__cxxabiv117__class_type_infoE
                   U _ZTVN10__cxxabiv120__si_class_type_infoE
                   U puts

So I think this patch is a workaround. I will take a look at the IR symbol table issue.

>> Only canBeVersioned symbols can have a versionId value not equal to VER_NDX_GLOBAL. How does the isDefine) -> !isLazy() change affect behaviors?
>
> I'm guessing that since the symbol is seen and defined during the initial symbol collection from `Cat.cpp.o` that the vtbl is `isDefined()` by the time the parsing of the version-script happens. Thus the version is marked as local but it is marked as `UndefinedKind`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98686