[ELF] Suppress "duplicate symbol" when resolving STB_WEAK and STB_GNU_UNIQUE in different COMDATs

[MaskRay]

Summary

template <typename T> struct A {
  A() {}
  int value = 0;
};

template <typename Value> struct B {
  static A<int> a;
};

template <typename Value> A<int> B<Value>::a;

inline int foo() {
  return B<int>::a.value;
}

clang++ -c -fno-pic a.cc -o weak.o
g++ -c -fno-pic a.cc -o unique.o  # --enable-gnu-unique-object

# Duplicate symbol error. In postParse, we do not check `sym.binding`
ld.lld -e 0 weak.o unique.o

Mixing GCC and Clang object files in this case is not ideal. .bss._ZGVN1BIiE1aE
has different COMDAT groups. It appears to work in practice because the guard
variable prevents harm due to double initialization.

For the linker, we just stick with the rule that a weak binding does not cause
"duplicate symbol" errors.

Close https://github.com/llvm/llvm-project/issues/58232

[peter.smith]

Is this symptomatic of a problem in Clang or GCC? It looks like they are not generating the same groups for the same source file which I would expect considering that both claim to implement the itanium ABI. I couldn't understand at first why the STB_GNU_UNIQUE symbols were not getting discarded as part of a non-prevailing group, however the clang and gcc (I used aarch64-none-linux-gnu-g++ (fsf-10.39) 10.1.1 20200629) generated groups have different signatures so we end up with two groups with one STB_GNU_UNIQUE and one STB_WEAK.

I've approved as the groups are different even when -fno-gnu-unique is used for GCC so it makes sense to treat STB_GNU_WEAK and STB_GNU_UNIQUE the same way. However this does feel less than ideal. We end up with sections from both groups in the ELF file but only one set of symbols. Probably the job of the compiler to fix, not sure there is much we can do at link time.

lld/test/ELF/comdat-binding2.s

33

Can we add to the comment as I found it difficult to spot.

1. Note the different group signature for the second group.

[MaskRay]

In D136381#3874126, @peter.smith wrote:

Is this symptomatic of a problem in Clang or GCC? It looks like they are not generating the same groups for the same source file which I would expect considering that both claim to implement the itanium ABI. I couldn't understand at first why the STB_GNU_UNIQUE symbols were not getting discarded as part of a non-prevailing group, however the clang and gcc (I used aarch64-none-linux-gnu-g++ (fsf-10.39) 10.1.1 20200629) generated groups have different signatures so we end up with two groups with one STB_GNU_UNIQUE and one STB_WEAK.

I need to do some research. I suspect GCC's scheme isn't ideal as generally we want to make related symbols in one comdat instead of two.

I've approved as the groups are different even when -fno-gnu-unique is used for GCC so it makes sense to treat STB_GNU_WEAK and STB_GNU_UNIQUE the same way. However this does feel less than ideal. We end up with sections from both groups in the ELF file but only one set of symbols. Probably the job of the compiler to fix, not sure there is much we can do at link time.

The mix is not ideal but appear to work fine in practice because the guard variable prevents harm due to double initialization....
For the linker we just stick with the rule that a weak binding does not cause duplicate symbol error.

[MaskRay]

add comment

lld/ELF/InputFiles.cpp

if (sec == &InputSection::discarded) {
       continue;
     }
 
     if (sym.file == this) {
       cast<Defined>(sym).section = sec;
       continue;
     }
 
-    if (binding == STB_WEAK)
+    if (sym.binding == STB_WEAK || binding == STB_WEAK)
       continue;
     std::lock_guard<std::mutex> lock(mu);
     ctx.duplicates.push_back({&sym, this, sec, eSym.st_value});
   }
 }
 
 // The handling of tentative definitions (COMMON symbols) in archives is murky.
 // A tentative definition will be promoted to a global definition if there are

lld/test/ELF/comdat-binding2.s

+# REQUIRES: x86
+## Test we don't report duplicate definition errors when mixing Clang STB_WEAK
+## and GCC STB_GNU_UNIQUE symbols.
+
+# RUN: rm -rf %t && split-file %s %t && cd %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 weak.s -o weak.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 unique.s -o unique.o
+# RUN: ld.lld weak.o unique.o -o weak
+# RUN: llvm-readelf -s weak | FileCheck %s --check-prefix=WEAK
+# RUN: ld.lld unique.o weak.o -o unique
+# RUN: llvm-readelf -s unique | FileCheck %s --check-prefix=UNIQUE
+
+# WEAK:   OBJECT  WEAK   DEFAULT [[#]] _ZN1BIiE1aE
+# UNIQUE: OBJECT  UNIQUE DEFAULT [[#]] _ZN1BIiE1aE
+
+#--- weak.s
+## Clang
+  .type _ZN1BIiE1aE,@object
+  .section  .bss._ZN1BIiE1aE,"aGwR",@nobits,_ZN1BIiE1aE,comdat
+  .weak _ZN1BIiE1aE
+_ZN1BIiE1aE:
+  .zero 4
+
+  .type _ZGVN1BIiE1aE,@object
+  .section  .bss._ZGVN1BIiE1aE,"aGw",@nobits,_ZN1BIiE1aE,comdat
+  .weak _ZGVN1BIiE1aE
+_ZGVN1BIiE1aE:
+  .quad 0
+
+#--- unique.s
+## GCC -fgnu-unique. Note the different group signature for the second group.
+  .weak _ZN1BIiE1aE
+  .section  .bss._ZN1BIiE1aE,"awG",@nobits,_ZN1BIiE1aE,comdat
+  .type _ZN1BIiE1aE, @gnu_unique_object
+_ZN1BIiE1aE:
+  .zero 4
+
+  .weak _ZGVN1BIiE1aE
+  .section  .bss._ZGVN1BIiE1aE,"awG",@nobits,_ZGVN1BIiE1aE,comdat
+  .type _ZGVN1BIiE1aE, @gnu_unique_object
+_ZGVN1BIiE1aE:
+  .zero 8