[ELF] - Store PhdrEntry values by pointers instead of storing by value.

[grimar]

Summary

Patch changes vector to store PhdrEntry by pointers,
at least 2 patches already tried to rely on this change: D35680 and D34956.

I think that happens because it is generally more convinent way,
so even if we will never land these patches it is still usefull to do IMO.

But if we land it I'll be able to rebase those ones to exclude unrelative changes.

[ruiu]

This doesn't really seem to improve the code, although it probably won't decrease the quality as well. I looked at your two other patches, but I cannot find why they need this change.

ELF/LinkerScript.cpp

801–806

FirstPTLoad is not a type that you can handle easily as it needs to dereference twice. You can do this.

auto It = llvm::find_if(...);
if (It == Phdrs.end())
  return;
PhdrEntry *FirstPTLoad = *It;

[grimar]

• Addressed review comment.

[grimar]

In D35832#820468, @ruiu wrote:

I looked at your two other patches, but I cannot find why they need this change.

For example in D35680 I am storing pointer to PT_LOAD segment instead of pointer to output section.
When we have vector of PhdrEntry (by value) it is hard to work with pointers on elements, because resize/remove operations
applied to vector may/will break the pointer value. If we have vector of pointers instead, it solves this problem easily.

ELF/LinkerScript.cpp

801–806

Yes, this looks better.

[ruiu]

LGTM

ELF/LinkerScript.h

public:
   bool isDefined(StringRef S);
 
   void fabricateDefaultCommands();
   void addOrphanSections(OutputSectionFactory &Factory);
   void removeEmptyCommands();
   void adjustSectionsBeforeSorting();
   void adjustSectionsAfterSorting();
 
-  std::vector<PhdrEntry> createPhdrs();
+  std::vector<PhdrEntry *> createPhdrs();
   bool ignoreInterpSection();
 
   bool shouldKeep(InputSectionBase *S);
   void assignOffsets(OutputSectionCommand *Cmd);
   void assignAddresses();
-  void allocateHeaders(std::vector<PhdrEntry> &Phdrs);
+  void allocateHeaders(std::vector<PhdrEntry *> &Phdrs);
   void addSymbol(SymbolAssignment *Cmd);
   void processCommands(OutputSectionFactory &Factory);
 
   // Parsed linker script configurations are set to this struct.
   ScriptConfiguration Opt;
 };
 
 extern LinkerScript *Script;
 
 } // end namespace elf
 } // end namespace lld
 
 #endif // LLD_ELF_LINKER_SCRIPT_H

ELF/LinkerScript.cpp

for (BaseCommand *Base : Opt.Commands) {
     } else {
       DefPhdrs = Cmd->Phdrs;
     }
   }
 
   removeEmptyCommands();
 }
 
-void LinkerScript::allocateHeaders(std::vector<PhdrEntry> &Phdrs) {
+void LinkerScript::allocateHeaders(std::vector<PhdrEntry *> &Phdrs) {
   uint64_t Min = std::numeric_limits<uint64_t>::max();
   for (OutputSectionCommand *Cmd : OutputSectionCommands) {
     OutputSection *Sec = Cmd->Sec;
     if (Sec->Flags & SHF_ALLOC)
       Min = std::min<uint64_t>(Min, Sec->Addr);
   }
 
-  auto FirstPTLoad = llvm::find_if(
-      Phdrs, [](const PhdrEntry &E) { return E.p_type == PT_LOAD; });
-  if (FirstPTLoad == Phdrs.end())
+  auto It = llvm::find_if(
+      Phdrs, [](const PhdrEntry *E) { return E->p_type == PT_LOAD; });
+  if (It == Phdrs.end())
     return;
+  PhdrEntry *FirstPTLoad = *It;
 
   uint64_t HeaderSize = getHeaderSize();
   if (HeaderSize <= Min || Script->hasPhdrsCommands()) {
     Min = alignDown(Min - HeaderSize, Config->MaxPageSize);
     Out::ElfHeader->Addr = Min;
     Out::ProgramHeaders->Addr = Min + Out::ElfHeader->Size;
     return;
   }
void LinkerScript::allocateHeaders(std::vector<PhdrEntry *> &Phdrs) {
   if (ActualFirst) {
     for (OutputSectionCommand *Cmd : OutputSectionCommands) {
       OutputSection *Sec = Cmd->Sec;
       if (Sec->FirstInPtLoad == Out::ElfHeader)
         Sec->FirstInPtLoad = ActualFirst;
     }
     FirstPTLoad->First = ActualFirst;
   } else {
-    Phdrs.erase(FirstPTLoad);
+    Phdrs.erase(It);
   }
 
   auto PhdrI = llvm::find_if(
-      Phdrs, [](const PhdrEntry &E) { return E.p_type == PT_PHDR; });
+      Phdrs, [](const PhdrEntry *E) { return E->p_type == PT_PHDR; });
   if (PhdrI != Phdrs.end())
     Phdrs.erase(PhdrI);
 }
 
 LinkerScript::AddressState::AddressState(const ScriptConfiguration &Opt) {
   for (auto &MRI : Opt.MemoryRegions) {
     const MemoryRegion *MR = &MRI.second;
     MemRegionOffset[MR] = MR->Origin;
for (BaseCommand *Base : Opt.Commands) {
 
     auto *Cmd = cast<OutputSectionCommand>(Base);
     assignOffsets(Cmd);
   }
   CurAddressState = nullptr;
 }
 
 // Creates program headers as instructed by PHDRS linker script command.
-std::vector<PhdrEntry> LinkerScript::createPhdrs() {
-  std::vector<PhdrEntry> Ret;
+std::vector<PhdrEntry *> LinkerScript::createPhdrs() {
+  std::vector<PhdrEntry *> Ret;
 
   // Process PHDRS and FILEHDR keywords because they are not
   // real output sections and cannot be added in the following loop.
   for (const PhdrsCommand &Cmd : Opt.PhdrsCommands) {
-    Ret.emplace_back(Cmd.Type, Cmd.Flags == UINT_MAX ? PF_R : Cmd.Flags);
-    PhdrEntry &Phdr = Ret.back();
+    PhdrEntry *Phdr =
+        make<PhdrEntry>(Cmd.Type, Cmd.Flags == UINT_MAX ? PF_R : Cmd.Flags);
 
     if (Cmd.HasFilehdr)
-      Phdr.add(Out::ElfHeader);
+      Phdr->add(Out::ElfHeader);
     if (Cmd.HasPhdrs)
-      Phdr.add(Out::ProgramHeaders);
+      Phdr->add(Out::ProgramHeaders);
 
     if (Cmd.LMAExpr) {
-      Phdr.p_paddr = Cmd.LMAExpr().getValue();
-      Phdr.HasLMA = true;
+      Phdr->p_paddr = Cmd.LMAExpr().getValue();
+      Phdr->HasLMA = true;
     }
+    Ret.push_back(Phdr);
   }
 
   // Add output sections to program headers.
   for (OutputSectionCommand *Cmd : OutputSectionCommands) {
     // Assign headers specified by linker script
     for (size_t Id : getPhdrIndices(Cmd)) {
       OutputSection *Sec = Cmd->Sec;
-      Ret[Id].add(Sec);
+      Ret[Id]->add(Sec);
       if (Opt.PhdrsCommands[Id].Flags == UINT_MAX)
-        Ret[Id].p_flags |= Sec->getPhdrFlags();
+        Ret[Id]->p_flags |= Sec->getPhdrFlags();
     }
   }
   return Ret;
 }
 
 bool LinkerScript::ignoreInterpSection() {
   // Ignore .interp section in case we have PHDRS specification
   // and PT_INTERP isn't listed.

ELF/Writer.cpp

private:
   void addSectionSymbols();
   void addReservedSymbols();
   void createSections();
   void forEachRelSec(std::function<void(InputSectionBase &)> Fn);
   void sortSections();
   void finalizeSections();
   void addPredefinedSections();
 
-  std::vector<PhdrEntry> createPhdrs();
+  std::vector<PhdrEntry *> createPhdrs();
   void removeEmptyPTLoad();
-  void addPtArmExid(std::vector<PhdrEntry> &Phdrs);
+  void addPtArmExid(std::vector<PhdrEntry *> &Phdrs);
   void assignFileOffsets();
   void assignFileOffsetsBinary();
   void setPhdrs();
   void fixSectionAlignments();
   void fixPredefinedSymbols();
   void openFile();
   void writeHeader();
   void writeSections();
   void writeSectionsBinary();
   void writeBuildId();
 
   std::unique_ptr<FileOutputBuffer> Buffer;
 
   OutputSectionFactory Factory;
 
   void addRelIpltSymbols();
   void addStartEndSymbols();
   void addStartStopSymbols(OutputSection *Sec);
   uint64_t getEntryAddr();
   OutputSection *findSectionInScript(StringRef Name);
   OutputSectionCommand *findSectionCommand(StringRef Name);
 
-  std::vector<PhdrEntry> Phdrs;
+  std::vector<PhdrEntry *> Phdrs;
 
   uint64_t FileSize;
   uint64_t SectionHeaderOff;
 
   bool HasGotBaseSym = false;
 };
 } // anonymous namespace
 

 template <class ELFT> static bool needsInterpSection() {
   return !Symtab<ELFT>::X->getSharedFiles().empty() &&
          !Config->DynamicLinker.empty() && !Script->ignoreInterpSection();
 }
 
 template <class ELFT> void elf::writeResult() { Writer<ELFT>().run(); }
 
 template <class ELFT> void Writer<ELFT>::removeEmptyPTLoad() {
-  auto I = std::remove_if(Phdrs.begin(), Phdrs.end(), [&](const PhdrEntry &P) {
-    if (P.p_type != PT_LOAD)
+  auto I = llvm::remove_if(Phdrs, [&](const PhdrEntry *P) {
+    if (P->p_type != PT_LOAD)
       return false;
-    if (!P.First)
+    if (!P->First)
       return true;
-    uint64_t Size = P.Last->Addr + P.Last->Size - P.First->Addr;
+    uint64_t Size = P->Last->Addr + P->Last->Size - P->First->Addr;
     return Size == 0;
   });
   Phdrs.erase(I, Phdrs.end());
 }
 
 template <class ELFT> static void combineEhFrameSections() {
   for (InputSectionBase *&S : InputSections) {
     EhInputSection *ES = dyn_cast<EhInputSection>(S);
if (Config->Omagic)
     return PF_R | PF_W | PF_X;
   if (Config->SingleRoRx && !(Flags & PF_W))
     return Flags | PF_X;
   return Flags;
 }
 
 // Decide which program headers to create and which sections to include in each
 // one.
-template <class ELFT> std::vector<PhdrEntry> Writer<ELFT>::createPhdrs() {
-  std::vector<PhdrEntry> Ret;
+template <class ELFT> std::vector<PhdrEntry *> Writer<ELFT>::createPhdrs() {
+  std::vector<PhdrEntry *> Ret;
   auto AddHdr = [&](unsigned Type, unsigned Flags) -> PhdrEntry * {
-    Ret.emplace_back(Type, Flags);
-    return &Ret.back();
+    Ret.push_back(make<PhdrEntry>(Type, Flags));
+    return Ret.back();
   };
 
   // The first phdr entry is PT_PHDR which describes the program header itself.
   AddHdr(PT_PHDR, PF_R)->add(Out::ProgramHeaders);
 
   // PT_INTERP must be the second entry if exists.
   if (OutputSection *Sec = findSectionInScript(".interp"))
     AddHdr(PT_INTERP, Sec->getPhdrFlags())->add(Sec);
if (Cmd->LMAExpr || Flags != NewFlags) {
       Load = AddHdr(PT_LOAD, NewFlags);
       Flags = NewFlags;
     }
 
     Load->add(Sec);
   }
 
   // Add a TLS segment if any.
-  PhdrEntry TlsHdr(PT_TLS, PF_R);
+  PhdrEntry *TlsHdr = make<PhdrEntry>(PT_TLS, PF_R);
   for (OutputSectionCommand *Cmd : OutputSectionCommands) {
     OutputSection *Sec = Cmd->Sec;
     if (Sec->Flags & SHF_TLS)
-      TlsHdr.add(Sec);
+      TlsHdr->add(Sec);
   }
-  if (TlsHdr.First)
-    Ret.push_back(std::move(TlsHdr));
+  if (TlsHdr->First)
+    Ret.push_back(TlsHdr);
 
   // Add an entry for .dynamic.
   if (InX::DynSymTab)
     AddHdr(PT_DYNAMIC, InX::Dynamic->getParent()->getPhdrFlags())
         ->add(InX::Dynamic->getParent());
 
   // PT_GNU_RELRO includes all sections that should be marked as
   // read-only by dynamic linker after proccessing relocations.
-  PhdrEntry RelRo(PT_GNU_RELRO, PF_R);
+  PhdrEntry *RelRo = make<PhdrEntry>(PT_GNU_RELRO, PF_R);
   for (OutputSectionCommand *Cmd : OutputSectionCommands) {
     OutputSection *Sec = Cmd->Sec;
     if (needsPtLoad(Sec) && isRelroSection(Sec))
-      RelRo.add(Sec);
+      RelRo->add(Sec);
   }
-  if (RelRo.First)
-    Ret.push_back(std::move(RelRo));
+  if (RelRo->First)
+    Ret.push_back(RelRo);
 
   // PT_GNU_EH_FRAME is a special section pointing on .eh_frame_hdr.
   if (!In<ELFT>::EhFrame->empty() && In<ELFT>::EhFrameHdr &&
       In<ELFT>::EhFrame->getParent() && In<ELFT>::EhFrameHdr->getParent())
     AddHdr(PT_GNU_EH_FRAME, In<ELFT>::EhFrameHdr->getParent()->getPhdrFlags())
         ->add(In<ELFT>::EhFrameHdr->getParent());
 
   // PT_OPENBSD_RANDOMIZE is an OpenBSD-specific feature. That makes
for (OutputSectionCommand *Cmd : OutputSectionCommands) {
     } else {
       Note = nullptr;
     }
   }
   return Ret;
 }
 
 template <class ELFT>
-void Writer<ELFT>::addPtArmExid(std::vector<PhdrEntry> &Phdrs) {
+void Writer<ELFT>::addPtArmExid(std::vector<PhdrEntry *> &Phdrs) {
   if (Config->EMachine != EM_ARM)
     return;
   auto I = llvm::find_if(OutputSectionCommands, [](OutputSectionCommand *Cmd) {
     return Cmd->Sec->Type == SHT_ARM_EXIDX;
   });
   if (I == OutputSectionCommands.end())
     return;
 
   // PT_ARM_EXIDX is the ARM EHABI equivalent of PT_GNU_EH_FRAME
-  PhdrEntry ARMExidx(PT_ARM_EXIDX, PF_R);
-  ARMExidx.add((*I)->Sec);
+  PhdrEntry *ARMExidx = make<PhdrEntry>(PT_ARM_EXIDX, PF_R);
+  ARMExidx->add((*I)->Sec);
   Phdrs.push_back(ARMExidx);
 }
 
 // The first section of each PT_LOAD, the first section in PT_GNU_RELRO and the
 // first section after PT_GNU_RELRO have to be page aligned so that the dynamic
 // linker can set the permissions.
 template <class ELFT> void Writer<ELFT>::fixSectionAlignments() {
   auto PageAlign = [](OutputSection *Sec) {
     OutputSectionCommand *Cmd = Script->getCmd(Sec);
     if (Cmd && !Cmd->AddrExpr)
       Cmd->AddrExpr = [=] {
         return alignTo(Script->getDot(), Config->MaxPageSize);
       };
   };
 
-  for (const PhdrEntry &P : Phdrs)
-    if (P.p_type == PT_LOAD && P.First)
-      PageAlign(P.First);
+  for (const PhdrEntry *P : Phdrs)
+    if (P->p_type == PT_LOAD && P->First)
+      PageAlign(P->First);
 
-  for (const PhdrEntry &P : Phdrs) {
-    if (P.p_type != PT_GNU_RELRO)
+  for (const PhdrEntry *P : Phdrs) {
+    if (P->p_type != PT_GNU_RELRO)
       continue;
-    if (P.First)
-      PageAlign(P.First);
+    if (P->First)
+      PageAlign(P->First);
     // Find the first section after PT_GNU_RELRO. If it is in a PT_LOAD we
     // have to align it to a page.
     auto End = OutputSectionCommands.end();
     auto I =
-        std::find(OutputSectionCommands.begin(), End, Script->getCmd(P.Last));
+        std::find(OutputSectionCommands.begin(), End, Script->getCmd(P->Last));
     if (I == End || (I + 1) == End)
       continue;
     OutputSection *Sec = (*(I + 1))->Sec;
     if (needsPtLoad(Sec))
       PageAlign(Sec);
   }
 }
 
template <class ELFT> void Writer<ELFT>::assignFileOffsets() {
   SectionHeaderOff = alignTo(Off, Config->Wordsize);
   FileSize =
       SectionHeaderOff + (OutputSectionCommands.size() + 1) * sizeof(Elf_Shdr);
 }
 
 // Finalize the program headers. We call this function after we assign
 // file offsets and VAs to all sections.
 template <class ELFT> void Writer<ELFT>::setPhdrs() {
-  for (PhdrEntry &P : Phdrs) {
-    OutputSection *First = P.First;
-    OutputSection *Last = P.Last;
+  for (PhdrEntry *P : Phdrs) {
+    OutputSection *First = P->First;
+    OutputSection *Last = P->Last;
     if (First) {
-      P.p_filesz = Last->Offset - First->Offset;
+      P->p_filesz = Last->Offset - First->Offset;
       if (Last->Type != SHT_NOBITS)
-        P.p_filesz += Last->Size;
-      P.p_memsz = Last->Addr + Last->Size - First->Addr;
-      P.p_offset = First->Offset;
-      P.p_vaddr = First->Addr;
-      if (!P.HasLMA)
-        P.p_paddr = First->getLMA();
-    }
-    if (P.p_type == PT_LOAD)
-      P.p_align = std::max<uint64_t>(P.p_align, Config->MaxPageSize);
-    else if (P.p_type == PT_GNU_RELRO) {
-      P.p_align = 1;
+        P->p_filesz += Last->Size;
+      P->p_memsz = Last->Addr + Last->Size - First->Addr;
+      P->p_offset = First->Offset;
+      P->p_vaddr = First->Addr;
+      if (!P->HasLMA)
+        P->p_paddr = First->getLMA();
+    }
+    if (P->p_type == PT_LOAD)
+      P->p_align = std::max<uint64_t>(P->p_align, Config->MaxPageSize);
+    else if (P->p_type == PT_GNU_RELRO) {
+      P->p_align = 1;
       // The glibc dynamic loader rounds the size down, so we need to round up
       // to protect the last page. This is a no-op on FreeBSD which always
       // rounds up.
-      P.p_memsz = alignTo(P.p_memsz, Target->PageSize);
+      P->p_memsz = alignTo(P->p_memsz, Target->PageSize);
     }
 
     // The TLS pointer goes after PT_TLS. At least glibc will align it,
     // so round up the size to make sure the offsets are correct.
-    if (P.p_type == PT_TLS) {
-      Out::TlsPhdr = &P;
-      if (P.p_memsz)
-        P.p_memsz = alignTo(P.p_memsz, P.p_align);
+    if (P->p_type == PT_TLS) {
+      Out::TlsPhdr = P;
+      if (P->p_memsz)
+        P->p_memsz = alignTo(P->p_memsz, P->p_align);
     }
   }
 }
 
 // The entry point address is chosen in the following ways.
 //
 // 1. the '-e' entry command-line option;
 // 2. the ENTRY(symbol) command in a linker control script;

 // symbol values that depend on section address and size.
 template <class ELFT> void Writer<ELFT>::fixPredefinedSymbols() {
   // _etext is the first location after the last read-only loadable segment.
   // _edata is the first location after the last read-write loadable segment.
   // _end is the first location after the uninitialized data region.
   PhdrEntry *Last = nullptr;
   PhdrEntry *LastRO = nullptr;
   PhdrEntry *LastRW = nullptr;
-  for (PhdrEntry &P : Phdrs) {
-    if (P.p_type != PT_LOAD)
+  for (PhdrEntry *P : Phdrs) {
+    if (P->p_type != PT_LOAD)
       continue;
-    Last = &P;
-    if (P.p_flags & PF_W)
-      LastRW = &P;
+    Last = P;
+    if (P->p_flags & PF_W)
+      LastRW = P;
     else
-      LastRO = &P;
+      LastRO = P;
   }
 
   auto Set = [](DefinedRegular *S, OutputSection *Sec, uint64_t Value) {
     if (S) {
       S->Section = Sec;
       S->Value = Value;
     }
   };
template <class ELFT> void Writer<ELFT>::writeHeader() {
 
   if (!Config->Relocatable) {
     EHdr->e_phoff = sizeof(Elf_Ehdr);
     EHdr->e_phentsize = sizeof(Elf_Phdr);
   }
 
   // Write the program header table.
   auto *HBuf = reinterpret_cast<Elf_Phdr *>(Buf + EHdr->e_phoff);
-  for (PhdrEntry &P : Phdrs) {
-    HBuf->p_type = P.p_type;
-    HBuf->p_flags = P.p_flags;
-    HBuf->p_offset = P.p_offset;
-    HBuf->p_vaddr = P.p_vaddr;
-    HBuf->p_paddr = P.p_paddr;
-    HBuf->p_filesz = P.p_filesz;
-    HBuf->p_memsz = P.p_memsz;
-    HBuf->p_align = P.p_align;
+  for (PhdrEntry *P : Phdrs) {
+    HBuf->p_type = P->p_type;
+    HBuf->p_flags = P->p_flags;
+    HBuf->p_offset = P->p_offset;
+    HBuf->p_vaddr = P->p_vaddr;
+    HBuf->p_paddr = P->p_paddr;
+    HBuf->p_filesz = P->p_filesz;
+    HBuf->p_memsz = P->p_memsz;
+    HBuf->p_align = P->p_align;
     ++HBuf;
   }
 
   // Write the section header table. Note that the first table entry is null.
   auto *SHdrs = reinterpret_cast<Elf_Shdr *>(Buf + EHdr->e_shoff);
   for (OutputSectionCommand *Cmd : OutputSectionCommands)
     Cmd->Sec->writeHeaderTo<ELFT>(++SHdrs);
 }