[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

 template <class ELFT> Symbol *SymbolTable::addUndefined(StringRef Name) {
   return addUndefined<ELFT>(Name, STB_GLOBAL, STV_DEFAULT,
                             /*Type*/ 0,
                             /*CanOmitFromDynSym*/ false, /*File*/ nullptr);
 }
 
 static uint8_t getVisibility(uint8_t StOther) { return StOther & 3; }
 
+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;
+  uint8_t Visibility = getVisibility(StOther);
+  std::tie(S, WasInserted) =
+      insert(Name, Type, Visibility, CanOmitFromDynSym, File);
+
+  // An undefined symbol with non default visibility must be satisfied
+  // in the same DSO.
+  if (WasInserted || (isa<SharedSymbol>(S) && Visibility != STV_DEFAULT)) {
+    replaceSymbol<Undefined>(S, File, Name, Binding, StOther, Type);
+    return S;
+  }
+
+  if (S->isShared() || S->isLazy() || (S->isUndefined() && Binding != STB_WEAK))
+    S->Binding = Binding;
+
+  if (!Config->GcSections && Binding != STB_WEAK)
+    if (auto *SS = dyn_cast<SharedSymbol>(S))
+      SS->getFile<ELFT>().IsNeeded = true;
+
+  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;
+    }
+
     // Do extra check for --warn-backrefs.
     //
     // --warn-backrefs is an option to prevent an undefined reference from
     // fetching an archive member written earlier in the command line. It can be
-// used to keep your program compatible with GNU linkers after you switch to
-// lld. I'll explain the feature and why you may find it useful in this
-// comment.
+    // used to keep compatibility with GNU linkers to some degree.
+    // I'll explain the feature and why you may find it useful in this comment.
     //
     // lld's symbol resolution semantics is more relaxed than traditional Unix
     // linkers. For example,
     //
     //   ld.lld foo.a bar.o
     //
-// succeeds even if bar.o contains an undefined symbol that have to be
+    // succeeds even if bar.o contains an undefined symbol that has to be
     // resolved by some object file in foo.a. Traditional Unix linkers don't
     // allow this kind of backward reference, as they visit each file only once
     // from left to right in the command line while resolving all undefined
     // symbols at the moment of visiting.
     //
     // In the above case, since there's no undefined symbol when a linker visits
     // foo.a, no files are pulled out from foo.a, and because the linker forgets
     // about foo.a after visiting, it can't resolve undefined symbols in bar.o
     // that could have been resolved otherwise.
     //
     // That lld accepts more relaxed form means that (besides it'd make more
     // sense) you can accidentally write a command line or a build file that
     // works only with lld, even if you have a plan to distribute it to wider
     // users who may be using GNU linkers. With --warn-backrefs, you can detect
     // a library order that doesn't work with other Unix linkers.
     //
     // The option is also useful to detect cyclic dependencies between static
     // archives. Again, lld accepts
     //
     //   ld.lld foo.a bar.a
     //
     // even if foo.a and bar.a depend on each other. With --warn-backrefs, it is
     // handled as an error.
     //
     // Here is how the option works. We assign a group ID to each file. A file
     // with a smaller group ID can pull out object files from an archive file
     // with an equal or greater group ID. Otherwise, it is a reverse dependency
     // and an error.
     //
     // 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));
-}
-
-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;
-  uint8_t Visibility = getVisibility(StOther);
-  std::tie(S, WasInserted) =
-      insert(Name, Type, Visibility, CanOmitFromDynSym, File);
-
-  // An undefined symbol with non default visibility must be satisfied
-  // in the same DSO.
-  if (WasInserted || (isa<SharedSymbol>(S) && Visibility != STV_DEFAULT)) {
-    replaceSymbol<Undefined>(S, File, Name, Binding, StOther, Type);
-    return S;
-  }
-
-  if (S->isShared() || S->isLazy() || (S->isUndefined() && Binding != STB_WEAK))
-    S->Binding = Binding;
-
-  if (!Config->GcSections && Binding != STB_WEAK)
-    if (auto *SS = dyn_cast<SharedSymbol>(S))
-      SS->getFile<ELFT>().IsNeeded = true;
-
-  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 =
+        Config->WarnBackrefs && File && S->File->GroupId < File->GroupId;
     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