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

[Nathan Lanza via Phabricator via llvm-commits]

lanza created this revision.
Herald added subscribers: steven_wu, hiraditya, arichardson, Prazek, emaste.
Herald added a reviewer: MaskRay.
lanza requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Given the following scenario:

  // Cat.cpp
  struct Animal { virtual void makeNoise() const = 0; };
  struct Cat : Animal { void makeNoise() const override; };
  
  extern "C" int puts(char const *);
  void Cat::makeNoise() const { puts("Meow"); }
  void doThingWithCat(Animal *a) { static_cast<Cat *>(a)->makeNoise(); }
  
  // CatUser.cpp
  struct Animal { virtual void makeNoise() const = 0; };
  struct Cat : Animal { void makeNoise() const override; };
  
  void doThingWithCat(Animal *a);
  
  void useDoThingWithCat() {
    Cat *d = new Cat;
    doThingWithCat(d);
  }
  
  // cat.ver
  {
    global: _Z17useDoThingWithCatv;
    local: *;
  };
  
  $ clang++ Cat.cpp CatUser.cpp -fpic -flto=thin -fwhole-program-vtables
  -shared -O3 -fuse-ld=lld -Wl,--lto-whole-program-visibility
  -Wl,--version-script,cat.ver

We cannot devirtualize `Cat::makeNoise`. The issue is complex:

Due to `-fsplit-lto-unit` and usage of type metadata, we place the Cat
vtable declaration into module 0 and the Cat vtable definition with type
metadata into module 1, causing duplicate entries (Undefined followed by
Defined) in the `lto::InputFile::symbols()` output.
In `BitcodeFile::parse`, after processing the `Undefined` then the
`Defined`, the final state is `Defined`.
In `BitcodeCompiler::add`, for the first symbol, `computeBinding`
returns `STB_LOCAL`, then we reset it to `Undefined` because it is
prevailing (`versionId` is `preserved`). For the second symbol, because
the state is now `Undefined`, `computeBinding` returns `STB_GLOBAL`,
causing `ExportDynamic` to be true and suppressing devirtualization.

In D77280 <https://reviews.llvm.org/D77280>, the `computeBinding` change used a stricter `isDefined()`
condition to make weak``Lazy` symbol work.
This patch relaxes the condition to weaker `!isLazy()` to keep it
working while making the devirtualization work as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98841

Files:
  lld/ELF/Symbols.cpp
  lld/test/ELF/lto/devirt_split_unit_localize.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D98841.331451.patch
Type: text/x-patch
Size: 6337 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210318/76d2a54c/attachment-0001.bin>