[objcopy] make objcopy follow program header standards

[amontanez]

Summary

Objects with unused program headers copied by objcopy would always have nonzero values for program header offset and program header entry size. While technically valid, this atypical behavior triggers warnings in some tools. This change sets the two fields to zero when the program header is unused, better fitting the general expectations for unused program header data.

Section headers behaved somewhat similarly (though only with the entry size), and are fixed in this revision as well.

[jakehehrlich]

A couple nits but this LGTM.

llvm/test/tools/llvm-objcopy/relocatable-phdr.test

5 ↗

(On Diff #164997)

You'll want to use llvm-readobj instead of llvm-readelf. It produces a more machine readable format.

llvm/tools/llvm-objcopy/Object.cpp

1053 ↗

(On Diff #164997)

I'd use phnum here instead of recalling size.

[rupprecht]

Thanks for fixing this!

[amontanez]

Now using llvm-readobj instead of llvm-readelf for test, updated patch to use Ehdr.e_phnum instead of calling size() repeatedly.

[amontanez]

Done, good points.

[jakehehrlich]

When you write your diff use git diff -U<some big number> origin/master so that the diff is relative to what's upstream not your last change locally. I think git show might be understood by fabricator as well but I'm not 100% sure.

llvm/test/tools/llvm-objcopy/relocatable-phdr.test

16 ↗

(On Diff #165003)

nit: add a space here

[jakehehrlich]

Oh also LGTM

[mcgrathr]

llvm/tools/llvm-objcopy/Object.cpp

1057 ↗

(On Diff #165003)

While you're at it, don't set shentsize when there are no sections.

[jakehehrlich]

llvm/tools/llvm-objcopy/Object.cpp

1057 ↗

(On Diff #165003)

You'll want to use !WriteSectionHeaders || size(Obj.sections()) == 0 as the condition for that BTW. WriteSectionHeaders is normally true but is false when --strip-sections is used. WriteSectionHeaders isn't set to false when size(Obj.sections()) == 0 is true

Thinking about it now the if (WriteSectionHeaders) below should probably be WriteSectionHeaders && size(Obj.sections()) != 0 and then you can just set e_shentsize inside each branch of that if statement. That way if a sectionless object is input to llvm-objcopy it will output it with all those fields set to zero even though --strip-sections wasn't used.

[jakehehrlich]

I'll LGTM after Roland's fixes are done since I agree, that would be nice to also fix.

[amontanez]

Fixes affected test cases, now includes fix for similar behavior from section headers.

[amontanez]

Done! I ran all the tests for llvm-objcopy and updated everything that was failing from these changes.

[jakehehrlich]

LGTM.

llvm/trunk/test/tools/llvm-objcopy/binary-input-arch.test

 # CHECK-NEXT:     Version: 1
 # CHECK-NEXT:     Entry: 0x0
 # CHECK-NEXT:     ProgramHeaderOffset:
 # CHECK-NEXT:     SectionHeaderOffset:
 # CHECK-NEXT:     Flags [ (0x0)
 # CHECK-NEXT:     ]
 # 32-NEXT:        HeaderSize: 52
 # 64-NEXT:        HeaderSize: 64
-# 32-NEXT:        ProgramHeaderEntrySize: 32
-# 64-NEXT:        ProgramHeaderEntrySize: 56
+# 32-NEXT:        ProgramHeaderEntrySize: 0
+# 64-NEXT:        ProgramHeaderEntrySize: 0
 # CHECK-NEXT:     ProgramHeaderCount: 0
 # 32-NEXT:        SectionHeaderEntrySize: 40
 # 64-NEXT:        SectionHeaderEntrySize: 64
 # CHECK-NEXT:     SectionHeaderCount: 4
 # CHECK-NEXT:     StringTableSectionIndex:
 # CHECK-NEXT:   }

llvm/trunk/test/tools/llvm-objcopy/many-sections.test

 EHDR-NEXT:     OS/ABI: SystemV (0x0)
 EHDR-NEXT:     ABIVersion: 0
 EHDR-NEXT:     Unused: (00 00 00 00 00 00 00)
 EHDR-NEXT:   }
 EHDR-NEXT:   Type: Relocatable (0x1)
 EHDR-NEXT:   Machine: EM_X86_64 (0x3E)
 EHDR-NEXT:   Version: 1
 EHDR-NEXT:   Entry: 0x0
-EHDR-NEXT:   ProgramHeaderOffset: 0x40
+EHDR-NEXT:   ProgramHeaderOffset: 0x0
 EHDR-NEXT:   SectionHeaderOffset:
 EHDR-NEXT:   Flags [ (0x0)
 EHDR-NEXT:   ]
 EHDR-NEXT:   HeaderSize: 64
-EHDR-NEXT:   ProgramHeaderEntrySize: 56
+EHDR-NEXT:   ProgramHeaderEntrySize: 0
 EHDR-NEXT:   ProgramHeaderCount: 0
 EHDR-NEXT:   SectionHeaderEntrySize: 64
 EHDR-NEXT:   SectionHeaderCount: 0
 EHDR-NEXT:   StringTableSectionIndex: 65535
 EHDR-NEXT: }
 
 SECS: Index: 65285
 SECS-NEXT: Name: .symtab

llvm/trunk/test/tools/llvm-objcopy/relocatable-phdr.test

+# This test ensures an object without a program header will retain zero values
+# for offset and header size when copied with llvm-objcopy.
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy %t %t2
+# RUN: llvm-readobj -file-headers %t2 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+
+# CHECK:      ProgramHeaderOffset: 0x0
+# CHECK:      ProgramHeaderEntrySize: 0
+# CHECK-NEXT: ProgramHeaderCount: 0

llvm/trunk/test/tools/llvm-objcopy/strip-sections.test

 #CHECK-NEXT:   Entry: 0x0
 #CHECK-NEXT:   ProgramHeaderOffset: 0x40
 #CHECK-NEXT:   SectionHeaderOffset: 0x0
 #CHECK-NEXT:   Flags [ (0x0)
 #CHECK-NEXT:   ]
 #CHECK-NEXT:   HeaderSize: 64
 #CHECK-NEXT:   ProgramHeaderEntrySize: 56
 #CHECK-NEXT:   ProgramHeaderCount: 1
-#CHECK-NEXT:   SectionHeaderEntrySize: 64
+#CHECK-NEXT:   SectionHeaderEntrySize: 0
 #CHECK-NEXT:   SectionHeaderCount: 0
 #CHECK-NEXT:   StringTableSectionIndex: 0
 #CHECK-NEXT: }
 
 #CHECK:     ProgramHeaders [
 #CHECK-NEXT:  ProgramHeader {
 #CHECK-NEXT:    Type: PT_LOAD (0x1)
 #CHECK-NEXT:    Offset: 0x1000

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

template <class ELFT> void ELFWriter<ELFT>::writeEhdr() {
   Ehdr.e_ident[EI_VERSION] = EV_CURRENT;
   Ehdr.e_ident[EI_OSABI] = ELFOSABI_NONE;
   Ehdr.e_ident[EI_ABIVERSION] = 0;
 
   Ehdr.e_type = Obj.Type;
   Ehdr.e_machine = Obj.Machine;
   Ehdr.e_version = Obj.Version;
   Ehdr.e_entry = Obj.Entry;
-  // TODO: Only set phoff when a program header exists, to avoid tools
-  // thinking this is corrupt data.
-  Ehdr.e_phoff = Obj.ProgramHdrSegment.Offset;
+  Ehdr.e_phnum = size(Obj.segments());
+  Ehdr.e_phoff = (Ehdr.e_phnum != 0) ? Obj.ProgramHdrSegment.Offset : 0;
+  Ehdr.e_phentsize = (Ehdr.e_phnum != 0) ? sizeof(Elf_Phdr) : 0;
   Ehdr.e_flags = Obj.Flags;
   Ehdr.e_ehsize = sizeof(Elf_Ehdr);
-  Ehdr.e_phentsize = sizeof(Elf_Phdr);
-  Ehdr.e_phnum = size(Obj.segments());
+  if (WriteSectionHeaders && size(Obj.sections()) != 0) {
     Ehdr.e_shentsize = sizeof(Elf_Shdr);
-  if (WriteSectionHeaders) {
     Ehdr.e_shoff = Obj.SHOffset;
     // """
     // 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.
     // """
     auto Shnum = size(Obj.sections()) + 1;
     if (Shnum >= SHN_LORESERVE)
       Ehdr.e_shnum = 0;
     else
       Ehdr.e_shnum = Shnum;
     // """
     // 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.
     // """
     if (Obj.SectionNames->Index >= SHN_LORESERVE)
       Ehdr.e_shstrndx = SHN_XINDEX;
     else
       Ehdr.e_shstrndx = Obj.SectionNames->Index;
   } else {
+    Ehdr.e_shentsize = 0;
     Ehdr.e_shoff = 0;
     Ehdr.e_shnum = 0;
     Ehdr.e_shstrndx = 0;
   }
 }
 
 template <class ELFT> void ELFWriter<ELFT>::writePhdrs() {
   for (auto &Seg : Obj.segments())