[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 subscriber: tejohnson.
MaskRay added a comment.

OK, I think IR symbol table probably cannot be fixed. `isDefined` -> `!isLazy` on lld side looks fine to me.

`llvm::lto::InputFile::symbols()` may return duplicate symbols because

(1) A symbol may appear in multiple modules in a regular LTO bitcode file (`llvm-cat a.bc b.bc`).
(2) Global values and assembly symbols are concatenated (`ModuleSymbolTable::addModule`) without deduplication.
(3) Due to -fsplit-lto-unit -flto={thin,full}, a symbol may appear as an undefined in module 0 and as an defined in module 1 (e.g. a vtable symbol with the `!type` metadata).

`LTO::addRegularLTO` and `lto::InputFile::create` need to be synchronized on the symbol table. The symbols from multiple modules are concatenated.
While I thought deduplicating the symbol table for (2) and (3) would be nice, (1) and (2) and the current symbol table representation make deduplication inconvenient.
In addition, the current symbol resolution communication from various customers (different ports of lld and LLVMgold.so) to lib/LTO can work around duplicate symbol table entries easily (the `r.Prevailing = ` line, its comment is a bit outdated though).
(@tejohnson )

So I think we should just live with duplicate symbols and don't bother fix (2) and (3), at least before something more clumsy is known.
For `-fwhole-program-vtables`, (1) and (2) are special cases. (3) is a practical usage (`-shared` +  `local:` version node in a version-script) and we should optimize it if we can.
And we can indeed optimize it with such a one-line change:)

You need to add the Cat/CatUser test as a new file (named `devirt_vcall*.ll`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98686