[ELF] Set the priority of STB_GNU_UNIQUE the same as STB_WEAK

[MaskRay]

Summary

In GCC -fgnu-unique output, STB_GNU_UNIQUE symbols are always defined relative
to a section in a COMDAT group. Currently other cannot be STB_GNU_UNIQUE for
valid input, so this patch is NFC.

If we switch to the model that ignores COMDAT resolution when performing symbol
resolution (D120626), this will fix bogus `relocation refers to a symbol in a
discarded section` errors when mixing -fno-gnu-unique objects with -fgnu-unique
objects.

[peter.smith]

lld/ELF/Symbols.cpp

511

Compare is probably a bit generic a name. I think it is only called from resolveDefined(). Perhaps shouldReplace.

536

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?

lld/test/ELF/comdat-binding.s

13

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?

[MaskRay]

improve comment

[MaskRay]

lld/ELF/Symbols.cpp

511

Change separately

lld/test/ELF/comdat-binding.s

13

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.

[peter.smith]

Thanks I'll try and look at D120626 tomorrow.

lld/ELF/Symbols.h

public:
 
   // True if the name contains '@'.
   uint8_t hasVersionSuffix : 1;
 
   inline void replace(const Symbol &other);
 
   bool includeInDynsym() const;
   uint8_t computeBinding() const;
+  bool isGlobal() const { return binding == llvm::ELF::STB_GLOBAL; }
   bool isWeak() const { return binding == llvm::ELF::STB_WEAK; }
 
   bool isUndefined() const { return symbolKind == UndefinedKind; }
   bool isCommon() const { return symbolKind == CommonKind; }
   bool isDefined() const { return symbolKind == DefinedKind; }
   bool isShared() const { return symbolKind == SharedKind; }
   bool isPlaceholder() const { return symbolKind == PlaceholderKind; }
 

lld/ELF/Symbols.cpp

if (isUndefined() || isShared()) {
     // weak. The binding has one opportunity to change to weak: if the first
     // reference is weak.
     if (other.binding != STB_WEAK || !referenced)
       binding = other.binding;
   }
 }
 
 // Compare two symbols. Return true if the new symbol should win.
 bool Symbol::shouldReplace(const Defined &other) const {
   if (LLVM_UNLIKELY(isCommon())) {
     if (config->warnCommon)
       warn("common " + getName() + " is overridden");
     return !other.isWeak();
   }
   if (!isDefined())
     return true;
 
   // .symver foo,foo@@VER unfortunately creates two defined symbols: foo and
   // foo@@VER. In GNU ld, if foo and foo@@VER are in the same file, foo is
   // ignored. In our implementation, when this is foo, this->getName() may still
   // contain @@, return true in this case as well.
   if (LLVM_UNLIKELY(file == other.file)) {
     if (other.getName().contains("@@"))
       return true;
     if (getName().contains("@@"))
       return false;
   }
 
-  return isWeak() && !other.isWeak();
+  // Incoming STB_GLOBAL overrides STB_WEAK/STB_GNU_UNIQUE. -fgnu-unique changes
+  // some vague linkage data in COMDAT from STB_WEAK to STB_GNU_UNIQUE. Treat
+  // STB_GNU_UNIQUE like STB_WEAK so that we prefer the first among all
+  // STB_WEAK/STB_GNU_UNIQUE copies. If we prefer an incoming STB_GNU_UNIQUE to
+  // an existing STB_WEAK, there may be discarded section errors because the
+  // selected copy may be in a non-prevailing COMDAT.
+  return !isGlobal() && other.isGlobal();
 }
 
 void elf::reportDuplicate(const Symbol &sym, InputFile *newFile,
                           InputSectionBase *errSec, uint64_t errOffset) {
   if (config->allowMultipleDefinition)
     return;
   const Defined *d = cast<Defined>(&sym);
   if (!d->section || !errSec) {

lld/test/ELF/comdat-binding.s

+# REQUIRES: x86
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/g.s -o %t/g.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/w.s -o %t/w.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/u.s -o %t/u.o
+# RUN: ld.lld -e 0 %t/w.o %t/u.o -o %t/w
+# RUN: llvm-readelf -s %t/w | FileCheck %s --check-prefix=WEAK
+# RUN: ld.lld -e 0 %t/u.o %t/w.o -o %t/u
+# RUN: llvm-readelf -s %t/u | FileCheck %s --check-prefix=UNIQUE
+
+# 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
+
+# WEAK:   NOTYPE WEAK   DEFAULT [[#]] _ZZ1fvE1x
+# UNIQUE: OBJECT UNIQUE DEFAULT [[#]] _ZZ1fvE1x
+
+#--- g.s
+movq _ZZ1fvE1x@gotpcrel(%rip), %rax
+
+.section .bss._ZZ1fvE1x,"awG",@nobits,_ZZ1fvE1x,comdat
+.globl _ZZ1fvE1x
+_ZZ1fvE1x:
+
+#--- w.s
+.section .bss._ZZ1fvE1x,"awG",@nobits,_ZZ1fvE1x,comdat
+.weak _ZZ1fvE1x
+_ZZ1fvE1x:
+
+#--- u.s
+.section .bss._ZZ1fvE1x,"awG",@nobits,_ZZ1fvE1x,comdat
+.type _ZZ1fvE1x, @gnu_unique_object
+_ZZ1fvE1x: