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

I'd completely rewrite the description. Consider this:

  // 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 `makeNoise`. The issue is complex:

1. 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.
2. In `BitcodeFile::parse`, after processing the Undefined then the Defined, the final state is Defined.
3. 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 TB_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.



================
Comment at: lld/test/ELF/lto/devirt_extern_vtable.ll:6
+;; Hybrid WPD
+; RUN: opt --thinlto-bc --thinlto-split-lto-unit -o %t1.o %s
+; RUN: opt --thinlto-bc --thinlto-split-lto-unit %S/Inputs/devirt_extern_vtable.ll -o %t2.o
----------------
You can use `split-file` to place auxiliary files in one file. See some newer tests for example.


================
Comment at: lld/test/ELF/lto/devirt_extern_vtable.ll:11
+; RUN: ld.lld %t1.o %t2.o -shared -o %t3 -save-temps --lto-whole-program-visibility \
+; RUN:   -mllvm -pass-remarks=. --version-script %t.ver 2>&1 \
+; RUN:   | FileCheck %s --check-prefix=REMARK
----------------
nit: ` | \` (the previous line instead of the continuation line)


================
Comment at: lld/test/ELF/lto/devirt_extern_vtable.ll:54
+  store %struct.Animal* %a, %struct.Animal** %a.addr, align 8
+  %0 = load %struct.Animal*, %struct.Animal** %a.addr, align 8
+  %1 = bitcast %struct.Animal* %0 to %struct.Cat*
----------------
Can you compile with higher optimization and remove unneeded instructions?


================
Comment at: lld/test/ELF/lto/devirt_extern_vtable.ll:84
+attributes #0 = { noinline optnone uwtable mustprogress "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #2 = { nofree nosync nounwind readnone willreturn }
----------------
Attributes need clean-up.


================
Comment at: lld/test/ELF/lto/devirt_extern_vtable.ll:89
+!llvm.linker.options = !{}
+!llvm.module.flags = !{!4, !5}
+
----------------
Delete unneeded module flags and !llvm.linker.options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98686