[ELF] Only allow the binding of SharedSymbol to change for the first undef ref

[MaskRay]

Summary

Fixes PR42442

t.o has a STB_GLOBAL undef ref to f
t2.so has a STB_WEAK undef ref to f
t1.so defines f

ld.lld t.o t1.so t2.so currently sets the binding of f to STB_WEAK.
This is not correct because there exists a STB_GLOBAL undef ref from a
regular object. The problem is that resolveUndefined() doesn't check
if the undef ref is seen for the first time:

if (isShared() || isLazy() || (isUndefined() && Other.Binding != STB_WEAK))
  Binding = Other.Binding;

The isShared() condition should be isShared() && !Referenced
where Referenced is set to true after an undef ref is seen.

In practice, when linking a pthread program with glibc:

// a.o
#include <pthread.h>
pthread_mutex_t mu = PTHREAD_MUTEX_INITIALIZER;
int main() { pthread_mutex_unlock(&mu); }

{clang,gcc} -fuse-ld=lld a.o -lpthread # libpthread.so is linked before libgcc_s.so.1

The weak undef pthread_mutex_unlock in libgcc_s.so.1 makes the result
weak, which diverges from GNU linkers where STB_DEFAULT is used:

23: 0000000000000000     0 FUNC    WEAK   DEFAULT  UND pthread_mutex_lock

(Note, if -pthread is used instead, libpthread.so will be linked after
libgcc_s.so.1 . lld sets the binding to the expected STB_GLOBAL)

Similar linking sequences (ld.lld t.o t1.so t2.so) appear to be used by
Go, which cause a build error https://github.com/golang/go/issues/31912.

[MaskRay]

Update test

[MaskRay]

Update description

[MaskRay]

Update description

[peter.smith]

ELF/Symbols.cpp

413 ↗

(On Diff #207201)

Coming at this part of the code with fresh eyes, I think a comment, or reference to a comment elsewhere, explaining why we are setting the binding of an existing shared symbol to weak would be helpful here. I managed to find one in Symbols.h that helped but I had to search for it:

// Symbol binding. This is not overwritten by replace() to track
// changes during resolution. In particular:
//  - An undefined weak is still weak when it resolves to a shared library.
//  - An undefined weak will not fetch archive members, but we have to
//    remember it is weak.

One other thing that might be worth mentioning is why lazy symbols don't need the same treatment. As I understand it a non-weak reference would immediately fetch the member to define the symbol so there isn't a case where the undefined weak reference changes the binding.

ELF/Symbols.h

368 ↗

(On Diff #207201)

I'd have thought it was false if no undefined reference has been seen yet?

370 ↗

(On Diff #207201)

I think BindingFinalized implies that it never changes but it can if we see a weak refrence first and then see a non-weak reference. As I understand the implementation, this will be set to true on first reference of the shared symbol. Some possible alternatives:

// This is true if no undefined reference to the symbol has been seen. We use this 
bool Unreferenced = true;

Note: this would need the logic reversed in the rest of the code.

if (Other.Binding != STB_WEAK || S->Unreferenced)
      Binding = Other.Binding;
S->Unreferenced = false;

Alternative retaining the existing logic.

// This is true if there has been at least one undefined reference to the symbol 
bool Referenced = false;

[MaskRay]

Rename BindingFinalized to Referenced.
Add comments

[MaskRay]

ELF/Symbols.cpp

413 ↗

(On Diff #207201)

Just refactored the code a bit to dispatch on symbol kinds:

if (undefined) {
} else if (SharedSymbol) {
} else if (lazy) {
}

I hope the logic will become clearer.

ELF/Symbols.h

370 ↗

(On Diff #207201)

Thanks for the suggestions! I'll go with Referenced (preferring positive forms to avoid double negative).

[MaskRay]

Simplify

[peter.smith]

Thanks for the update, looks easier to understand to now.

[grimar]

LGTM.

ELF/Symbols.h

370 ↗

(On Diff #207201)

FWIW

This is false if no undefined reference has been seen yet

Peter's version which avoided using negation looks a bit simpler to me:
"This is true if there has been at least one undefined reference to the symbol"

[ruiu]

ELF/Symbols.h

370 ↗

(On Diff #207359)

Does this increase the size of the symbol?

[MaskRay]

Reorder SharedSymbols fields to make it obvious there is no size increase
Reword the comment of Referenced.

[MaskRay]

ELF/Symbols.h

370 ↗

(On Diff #207359)

sizeof(Defined) == 80
sizeof(SharedSymbol) == 80. Alignment occupied bytes [52,56). This was not obvious, so I reordered the fields to make it obvious.

sizeof(SharedSymbol) is still 80 after reordering.

[ruiu]

LGTM

[ruiu]

Maybe it's good to have a static_assert to ensure that the symbol size is 80 bytes.

[MaskRay]

In D63974#1566354, @ruiu wrote:

Maybe it's good to have a static_assert to ensure that the symbol size is 80 bytes.

The static_assert change may be a bit risky (32-bit 64-bit ELF, Windows, etc). Let me do it in another change.

lld/trunk/ELF/Symbols.h

 
 class SharedSymbol : public Symbol {
 public:
   static bool classof(const Symbol *S) { return S->kind() == SharedKind; }
 
   SharedSymbol(InputFile &File, StringRef Name, uint8_t Binding,
                uint8_t StOther, uint8_t Type, uint64_t Value, uint64_t Size,
                uint32_t Alignment, uint32_t VerdefIndex)
-      : Symbol(SharedKind, &File, Name, Binding, StOther, Type),
-        Alignment(Alignment), Value(Value), Size(Size) {
+      : Symbol(SharedKind, &File, Name, Binding, StOther, Type), Value(Value),
+        Size(Size), Alignment(Alignment) {
     this->VerdefIndex = VerdefIndex;
     // GNU ifunc is a mechanism to allow user-supplied functions to
     // resolve PLT slot values at load-time. This is contrary to the
     // regular symbol resolution scheme in which symbols are resolved just
     // by name. Using this hook, you can program how symbols are solved
     // for you program. For example, you can make "memcpy" to be resolved
     // to a SSE-enabled version of memcpy only when a machine running the
     // program supports the SSE instruction set.
     //
     // Naturally, such symbols should always be called through their PLT
     // slots. What GNU ifunc symbols point to are resolver functions, and
     // calling them directly doesn't make sense (unless you are writing a
     // loader).
     //
     // For DSO symbols, we always call them through PLT slots anyway.
     // So there's no difference between GNU ifunc and regular function
     // symbols if they are in DSOs. So we can handle GNU_IFUNC as FUNC.
     if (this->Type == llvm::ELF::STT_GNU_IFUNC)
       this->Type = llvm::ELF::STT_FUNC;
   }
 
   SharedFile &getFile() const { return *cast<SharedFile>(File); }
 
-  uint32_t Alignment;
-
   uint64_t Value; // st_value
   uint64_t Size;  // st_size
+  uint32_t Alignment;
+
+  // This is true if there has been at least one undefined reference to the
+  // symbol. The binding may change to STB_WEAK if the first undefined reference
+  // is weak.
+  bool Referenced = false;
 };
 
 // LazyArchive and LazyObject represent a symbols that is not yet in the link,
 // but we know where to find it if needed. If the resolver finds both Undefined
 // and Lazy for the same name, it will ask the Lazy to load a file.
 //
 // A special complication is the handling of weak undefined symbols. They should
 // not load a file, but we have to remember we have seen both the weak undefined

lld/trunk/ELF/Symbols.cpp

if ((isShared() && Other.Visibility != STV_DEFAULT) ||
       (isUndefined() && Other.Binding != STB_WEAK && Other.DiscardedSecIdx)) {
     replace(Other);
     return;
   }
 
   if (Traced)
     printTraceSymbol(&Other);
 
-  if (isShared() || isLazy() || (isUndefined() && Other.Binding != STB_WEAK))
+  if (isUndefined()) {
+    // The binding may "upgrade" from weak to non-weak.
+    if (Other.Binding != STB_WEAK)
       Binding = Other.Binding;
-
-  if (isLazy()) {
+  } 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.
+    if (Other.Binding != STB_WEAK || !S->Referenced)
+      Binding = Other.Binding;
+    S->Referenced = true;
+  } else if (isLazy()) {
     // An undefined weak will not fetch archive members. See comment on Lazy in
     // Symbols.h for the details.
     if (Other.Binding == STB_WEAK) {
+      Binding = STB_WEAK;
       Type = Other.Type;
       return;
     }
 
     // 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

 
 void Symbol::resolveShared(const SharedSymbol &Other) {
   if (Visibility == STV_DEFAULT && (isUndefined() || isLazy())) {
     // An undefined symbol with non default visibility must be satisfied
     // in the same DSO.
     uint8_t Bind = Binding;
     replace(Other);
     Binding = Bind;
+    cast<SharedSymbol>(this)->Referenced = true;
   }
 }

lld/trunk/test/ELF/weak-undef-shared.s

-// REQUIRES: x86
-// RUN: llvm-mc %s -o %t.o -filetype=obj -triple=x86_64-pc-linux
-// RUN: llvm-mc %p/Inputs/shared.s -o %t2.o -filetype=obj -triple=x86_64-pc-linux
-// RUN: ld.lld %t2.o -o %t2.so -shared
-// RUN: ld.lld %t.o %t2.so -o %t.exe
-// RUN: llvm-readobj --symbols %t.exe | FileCheck %s
-
-// CHECK:      Name: bar
-// CHECK-NEXT: Value: 0x201020
-// CHECK-NEXT: Size: 0
-// CHECK-NEXT: Binding: Weak
-// CHECK-NEXT: Type: Function
-// CHECK-NEXT: Other: 0
-// CHECK-NEXT: Section: Undefined
-
-.global _start
-_start:
-        .weak bar
-        .quad bar
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: ld.lld -shared %t.o -o %t.so
+
+# RUN: echo '.data; .weak foo; .quad foo' | llvm-mc -filetype=obj -triple=x86_64 - -o %t1.o
+# RUN: echo '.data; .quad foo' | llvm-mc -filetype=obj -triple=x86_64 - -o %t2.o
+
+## If the first undefined reference is weak, the binding changes to
+## STB_WEAK.
+# RUN: ld.lld %t1.o %t.so -o %t
+# RUN: llvm-readelf --dyn-syms %t | FileCheck --check-prefix=WEAK %s
+# RUN: ld.lld %t.so %t1.o -o %t
+# RUN: llvm-readelf --dyn-syms %t | FileCheck --check-prefix=WEAK %s
+
+## The binding remains STB_WEAK if there is no STB_GLOBAL undefined reference.
+# RUN: ld.lld %t1.o %t.so %t1.o -o %t
+# RUN: llvm-readelf --dyn-syms %t | FileCheck --check-prefix=WEAK %s
+# RUN: ld.lld %t.so %t1.o %t1.o -o %t
+# 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
+
+# WEAK:   NOTYPE WEAK   DEFAULT UND foo
+# GLOBAL: NOTYPE GLOBAL DEFAULT UND foo
+
+.globl foo
+foo:

lld/trunk/test/ELF/weak-undef-shared2.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo '.globl f; f:' | llvm-mc -filetype=obj -triple=x86_64 - -o %t1.o
+# RUN: echo '.weak f; .data; .quad f' | llvm-mc -filetype=obj -triple=x86_64 - -o %t2.o
+# RUN: ld.lld -shared %t1.o -o %t1.so
+# RUN: ld.lld -shared %t2.o -o %t2.so
+
+## The undefined reference is STB_GLOBAL in %t.o while STB_WEAK in %t2.so.
+## Check the binding of the result is STB_GLOBAL.
+
+# RUN: ld.lld %t.o %t1.so %t2.so -o %t
+# RUN: llvm-readelf --dyn-syms %t | FileCheck %s
+# RUN: ld.lld %t1.so %t.o %t2.so -o %t
+# RUN: llvm-readelf --dyn-syms %t | FileCheck %s
+# RUN: ld.lld %t1.so %t2.so %t.o -o %t
+# RUN: llvm-readelf --dyn-syms %t | FileCheck %s
+
+# CHECK: NOTYPE GLOBAL DEFAULT UND f
+
+.data
+.quad f