[ELF] --warn-backref: don't report backref to weak symbols.

[MaskRay]

Summary

Suppose we visit symbols in this order:

1. weak definition of foo in a lazy object
2. reference of foo

3 (optional). definition of foo

bfd/gold allows 123 but not 12.

Current --warn-backrefs implementation will report both cases as a backward reference. With this change, both 123 (intended) and 12 (unintended) are allowed. The usage of weak definitions usually imply there are also global definitions, so the trade-off is justified.

[MaskRay]

Add -o %t.exe

[MaskRay]

A real world example:

https://git.gnome.org/browse/libxml2/tree/threads.c defines weak symbols #pragma weak pthread_self that are intended to be overriden later by (libpthread.a)

[ruiu]

I do understand the intent of this change, but it feels like it can be simplified even more. You don't need to keep the existing function, feel free to edit everything to make code clean.

[MaskRay]

Delete checkBackrefs.

[ruiu]

LGTM

ELF/SymbolTable.cpp

 // A file outside --{start,end}-group gets a fresh ID when instantiated. All
 // files within the same --{start,end}-group get the same group ID. E.g.
 //
 //   ld.lld A B --start-group C D --end-group E
 //
 // A forms group 0. B form group 1. C and D (including their member object
 // files) form group 2. E forms group 3. I think that you can see how this
 // group assignment rule simulates the traditional linker's semantics.
-static void checkBackrefs(StringRef Name, InputFile *Old, InputFile *New) {
-  if (Config->WarnBackrefs && New && Old->GroupId < New->GroupId)
-    warn("backward reference detected: " + Name + " in " + toString(New) +
-         " refers to " + toString(Old));
+static bool checkBackrefs(StringRef Name, InputFile *Old, InputFile *New) {
+  return Config->WarnBackrefs && New && Old->GroupId < New->GroupId;
 }
 
 template <class ELFT>
 Symbol *SymbolTable::addUndefined(StringRef Name, uint8_t Binding,
                                   uint8_t StOther, uint8_t Type,
                                   bool CanOmitFromDynSym, InputFile *File) {
   Symbol *S;
   bool WasInserted;
Symbol *SymbolTable::addUndefined(StringRef Name, uint8_t Binding,
   if (S->isLazy()) {
     // An undefined weak will not fetch archive members. See comment on Lazy in
     // Symbols.h for the details.
     if (Binding == STB_WEAK) {
       S->Type = Type;
       return S;
     }
 
-    checkBackrefs(Name, S->File, File);
+    bool Backref = checkBackrefs(Name, S->File, File);
     fetchLazy<ELFT>(S);
+    // We don't report backward references to weak symbols as they can be
+    // overriden later.
+    if (Backref && S->Binding != STB_WEAK)
+      warn("backward reference detected: " + Name + " in " + toString(File) +
+           " refers to " + toString(S->File));
   }
   return S;
 }
 
 // Using .symver foo,foo@@VER unfortunately creates two symbols: foo and
 // foo@@VER. We want to effectively ignore foo, so give precedence to
 // foo@@VER.
 // FIXME: If users can transition to using

test/ELF/warn-backrefs.s

 
 # RUN: not ld.lld --fatal-warnings --end-group 2>&1 | FileCheck -check-prefix=END %s
 # END: stray --end-group
 
 # RUN: echo ".globl bar; bar:" | llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %t3.o
 # RUN: echo ".globl foo; foo: call bar" | llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %t4.o
 # RUN: ld.lld --fatal-warnings --warn-backrefs %t1.o --start-lib %t3.o %t4.o --end-lib -o %t.exe
 
+# We don't report backward references to weak symbols as they can be overriden later.
+# RUN: echo ".weak foo; foo:" | llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %t5.o
+# RUN: ld.lld --fatal-warnings --warn-backrefs --start-lib %t5.o --end-lib %t1.o %t2.o -o %t.exe
+
 .globl _start, foo
 _start:
   call foo