[ELF] Make binding (weak or non-weak) logic consistent for Undefined and SharedSymbol

[MaskRay]

Summary

This is a case missed by D64136. If %t1.o has a weak reference on foo,
and %t2.so has a non-weak reference on foo:

0. ld.lld %t1.o %t2.so          # ok; STB_WEAK; accepted since D64136
1. ld.lld %t2.so %t1.o          # undefined symbol: foo; STB_GLOBAL
2. gold %t1.o %t2.so            # ok; STB_WEAK
3. gold %t2.so %t1.o            # undefined reference to 'foo'; STB_GLOBAL
4. ld.bfd %t1.o %t2.so          # undefined reference to `foo'; STB_WEAK
5. ld.bfd %t2.so %t1.o          # undefined reference to `foo'; STB_WEAK

It can be argued that in both cases, the binding of the undefined foo
should be set to STB_WEAK, because the binding should not be affected by
referenced from shared objects.

--allow-shlib-undefined doesn't suppress errors (3,4,5), but -shared or
--noinhibit-exec allows ld.bfd/gold to produce a binary:

3. gold -shared %t2.so %t1.o    # ok; STB_GLOBAL
4. ld.bfd -shared %t2.so %t1.o  # ok; STB_WEAK
5. ld.bfd -shared %t1.o %t1.o   # ok; STB_WEAK

If %t2.so has DT_NEEDED entries, ld.bfd will load them (lld/gold don't
have the behavior). If one of the DSO defines foo and it is in the
link-time search path (e.g. DT_NEEDED entry is an absolute path, via
-rpath=, via -rpath-link=, etc),
ld.bfd %t1.o %t2.so and ld.bfd %t1.o %t2.so will not error.

In this patch, we make Undefined and SharedSymbol share the same binding
computing logic. Case 1 will be allowed:

0. ld.lld %t1.o %t2.so          # ok; STB_WEAK; accepted since D64136
1. ld.lld %t2.so %t1.o          # ok; STB_WEAK; changed by this patch

In the future, we can explore the option that turns both (0,1) into
errors if --no-allow-shlib-undefined (default when linking an
executable) is in action.

[MaskRay]

Mention --allow-shlib-undefined

[MaskRay]

Retitle

[peter.smith]

IIUC the desired state you are proposing is:

ld.lld %t1.o %t2.so          # ok; STB_WEAK
ld.lld %t2.so %t1.o          # ok; STB_WEAK

I agree that ELF is underspecified here and I agree that the shared library shouldn't change the binding to non-weak in the executable (in the case where we allow undefined symbols in shared libraries). Personally I think ld.bfd's behaviour is preferable given the intention behind giving error messages from undefined symbols; i.e. catching likely errors at the earliest possible stage. If we permit it then the unfortunate user will get an undefined symbol error at run time from the dynamic loader instead of the static linker.

[MaskRay]

Forget to initialize referenced

[MaskRay]

I agree that ELF is underspecified here and I agree that the shared library shouldn't change the binding to non-weak in the executable (in the case where we allow undefined symbols in shared libraries). Personally I think ld.bfd's behaviour is preferable given the intention behind giving error messages from undefined symbols; i.e. catching likely errors at the earliest possible stage. If we permit it then the unfortunate user will get an undefined symbol error at run time from the dynamic loader instead of the static linker.

I agree erroring would be preferable. However, ld.bfd recursively loads DT_NEEDED entries to resolve undefined referenced. This behavior is not implemented in gold and lld. We may have to be permissive, as what we did in the implementation of --no-allow-shlib-undefined.

Added an example to the description:

If one of the DSO defines foo and it is in the link-time search path (e.g. DT_NEEDED entry is an absolute path, via -rpath=, via -rpath-link=, etc), ld.bfd %t1.o %t2.so and ld.bfd %t1.o %t2.so will not error.

[MaskRay]

Retitle. Mention that:

In the future, we can explore the option that turns both (0,1) into errors.
--no-allow-shlib-undefined is in action.

[MaskRay]

🤖

[ruiu]

Honestly I don't have a strong preference on the behavior of weak symbols in this case. I have a slight concern that if there is a program that depends on the current behavior, they may break by this change, but this case is too subtle and linker-dependent, so I'm perhaps worried too much.

LGTM

[MaskRay]

In D65584#1616759, @ruiu wrote:

Honestly I don't have a strong preference on the behavior of weak symbols in this case. I have a slight concern that if there is a program that depends on the current behavior, they may break by this change, but this case is too subtle and linker-dependent, so I'm perhaps worried too much.

LGTM

Don't worry! After the binding fix, the check is actually relaxed (weak undef doesn't error in lld while it may in a few scenarios in ld.bfd).

ELF/InputFiles.cpp

if (binding == STB_LOCAL) {
         this->symbols[i] =
             make<Defined>(this, name, binding, stOther, type, value, size, sec);
       continue;
     }
 
     // Handle global undefined symbols.
     if (eSym.st_shndx == SHN_UNDEF) {
       this->symbols[i]->resolve(Undefined{this, name, binding, stOther, type});
+      this->symbols[i]->referenced = true;
       continue;
     }
 
     // Handle global common symbols.
     if (eSym.st_shndx == SHN_COMMON) {
       if (value == 0 || value >= UINT32_MAX)
         fatal(toString(this) + ": common symbol '" + StringRef(name.data) +
               "' has invalid alignment: " + Twine(value));

ELF/Symbols.h

public:
   // --export-dynamic, and by dynamic lists.
   unsigned exportDynamic : 1;
 
   // False if LTO shouldn't inline whatever this symbol points to. If a symbol
   // is overwritten after LTO, LTO shouldn't inline the symbol because it
   // doesn't know the final contents of the symbol.
   unsigned canInline : 1;
 
+  // Used by Undefined and SharedSymbol to track if there has been at least one
+  // undefined reference to the symbol. The binding may change to STB_WEAK if
+  // the first undefined reference from a non-shared object is weak.
+  unsigned referenced : 1;
+
   // True if this symbol is specified by --trace-symbol option.
   unsigned traced : 1;
 
   inline void replace(const Symbol &New);
 
   bool includeInDynsym() const;
   uint8_t computeBinding() const;
   bool isWeak() const { return binding == llvm::ELF::STB_WEAK; }

 
 protected:
   Symbol(Kind k, InputFile *file, StringRefZ name, uint8_t binding,
          uint8_t stOther, uint8_t type)
       : file(file), nameData(name.data), nameSize(name.size), binding(binding),
         type(type), stOther(stOther), symbolKind(k), visibility(stOther & 3),
         isUsedInRegularObj(!file || file->kind() == InputFile::ObjKind),
         exportDynamic(isExportDynamic(k, visibility)), canInline(false),
-        traced(false), needsPltAddr(false), isInIplt(false), gotInIgot(false),
-        isPreemptible(false), used(!config->gcSections), needsTocRestore(false),
-        scriptDefined(false) {}
+        referenced(false), traced(false), needsPltAddr(false), isInIplt(false),
+        gotInIgot(false), isPreemptible(false), used(!config->gcSections),
+        needsTocRestore(false), scriptDefined(false) {}
 
 public:
   // True the symbol should point to its PLT entry.
   // For SharedSymbol only.
   unsigned needsPltAddr : 1;
 
   // True if this symbol is in the Iplt sub-section of the Plt and the Igot
   // sub-section of the .got.plt or .got.
if (this->type == llvm::ELF::STT_GNU_IFUNC)
       this->type = llvm::ELF::STT_FUNC;
   }
 
   SharedFile &getFile() const { return *cast<SharedFile>(file); }
 
   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
void Symbol::replace(const Symbol &New) {
   Symbol old = *this;
   memcpy(this, &New, New.getSymbolSize());
 
   versionId = old.versionId;
   visibility = old.visibility;
   isUsedInRegularObj = old.isUsedInRegularObj;
   exportDynamic = old.exportDynamic;
   canInline = old.canInline;
+  referenced = old.referenced;
   traced = old.traced;
   isPreemptible = old.isPreemptible;
   scriptDefined = old.scriptDefined;
   partition = old.partition;
 
   // Symbol length is computed lazily. If we already know a symbol length,
   // propagate it.
   if (nameData == old.nameData && nameSize == 0 && old.nameSize != 0)

ELF/Symbols.cpp

if (backref && !isWeak())
            toString(other.file) + " refers to " + toString(file));
     return;
   }
 
   // Undefined symbols in a SharedFile do not change the binding.
   if (dyn_cast_or_null<SharedFile>(other.file))
     return;
 
-  if (isUndefined()) {
-    // The binding may "upgrade" from weak to non-weak.
-    if (other.binding != STB_WEAK)
+  if (isUndefined() || isShared()) {
+    // The binding 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 || !referenced)
       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.
-    if (other.binding != STB_WEAK || !s->referenced)
-      binding = other.binding;
-    s->referenced = true;
+    referenced = true;
   }
 }
 
 // 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

 
 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;
+    referenced = true;
   }
 }

test/ELF/weak-undef-shared.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
 
+# RUN: ld.lld %t2.so %t1.o -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: