[lld-macho] Emit binding opcodes for defined symbols that override weak dysyms

[int3]

Summary

These opcodes tell dyld to coalesce the overridden weak dysyms to this
particular symbol definition.

[int3]

rebase

[smeenai]

lld/MachO/SymbolTable.cpp

85

Doesn't this need to check isWeakDef as well?

lld/test/MachO/weak-header-flags.s

23

Similar comment as the last one, although this time it's not hypothetical ... this is only checking that there's no MH_BINDS_TO_WEAK after MH_WEAK_DEFINES, and won't catch an MH_BINDS_TO_WEAK that appears before the MH_WEAK_DEFINES.

[int3]

fix

lld/MachO/SymbolTable.cpp

85

oops. added a test...

[smeenai]

LGTM

lld/MachO/SymbolTable.cpp

std::pair<Symbol *, bool> SymbolTable::insert(StringRef name) {
   symVector.push_back(sym);
   return {sym, true};
 }
 
 Symbol *SymbolTable::addDefined(StringRef name, InputSection *isec,
                                 uint32_t value, bool isWeakDef) {
   Symbol *s;
   bool wasInserted;
+  bool overridesWeakDef = false;
   std::tie(s, wasInserted) = insert(name);
 
   if (!wasInserted) {
     if (auto *defined = dyn_cast<Defined>(s)) {
       if (isWeakDef)
         return s;
       if (!defined->isWeakDef())
         error("duplicate symbol: " + name);
+    } else if (auto *dysym = dyn_cast<DylibSymbol>(s)) {
+      overridesWeakDef = !isWeakDef && dysym->isWeakDef();
     }
     // Defined symbols take priority over other types of symbols, so in case
     // of a name conflict, we fall through to the replaceSymbol() call below.
   }
 
-  replaceSymbol<Defined>(s, name, isec, value, isWeakDef, /*isExternal=*/true);
+  Defined *defined = replaceSymbol<Defined>(s, name, isec, value, isWeakDef,
+                                            /*isExternal=*/true);
+  defined->overridesWeakDef = overridesWeakDef;
   return s;
 }
 
 Symbol *SymbolTable::addUndefined(StringRef name) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(name);
 
   if (wasInserted)
     replaceSymbol<Undefined>(s, name);
   else if (LazySymbol *lazy = dyn_cast<LazySymbol>(s))
     lazy->fetchArchiveMember();
   return s;
 }
 
 Symbol *SymbolTable::addDylib(StringRef name, DylibFile *file, bool isWeakDef,
                               bool isTlv) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(name);
 
+  if (!wasInserted)
+    if (auto *defined = dyn_cast<Defined>(s))
+      if (!defined->isWeakDef())
+        defined->overridesWeakDef = true;
+
   if (wasInserted || isa<Undefined>(s) ||
       (isa<DylibSymbol>(s) && !isWeakDef && s->isWeakDef()))
     replaceSymbol<DylibSymbol>(s, file, name, isWeakDef, isTlv);
 
   return s;
 }
 
 Symbol *SymbolTable::addLazy(StringRef name, ArchiveFile *file,

lld/MachO/Symbols.h

protected:
   Kind symbolKind;
   StringRefZ name;
 };
 
 class Defined : public Symbol {
 public:
   Defined(StringRefZ name, InputSection *isec, uint32_t value, bool isWeakDef,
           bool isExternal)
-      : Symbol(DefinedKind, name), isec(isec), value(value), weakDef(isWeakDef),
-        external(isExternal) {}
+      : Symbol(DefinedKind, name), isec(isec), value(value),
+        overridesWeakDef(false), weakDef(isWeakDef), external(isExternal) {}
 
   bool isWeakDef() const override { return weakDef; }
 
   bool isTlv() const override { return isThreadLocalVariables(isec->flags); }
 
   bool isExternal() const { return external; }
 
   static bool classof(const Symbol *s) { return s->kind() == DefinedKind; }
 
   uint64_t getVA() const override { return isec->getVA() + value; }
 
   uint64_t getFileOffset() const override {
     return isec->getFileOffset() + value;
   }
 
   InputSection *isec;
   uint32_t value;
 
+  bool overridesWeakDef : 1;
+
 private:
-  const bool weakDef;
-  const bool external;
+  const bool weakDef : 1;
+  const bool external : 1;
 };
 
 class Undefined : public Symbol {
 public:
   Undefined(StringRefZ name) : Symbol(UndefinedKind, name) {}
 
   static bool classof(const Symbol *s) { return s->kind() == UndefinedKind; }
 };

 union SymbolUnion {
   alignas(Defined) char a[sizeof(Defined)];
   alignas(Undefined) char b[sizeof(Undefined)];
   alignas(DylibSymbol) char c[sizeof(DylibSymbol)];
   alignas(LazySymbol) char d[sizeof(LazySymbol)];
 };
 
 template <typename T, typename... ArgT>
-void replaceSymbol(Symbol *s, ArgT &&... arg) {
+T *replaceSymbol(Symbol *s, ArgT &&... arg) {
   static_assert(sizeof(T) <= sizeof(SymbolUnion), "SymbolUnion too small");
   static_assert(alignof(T) <= alignof(SymbolUnion),
                 "SymbolUnion not aligned enough");
   assert(static_cast<Symbol *>(static_cast<T *>(nullptr)) == nullptr &&
          "Not a Symbol");
 
-  new (s) T(std::forward<ArgT>(arg)...);
+  return new (s) T(std::forward<ArgT>(arg)...);
 }
 
 } // namespace macho
 
 std::string toString(const macho::Symbol &);
 } // namespace lld
 
 #endif

lld/MachO/SyntheticSections.h

 constexpr const char export_[] = "__export";
 constexpr const char symbolTable[] = "__symbol_table";
 constexpr const char stringTable[] = "__string_table";
 constexpr const char got[] = "__got";
 constexpr const char threadPtrs[] = "__thread_ptrs";
 
 } // namespace section_names
 
+class Defined;
 class DylibSymbol;
 class LoadCommand;
 
 class SyntheticSection : public OutputSection {
 public:
   SyntheticSection(const char *segname, const char *name);
   virtual ~SyntheticSection() = default;
 

 
 struct WeakBindingEntry {
   const Symbol *symbol;
   BindingTarget target;
   WeakBindingEntry(const Symbol *symbol, BindingTarget target)
       : symbol(symbol), target(std::move(target)) {}
 };
 
-// Stores bind opcodes for telling dyld which weak symbols to load. Note that
-// the bind opcodes will only refer to these symbols by name, but will not
-// specify which dylib to load them from.
+// Stores bind opcodes for telling dyld which weak symbols need coalescing.
+// There are two types of entries in this section:
+//
+//   1) Non-weak definitions: This is a symbol definition that weak symbols in
+//   other dylibs should coalesce to.
+//
+//   2) Weak bindings: These tell dyld that a given symbol reference should
+//   coalesce to a non-weak definition if one is found. Note that unlike in the
+//   entries in the BindingSection, the bindings here only refer to these
+//   symbols by name, but do not specify which dylib to load them from.
 class WeakBindingSection : public LinkEditSection {
 public:
   WeakBindingSection();
   void finalizeContents();
   uint64_t getRawSize() const override { return contents.size(); }
   // Like other sections in __LINKEDIT, the binding section is special: its
   // offsets are recorded in the LC_DYLD_INFO_ONLY load command, instead of in
   // section headers.
   bool isHidden() const override { return true; }
-  bool isNeeded() const override { return !bindings.empty(); }
+  bool isNeeded() const override {
+    return !bindings.empty() || !definitions.empty();
+  }
 
   void writeTo(uint8_t *buf) const override;
 
   void addEntry(const Symbol *symbol, SectionPointerUnion section,
                 uint64_t offset, int64_t addend = 0) {
     bindings.emplace_back(symbol, BindingTarget(section, offset, addend));
   }
 
+  bool hasEntry() const { return !bindings.empty(); }
+
+  void addNonWeakDefinition(const Defined *defined) {
+    definitions.emplace_back(defined);
+  }
+
+  bool hasNonWeakDefinition() const { return !definitions.empty(); }
+
 private:
   std::vector<WeakBindingEntry> bindings;
+  std::vector<const Defined *> definitions;
   SmallVector<char, 128> contents;
 };
 
 // Add bindings for symbols that need weak or non-lazy bindings.
 void addNonLazyBindingEntries(const Symbol *, SectionPointerUnion,
                               uint64_t offset, int64_t addend = 0);
 
 // The following sections implement lazy symbol binding -- very similar to the

lld/MachO/SyntheticSections.cpp

void MachHeaderSection::writeTo(uint8_t *buf) const {
   hdr->filetype = config->outputType;
   hdr->ncmds = loadCommands.size();
   hdr->sizeofcmds = sizeOfCmds;
   hdr->flags = MachO::MH_NOUNDEFS | MachO::MH_DYLDLINK | MachO::MH_TWOLEVEL;
 
   if (config->outputType == MachO::MH_DYLIB && !config->hasReexports)
     hdr->flags |= MachO::MH_NO_REEXPORTED_DYLIBS;
 
-  // TODO: ld64 also sets this flag if we have defined a non-weak symbol that
-  // overrides a weak symbol from an imported dylib.
-  if (in.exports->hasWeakSymbol)
+  if (in.exports->hasWeakSymbol || in.weakBinding->hasNonWeakDefinition())
     hdr->flags |= MachO::MH_WEAK_DEFINES;
 
-  if (in.exports->hasWeakSymbol || in.weakBinding->isNeeded())
+  if (in.exports->hasWeakSymbol || in.weakBinding->hasEntry())
     hdr->flags |= MachO::MH_BINDS_TO_WEAK;
 
   for (OutputSegment *seg : outputSegments) {
     for (OutputSection *osec : seg->getSections()) {
       if (isThreadLocalVariables(osec->flags)) {
         hdr->flags |= MachO::MH_HAS_TLV_DESCRIPTORS;
         break;
       }
if (lastBinding.ordinal != dysym->file->ordinal) {
     } else {
       os << static_cast<uint8_t>(BIND_OPCODE_SET_DYLIB_ORDINAL_ULEB);
       encodeULEB128(dysym->file->ordinal, os);
     }
     lastBinding.ordinal = dysym->file->ordinal;
   }
 }
 
+static void encodeWeakOverride(const Defined *defined,
+                               raw_svector_ostream &os) {
+  using namespace llvm::MachO;
+  os << static_cast<uint8_t>(BIND_OPCODE_SET_SYMBOL_TRAILING_FLAGS_IMM |
+                             BIND_SYMBOL_FLAGS_NON_WEAK_DEFINITION)
+     << defined->getName() << '\0';
+}
+
 uint64_t BindingTarget::getVA() const {
   if (auto *isec = section.dyn_cast<const InputSection *>())
     return isec->getVA() + offset;
   return section.get<const OutputSection *>()->addr + offset;
 }
 
 // Emit bind opcodes, which are a stream of byte-sized opcodes that dyld
 // interprets to update a record with the following fields:

 
 WeakBindingSection::WeakBindingSection()
     : LinkEditSection(segment_names::linkEdit, section_names::weakBinding) {}
 
 void WeakBindingSection::finalizeContents() {
   raw_svector_ostream os{contents};
   Binding lastBinding;
 
+  for (const Defined *defined : definitions)
+    encodeWeakOverride(defined, os);
+
   // Since bindings are delta-encoded, sorting them allows for a more compact
   // result.
   llvm::sort(bindings,
              [](const WeakBindingEntry &a, const WeakBindingEntry &b) {
                return a.target.getVA() < b.target.getVA();
              });
   for (const WeakBindingEntry &b : bindings) {
     if (auto *isec = b.target.section.dyn_cast<const InputSection *>()) {
       encodeBinding(b.symbol, isec->parent, isec->outSecOff + b.target.offset,
                     b.target.addend, lastBinding, os);
     } else {
       auto *osec = b.target.section.get<const OutputSection *>();
       encodeBinding(b.symbol, osec, b.target.offset, b.target.addend,
                     lastBinding, os);
     }
   }
-  if (!bindings.empty())
+  if (!bindings.empty() || !definitions.empty())
     os << static_cast<uint8_t>(MachO::BIND_OPCODE_DONE);
 }
 
 void WeakBindingSection::writeTo(uint8_t *buf) const {
   memcpy(buf, contents.data(), contents.size());
 }
 
 void macho::addNonLazyBindingEntries(const Symbol *sym,

lld/MachO/Writer.cpp

void Writer::run() {
   // dyld requires __LINKEDIT segment to always exist (even if empty).
   OutputSegment *linkEditSegment =
       getOrCreateOutputSegment(segment_names::linkEdit);
 
   scanRelocations();
   if (in.stubHelper->isNeeded())
     in.stubHelper->setup();
 
+  for (const macho::Symbol *sym : symtab->getSymbols())
+    if (auto *defined = dyn_cast<Defined>(sym))
+      if (defined->overridesWeakDef)
+        in.weakBinding->addNonWeakDefinition(defined);
+
   // Sort and assign sections to their respective segments. No more sections nor
   // segments may be created after these methods run.
   createOutputSections();
   sortSegmentsAndSections();
 
   createLoadCommands();
 
   // Ensure that segments (and the sections they contain) are allocated

lld/test/MachO/nonweak-definition-override.s

+# REQUIRES: x86
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/libfoo.s -o %t/libfoo.o
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -dylib %t/libfoo.o -o %t/libfoo.dylib
+
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk %t/test.o -L%t -lfoo -o %t/test -lSystem
+# RUN: llvm-objdump --macho --weak-bind %t/test | FileCheck %s
+
+## Test loading the dylib before the obj file.
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -L%t -lfoo %t/test.o -o %t/test -lSystem
+# RUN: llvm-objdump --macho --weak-bind %t/test | FileCheck %s
+
+# CHECK:      Weak bind table:
+# CHECK-NEXT: segment  section            address     type       addend   symbol
+# CHECK-NEXT:                                         strong              _foo
+
+#--- libfoo.s
+
+.globl _foo
+
+.weak_definition _foo
+_foo:
+
+#--- test.s
+
+.globl _main, _foo
+
+_foo:
+
+_main:
+  ret

lld/test/MachO/weak-header-flags.s

 # REQUIRES: x86
 # RUN: split-file %s %t
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/libweak-defines.s -o %t/libweak-defines.o
 # RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -dylib %t/libweak-defines.o -o %t/libweak-defines.dylib
 # RUN: llvm-readobj --file-headers %t/libweak-defines.dylib | FileCheck %s --check-prefix=WEAK-DEFINES-AND-BINDS
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/binds-to-weak.s -o %t/binds-to-weak.o
 # RUN: lld -flavor darwinnew -L%S/Inputs/MacOSX.sdk/usr/lib -lSystem -L%t -lweak-defines -o %t/binds-to-weak %t/binds-to-weak.o
 # RUN: llvm-readobj --file-headers %t/binds-to-weak | FileCheck %s --check-prefix=WEAK-BINDS-ONLY
 
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/overrides-weak.s -o %t/overrides-weak.o
+# RUN: lld -flavor darwinnew -L%S/Inputs/MacOSX.sdk/usr/lib -lSystem -L%t -lweak-defines -o %t/overrides-weak %t/overrides-weak.o
+# RUN: llvm-readobj --file-headers %t/overrides-weak | FileCheck %s --check-prefix=WEAK-DEFINES-ONLY
+
 # WEAK-DEFINES-AND-BINDS: MH_BINDS_TO_WEAK
 # WEAK-DEFINES-AND-BINDS: MH_WEAK_DEFINES
 
 # WEAK-BINDS-ONLY:        MH_BINDS_TO_WEAK
 # WEAK-BINDS-ONLY-NOT:    MH_WEAK_DEFINES
 
+# WEAK-DEFINES-ONLY:      MH_WEAK_DEFINES
+# WEAK-DEFINES-ONLY-NOT:  MH_BINDS_TO_WEAK
+
 #--- libweak-defines.s
 
 .globl _foo
 .weak_definition _foo
 _foo:
   ret
 
 #--- binds-to-weak.s
 
 .globl _main
 _main:
   callq _foo
   ret
 
 ## Don't generate MH_WEAK_DEFINES for weak locals
 .weak_definition _weak_local
 _weak_local:
+
+#--- overrides-weak.s
+
+.globl _main, _foo
+_foo:
+
+_main:
+  ret