[ELF] resolveUndefined: ignore undefined symbols in SharedFile for Undefined and SharedSymbol

[MaskRay]

Summary

If %t1.o has a weak reference on foo, and %t2.so has a non-weak
reference on foo: ld.lld %t1.o %t2.so -o %t

We incorrectly set the binding of the undefined foo to STB_GLOBAL.
Fix this by ignoring undefined symbols in a SharedFile for Undefined and
SharedSymbol.

This fixes the binding of pthread_once when the program links against
both librt.so and libpthread.so

a.o: STB_WEAK reference to pthread_once
librt.so: STB_GLOBAL reference to pthread_once    # should be ignored
libstdc++.so: STB_WEAK reference to pthread_once  # should be ignored
libgcc_s.so.1: STB_WEAK reference to pthread_once # should be ignored

The STB_GLOBAL pthread_once issue (not fixed by D63974) can cause a link error when the result
DSO is used to link another DSO with -z defs if -lpthread is not specified. (libstdc++.so.6 not having a dependency on libpthread.so is a really nasty hack...)

We happened to create a weak undef before D63974 because libgcc_s.so.1
was linked the last and it changed the binding again to weak.

[MaskRay]

Reword

[peter.smith]

ELF/Symbols.cpp

413

Wouldn't this prevent a non-weak reference from a shared library from fetching an archive member? Or am I missing something else that permits that. Apologies I've not got much time left today.

[MaskRay]

Check an undef ref from a SharedFile can fetch an archive.

This was not tested before.

[MaskRay]

ELF/Symbols.cpp

413

No worries!

Thank you for noticing this. It turns out we do not have a test to check a shared reference can fetch an archive. I thus added the test to test/ELF/archive-fetch.s

Moved the logic below the if (isLazy()) {...} branch.

[MaskRay]

Simpler example

[MaskRay]

Simplify description

[grimar]

LG, I have no other comments except a minor suggestion below.

ELF/Symbols.cpp

496

Perhaps an early return would look a bit better here:

if (isLazy()) {
...
return;
}

// Undefined symbols in a SharedFile do not change the binding.
if (isa_and_nonnull<SharedFile>(Other.File))
  return;

...

[peter.smith]

Thanks for the update and the extra test. Functionally I think this looks good.

[MaskRay]

use early return

[ruiu]

LGTM

ELF/Symbols.cpp

500

Little off-topic, but I'd probably just use dyn_cast instead of this new predicate so that code readers don't have to memorize one more predicate.

[MaskRay]

ELF/Symbols.cpp

500

Do you mean dyn_cast_or_null?

dyn_cast cannot be used because it would assert on a nullptr.

[ruiu]

ELF/Symbols.cpp

500

Yes, sorry, I meant dyn_cast_or_null. If you prefer isa_and_nonnull, I'm fine, though.

ELF/Symbols.cpp

if ((isShared() && Other.Visibility != STV_DEFAULT) ||
       (isUndefined() && Other.Binding != STB_WEAK && Other.DiscardedSecIdx)) {
     replace(Other);
     return;
   }
 
   if (Traced)
     printTraceSymbol(&Other);
 
+  // Ignore undefined symbols in a SharedFile.
+  if (isa_and_nonnull<SharedFile>(Other.File))
+    return;
+
   if (isUndefined()) {
     // The binding may "upgrade" from weak to non-weak.
     if (Other.Binding != STB_WEAK)
       Binding = Other.Binding;
   } else if (auto *S = dyn_cast<SharedSymbol>(this)) {
     // The binding of a SharedSymbol will be weak if there is at least one
     // reference and all are weak. The binding has one opportunity to change to
     // weak: if the first reference is weak.
bool Backref = Config->WarnBackrefs && Other.File &&
                    File->GroupId < Other.File->GroupId;
     fetch();
 
     // We don't report backward references to weak symbols as they can be
     // overridden later.
     if (Backref && !isWeak())
       warn("backward reference detected: " + Other.getName() + " in " +
            toString(Other.File) + " refers to " + toString(File));
   }
 }
 
 // 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
 // .symver foo,foo@@@VER
 // we can delete this hack.
 static int compareVersion(StringRef A, StringRef B) {
   bool X = A.contains("@@");
   bool Y = B.contains("@@");
   if (!X && Y)

test/ELF/weak-undef-shared.s

 # RUN: llvm-readelf --dyn-syms %t | FileCheck --check-prefix=WEAK %s
 
 ## The binding changes back to STB_GLOBAL if there is a STB_GLOBAL undefined reference.
 # RUN: ld.lld %t1.o %t.so %t2.o -o %t
 # RUN: llvm-readelf --dyn-syms %t | FileCheck --check-prefix=GLOBAL %s
 # RUN: ld.lld %t2.o %t.so %t1.o -o %t
 # RUN: llvm-readelf --dyn-syms %t | FileCheck --check-prefix=GLOBAL %s
 
+## Check the binding (weak) is not affected by the STB_GLOBAL undefined
+## reference in %t2.so
+# RUN: ld.lld %t1.o %t2.so -o %t
+# RUN: llvm-readelf --dyn-syms %t | FileCheck --check-prefix=WEAK %s
+
 # WEAK:   NOTYPE WEAK   DEFAULT UND foo
 # GLOBAL: NOTYPE GLOBAL DEFAULT UND foo
 
 .globl foo
 foo: