[lld-macho][NFC] Minor refactor of Writer::run()

[gkm]

Summary

Move some functions closer to their uses. Move detailed address-assignment logic out of the otherwise abstract Writer::run(). This prepares the ground for a diff to implement branch range extension thunks.

• SyntheticSections.cpp
  • move needsBinding() and prepareBranchTarget() into Writer.cpp
  • move addNonLazyBindingEntries() adjacent to its use.

• Writer.cpp
  • move address-assignment logic from Writer::run() into new function Writer::assignAddresses()
  • move needsBinding() and prepareBranchTarget() from SyntheticSections.cpp

• Target.h
  • remove orphaned decls of prepareSymbolRelocation() and validateRelocationInfo() which were moved to other files in earlier diffs.

[gkm]

• Now that addNonLazyBindingEntries() resides in Writer.cpp, also move its decl to Writer.h

[int3]

lld/MachO/Writer.cpp

448

nit: extra line

473

ditto

833

This function feels misleadingly named. Also I'm not sure why we need to put all the finalizeContents() calls together with the address assignment loop. Can we make linkEditSegment a member of Writer and then split all the finalizeContents() calls into a separate finalizeLinkEdit()?

858–859

This comment seems worth keeping

[gkm]

lld/MachO/Writer.cpp

858–859

OK for now ... until BRET (branch-range-extension thunks) creates more sections after this point during address assignment. ;)

[gkm]

• revise according to review feedback

[int3]

lld/MachO/Writer.cpp

834–835

move this back to where it originally was (at the start of Writer::run()) so the "No more sections nor segments may be created after these methods run" comment holds true?

858–859

hmm... "No more segments or sections (aside from branch range extension thunks) may be created after this point"?

[gkm]

• resolve clang-tidy complaints

[gkm]

• revise according to review feedback

lld/MachO/SyntheticSections.h

public:
   bool hasNonWeakDefinition() const { return !definitions.empty(); }
 
 private:
   std::vector<WeakBindingEntry> bindings;
   std::vector<const Defined *> definitions;
   SmallVector<char, 128> contents;
 };
 
-// Whether a given symbol's address can only be resolved at runtime.
-bool needsBinding(const Symbol *);
-
-// Add bindings for symbols that need weak or non-lazy bindings.
-void addNonLazyBindingEntries(const Symbol *, const InputSection *,
-                              uint64_t offset, int64_t addend = 0);
-
 // The following sections implement lazy symbol binding -- very similar to the
 // PLT mechanism in ELF.
 //
 // ELF's .plt section is broken up into two sections in Mach-O: StubsSection
 // and StubHelperSection. Calls to functions in dylibs will end up calling into
 // StubsSection, which contains indirect jumps to addresses stored in the
 // LazyPointerSection (the counterpart to ELF's .plt.got).
 //

 private:
   uint32_t encode(const DylibSymbol &);
 
   llvm::SetVector<DylibSymbol *> entries;
   SmallVector<char, 128> contents;
   llvm::raw_svector_ostream os{contents};
 };
 
-// Adds stubs and bindings where necessary (e.g. if the symbol is a
-// DylibSymbol.)
-void prepareBranchTarget(Symbol *);
-
 // Stores a trie that describes the set of exported symbols.
 class ExportSection : public LinkEditSection {
 public:
   ExportSection();
   void finalizeContents();
   uint64_t getRawSize() const override { return size; }
   void writeTo(uint8_t *buf) const override;
 

lld/MachO/SyntheticSections.cpp

 
 NonLazyPointerSectionBase::NonLazyPointerSectionBase(const char *segname,
                                                      const char *name)
     : SyntheticSection(segname, name) {
   align = WordSize;
   flags = S_NON_LAZY_SYMBOL_POINTERS;
 }
 
+void macho::addNonLazyBindingEntries(const Symbol *sym,
+                                     const InputSection *isec, uint64_t offset,
+                                     int64_t addend) {
+  if (auto *dysym = dyn_cast<DylibSymbol>(sym)) {
+    in.binding->addEntry(dysym, isec, offset, addend);
+    if (dysym->isWeakDef())
+      in.weakBinding->addEntry(sym, isec, offset, addend);
+  } else if (auto *defined = dyn_cast<Defined>(sym)) {
+    in.rebase->addEntry(isec, offset);
+    if (defined->isExternalWeakDef())
+      in.weakBinding->addEntry(sym, isec, offset, addend);
+  } else {
+    // Undefined symbols are filtered out in scanRelocations(); we should never
+    // get here
+    llvm_unreachable("cannot bind to an undefined symbol");
+  }
+}
+
 void NonLazyPointerSectionBase::addEntry(Symbol *sym) {
   if (entries.insert(sym)) {
     assert(!sym->isInGot());
     sym->gotIndex = entries.size() - 1;
 
     addNonLazyBindingEntries(sym, isec, sym->gotIndex * WordSize);
   }
 }
void WeakBindingSection::finalizeContents() {
   if (!bindings.empty() || !definitions.empty())
     os << static_cast<uint8_t>(BIND_OPCODE_DONE);
 }
 
 void WeakBindingSection::writeTo(uint8_t *buf) const {
   memcpy(buf, contents.data(), contents.size());
 }
 
-bool macho::needsBinding(const Symbol *sym) {
-  if (isa<DylibSymbol>(sym))
-    return true;
-  if (const auto *defined = dyn_cast<Defined>(sym))
-    return defined->isExternalWeakDef();
-  return false;
-}
-
-void macho::addNonLazyBindingEntries(const Symbol *sym,
-                                     const InputSection *isec, uint64_t offset,
-                                     int64_t addend) {
-  if (auto *dysym = dyn_cast<DylibSymbol>(sym)) {
-    in.binding->addEntry(dysym, isec, offset, addend);
-    if (dysym->isWeakDef())
-      in.weakBinding->addEntry(sym, isec, offset, addend);
-  } else if (auto *defined = dyn_cast<Defined>(sym)) {
-    in.rebase->addEntry(isec, offset);
-    if (defined->isExternalWeakDef())
-      in.weakBinding->addEntry(sym, isec, offset, addend);
-  } else {
-    // Undefined symbols are filtered out in scanRelocations(); we should never
-    // get here
-    llvm_unreachable("cannot bind to an undefined symbol");
-  }
-}
-
 StubsSection::StubsSection()
     : SyntheticSection(segment_names::text, "__stubs") {
   flags = S_SYMBOL_STUBS | S_ATTR_SOME_INSTRUCTIONS | S_ATTR_PURE_INSTRUCTIONS;
   // The stubs section comprises machine instructions, which are aligned to
   // 4 bytes on the archs we care about.
   align = 4;
   reserved2 = target->stubSize;
 }
if (sym.isWeakRef())
     flags |= BIND_SYMBOL_FLAGS_WEAK_IMPORT;
 
   os << flags << sym.getName() << '\0'
      << static_cast<uint8_t>(BIND_OPCODE_DO_BIND)
      << static_cast<uint8_t>(BIND_OPCODE_DONE);
   return opstreamOffset;
 }
 
-void macho::prepareBranchTarget(Symbol *sym) {
-  if (auto *dysym = dyn_cast<DylibSymbol>(sym)) {
-    if (in.stubs->addEntry(dysym)) {
-      if (sym->isWeakDef()) {
-        in.binding->addEntry(dysym, in.lazyPointers->isec,
-                             sym->stubsIndex * WordSize);
-        in.weakBinding->addEntry(sym, in.lazyPointers->isec,
-                                 sym->stubsIndex * WordSize);
-      } else {
-        in.lazyBinding->addEntry(dysym);
-      }
-    }
-  } else if (auto *defined = dyn_cast<Defined>(sym)) {
-    if (defined->isExternalWeakDef()) {
-      if (in.stubs->addEntry(sym)) {
-        in.rebase->addEntry(in.lazyPointers->isec, sym->stubsIndex * WordSize);
-        in.weakBinding->addEntry(sym, in.lazyPointers->isec,
-                                 sym->stubsIndex * WordSize);
-      }
-    }
-  }
-}
-
 ExportSection::ExportSection()
     : LinkEditSection(segment_names::linkEdit, section_names::export_) {}
 
 static void validateExportSymbol(const Defined *defined) {
   StringRef symbolName = defined->getName();
   if (defined->privateExtern && config->exportedSymbols.match(symbolName))
     error("cannot export hidden symbol " + symbolName + "\n>>> defined in " +
           toString(defined->getFile()));

lld/MachO/Target.h

public:
   virtual const RelocAttrs &getRelocAttrs(uint8_t type) const = 0;
 
   virtual uint64_t getPageSize() const = 0;
 
   bool hasAttr(uint8_t type, RelocAttrBits bit) const {
     return getRelocAttrs(type).hasAttr(bit);
   }
 
-  bool validateRelocationInfo(llvm::MemoryBufferRef,
-                              const llvm::MachO::section_64 &sec,
-                              llvm::MachO::relocation_info);
-  void prepareSymbolRelocation(Symbol *, const InputSection *, const Reloc &);
-
   uint32_t cpuType;
   uint32_t cpuSubtype;
 
   size_t stubSize;
   size_t stubHelperHeaderSize;
   size_t stubHelperEntrySize;
 };
 

lld/MachO/Writer.h

 #define LLD_MACHO_WRITER_H
 
 #include <cstdint>
 
 namespace lld {
 namespace macho {
 
 class OutputSection;
+class InputSection;
+class Symbol;
 
 class LoadCommand {
 public:
   virtual ~LoadCommand() = default;
   virtual uint32_t getSize() const = 0;
   virtual void writeTo(uint8_t *buf) const = 0;
 };
 
 void writeResult();
 
 void createSyntheticSections();
 
+// Add bindings for symbols that need weak or non-lazy bindings.
+void addNonLazyBindingEntries(const Symbol *, const InputSection *,
+                              uint64_t offset, int64_t addend = 0);
+
 extern OutputSection *firstTLVDataSection;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/Writer.cpp

 class Writer {
 public:
   Writer() : buffer(errorHandler().outputBuffer) {}
 
   void scanRelocations();
   void scanSymbols();
   void createOutputSections();
   void createLoadCommands();
+  void assignAddresses();
   void assignAddresses(OutputSegment *);
 
   void openFile();
   void writeSections();
   void writeUuid();
   void writeCodeSignature();
 
   void run();
void writeTo(uint8_t *buf) const override {
     c->datasize = section->getSize();
   }
 
   CodeSignatureSection *section;
 };
 
 } // namespace
 
+// Adds stubs and bindings where necessary (e.g. if the symbol is a
+// DylibSymbol.)
+
+static void prepareBranchTarget(Symbol *sym) {
+  if (auto *dysym = dyn_cast<DylibSymbol>(sym)) {
+    if (in.stubs->addEntry(dysym)) {
+      if (sym->isWeakDef()) {
+        in.binding->addEntry(dysym, in.lazyPointers->isec,
+                             sym->stubsIndex * WordSize);
+        in.weakBinding->addEntry(sym, in.lazyPointers->isec,
+                                 sym->stubsIndex * WordSize);
+      } else {
+        in.lazyBinding->addEntry(dysym);
+      }
+    }
+  } else if (auto *defined = dyn_cast<Defined>(sym)) {
+    if (defined->isExternalWeakDef()) {
+      if (in.stubs->addEntry(sym)) {
+        in.rebase->addEntry(in.lazyPointers->isec, sym->stubsIndex * WordSize);
+        in.weakBinding->addEntry(sym, in.lazyPointers->isec,
+                                 sym->stubsIndex * WordSize);
+      }
+    }
+  }
+}
+
+// Can a symbol's address can only be resolved at runtime?
+
+static bool needsBinding(const Symbol *sym) {
+  if (isa<DylibSymbol>(sym))
+    return true;
+  if (const auto *defined = dyn_cast<Defined>(sym))
+    return defined->isExternalWeakDef();
+  return false;
+}
+
 static void prepareSymbolRelocation(lld::macho::Symbol *sym,
                                     const InputSection *isec, const Reloc &r) {
   const RelocAttrs &relocAttrs = target->getRelocAttrs(r.type);
 
   if (relocAttrs.hasAttr(RelocAttrBits::BRANCH)) {
     prepareBranchTarget(sym);
   } else if (relocAttrs.hasAttr(RelocAttrBits::GOT)) {
     if (relocAttrs.hasAttr(RelocAttrBits::POINTER) || needsBinding(sym))
for (SyntheticSection *ssec : syntheticSections) {
     } else {
       error("section from " + toString(it->second->firstSection()->file) +
             " conflicts with synthetic section " + ssec->segname + "," +
             ssec->name);
     }
   }
 }
 
+void Writer::assignAddresses() {
+  // dyld requires __LINKEDIT segment to always exist (even if empty).
+  OutputSegment *linkEditSegment =
+      getOrCreateOutputSegment(segment_names::linkEdit);
+
+  // Ensure that segments (and the sections they contain) are allocated
+  // addresses in ascending order, which dyld requires.
+  //
+  // Note that at this point, __LINKEDIT sections are empty, but we need to
+  // determine addresses of other segments/sections before generating its
+  // contents.
+  for (OutputSegment *seg : outputSegments)
+    if (seg != linkEditSegment)
+      assignAddresses(seg);
+
+  // FIXME(gkm): create branch-extension thunks here, then adjust addresses
+
+  // Fill __LINKEDIT contents.
+  in.rebase->finalizeContents();
+  in.binding->finalizeContents();
+  in.weakBinding->finalizeContents();
+  in.lazyBinding->finalizeContents();
+  in.exports->finalizeContents();
+  in.functionStarts->finalizeContents();
+  symtabSection->finalizeContents();
+  indirectSymtabSection->finalizeContents();
+
+  // Now that __LINKEDIT is filled out, do a proper calculation of its
+  // addresses and offsets.
+  assignAddresses(linkEditSegment);
+}
+
 void Writer::assignAddresses(OutputSegment *seg) {
   uint64_t pageSize = target->getPageSize();
   addr = alignTo(addr, pageSize);
   fileOff = alignTo(fileOff, pageSize);
   seg->fileOff = fileOff;
 
   for (OutputSection *osec : seg->getSections()) {
     if (!osec->isNeeded())

 }
 
 void Writer::writeCodeSignature() {
   if (codeSignatureSection)
     codeSignatureSection->writeHashes(buffer->getBufferStart());
 }
 
 void Writer::run() {
-  // dyld requires __LINKEDIT segment to always exist (even if empty).
-  OutputSegment *linkEditSegment =
-      getOrCreateOutputSegment(segment_names::linkEdit);
-
   prepareBranchTarget(config->entry);
   scanRelocations();
   if (in.stubHelper->isNeeded())
     in.stubHelper->setup();
   scanSymbols();
 
-  // 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
-  // addresses in ascending order, which dyld requires.
-  //
-  // Note that at this point, __LINKEDIT sections are empty, but we need to
-  // determine addresses of other segments/sections before generating its
-  // contents.
-  for (OutputSegment *seg : outputSegments)
-    if (seg != linkEditSegment)
-      assignAddresses(seg);
-
-  // Fill __LINKEDIT contents.
-  in.rebase->finalizeContents();
-  in.binding->finalizeContents();
-  in.weakBinding->finalizeContents();
-  in.lazyBinding->finalizeContents();
-  in.exports->finalizeContents();
-  in.functionStarts->finalizeContents();
-  symtabSection->finalizeContents();
-  indirectSymtabSection->finalizeContents();
-
-  // Now that __LINKEDIT is filled out, do a proper calculation of its
-  // addresses and offsets.
-  assignAddresses(linkEditSegment);
-
+  assignAddresses();
   openFile();
   if (errorCount())
     return;
 
   writeSections();
   writeUuid();
   writeCodeSignature();