[ELF] - Do not produce broken output when amount of sections is > ~65k

[grimar]

Summary

This is https://bugs.llvm.org//show_bug.cgi?id=38119

We produce broken ELF header now when the number of output sections is >= SHN_LORESERVE (0xff00).

ELF spec says (http://www.sco.com/developers/gabi/2003-12-17/ch4.eheader.html):

e_shnum:
If the number of sections is greater than or equal to SHN_LORESERVE (0xff00), this member has the value zero
and the actual number of section header table entries is contained in the sh_size field of the section header at index 0.
(Otherwise, the sh_size member of the initial entry contains 0.)

e_shstrndx
If the section name string table section index is greater than or equal to SHN_LORESERVE (0xff00), this member has the
value SHN_XINDEX (0xffff) and the actual index of the section name string table section is contained in the sh_link field of
the section header at index 0. (Otherwise, the sh_link member of the initial entry contains 0.)

We did not set these fields correctly earlier. The patch fixes the issue.

[peter.smith]

This looks correct to me. I've got a few small suggestions.

ELF/Writer.cpp

2267 ↗

(On Diff #155657)

I suggest combining the two comments for SHN_LORESERVE as they are closely related. For example:

The ELF header can only store numbers up to SHN_LORESERVE in the e_shnum and e_shstrndx fields. When the value of one of these fields exceeds SHN_LORESERVE ELF requires us to put sentinel values in
the ELF header and use fields in the section header at index 0 to store the value. The sentinel values and fields are:
e_shnum = 0, SHdrs[0].sh_size = number of sections.
e_shstrndx = SHN_XINDEX, SHdrs[0].sh_link = strtab section index.

2271 ↗

(On Diff #155657)

Is it worth something like NumElfSections = OutputSections.size() + 1; as we use it 3 times in a row, and it makes the +1 for the 0th Section Header a bit more obvious.

2272 ↗

(On Diff #155657)

Should we explicitly set EHdr->e_shnum to 0 here as we've called it out in the comment.

[grimar]

Addressed comments. Thanks, Peter!

[grimar]

ELF/Writer.cpp

2272 ↗

(On Diff #155657)

Above (line 2246) we have a code that does not do that, assuming fields are zero by default:

if (!Config->Relocatable) {
  EHdr->e_phoff = sizeof(Elf_Ehdr);
  EHdr->e_phentsize = sizeof(Elf_Phdr);
}

So to be consistent, I would not do that probably. Though both ways would be OK to me.

[peter.smith]

Looks good to me thanks.

[ruiu]

Do you really need this? That the spec says something doesn't always mean that we need it. I'm afraid that all these nitpicky details would accumulate and make the entire program hard to read. Even though I'm not against "doing it right", I really want you to show some examples to convince others that we need it. Also, please wait for me for a few days (I believe you've got my vacation responder email) to review your change. In the meantime, could you please revert this change?

[ruiu]

Hm, maybe this particular patch is too easy that it doesn't have to be reverted, but please allow me time to review your patches. I believe there's no need to hurry to submit these kind of changes.

[grimar]

In D49371#1166746, @ruiu wrote:

Do you really need this? That the spec says something doesn't always mean that we need it. I'm afraid that all these nitpicky details would accumulate and make the entire program hard to read. Even though I'm not against "doing it right", I really want you to show some examples to convince others that we need it.

Hi Rui, this was https://bugs.llvm.org//show_bug.cgi?id=38119. We produced broken output previously and user faced it.
Tools did not allow to dump the content correctly and things looked like some kind of memory corruption.

Hm, maybe this particular patch is too easy that it doesn't have to be reverted, but please allow me time to review your patches.
I believe there's no need to hurry to submit these kind of changes.

Sure. Thanks.

(I believe you've got my vacation responder email.)

(FYI I did not get any responder emails from you, I guess because using phab and not direct mails may be. Not sure)

lld/trunk/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::writeHeader() {
   EHdr->e_machine = Config->EMachine;
   EHdr->e_version = EV_CURRENT;
   EHdr->e_entry = getEntryAddr();
   EHdr->e_shoff = SectionHeaderOff;
   EHdr->e_flags = Config->EFlags;
   EHdr->e_ehsize = sizeof(Elf_Ehdr);
   EHdr->e_phnum = Phdrs.size();
   EHdr->e_shentsize = sizeof(Elf_Shdr);
-  EHdr->e_shnum = OutputSections.size() + 1;
-  EHdr->e_shstrndx = InX::ShStrTab->getParent()->SectionIndex;
 
   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;
     ++HBuf;
   }
 
-  // Write the section header table. Note that the first table entry is null.
+  // Write the section header table.
+  //
+  // The ELF header can only store numbers up to SHN_LORESERVE in the e_shnum
+  // and e_shstrndx fields. When the value of one of these fields exceeds
+  // SHN_LORESERVE ELF requires us to put sentinel values in the ELF header and
+  // use fields in the section header at index 0 to store
+  // the value. The sentinel values and fields are:
+  // e_shnum = 0, SHdrs[0].sh_size = number of sections.
+  // e_shstrndx = SHN_XINDEX, SHdrs[0].sh_link = .shstrtab section index.
   auto *SHdrs = reinterpret_cast<Elf_Shdr *>(Buf + EHdr->e_shoff);
+  size_t Num = OutputSections.size() + 1;
+  if (Num >= SHN_LORESERVE)
+    SHdrs->sh_size = Num;
+  else
+    EHdr->e_shnum = Num;
+
+  uint32_t StrTabIndex = InX::ShStrTab->getParent()->SectionIndex;
+  if (StrTabIndex >= SHN_LORESERVE) {
+    SHdrs->sh_link = StrTabIndex;
+    EHdr->e_shstrndx = SHN_XINDEX;
+  } else {
+    EHdr->e_shstrndx = StrTabIndex;
+  }
+
   for (OutputSection *Sec : OutputSections)
     Sec->writeHeaderTo<ELFT>(++SHdrs);
 }
 
 // Open a result file.
 template <class ELFT> void Writer<ELFT>::openFile() {
   if (!Config->Is64 && FileSize > UINT32_MAX) {
     error("output file too large: " + Twine(FileSize) + " bytes");

lld/trunk/test/ELF/relocatable-many-sections.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o %t.o
+# RUN: ld.lld -r %t.o -o %t
+# RUN: llvm-readobj -file-headers %t | FileCheck %s
+
+## Check we are able to emit a valid ELF header when
+## sections amount is greater than SHN_LORESERVE.
+# CHECK:      ElfHeader {
+# CHECK:        SectionHeaderCount: 0 (65541)
+# CHECK-NEXT:   StringTableSectionIndex: 65535 (65539)
+
+.macro gen_sections4 x
+  .section a\x
+  .section b\x
+  .section c\x
+  .section d\x
+.endm
+
+.macro gen_sections8 x
+  gen_sections4 a\x
+  gen_sections4 b\x
+.endm
+
+.macro gen_sections16 x
+  gen_sections8 a\x
+  gen_sections8 b\x
+.endm
+
+.macro gen_sections32 x
+  gen_sections16 a\x
+  gen_sections16 b\x
+.endm
+
+.macro gen_sections64 x
+  gen_sections32 a\x
+  gen_sections32 b\x
+.endm
+
+.macro gen_sections128 x
+  gen_sections64 a\x
+  gen_sections64 b\x
+.endm
+
+.macro gen_sections256 x
+  gen_sections128 a\x
+  gen_sections128 b\x
+.endm
+
+.macro gen_sections512 x
+  gen_sections256 a\x
+  gen_sections256 b\x
+.endm
+
+.macro gen_sections1024 x
+  gen_sections512 a\x
+  gen_sections512 b\x
+.endm
+
+.macro gen_sections2048 x
+  gen_sections1024 a\x
+  gen_sections1024 b\x
+.endm
+
+.macro gen_sections4096 x
+  gen_sections2048 a\x
+  gen_sections2048 b\x
+.endm
+
+.macro gen_sections8192 x
+  gen_sections4096 a\x
+  gen_sections4096 b\x
+.endm
+
+.macro gen_sections16384 x
+  gen_sections8192 a\x
+  gen_sections8192 b\x
+.endm
+
+gen_sections16384 a
+gen_sections16384 b
+gen_sections16384 c
+gen_sections16384 d
+
+.global _start
+_start: