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

[Peter Smith via Phabricator via llvm-commits]

peter.smith 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())) {
----------------
Compare is probably a bit generic a name. I think it is only called from resolveDefined(). Perhaps `shouldReplace`.


================
Comment at: lld/ELF/Symbols.cpp:536
+  // the the incoming copy in a non-prevailing COMDAT is selected.
+  return !isGlobal() && other.isGlobal();
 }
----------------
Presumably an existing `STB_GNU_UNIQUE` is also preferred over an incoming `STB_WEAK` because if it exists it must have come from a prevailing COMDAT?

May be worth adding that to the comment?


================
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
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120640