[llvm-objcopy] Refactor code to include initialize method

[jakehehrlich]

Summary

This change refactors some of the code to allow for some code deduplication in later diffs as well as just to make adding a new section type more self contained to the class itself. The idea for this was first mentioned by James in D 37915 and will be used in that change as recommended.

This change follows changes for dynamic sections but precedes support for dynamic relocations.

[jhenderson]

Just to point out, there's no diff context here! Aside from that, the overall change here looks good. I'm slightly surprised to see the SectionTable being copied everywhere, as I'd expect there to only be one, but I guess it doesn't matter since it's just a wrapper around a reference to an array of pointers.

tools/llvm-objcopy/Object.h

36 ↗

(On Diff #115748)

See my comment in D36560 regarding the argument names here.

[jakehehrlich]

In D38008#874894, @jhenderson wrote:

Just to point out, there's no diff context here! Aside from that, the overall change here looks good. I'm slightly surprised to see the SectionTable being copied everywhere, as I'd expect there to only be one, but I guess it doesn't matter since it's just a wrapper around a reference to an array of pointers.

My intent was for it to be a lightly copyable type like ArrayRef is. Maybe I could call it "SectionTableRef" to clarify this? That way it follows the way StringRef and ArrayRef are used.

[jhenderson]

In D38008#875365, @jakehehrlich wrote:

In D38008#874894, @jhenderson wrote:

Just to point out, there's no diff context here! Aside from that, the overall change here looks good. I'm slightly surprised to see the SectionTable being copied everywhere, as I'd expect there to only be one, but I guess it doesn't matter since it's just a wrapper around a reference to an array of pointers.

My intent was for it to be a lightly copyable type like ArrayRef is. Maybe I could call it "SectionTableRef" to clarify this? That way it follows the way StringRef and ArrayRef are used.

I think this might be wise. It will ensure that people realise that it doesn't own the underlying sections, and that copying it is safe.

[jakehehrlich]

Changed SectionTable to SectionTableRef to clarify intended usage.

[jhenderson]

Looks fine to me. Just a couple of variable names that need dealing with and then I think this can go in.

tools/llvm-objcopy/Object.cpp

405–406 ↗

(On Diff #116066)

I think you can probably just call "TSec", "Sec".

tools/llvm-objcopy/Object.h

60 ↗

(On Diff #116066)

Not sure "Obj" is the right argument name here! See also all the overrides.

[jakehehrlich]

tools/llvm-objcopy/Object.h

60 ↗

(On Diff #116066)

Yep. That was leftover with my playing around with how this should work. I believe I originally tried passing the whole object

[jakehehrlich]

Changed a few things as requested by James

[jhenderson]

LGTM.

llvm/trunk/tools/llvm-objcopy/Object.h

 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Support/FileOutputBuffer.h"
 
 #include <memory>
 #include <set>
 
 class Segment;
+class SectionBase;
+
+class SectionTableRef {
+private:
+  llvm::ArrayRef<std::unique_ptr<SectionBase>> Sections;
+
+public:
+  SectionTableRef(llvm::ArrayRef<std::unique_ptr<SectionBase>> Secs)
+      : Sections(Secs) {}
+  SectionTableRef(const SectionTableRef &) = default;
+
+  SectionBase *getSection(uint16_t Index, llvm::Twine ErrMsg);
+
+  template <class T>
+
+  T *getSectionOfType(uint16_t Index, llvm::Twine IndexErrMsg,
+                      llvm::Twine TypeErrMsg);
+};
 
 class SectionBase {
 public:
   llvm::StringRef Name;
   Segment *ParentSegment = nullptr;
   uint64_t HeaderOffset;
   uint64_t OriginalOffset;
   uint32_t Index;
 
   uint64_t Addr = 0;
   uint64_t Align = 1;
   uint32_t EntrySize = 0;
   uint64_t Flags = 0;
   uint64_t Info = 0;
   uint64_t Link = llvm::ELF::SHN_UNDEF;
   uint64_t NameIndex = 0;
   uint64_t Offset = 0;
   uint64_t Size = 0;
   uint64_t Type = llvm::ELF::SHT_NULL;
 
   virtual ~SectionBase() {}
+  virtual void initialize(SectionTableRef SecTable);
   virtual void finalize();
   template <class ELFT> void writeHeader(llvm::FileOutputBuffer &Out) const;
   virtual void writeSection(llvm::FileOutputBuffer &Out) const = 0;
 };
 
 class Segment {
 private:
   struct SectionCompare {

 
 public:
   void setStrTab(StringTableSection *StrTab) { SymbolNames = StrTab; }
   void addSymbol(llvm::StringRef Name, uint8_t Bind, uint8_t Type,
                  SectionBase *DefinedIn, uint64_t Value, uint16_t Shndx,
                  uint64_t Sz);
   void addSymbolNames();
   const Symbol *getSymbolByIndex(uint32_t Index) const;
+  void initialize(SectionTableRef SecTable) override;
   void finalize() override;
   static bool classof(const SectionBase *S) {
     return S->Type == llvm::ELF::SHT_SYMTAB;
   }
 };
 
 // Only writeSection depends on the ELF type so we implement it in a subclass.
 template <class ELFT> class SymbolTableSectionImpl : public SymbolTableSection {
private:
   SectionBase *SecToApplyRel;
 
   template <class T> void writeRel(T *Buf) const;
 
 public:
   void setSymTab(SymbolTableSection *StrTab) { Symbols = StrTab; }
   void setSection(SectionBase *Sec) { SecToApplyRel = Sec; }
   void addRelocation(Relocation Rel) { Relocations.push_back(Rel); }
+  void initialize(SectionTableRef SecTable) override;
   void finalize() override;
   void writeSection(llvm::FileOutputBuffer &Out) const override;
   static bool classof(const SectionBase *S) {
     return S->Type == llvm::ELF::SHT_REL || S->Type == llvm::ELF::SHT_RELA;
   }
 };
 
 class SectionWithStrTab : public Section {
 private:
   StringTableSection *StrTab;
 
 public:
   SectionWithStrTab(llvm::ArrayRef<uint8_t> Data) : Section(Data) {}
   void setStrTab(StringTableSection *StringTable) { StrTab = StringTable; }
+  void initialize(SectionTableRef SecTable) override;
   void finalize() override;
   static bool classof(const SectionBase *S);
 };
 
 class DynamicSymbolTableSection : public SectionWithStrTab {
 public:
   DynamicSymbolTableSection(llvm::ArrayRef<uint8_t> Data)
       : SectionWithStrTab(Data) {}
private:
   typedef std::unique_ptr<SectionBase> SecPtr;
   typedef std::unique_ptr<Segment> SegPtr;
 
   typedef typename ELFT::Shdr Elf_Shdr;
   typedef typename ELFT::Ehdr Elf_Ehdr;
   typedef typename ELFT::Phdr Elf_Phdr;
 
   void initSymbolTable(const llvm::object::ELFFile<ELFT> &ElfFile,
-                       SymbolTableSection *SymTab);
+                       SymbolTableSection *SymTab, SectionTableRef SecTable);
   SecPtr makeSection(const llvm::object::ELFFile<ELFT> &ElfFile,
                      const Elf_Shdr &Shdr);
   void readProgramHeaders(const llvm::object::ELFFile<ELFT> &ElfFile);
-  void readSectionHeaders(const llvm::object::ELFFile<ELFT> &ElfFile);
+  SectionTableRef readSectionHeaders(const llvm::object::ELFFile<ELFT> &ElfFile);
 
   SectionBase *getSection(uint16_t Index, llvm::Twine ErrMsg);
 
   template <class T>
   T *getSectionOfType(uint16_t Index, llvm::Twine IndexErrMsg,
                       llvm::Twine TypeErrMsg);
 
 protected:

llvm/trunk/tools/llvm-objcopy/Object.cpp

 
 void Segment::writeSegment(FileOutputBuffer &Out) const {
   uint8_t *Buf = Out.getBufferStart() + Offset;
   // We want to maintain segments' interstitial data and contents exactly.
   // This lets us just copy segments directly.
   std::copy(std::begin(Contents), std::end(Contents), Buf);
 }
 
+void SectionBase::initialize(SectionTableRef SecTable) {}
 void SectionBase::finalize() {}
 
 template <class ELFT>
 void SectionBase::writeHeader(FileOutputBuffer &Out) const {
   uint8_t *Buf = Out.getBufferStart();
   Buf += HeaderOffset;
   typename ELFT::Shdr &Shdr = *reinterpret_cast<typename ELFT::Shdr *>(Buf);
   Shdr.sh_name = NameIndex;
void SymbolTableSection::addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
   }
   Sym.Value = Value;
   Sym.Size = Sz;
   Sym.Index = Symbols.size();
   Symbols.emplace_back(llvm::make_unique<Symbol>(Sym));
   Size += this->EntrySize;
 }
 
+void SymbolTableSection::initialize(SectionTableRef SecTable) {
+  Size = 0;
+  setStrTab(SecTable.getSectionOfType<StringTableSection>(
+      Link,
+      "Symbol table has link index of " + Twine(Link) +
+          " which is not a valid index",
+      "Symbol table has link index of " + Twine(Link) +
+          " which is not a string table"));
+}
+
 void SymbolTableSection::finalize() {
   // Make sure SymbolNames is finalized before getting name indexes.
   SymbolNames->finalize();
 
   uint32_t MaxLocalIndex = 0;
   for (auto &Sym : Symbols) {
     Sym->NameIndex = SymbolNames->findIndex(Sym->Name);
     if (Sym->Binding == STB_LOCAL)
for (auto &Symbol : Symbols) {
     Sym->st_size = Symbol->Size;
     Sym->setBinding(Symbol->Binding);
     Sym->setType(Symbol->Type);
     Sym->st_shndx = Symbol->getShndx();
     ++Sym;
   }
 }
 
+template <class ELFT>
+void RelocationSection<ELFT>::initialize(SectionTableRef SecTable) {
+  setSymTab(SecTable.getSectionOfType<SymbolTableSection>(
+      Link,
+      "Link field value " + Twine(Link) + " in section " + Name + " is invalid",
+      "Link field value " + Twine(Link) + " in section " + Name +
+          " is not a symbol table"));
+
+  setSection(SecTable.getSection(Info,
+                                 "Info field value " + Twine(Info) +
+                                     " in section " + Name + " is invalid"));
+}
+
 template <class ELFT> void RelocationSection<ELFT>::finalize() {
   this->Link = Symbols->Index;
   this->Info = SecToApplyRel->Index;
 }
 
 template <class ELFT>
 void setAddend(Elf_Rel_Impl<ELFT, false> &Rel, uint64_t Addend) {}
 
void RelocationSection<ELFT>::writeSection(llvm::FileOutputBuffer &Out) const {
   else
     writeRel(reinterpret_cast<Elf_Rela *>(Buf));
 }
 
 bool SectionWithStrTab::classof(const SectionBase *S) {
   return isa<DynamicSymbolTableSection>(S) || isa<DynamicSection>(S);
 }
 
+void SectionWithStrTab::initialize(SectionTableRef SecTable) {
+  setStrTab(SecTable.getSectionOfType<StringTableSection>(
+      Link,
+      "Link field value " + Twine(Link) + " in section " + Name + " is invalid",
+      "Link field value " + Twine(Link) + " in section " + Name +
+          " is not a string table"));
+}
+
 void SectionWithStrTab::finalize() { this->Link = StrTab->Index; }
 
 // Returns true IFF a section is wholly inside the range of a segment
 static bool sectionWithinSegment(const SectionBase &Section,
                                  const Segment &Segment) {
   // If a section is empty it should be treated like it has a size of 1. This is
   // to clarify the case when an empty section lies on a boundary between two
   // segments and ensures that the section "belongs" to the second segment and
for (auto &Parent : Segments) {
         }
       }
     }
   }
 }
 
 template <class ELFT>
 void Object<ELFT>::initSymbolTable(const llvm::object::ELFFile<ELFT> &ElfFile,
-                                   SymbolTableSection *SymTab) {
-
-  SymTab->Size = 0;
-  SymTab->setStrTab(getSectionOfType<StringTableSection>(
-      SymbolTable->Link,
-      "Symbol table has link index of " + Twine(SymTab->Link) +
-          " which is not a valid index",
-      "Symbol table has link index of " + Twine(SymTab->Link) +
-          " which is not a string table"));
+                                   SymbolTableSection *SymTab,
+                                   SectionTableRef SecTable) {
 
   const Elf_Shdr &Shdr = *unwrapOrError(ElfFile.getSection(SymTab->Index));
   StringRef StrTabData = unwrapOrError(ElfFile.getStringTableForSymtab(Shdr));
 
   for (const auto &Sym : unwrapOrError(ElfFile.symbols(&Shdr))) {
     SectionBase *DefSection = nullptr;
     StringRef Name = unwrapOrError(Sym.getName(StrTabData));
 
     if (Sym.st_shndx >= SHN_LORESERVE) {
       if (!isValidReservedSectionIndex(Sym.st_shndx, Machine)) {
         error(
             "Symbol '" + Name +
             "' has unsupported value greater than or equal to SHN_LORESERVE: " +
             Twine(Sym.st_shndx));
       }
     } else if (Sym.st_shndx != SHN_UNDEF) {
-      DefSection = getSection(
+      DefSection = SecTable.getSection(
           Sym.st_shndx,
           "Symbol '" + Name + "' is defined in invalid section with index " +
               Twine(Sym.st_shndx));
     }
 
     SymTab->addSymbol(Name, Sym.getBinding(), Sym.getType(), DefSection,
                       Sym.getValue(), Sym.st_shndx, Sym.st_size);
   }
for (const auto &Rel : RelRange) {
     ToAdd.Offset = Rel.r_offset;
     getAddend(ToAdd.Addend, Rel);
     ToAdd.Type = Rel.getType(false);
     ToAdd.RelocSymbol = SymbolTable->getSymbolByIndex(Rel.getSymbol(false));
     Relocs->addRelocation(ToAdd);
   }
 }
 
+SectionBase *SectionTableRef::getSection(uint16_t Index, Twine ErrMsg) {
+  if (Index == SHN_UNDEF || Index > Sections.size())
+    error(ErrMsg);
+  return Sections[Index - 1].get();
+}
+
+template <class T>
+T *SectionTableRef::getSectionOfType(uint16_t Index, Twine IndexErrMsg,
+                                  Twine TypeErrMsg) {
+  if (T *Sec = llvm::dyn_cast<T>(getSection(Index, IndexErrMsg)))
+    return Sec;
+  error(TypeErrMsg);
+}
+
 template <class ELFT>
 SectionBase *Object<ELFT>::getSection(uint16_t Index, Twine ErrMsg) {
   if (Index == SHN_UNDEF || Index > Sections.size())
     error(ErrMsg);
   return Sections[Index - 1].get();
 }
 
 template <class ELFT>
case SHT_NOBITS:
     return llvm::make_unique<Section>(Data);
   default:
     Data = unwrapOrError(ElfFile.getSectionContents(&Shdr));
     return llvm::make_unique<Section>(Data);
   }
 }
 
 template <class ELFT>
-void Object<ELFT>::readSectionHeaders(const ELFFile<ELFT> &ElfFile) {
+SectionTableRef Object<ELFT>::readSectionHeaders(const ELFFile<ELFT> &ElfFile) {
   uint32_t Index = 0;
   for (const auto &Shdr : unwrapOrError(ElfFile.sections())) {
     if (Index == 0) {
       ++Index;
       continue;
     }
     SecPtr Sec = makeSection(ElfFile, Shdr);
     Sec->Name = unwrapOrError(ElfFile.getSectionName(&Shdr));
     Sec->Type = Shdr.sh_type;
     Sec->Flags = Shdr.sh_flags;
     Sec->Addr = Shdr.sh_addr;
     Sec->Offset = Shdr.sh_offset;
     Sec->OriginalOffset = Shdr.sh_offset;
     Sec->Size = Shdr.sh_size;
     Sec->Link = Shdr.sh_link;
     Sec->Info = Shdr.sh_info;
     Sec->Align = Shdr.sh_addralign;
     Sec->EntrySize = Shdr.sh_entsize;
     Sec->Index = Index++;
     Sections.push_back(std::move(Sec));
   }
 
+  SectionTableRef SecTable(Sections);
+
   // Now that all of the sections have been added we can fill out some extra
-  // details about symbol tables.
-  if (SymbolTable)
-    initSymbolTable(ElfFile, SymbolTable);
+  // details about symbol tables. We need the symbol table filled out before
+  // any relocations.
+  if (SymbolTable) {
+    SymbolTable->initialize(SecTable);
+    initSymbolTable(ElfFile, SymbolTable, SecTable);
+  }
 
   // Now that all sections and symbols have been added we can add
   // relocations that reference symbols and set the link and info fields for
   // relocation sections.
   for (auto &Section : Sections) {
+    if (Section.get() == SymbolTable)
+      continue;
+    Section->initialize(SecTable);
     if (auto RelSec = dyn_cast<RelocationSection<ELFT>>(Section.get())) {
-
-      auto SymTab = getSectionOfType<SymbolTableSection>(
-          RelSec->Link,
-          "Link field value " + Twine(RelSec->Link) + " in section " +
-              RelSec->Name + " is invalid",
-          "Link field value " + Twine(RelSec->Link) + " in section " +
-              RelSec->Name + " is not a symbol table");
-      RelSec->setSymTab(SymTab);
-
-      RelSec->setSection(getSection(RelSec->Info,
-                                    "Info field value " + Twine(RelSec->Link) +
-                                        " in section " + RelSec->Name +
-                                        " is invalid"));
-
       auto Shdr = unwrapOrError(ElfFile.sections()).begin() + RelSec->Index;
       if (RelSec->Type == SHT_REL)
-        initRelocations(RelSec, SymTab, unwrapOrError(ElfFile.rels(Shdr)));
+        initRelocations(RelSec, SymbolTable, unwrapOrError(ElfFile.rels(Shdr)));
       else
-        initRelocations(RelSec, SymTab, unwrapOrError(ElfFile.relas(Shdr)));
+        initRelocations(RelSec, SymbolTable,
+                        unwrapOrError(ElfFile.relas(Shdr)));
     }
 
     if (auto Sec = dyn_cast<SectionWithStrTab>(Section.get())) {
       Sec->setStrTab(getSectionOfType<StringTableSection>(
           Sec->Link,
           "Link field value " + Twine(Sec->Link) + " in section " + Sec->Name +
               " is invalid",
           "Link field value " + Twine(Sec->Link) + " in section " + Sec->Name +
               " is not a string table"));
     }
   }
+
+  return SecTable;
 }
 
 template <class ELFT> Object<ELFT>::Object(const ELFObjectFile<ELFT> &Obj) {
   const auto &ElfFile = *Obj.getELFFile();
   const auto &Ehdr = *ElfFile.getHeader();
 
   std::copy(Ehdr.e_ident, Ehdr.e_ident + 16, Ident);
   Type = Ehdr.e_type;
   Machine = Ehdr.e_machine;
   Version = Ehdr.e_version;
   Entry = Ehdr.e_entry;
   Flags = Ehdr.e_flags;
 
-  readSectionHeaders(ElfFile);
+  SectionTableRef SecTable = readSectionHeaders(ElfFile);
   readProgramHeaders(ElfFile);
 
-  SectionNames = getSectionOfType<StringTableSection>(
+  SectionNames = SecTable.getSectionOfType<StringTableSection>(
       Ehdr.e_shstrndx,
       "e_shstrndx field value " + Twine(Ehdr.e_shstrndx) + " in elf header " +
           " is invalid",
       "e_shstrndx field value " + Twine(Ehdr.e_shstrndx) + " in elf header " +
           " is not a string table");
 }
 
 template <class ELFT>