[lld-macho][nfc] Eliminate InputSection::Shared
authorJez Ng <jezng@fb.com>
Fri, 4 Feb 2022 00:53:29 +0000 (19:53 -0500)
committerJez Ng <jezng@fb.com>
Fri, 4 Feb 2022 00:55:42 +0000 (19:55 -0500)
Earlier in LLD's evolution, I tried to create the illusion that
subsections were indistinguishable from "top-level" sections. Thus, even
though the subsections shared many common field values, I hid those
common values away in a private Shared struct (see D105305). More
recently, however, @gkm added a public `Section` struct in D113241 that
served as an explicit way to store values that are common to an entire
set of subsections (aka InputSections). Now that we have another "common
value" struct, `Shared` has been rendered redundant. All its fields can
be moved into `Section` instead, and the pointer to `Shared` can be replaced
with a pointer to `Section`.

This `Section` pointer also has the advantage of letting us inspect other
subsections easily, simplifying the implementation of {D118798}.

P.S. I do think that having both `Section` and `InputSection` makes for
a slightly confusing naming scheme. I considered renaming `InputSection`
to `Subsection`, but that would break the symmetry with `OutputSection`.
It would also make us deviate from LLD-ELF's naming scheme.

This change is perf-neutral on my 3.2 GHz 16-Core Intel Xeon W machine:

             base           diff           difference (95% CI)
  sys_time   1.258 ± 0.031  1.248 ± 0.023  [  -1.6% ..   +0.1%]
  user_time  3.659 ± 0.047  3.658 ± 0.041  [  -0.5% ..   +0.4%]
  wall_time  4.640 ± 0.085  4.625 ± 0.063  [  -1.0% ..   +0.3%]
  samples    49             61

There's also no stat sig change in RSS (as measured by `time -l`):

           base                         diff                           difference (95% CI)
  time     998038627.097 ± 13567305.958 1003327715.556 ± 15210451.236  [  -0.2% ..   +1.2%]
  samples  31                           36

Reviewed By: #lld-macho, oontvoo

Differential Revision: https://reviews.llvm.org/D118797

lld/MachO/ConcatOutputSection.cpp
lld/MachO/Driver.cpp
lld/MachO/InputFiles.cpp
lld/MachO/InputFiles.h
lld/MachO/InputSection.cpp
lld/MachO/InputSection.h
lld/MachO/SymbolTable.cpp
lld/MachO/SyntheticSections.cpp
lld/MachO/Writer.cpp

index 4fae934..4cec418 100644 (file)
@@ -313,7 +313,7 @@ void ConcatOutputSection::finalize() {
         fatal(Twine(__FUNCTION__) + ": FIXME: thunk range overrun");
       }
       thunkInfo.isec =
-          make<ConcatInputSection>(isec->getSegName(), isec->getName());
+          makeSyntheticInputSection(isec->getSegName(), isec->getName());
       thunkInfo.isec->parent = this;
 
       // This code runs after dead code removal. Need to set the `live` bit
index 58a4436..bf5e97a 100644 (file)
@@ -540,9 +540,11 @@ static void replaceCommonSymbols() {
     // but it's not really worth supporting the linking of 64-bit programs on
     // 32-bit archs.
     ArrayRef<uint8_t> data = {nullptr, static_cast<size_t>(common->size)};
-    auto *isec = make<ConcatInputSection>(
-        segment_names::data, section_names::common, common->getFile(), data,
-        common->align, S_ZEROFILL);
+    // FIXME avoid creating one Section per symbol?
+    auto *section =
+        make<Section>(common->getFile(), segment_names::data,
+                      section_names::common, S_ZEROFILL, /*addr=*/0);
+    auto *isec = make<ConcatInputSection>(*section, data, common->align);
     if (!osec)
       osec = ConcatOutputSection::getOrCreateForInput(isec);
     isec->parent = osec;
@@ -550,7 +552,7 @@ static void replaceCommonSymbols() {
 
     // FIXME: CommonSymbol should store isReferencedDynamically, noDeadStrip
     // and pass them on here.
-    replaceSymbol<Defined>(sym, sym->getName(), isec->getFile(), isec,
+    replaceSymbol<Defined>(sym, sym->getName(), common->getFile(), isec,
                            /*value=*/0,
                            /*size=*/0,
                            /*isWeakDef=*/false,
@@ -1005,8 +1007,8 @@ static void gatherInputSections() {
   TimeTraceScope timeScope("Gathering input sections");
   int inputOrder = 0;
   for (const InputFile *file : inputFiles) {
-    for (const Section &section : file->sections) {
-      const Subsections &subsections = section.subsections;
+    for (const Section *section : file->sections) {
+      const Subsections &subsections = section->subsections;
       if (subsections.empty())
         continue;
       if (subsections[0].isec->getName() == section_names::compactUnwind)
index 74cccff..f82aa81 100644 (file)
@@ -277,28 +277,24 @@ void ObjFile::parseSections(ArrayRef<SectionHeader> sectionHeaders) {
     ArrayRef<uint8_t> data = {isZeroFill(sec.flags) ? nullptr
                                                     : buf + sec.offset,
                               static_cast<size_t>(sec.size)};
-    sections.push_back(sec.addr);
+    sections.push_back(make<Section>(this, segname, name, sec.flags, sec.addr));
     if (sec.align >= 32) {
       error("alignment " + std::to_string(sec.align) + " of section " + name +
             " is too large");
       continue;
     }
+    const Section &section = *sections.back();
     uint32_t align = 1 << sec.align;
-    uint32_t flags = sec.flags;
 
     auto splitRecords = [&](int recordSize) -> void {
       if (data.empty())
         return;
-      Subsections &subsections = sections.back().subsections;
+      Subsections &subsections = sections.back()->subsections;
       subsections.reserve(data.size() / recordSize);
-      auto *isec = make<ConcatInputSection>(
-          segname, name, this, data.slice(0, recordSize), align, flags);
-      subsections.push_back({0, isec});
-      for (uint64_t off = recordSize; off < data.size(); off += recordSize) {
-        // Copying requires less memory than constructing a fresh InputSection.
-        auto *copy = make<ConcatInputSection>(*isec);
-        copy->data = data.slice(off, recordSize);
-        subsections.push_back({off, copy});
+      for (uint64_t off = 0; off < data.size(); off += recordSize) {
+        auto *isec = make<ConcatInputSection>(
+            section, data.slice(off, recordSize), align);
+        subsections.push_back({off, isec});
       }
     };
 
@@ -312,19 +308,17 @@ void ObjFile::parseSections(ArrayRef<SectionHeader> sectionHeaders) {
 
       InputSection *isec;
       if (sectionType(sec.flags) == S_CSTRING_LITERALS) {
-        isec =
-            make<CStringInputSection>(segname, name, this, data, align, flags);
+        isec = make<CStringInputSection>(section, data, align);
         // FIXME: parallelize this?
         cast<CStringInputSection>(isec)->splitIntoPieces();
       } else {
-        isec = make<WordLiteralInputSection>(segname, name, this, data, align,
-                                             flags);
+        isec = make<WordLiteralInputSection>(section, data, align);
       }
-      sections.back().subsections.push_back({0, isec});
+      sections.back()->subsections.push_back({0, isec});
     } else if (auto recordSize = getRecordSize(segname, name)) {
       splitRecords(*recordSize);
       if (name == section_names::compactUnwind)
-        compactUnwindSection = &sections.back();
+        compactUnwindSection = sections.back();
     } else if (segname == segment_names::llvm) {
       if (name == "__cg_profile" && config->callGraphProfileSort) {
         TimeTraceScope timeScope("Parsing call graph section");
@@ -352,8 +346,7 @@ void ObjFile::parseSections(ArrayRef<SectionHeader> sectionHeaders) {
       // spurious duplicate symbol errors, we do not parse these sections.
       // TODO: Evaluate whether the bitcode metadata is needed.
     } else {
-      auto *isec =
-          make<ConcatInputSection>(segname, name, this, data, align, flags);
+      auto *isec = make<ConcatInputSection>(section, data, align);
       if (isDebugSection(isec->getFlags()) &&
           isec->getSegName() == segment_names::dwarf) {
         // Instead of emitting DWARF sections, we emit STABS symbols to the
@@ -361,7 +354,7 @@ void ObjFile::parseSections(ArrayRef<SectionHeader> sectionHeaders) {
         // parsing their relocations unnecessarily.
         debugSections.push_back(isec);
       } else {
-        sections.back().subsections.push_back({0, isec});
+        sections.back()->subsections.push_back({0, isec});
       }
     }
   }
@@ -506,7 +499,7 @@ void ObjFile::parseRelocations(ArrayRef<SectionHeader> sectionHeaders,
         referentOffset = totalAddend - referentSecHead.addr;
       }
       Subsections &referentSubsections =
-          sections[relInfo.r_symbolnum - 1].subsections;
+          sections[relInfo.r_symbolnum - 1]->subsections;
       r.referent =
           findContainingSubsection(referentSubsections, &referentOffset);
       r.addend = referentOffset;
@@ -547,7 +540,7 @@ void ObjFile::parseRelocations(ArrayRef<SectionHeader> sectionHeaders,
         uint64_t referentOffset =
             totalAddend - sectionHeaders[minuendInfo.r_symbolnum - 1].addr;
         Subsections &referentSubsectVec =
-            sections[minuendInfo.r_symbolnum - 1].subsections;
+            sections[minuendInfo.r_symbolnum - 1]->subsections;
         p.referent =
             findContainingSubsection(referentSubsectVec, &referentOffset);
         p.addend = referentOffset;
@@ -699,7 +692,7 @@ void ObjFile::parseSymbols(ArrayRef<typename LP::section> sectionHeaders,
 
     StringRef name = strtab + sym.n_strx;
     if ((sym.n_type & N_TYPE) == N_SECT) {
-      Subsections &subsections = sections[sym.n_sect - 1].subsections;
+      Subsections &subsections = sections[sym.n_sect - 1]->subsections;
       // parseSections() may have chosen not to parse this section.
       if (subsections.empty())
         continue;
@@ -712,7 +705,7 @@ void ObjFile::parseSymbols(ArrayRef<typename LP::section> sectionHeaders,
   }
 
   for (size_t i = 0; i < sections.size(); ++i) {
-    Subsections &subsections = sections[i].subsections;
+    Subsections &subsections = sections[i]->subsections;
     if (subsections.empty())
       continue;
     InputSection *lastIsec = subsections.back().isec;
@@ -823,12 +816,13 @@ OpaqueFile::OpaqueFile(MemoryBufferRef mb, StringRef segName,
     : InputFile(OpaqueKind, mb) {
   const auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
   ArrayRef<uint8_t> data = {buf, mb.getBufferSize()};
-  ConcatInputSection *isec =
-      make<ConcatInputSection>(segName.take_front(16), sectName.take_front(16),
-                               /*file=*/this, data);
+  sections.push_back(make<Section>(/*file=*/this, segName.take_front(16),
+                                   sectName.take_front(16),
+                                   /*flags=*/0, /*addr=*/0));
+  Section &section = *sections.back();
+  ConcatInputSection *isec = make<ConcatInputSection>(section, data);
   isec->live = true;
-  sections.push_back(0);
-  sections.back().subsections.push_back({0, isec});
+  section.subsections.push_back({0, isec});
 }
 
 ObjFile::ObjFile(MemoryBufferRef mb, uint32_t modTime, StringRef archiveName,
@@ -898,9 +892,9 @@ template <class LP> void ObjFile::parse() {
   // The relocations may refer to the symbols, so we parse them after we have
   // parsed all the symbols.
   for (size_t i = 0, n = sections.size(); i < n; ++i)
-    if (!sections[i].subsections.empty())
+    if (!sections[i]->subsections.empty())
       parseRelocations(sectionHeaders, sectionHeaders[i],
-                       sections[i].subsections);
+                       sections[i]->subsections);
 
   parseDebugInfo();
   if (compactUnwindSection)
index 0b661c8..432cddd 100644 (file)
@@ -58,11 +58,24 @@ struct Subsection {
 };
 
 using Subsections = std::vector<Subsection>;
+class InputFile;
 
 struct Section {
-  uint64_t address = 0;
+  InputFile *file;
+  StringRef segname;
+  StringRef name;
+  uint32_t flags;
+  uint64_t addr;
   Subsections subsections;
-  Section(uint64_t addr) : address(addr){};
+
+  Section(InputFile *file, StringRef segname, StringRef name, uint32_t flags,
+          uint64_t addr)
+      : file(file), segname(segname), name(name), flags(flags), addr(addr) {}
+  // Ensure pointers to Sections are never invalidated.
+  Section(const Section &) = delete;
+  Section &operator=(const Section &) = delete;
+  Section(Section &&) = delete;
+  Section &operator=(Section &&) = delete;
 };
 
 // Represents a call graph profile edge.
@@ -93,7 +106,7 @@ public:
   MemoryBufferRef mb;
 
   std::vector<Symbol *> symbols;
-  std::vector<Section> sections;
+  std::vector<Section *> sections;
 
   // If not empty, this stores the name of the archive containing this file.
   // We use this string for creating error messages.
index d420857..a2213f8 100644 (file)
@@ -177,6 +177,18 @@ void ConcatInputSection::writeTo(uint8_t *buf) {
   }
 }
 
+ConcatInputSection *macho::makeSyntheticInputSection(StringRef segName,
+                                                     StringRef sectName,
+                                                     uint32_t flags,
+                                                     ArrayRef<uint8_t> data,
+                                                     uint32_t align) {
+  Section &section =
+      *make<Section>(/*file=*/nullptr, segName, sectName, flags, /*addr=*/0);
+  auto isec = make<ConcatInputSection>(section, data, align);
+  section.subsections.push_back({0, isec});
+  return isec;
+}
+
 void CStringInputSection::splitIntoPieces() {
   size_t off = 0;
   StringRef s = toStringRef(data);
@@ -211,13 +223,11 @@ uint64_t CStringInputSection::getOffset(uint64_t off) const {
   return piece.outSecOff + addend;
 }
 
-WordLiteralInputSection::WordLiteralInputSection(StringRef segname,
-                                                 StringRef name,
-                                                 InputFile *file,
+WordLiteralInputSection::WordLiteralInputSection(const Section &section,
                                                  ArrayRef<uint8_t> data,
-                                                 uint32_t align, uint32_t flags)
-    : InputSection(WordLiteralKind, segname, name, file, data, align, flags) {
-  switch (sectionType(flags)) {
+                                                 uint32_t align)
+    : InputSection(WordLiteralKind, section, data, align) {
+  switch (sectionType(getFlags())) {
   case S_4BYTE_LITERALS:
     power2LiteralSize = 2;
     break;
index 5535e87..52f851d 100644 (file)
@@ -29,20 +29,20 @@ class OutputSection;
 
 class InputSection {
 public:
-  enum Kind {
+  enum Kind : uint8_t {
     ConcatKind,
     CStringLiteralKind,
     WordLiteralKind,
   };
 
-  Kind kind() const { return shared->sectionKind; }
+  Kind kind() const { return sectionKind; }
   virtual ~InputSection() = default;
   virtual uint64_t getSize() const { return data.size(); }
   virtual bool empty() const { return data.empty(); }
-  InputFile *getFile() const { return shared->file; }
-  StringRef getName() const { return shared->name; }
-  StringRef getSegName() const { return shared->segname; }
-  uint32_t getFlags() const { return shared->flags; }
+  InputFile *getFile() const { return section.file; }
+  StringRef getName() const { return section.name; }
+  StringRef getSegName() const { return section.segname; }
+  uint32_t getFlags() const { return section.flags; }
   uint64_t getFileSize() const;
   // Translates \p off -- an offset relative to this InputSection -- into an
   // offset from the beginning of its parent OutputSection.
@@ -55,12 +55,23 @@ public:
   virtual InputSection *canonical() { return this; }
   virtual const InputSection *canonical() const { return this; }
 
-  OutputSection *parent = nullptr;
+protected:
+  InputSection(Kind kind, const Section &section, ArrayRef<uint8_t> data,
+               uint32_t align)
+      : sectionKind(kind), align(align), data(data), section(section) {}
 
-  uint32_t align = 1;
+  InputSection(const InputSection &rhs)
+      : sectionKind(rhs.sectionKind), align(rhs.align), data(rhs.data),
+        section(rhs.section) {}
+
+  Kind sectionKind;
+
+public:
   // is address assigned?
   bool isFinal = false;
+  uint32_t align = 1;
 
+  OutputSection *parent = nullptr;
   ArrayRef<uint8_t> data;
   std::vector<Reloc> relocs;
   // The symbols that belong to this InputSection, sorted by value. With
@@ -68,31 +79,7 @@ public:
   llvm::TinyPtrVector<Defined *> symbols;
 
 protected:
-  // The fields in this struct are immutable. Since we create a lot of
-  // InputSections with identical values for them (due to
-  // .subsections_via_symbols), factoring them out into a shared struct reduces
-  // memory consumption and makes copying cheaper.
-  struct Shared {
-    InputFile *file;
-    StringRef name;
-    StringRef segname;
-    uint32_t flags;
-    Kind sectionKind;
-    Shared(InputFile *file, StringRef name, StringRef segname, uint32_t flags,
-           Kind kind)
-        : file(file), name(name), segname(segname), flags(flags),
-          sectionKind(kind) {}
-  };
-
-  InputSection(Kind kind, StringRef segname, StringRef name, InputFile *file,
-               ArrayRef<uint8_t> data, uint32_t align, uint32_t flags)
-      : align(align), data(data),
-        shared(make<Shared>(file, name, segname, flags, kind)) {}
-
-  InputSection(const InputSection &rhs)
-      : align(rhs.align), data(rhs.data), shared(rhs.shared) {}
-
-  const Shared *const shared;
+  const Section &section;
 };
 
 // ConcatInputSections are combined into (Concat)OutputSections through simple
@@ -100,15 +87,9 @@ protected:
 // contents merged before output.
 class ConcatInputSection final : public InputSection {
 public:
-  ConcatInputSection(StringRef segname, StringRef name, InputFile *file,
-                     ArrayRef<uint8_t> data, uint32_t align = 1,
-                     uint32_t flags = 0)
-      : InputSection(ConcatKind, segname, name, file, data, align, flags) {}
-
-  ConcatInputSection(StringRef segname, StringRef name)
-      : ConcatInputSection(segname, name, /*file=*/nullptr,
-                           /*data=*/{},
-                           /*align=*/1, /*flags=*/0) {}
+  ConcatInputSection(const Section &section, ArrayRef<uint8_t> data,
+                     uint32_t align = 1)
+      : InputSection(ConcatKind, section, data, align) {}
 
   uint64_t getOffset(uint64_t off) const override { return outSecOff + off; }
   uint64_t getVA() const { return InputSection::getVA(0); }
@@ -152,6 +133,13 @@ public:
   uint64_t outSecOff = 0;
 };
 
+// Initialize a fake InputSection that does not belong to any InputFile.
+ConcatInputSection *makeSyntheticInputSection(StringRef segName,
+                                              StringRef sectName,
+                                              uint32_t flags = 0,
+                                              ArrayRef<uint8_t> data = {},
+                                              uint32_t align = 1);
+
 // Helper functions to make it easy to sprinkle asserts.
 
 inline bool shouldOmitFromOutput(InputSection *isec) {
@@ -193,10 +181,9 @@ static_assert(sizeof(StringPiece) == 16, "StringPiece is too big!");
 // conservative behavior we can certainly implement that.
 class CStringInputSection final : public InputSection {
 public:
-  CStringInputSection(StringRef segname, StringRef name, InputFile *file,
-                      ArrayRef<uint8_t> data, uint32_t align, uint32_t flags)
-      : InputSection(CStringLiteralKind, segname, name, file, data, align,
-                     flags) {}
+  CStringInputSection(const Section &section, ArrayRef<uint8_t> data,
+                      uint32_t align)
+      : InputSection(CStringLiteralKind, section, data, align) {}
   uint64_t getOffset(uint64_t off) const override;
   bool isLive(uint64_t off) const override { return getStringPiece(off).live; }
   void markLive(uint64_t off) override { getStringPiece(off).live = true; }
@@ -231,9 +218,8 @@ public:
 
 class WordLiteralInputSection final : public InputSection {
 public:
-  WordLiteralInputSection(StringRef segname, StringRef name, InputFile *file,
-                          ArrayRef<uint8_t> data, uint32_t align,
-                          uint32_t flags);
+  WordLiteralInputSection(const Section &section, ArrayRef<uint8_t> data,
+                          uint32_t align);
   uint64_t getOffset(uint64_t off) const override;
   bool isLive(uint64_t off) const override {
     return live[off >> power2LiteralSize];
index c7017c8..0f23bf3 100644 (file)
@@ -266,7 +266,7 @@ static void handleSectionBoundarySymbol(const Undefined &sym, StringRef segSect,
     }
 
   if (!osec) {
-    ConcatInputSection *isec = make<ConcatInputSection>(segName, sectName);
+    ConcatInputSection *isec = makeSyntheticInputSection(segName, sectName);
 
     // This runs after markLive() and is only called for Undefineds that are
     // live. Marking the isec live ensures an OutputSection is created that the
index 3fe551f..9d246e9 100644 (file)
@@ -49,7 +49,7 @@ std::vector<SyntheticSection *> macho::syntheticSections;
 SyntheticSection::SyntheticSection(const char *segname, const char *name)
     : OutputSection(SyntheticKind, name) {
   std::tie(this->segname, this->name) = maybeRenameSection({segname, name});
-  isec = make<ConcatInputSection>(segname, name);
+  isec = makeSyntheticInputSection(segname, name);
   isec->parent = this;
   syntheticSections.push_back(this);
 }
@@ -748,14 +748,14 @@ static std::vector<MachO::data_in_code_entry> collectDataInCodeEntries() {
     // For each code subsection find 'data in code' entries residing in it.
     // Compute the new offset values as
     // <offset within subsection> + <subsection address> - <__TEXT address>.
-    for (const Section &section : objFile->sections) {
-      for (const Subsection &subsec : section.subsections) {
+    for (const Section *section : objFile->sections) {
+      for (const Subsection &subsec : section->subsections) {
         const InputSection *isec = subsec.isec;
         if (!isCodeSection(isec))
           continue;
         if (cast<ConcatInputSection>(isec)->shouldOmitFromOutput())
           continue;
-        const uint64_t beginAddr = section.address + subsec.offset;
+        const uint64_t beginAddr = section->addr + subsec.offset;
         auto it = llvm::lower_bound(
             entries, beginAddr,
             [](const MachO::data_in_code_entry &entry, uint64_t addr) {
index 851cb3d..6aaefaa 100644 (file)
@@ -1172,10 +1172,10 @@ void macho::createSyntheticSections() {
   // dyld to cache an address to the image loader it uses.
   uint8_t *arr = bAlloc().Allocate<uint8_t>(target->wordSize);
   memset(arr, 0, target->wordSize);
-  in.imageLoaderCache = make<ConcatInputSection>(
-      segment_names::data, section_names::data, /*file=*/nullptr,
+  in.imageLoaderCache = makeSyntheticInputSection(
+      segment_names::data, section_names::data, S_REGULAR,
       ArrayRef<uint8_t>{arr, target->wordSize},
-      /*align=*/target->wordSize, /*flags=*/S_REGULAR);
+      /*align=*/target->wordSize);
   // References from dyld are not visible to us, so ensure this section is
   // always treated as live.
   in.imageLoaderCache->live = true;