[PATCH] D120640: [ELF] Set the priority of STB_GNU_UNIQUE the same as STB_WEAK

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Symbols.cpp:511
 // Compare two symbols. Return true if the new symbol should win.
 bool Symbol::compare(const Defined &other) const {
   if (LLVM_UNLIKELY(isCommon())) {
----------------
peter.smith wrote:
> Compare is probably a bit generic a name. I think it is only called from resolveDefined(). Perhaps `shouldReplace`.
Change separately


================
Comment at: lld/test/ELF/comdat-binding.s:12
+# RUN: ld.lld -e 0 %t/w.o %t/g.o -o %t/w
+# RUN: llvm-readelf -s %t/w | FileCheck %s --check-prefix=WEAK
+
----------------
peter.smith wrote:
> If I've understood this run, we've got a prevailing group from w.o so we'd expect to see the STB_WEAK symbol and not the STB_GLOBAL. Although looking at reason the code was added (to stop a STB_GNU_UNIQUE from a non prevailing group replacing a STB_WEAK), I'm wondering why as `return !isGlobal() && other.isGlobal();` would imply that it should. Perhaps I need to understand https://reviews.llvm.org/D120626 first?
> If I've understood this run, we've got a prevailing group from w.o so we'd expect to see the STB_WEAK symbol and not the STB_GLOBAL.

Yes.

This will be changed after D120626 when we ignore COMDAT resolution when deciding whether an incoming Defined replaces an existing Defined. The existing-weak-incoming-global case is the new code cannot handle but fortunately this doesn't happen in the wild.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120640