[elf2] Combine adjacent compatible OutputSections in PT_LOADs.

[• rafael]

This doesn't build:

/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:289:9: error:
member initializer 'ProgramHeader' does not name a non-static data
member or base class

: ProgramHeader(PT_DYNAMIC,
  ^~~~~~~~~~~~~~~~~~~~~~~~~

/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:291:5: error: use
of undeclared identifier 'Header'

Header.p_offset = Sec->getFileOff();
^

...

[Bigcheese]

Rebase and fix compile issues on non-msvc.

[• rafael]

git-clang-format the patch

+ ProgramHeader(uintX_t p_type, uintX_t p_flags) : Closed(false) {

Used

bool Closed = false;

std::unique_ptr<llvm::FileOutputBuffer> Buffer;

+

llvm::SpecificBumpPtrAllocator<OutputSection<ELFT>> CAlloc;

Why?

+ void setFromSection(OutputSectionBase<Is64Bits> &Sec) {

Set what from section?

+ // Create a PHDR for the first output section which covers the file header.
+ PHDRs.push_back(new (PAlloc) ProgramHeader<ELFT::Is64Bits>(
+ PT_LOAD, convertSectionFlagsToPHDRFlags(OutputSections[0]->getFlags())));

This means that the file and program headers are now executable. While
that is the common default for elf linkers, it is undesirable in
security sensitive contexts. For now, just keep creating the first
PT_LOAD with just PF_R. We can change that (possible with an option)
on another patch.

+ if (Sec->getSize()) {

All sh_alloc segments come before all others, so it might be
possible/cleaner to write this with

if (outputSectionHasPHDR<ELFT>(Sec)) {

And just close the last program header out of the loop, no?

I noticed that this patch doesn't change compSec. This means that we
might alternate from, for example, PF_R to PF_R|PF_W back to PF_R and
create more segments than necessary. Changing this on another patch is
preferable to changing it on this one. I just wanted to point it out.

Cheers,
Rafael

[Bigcheese]

std::unique_ptr<llvm::FileOutputBuffer> Buffer;

+

llvm::SpecificBumpPtrAllocator<OutputSection<ELFT>> CAlloc;

Why?

Separating out logically related members.

+ if (Sec->getSize()) {

All sh_alloc segments come before all others, so it might be
possible/cleaner to write this with

if (outputSectionHasPHDR<ELFT>(Sec)) {

And just close the last program header out of the loop, no?

There are zero sized allocated sections (.data, .bss) in some tests that have different flags, and would create zero sized PHDRs. This ignores them.

Closing the last program header outside of the loop makes that PT_LOAD include all of the non SHF_ALLOC sections.

[• rafael]

test/elf2/basic.s

159 ↗

(On Diff #34746)

No point in checking the numeric value.

test/elf2/basic64be.s

152 ↗

(On Diff #34746)

Please make the witespace changes first if needed and rebase this patch on top.

[Bigcheese]

Fix the default initializer.

[Bigcheese]

Remove accidental test changes.

[• rafael]

This now needs more tests.

The main thing this patch does is put different output sections into
the same program header. Add a new test that checks for just that:

The file has two sections that cannot possibly be in the section
output section (different names for example).

The sections have the same flags, so they end up in the same program header.

Cheers,
Rafael

[Bigcheese]

Add tests and rebase.

[ruiu]

ELF/Writer.cpp

397–404

Can this just be *PHDR = Header?

[Bigcheese]

ELF/Writer.cpp

397–404

No, they are different types.

test/elf2/basic64be.s

152–175 ↗

(On Diff #34751)

Ah, that wasn't supposed to happen.

[• rafael]

ELF/Writer.cpp

823

This creates an output section even if the section is empty.

833–853

This does not. We should to treat the first section specially. What I would suggest is to have a "ProgramHeader *LastHeader" variable and initialize it to the file header program header.

In the loop you can check if the current section is compatible with the previous one. If not, create a new one and add to LastHeader.

Note in particular that if the first section that needs a program header is read only, the original program header gets extended.

test/elf2/program-header-layout.s

4

It doesn't fit in 80 columns?

[• rafael]

This does not. We should to treat the first section specially. What I would suggest is to have a "ProgramHeader *LastHeader" variable and initialize it to the file header program header.

We should *not* treat ...

[Bigcheese]

Don't treat the first PHDR specially.

[• rafael]

LGTM on the condition that one extra test is added showing a read only section being included in the initial PT_LOAD.

[• rafael]

And please update cmpSec in a followup patch.

[Eugene.Zelenko]

Committed in rL247925.

ELF/Writer.cpp

 
 private:
   HashTableSection<ELFT> &HashSec;
   SymbolTableSection<ELFT> &DynSymSec;
   StringTableSection<ELFT::Is64Bits> &DynStrSec;
   SymbolTable &SymTab;
 };
 
+static uint32_t convertSectionFlagsToPHDRFlags(uint64_t Flags) {
+  uint32_t Ret = PF_R;
+  if (Flags & SHF_WRITE)
+    Ret |= PF_W;
+
+  if (Flags & SHF_EXECINSTR)
+    Ret |= PF_X;
+
+  return Ret;
+}
+
+template <bool Is64Bits>
+class ProgramHeader {
+public:
+  typedef typename std::conditional<Is64Bits, uint64_t, uint32_t>::type uintX_t;
+  typedef
+    typename std::conditional<Is64Bits, Elf64_Phdr, Elf32_Phdr>::type HeaderT;
+
+  ProgramHeader(uintX_t p_type, uintX_t p_flags) {
+    std::memset(&Header, 0, sizeof(HeaderT));
+    Header.p_type = p_type;
+    Header.p_flags = p_flags;
+    Header.p_align = PageSize;
+  }
+
+  void setValuesFromSection(OutputSectionBase<Is64Bits> &Sec) {
+    Header.p_flags = convertSectionFlagsToPHDRFlags(Sec.getFlags());
+    Header.p_offset = Sec.getFileOff();
+    Header.p_vaddr = Sec.getVA();
+    Header.p_paddr = Header.p_vaddr;
+    Header.p_filesz = Sec.getSize();
+    Header.p_memsz = Header.p_filesz;
+    Header.p_align = Sec.getAlign();
+  }
+
+  template <endianness E>
+  void writeHeaderTo(typename ELFFile<ELFType<E, Is64Bits>>::Elf_Phdr *PHDR) {
+    PHDR->p_type = Header.p_type;
+    PHDR->p_flags = Header.p_flags;
+    PHDR->p_offset = Header.p_offset;
+    PHDR->p_vaddr = Header.p_vaddr;
+    PHDR->p_paddr = Header.p_paddr;
+    PHDR->p_filesz = Header.p_filesz;
+    PHDR->p_memsz = Header.p_memsz;
+    PHDR->p_align = Header.p_align;
+  }
+
+  HeaderT Header;
+  bool Closed = false;
+};
+
 // The writer writes a SymbolTable result to a file.
 template <class ELFT> class Writer {
 public:
   typedef typename ELFFile<ELFT>::uintX_t uintX_t;
   typedef typename ELFFile<ELFT>::Elf_Shdr Elf_Shdr;
   typedef typename ELFFile<ELFT>::Elf_Ehdr Elf_Ehdr;
   typedef typename ELFFile<ELFT>::Elf_Phdr Elf_Phdr;
   typedef typename ELFFile<ELFT>::Elf_Sym Elf_Sym;
return !SymTabSec.getSymTable().getSharedFiles().empty() &&
            !Config->DynamicLinker.empty();
   }
   bool needsDynamicSections() const {
     return !SymTabSec.getSymTable().getSharedFiles().empty() || Config->Shared;
   }
   unsigned getVAStart() const { return Config->Shared ? 0 : VAStart; }
 
   std::unique_ptr<llvm::FileOutputBuffer> Buffer;
+
   llvm::SpecificBumpPtrAllocator<OutputSection<ELFT>> CAlloc;
   std::vector<OutputSectionBase<ELFT::Is64Bits> *> OutputSections;
   unsigned getNumSections() const { return OutputSections.size() + 1; }
 
+  llvm::BumpPtrAllocator PAlloc;
+  std::vector<ProgramHeader<ELFT::Is64Bits> *> PHDRs;
+  ProgramHeader<ELFT::Is64Bits> FileHeaderPHDR{PT_LOAD, PF_R};
+  ProgramHeader<ELFT::Is64Bits> InterpPHDR{PT_INTERP, 0};
+  ProgramHeader<ELFT::Is64Bits> DynamicPHDR{PT_DYNAMIC, 0};
+
   uintX_t FileSize;
   uintX_t ProgramHeaderOff;
   uintX_t SectionHeaderOff;
-  unsigned NumPhdrs;
 
   StringTableSection<ELFT::Is64Bits> StrTabSec = { /*dynamic=*/false };
   StringTableSection<ELFT::Is64Bits> DynStrSec = { /*dynamic=*/true };
 
   SymbolTableSection<ELFT> SymTabSec;
   SymbolTableSection<ELFT> DynSymSec;
 
   HashTableSection<ELFT> HashSec;
template <class ELFT> void Writer<ELFT>::createSections() {
   std::stable_sort(OutputSections.begin(), OutputSections.end(),
                    compSec<ELFT::Is64Bits>);
   for (unsigned I = 0, N = OutputSections.size(); I < N; ++I)
     OutputSections[I]->setSectionIndex(I + 1);
 }
 
 template <class ELFT>
 static bool outputSectionHasPHDR(OutputSectionBase<ELFT::Is64Bits> *Sec) {
-  return (Sec->getSize() != 0) && (Sec->getFlags() & SHF_ALLOC);
+  return Sec->getFlags() & SHF_ALLOC;
 }
 
 // Visits all sections to assign incremental, non-overlapping RVAs and
 // file offsets.
 template <class ELFT> void Writer<ELFT>::assignAddresses() {
+  assert(!OutputSections.empty() && "No output sections to layout!");
   uintX_t VA = getVAStart();
   uintX_t FileOff = 0;
 
   FileOff += sizeof(Elf_Ehdr);
   VA += sizeof(Elf_Ehdr);
 
   // Reserve space for PHDRs.
   ProgramHeaderOff = FileOff;
   FileOff = RoundUpToAlignment(FileOff, PageSize);
   VA = RoundUpToAlignment(VA, PageSize);
 
-  NumPhdrs = 0;
-
-  // Add a PHDR for PT_INTERP.
   if (needsInterpSection())
-    ++NumPhdrs;
+    PHDRs.push_back(&InterpPHDR);
 
-  // Add a PHDR for the elf header and program headers. Some dynamic linkers
-  // (musl at least) require them to be covered by a PT_LOAD.
-  ++NumPhdrs;
+  ProgramHeader<ELFT::Is64Bits> *LastPHDR = &FileHeaderPHDR;
+  // Create a PHDR for the file header.
+  PHDRs.push_back(&FileHeaderPHDR);
+  FileHeaderPHDR.Header.p_vaddr = getVAStart();
+  FileHeaderPHDR.Header.p_paddr = getVAStart();
+  FileHeaderPHDR.Header.p_align = PageSize;
 
   for (OutputSectionBase<ELFT::Is64Bits> *Sec : OutputSections) {
     StrTabSec.add(Sec->getName());
     Sec->finalize();
 
-    // Since each output section gets its own PHDR, align each output section to
-    // a page.
+    if (Sec->getSize()) {
+      uintX_t Flags = convertSectionFlagsToPHDRFlags(Sec->getFlags());
+      if (LastPHDR->Header.p_flags != Flags ||
+          !outputSectionHasPHDR<ELFT>(Sec)) {
+        // Flags changed. End current PHDR and potentially create a new one.
+        if (!LastPHDR->Closed) {
+          LastPHDR->Header.p_filesz = FileOff - LastPHDR->Header.p_offset;
+          LastPHDR->Header.p_memsz = VA - LastPHDR->Header.p_vaddr;
+          LastPHDR->Closed = true;
+        }
+
         if (outputSectionHasPHDR<ELFT>(Sec)) {
-      ++NumPhdrs;
+          LastPHDR = new (PAlloc) ProgramHeader<ELFT::Is64Bits>(PT_LOAD, Flags);
+          PHDRs.push_back(LastPHDR);
           VA = RoundUpToAlignment(VA, PageSize);
           FileOff = RoundUpToAlignment(FileOff, PageSize);
+          LastPHDR->Header.p_offset = FileOff;
+          LastPHDR->Header.p_vaddr = VA;
+          LastPHDR->Header.p_paddr = VA;
+        }
+      }
     }
 
     uintX_t Align = Sec->getAlign();
     uintX_t Size = Sec->getSize();
     if (Sec->getFlags() & SHF_ALLOC) {
       VA = RoundUpToAlignment(VA, Align);
       Sec->setVA(VA);
       VA += Size;
     }
     FileOff = RoundUpToAlignment(FileOff, Align);
     Sec->setFileOffset(FileOff);
     if (Sec->getType() != SHT_NOBITS)
       FileOff += Size;
   }
 
   // Add a PHDR for the dynamic table.
   if (needsDynamicSections())
-    ++NumPhdrs;
+    PHDRs.push_back(&DynamicPHDR);
 
   FileOff += OffsetToAlignment(FileOff, ELFT::Is64Bits ? 8 : 4);
 
   // Add space for section headers.
   SectionHeaderOff = FileOff;
   FileOff += getNumSections() * sizeof(Elf_Shdr);
   FileSize = FileOff;
 }
 
-static uint32_t convertSectionFlagsToPHDRFlags(uint64_t Flags) {
-  uint32_t Ret = PF_R;
-  if (Flags & SHF_WRITE)
-    Ret |= PF_W;
-
-  if (Flags & SHF_EXECINSTR)
-    Ret |= PF_X;
-
-  return Ret;
-}
-
-template <class ELFT>
-static void setValuesFromSection(typename ELFFile<ELFT>::Elf_Phdr &P,
-                                 OutputSectionBase<ELFT::Is64Bits> &S) {
-  P.p_flags = convertSectionFlagsToPHDRFlags(S.getFlags());
-  P.p_offset = S.getFileOff();
-  P.p_vaddr = S.getVA();
-  P.p_paddr = P.p_vaddr;
-  P.p_filesz = S.getSize();
-  P.p_memsz = P.p_filesz;
-  P.p_align = S.getAlign();
-}
-
 template <class ELFT> void Writer<ELFT>::writeHeader() {
   uint8_t *Buf = Buffer->getBufferStart();
   auto *EHdr = reinterpret_cast<Elf_Ehdr *>(Buf);
   EHdr->e_ident[EI_MAG0] = 0x7F;
   EHdr->e_ident[EI_MAG1] = 0x45;
   EHdr->e_ident[EI_MAG2] = 0x4C;
   EHdr->e_ident[EI_MAG3] = 0x46;
   EHdr->e_ident[EI_CLASS] = ELFT::Is64Bits ? ELFCLASS64 : ELFCLASS32;
template <class ELFT> void Writer<ELFT>::writeHeader() {
   EHdr->e_machine = FirstObj.getEMachine();
   EHdr->e_version = EV_CURRENT;
   SymbolBody *Entry = Symtab.getEntrySym();
   EHdr->e_entry = Entry ? getSymVA(cast<DefinedRegular<ELFT>>(Entry)) : 0;
   EHdr->e_phoff = ProgramHeaderOff;
   EHdr->e_shoff = SectionHeaderOff;
   EHdr->e_ehsize = sizeof(Elf_Ehdr);
   EHdr->e_phentsize = sizeof(Elf_Phdr);
-  EHdr->e_phnum = NumPhdrs;
+  EHdr->e_phnum = PHDRs.size();
   EHdr->e_shentsize = sizeof(Elf_Shdr);
   EHdr->e_shnum = getNumSections();
   EHdr->e_shstrndx = StrTabSec.getSectionIndex();
 
-  auto PHdrs = reinterpret_cast<Elf_Phdr *>(Buf + EHdr->e_phoff);
-  if (needsInterpSection()) {
-    PHdrs->p_type = PT_INTERP;
-    setValuesFromSection<ELFT>(*PHdrs, InterpSec);
-    ++PHdrs;
-  }
-
-  PHdrs->p_type = PT_LOAD;
-  PHdrs->p_flags = PF_R;
-  PHdrs->p_offset = 0;
-  PHdrs->p_vaddr = getVAStart();
-  PHdrs->p_paddr = PHdrs->p_vaddr;
-  PHdrs->p_filesz = ProgramHeaderOff + NumPhdrs * sizeof(Elf_Phdr);
-  PHdrs->p_memsz = PHdrs->p_filesz;
-  PHdrs->p_align = PageSize;
-  ++PHdrs;
+  // If nothing was merged into the file header PT_LOAD, set the size correctly.
+  if (FileHeaderPHDR.Header.p_filesz == PageSize)
+    FileHeaderPHDR.Header.p_filesz = FileHeaderPHDR.Header.p_memsz =
+        sizeof(Elf_Ehdr) + sizeof(Elf_Phdr) * PHDRs.size();
 
-  for (OutputSectionBase<ELFT::Is64Bits> *Sec : OutputSections) {
-    if (!outputSectionHasPHDR<ELFT>(Sec))
-      continue;
-    PHdrs->p_type = PT_LOAD;
-    PHdrs->p_flags = convertSectionFlagsToPHDRFlags(Sec->getFlags());
-    PHdrs->p_offset = Sec->getFileOff();
-    PHdrs->p_vaddr = Sec->getVA();
-    PHdrs->p_paddr = PHdrs->p_vaddr;
-    PHdrs->p_filesz = Sec->getType() == SHT_NOBITS ? 0 : Sec->getSize();
-    PHdrs->p_memsz = Sec->getSize();
-    PHdrs->p_align = PageSize;
-    ++PHdrs;
-  }
+  if (needsInterpSection())
+    InterpPHDR.setValuesFromSection(InterpSec);
+  if (needsDynamicSections())
+    DynamicPHDR.setValuesFromSection(DynamicSec);
 
-  if (needsDynamicSections()) {
-    PHdrs->p_type = PT_DYNAMIC;
-    setValuesFromSection<ELFT>(*PHdrs, DynamicSec);
-  }
+  auto PHdrs = reinterpret_cast<Elf_Phdr *>(Buf + EHdr->e_phoff);
+  for (ProgramHeader<ELFT::Is64Bits> *PHDR : PHDRs)
+    PHDR->template writeHeaderTo<ELFT::TargetEndianness>(PHdrs++);
 
   auto SHdrs = reinterpret_cast<Elf_Shdr *>(Buf + EHdr->e_shoff);
   // First entry is null.
   ++SHdrs;
   for (OutputSectionBase<ELFT::Is64Bits> *Sec : OutputSections) {
     Sec->setNameOffset(StrTabSec.getFileOff(Sec->getName()));
     Sec->template writeHeaderTo<ELFT::TargetEndianness>(SHdrs++);
   }

test/elf2/program-header-layout.s

+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: lld -flavor gnu2 %t -o %t2
+# RUN: llvm-readobj -program-headers %t2 | FileCheck %s
+# REQUIRES: x86
+
+# Check that different output sections with the same flags are merged into a
+# single Read/Write PT_LOAD.
+
+.globl _start
+_start:
+
+.section .a,"aw"
+.quad 1
+
+.section .b,"aw"
+.quad 2
+
+# CHECK:      ProgramHeaders [
+# CHECK-NEXT:   ProgramHeader {
+# CHECK-NEXT:     Type: PT_LOAD
+# CHECK-NEXT:     Offset: 0x0
+# CHECK-NEXT:     VirtualAddress:
+# CHECK-NEXT:     PhysicalAddress:
+# CHECK-NEXT:     FileSize:
+# CHECK-NEXT:     MemSize:
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       PF_R
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Alignment:
+# CHECK-NEXT:   }
+# CHECK-NEXT:   ProgramHeader {
+# CHECK-NEXT:     Type: PT_LOAD
+# CHECK-NEXT:     Offset:
+# CHECK-NEXT:     VirtualAddress:
+# CHECK-NEXT:     PhysicalAddress:
+# CHECK-NEXT:     FileSize: 16
+# CHECK-NEXT:     MemSize: 16
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       PF_R
+# CHECK-NEXT:       PF_W
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Alignment:
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]

test/elf2/relocation.s

 .global R_X86_64_32
 R_X86_64_32:
   movl $R_X86_64_32, %edx
 
 // FIXME: this would be far more self evident if llvm-objdump printed
 // constants in hex.
 // CHECK: Disassembly of section .text2:
 // CHECK-NEXT: R_X86_64_32:
-// CHECK-NEXT:  12000: {{.*}} movl $73728, %edx
+// CHECK-NEXT:  1100c: {{.*}} movl $69644, %edx
 
 .section .R_X86_64_64,"a",@progbits
 .global R_X86_64_64
 R_X86_64_64:
  .quad R_X86_64_64
 
 // CHECK:      Contents of section .R_X86_64_64:
-// CHECK-NEXT:   13000 00300100 00000000
+// CHECK-NEXT:   12000 00200100 00000000

test/elf2/shared.s

 // RUN: llvm-mc -filetype=obj -triple=i686-unknown-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=i686-unknown-linux %p/Inputs/shared.s -o %t2.o
 // RUN: lld -flavor gnu2 -shared %t2.o -o %t2.so
 // RUN: llvm-readobj -s %t2.so | FileCheck --check-prefix=SO %s
 // RUN: lld -flavor gnu2 -dynamic-linker /lib64/ld-linux-x86-64.so.2 -rpath foo -rpath bar %t.o %t2.so -o %t
 // RUN: llvm-readobj --program-headers --dynamic-table -t -s -dyn-symbols -section-data -hash-table %t | FileCheck %s
 // REQUIRES: x86
 
 // Make sure .symtab is properly aligned.
 // SO:      Name: .symtab
 // SO-NEXT: Type: SHT_SYMTAB
 // SO-NEXT: Flags [
 // SO-NEXT: ]
 // SO-NEXT: Address:
-// SO-NEXT: Offset: 0x400C
+// SO-NEXT: Offset: 0x300C
 // SO-NEXT: Size:
 // SO-NEXT: Link:
 // SO-NEXT: Info:
 // SO-NEXT: AddressAlignment: 4
 
 // CHECK:        Name: .interp
 // CHECK-NEXT:   Type: SHT_PROGBITS
 // CHECK-NEXT:   Flags [

 // test that .hash is linked to .dynsym
 // CHECK:        Index: 5
 // CHECK-NEXT:   Name: .dynsym
 // CHECK-NEXT:   Type: SHT_DYNSYM
 // CHECK-NEXT:   Flags [
 // CHECK-NEXT:     SHF_ALLOC
 // CHECK-NEXT:   ]
 // CHECK-NEXT:   Address: [[DYNSYMADDR:.*]]
-// CHECK-NEXT:   Offset: 0x3000
+// CHECK-NEXT:   Offset: 0x201C
 // CHECK-NEXT:   Size: 48
 // CHECK-NEXT:   Link: [[DYNSTR:.*]]
 // CHECK-NEXT:   Info: 1
 // CHECK-NEXT:   AddressAlignment: 4
 // CHECK-NEXT:   EntrySize: 16
 // CHECK-NEXT:   SectionData (
 // CHECK-NEXT:     0000:
 // CHECK-NEXT:     0010: