[ELF] - Store pointer to PT_LOAD instead of pointer to first section (FirstInPtLoad) in OutputSection

[grimar]

Summary

I found that it is probably a bit more convinent and helps to simplify logic
of program headers allocation a little.

[ruiu]

ELF/LinkerScript.cpp

753–754

This change seems fine, but I realized that I do not understand what this function is supposed to do. What is this for?

[grimar]

ELF/LinkerScript.cpp

753–754

Function checks if elf header and program header can be allocated.
If there is no room for them, PT_PHDR should not be emited.

See:
PT_PHDR description:
"Specifies the location and size of the program header table itself, both in the file and in the memory image of the program. This segment type cannot occur more than once in a file. Moreover, it can occur only if the program header table is part of the memory image of the program." (https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-83432/index.html)

Imagine we have script like:
SECTIONS {foo 0 : {*(foo*)} } (took it from no-space.s testcase)

In that case there is no VA space for headers. That means we do not allocate them in PT_LOAD. Output will be:

Program Headers:
Type  Offset   VirtAddr           PhysAddr
LOAD  0x001000 0x0000000000000000 0x0000000000000000

[ruiu]

ELF/LinkerScript.cpp

753–754

There's no rule or something that says a program header must be at beginning of a file, right? If there's no space at beginning of a file for a program header, why don't you put it at end of the file?

[grimar]

ELF/LinkerScript.cpp

753–754

But why would we want to do that ? Neither bfd or gold allocate program headers in this case I think, why should we ?

Spec says about PT_PHDR: "This type, if present, must precede any loadable segment entry." (https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-83432/index.html).
In that case we would need probably to place program headers at the end of last segment and move PT_PHDR before it. Additional complication for no visible reason probably.

Also I think if script is writen in the way when there is no room for headers it may assume them are not allocated.
Script may include asserts of location counter or something that may be broken after this change.

And the last - there is still ELF file header there which must be placed at the begining of the file and we can't move it anyways.

Basing on all above I believe moving of program headers at least does not make sense (or even may be harmfull).

Just in case, this patch does not change any logic, only simplifies the existent code.

[grimar]

• Rebased.

[phosek]

ELF/LinkerScript.cpp

753–754

We actually ran into this in our kernel: we're using a custom linker script to avoid emitting ELF file or program headers because we don't want them in the kernel image. Since the kernel doesn't start at the beginning of the file, there's always a space so LLD always emits those headers unlike BFD ld or gold.

I have a WIP change that avoids emitting the headers if there's no output section covering them which is the behavior BFD ld and gold use.

[grimar]

• Rebased.

[grimar]

ping

[grimar]

• Rebased.

[ruiu]

ELF/OutputSections.h

71

Load is not a good name, no? Phdr is better.

ELF/Writer.cpp

1584–1587

Is it possible that Cmd->Load exists but Cmd->Load->First is null? If not, simplify this to:

if (!Cmd->Load)
  return alignTo(Off, Cmd->Alignment):

[grimar]

• Addressed review comments.

ELF/OutputSections.h

71

Phdr stands for "program header", there are lot of them, isn't it too generic name ?
We want to store pointer to loadable segment section resides in, so may be LoadPhdr or PtLoad
is better ?

ELF/Writer.cpp

1584–1587

Done.

[ruiu]

ELF/LinkerScript.cpp

730

I'd name this findSection as it finds a given argument from a list. "get" prefix doesn't feel perfectly match to what this function does.

ELF/OutputSections.h

71

I think I prefer PtLoad over LoadPhdr.

[grimar]

• Addressed comments.

ELF/LinkerScript.cpp

730

Did you mean findFirstSection ? (as it finds first section in a given Load)

ELF/OutputSections.h

71

Renamed to PtLoad.

[ruiu]

LGTM

ELF/LinkerScript.cpp

774

First is too terse. The member should be renamed FirstSec in a follow-up patch.

[grimar]

ELF/LinkerScript.cpp

774

Will do, thanks for review !

ELF/InputSection.cpp

 // The ARM ABI defines base to be "addressing origin of the output segment
 // defining the symbol S". We defined the "addressing origin"/static base to be
 // the base of the PT_LOAD segment containing the Body.
 // The procedure call standard only defines a Read Write Position Independent
 // RWPI variant so in practice we should expect the static base to be the base
 // of the RW segment.
 static uint64_t getARMStaticBase(const SymbolBody &Body) {
   OutputSection *OS = Body.getOutputSection();
-  if (!OS || !OS->FirstInPtLoad)
+  if (!OS || !OS->Load || !OS->Load->First)
     fatal("SBREL relocation to " + Body.getName() + " without static base");
-  return OS->FirstInPtLoad->Addr;
+  return OS->Load->First->Addr;
 }
 
 static uint64_t getRelocTargetVA(uint32_t Type, int64_t A, uint64_t P,
                                  const SymbolBody &Body, RelExpr Expr) {
   switch (Expr) {
   case R_ABS:
   case R_RELAX_GOT_PC_NOPIC:
     return Body.getVA(A);

ELF/LinkerScript.cpp

for (BaseCommand *Base : Opt.Commands) {
     } else {
       DefPhdrs = Sec->Phdrs;
     }
   }
 
   removeEmptyCommands();
 }
 
+static OutputSection *getFirstSection(PhdrEntry *Load) {
+  for (OutputSection *Sec : OutputSections)
+    if (Sec->Load == Load)
+      return Sec;
+  return nullptr;
+}
+
 // Try to find an address for the file and program headers output sections,
 // which were unconditionally added to the first PT_LOAD segment earlier.
 //
 // When using the default layout, we check if the headers fit below the first
 // allocated section. When using a linker script, we also check if the headers
 // are covered by the output section. This allows omitting the headers by not
 // leaving enough space for them in the linker script; this pattern is common
 // in embedded systems.
 //
 // If there isn't enough space for these sections, we'll remove them from the
 // PT_LOAD segment, and we'll also remove the PT_PHDR segment.
 void LinkerScript::allocateHeaders(std::vector<PhdrEntry *> &Phdrs) {
   uint64_t Min = std::numeric_limits<uint64_t>::max();
   for (OutputSection *Sec : OutputSections)
     if (Sec->Flags & SHF_ALLOC)
       Min = std::min<uint64_t>(Min, Sec->Addr);
 
   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();
   // When linker script with SECTIONS is being used, don't output headers
   // unless there's a space for them.
   uint64_t Base = Opt.HasSections ? alignDown(Min, Config->MaxPageSize) : 0;
   if (HeaderSize <= Min - Base || Script->hasPhdrsCommands()) {
     Min = Opt.HasSections ? Base
                           : alignDown(Min - HeaderSize, Config->MaxPageSize);
     Out::ElfHeader->Addr = Min;
     Out::ProgramHeaders->Addr = Min + Out::ElfHeader->Size;
     return;
   }
 
-  OutputSection *ActualFirst = nullptr;
-  for (OutputSection *Sec : OutputSections) {
-    if (Sec->FirstInPtLoad == Out::ElfHeader) {
-      ActualFirst = Sec;
-      break;
-    }
-  }
-  if (ActualFirst) {
-    for (OutputSection *Sec : OutputSections)
-      if (Sec->FirstInPtLoad == Out::ElfHeader)
-        Sec->FirstInPtLoad = ActualFirst;
-    FirstPTLoad->First = ActualFirst;
-  } else {
-    Phdrs.erase(It);
-  }
+  Out::ElfHeader->Load = nullptr;
+  Out::ProgramHeaders->Load = nullptr;
+  FirstPTLoad->First = getFirstSection(FirstPTLoad);
 
   auto PhdrI = llvm::find_if(
       Phdrs, [](const PhdrEntry *E) { return E->p_type == PT_PHDR; });
   if (PhdrI != Phdrs.end())
     Phdrs.erase(PhdrI);
 }
 
 LinkerScript::AddressState::AddressState(const ScriptConfiguration &Opt) {

ELF/OutputSections.h

public:
 
   uint32_t getPhdrFlags() const;
 
   void updateAlignment(uint32_t Val) {
     if (Val > Alignment)
       Alignment = Val;
   }
 
-  // Pointer to the first section in PT_LOAD segment, which this section
-  // also resides in. This field is used to correctly compute file offset
-  // of a section. When two sections share the same load segment, difference
-  // between their file offsets should be equal to difference between their
-  // virtual addresses. To compute some section offset we use the following
-  // formula: Off = Off_first + VA - VA_first.
-  OutputSection *FirstInPtLoad = nullptr;
+  // Pointer to the PT_LOAD segment, which this section resides in. This field
+  // is used to correctly compute file offset of a section. When two sections
+  // share the same load segment, difference between their file offsets should
+  // be equal to difference between their virtual addresses. To compute some
+  // section offset we use the following formula: Off = Off_first + VA -
+  // VA_first, where Off_first and VA_first is file offset and VA of first
+  // section in PT_LOAD.
+  PhdrEntry *Load = nullptr;
 
   // Pointer to a relocation section for this section. Usually nullptr because
   // we consume relocations, but if --emit-relocs is specified (which is rare),
   // it may have a non-null value.
   OutputSection *RelocationSection = nullptr;
 
   // The following fields correspond to Elf_Shdr members.
   uint64_t Size = 0;

ELF/Writer.cpp

 }
 
 void PhdrEntry::add(OutputSection *Sec) {
   Last = Sec;
   if (!First)
     First = Sec;
   p_align = std::max(p_align, Sec->Alignment);
   if (p_type == PT_LOAD)
-    Sec->FirstInPtLoad = First;
+    Sec->Load = this;
 }
 
 template <class ELFT>
 static Symbol *addRegular(StringRef Name, SectionBase *Sec, uint64_t Value,
                           uint8_t StOther = STV_HIDDEN,
                           uint8_t Binding = STB_WEAK) {
   // The linker generated symbols are added as STB_WEAK to allow user defined
   // ones to override them.
template <class ELFT> void Writer<ELFT>::fixSectionAlignments() {
   }
 }
 
 // Adjusts the file alignment for a given output section and returns
 // its new file offset. The file offset must be the same with its
 // virtual address (modulo the page size) so that the loader can load
 // executables without any address adjustment.
 static uint64_t getFileAlignment(uint64_t Off, OutputSection *Cmd) {
-  OutputSection *First = Cmd->FirstInPtLoad;
+  OutputSection *First = Cmd->Load ? Cmd->Load->First : nullptr;
   // If the section is not in a PT_LOAD, we just have to align it.
   if (!First)
     return alignTo(Off, Cmd->Alignment);
 
   // The first section in a PT_LOAD has to have congruent offset and address
   // module the page size.
   if (Cmd == First)
     return alignTo(Off, std::max<uint64_t>(Cmd->Alignment, Config->MaxPageSize),
                    Cmd->Addr);
 
   // If two sections share the same PT_LOAD the file offset is calculated