[yaml2obj] Simplify p_filesz/p_memsz computing

[MaskRay]

Summary

This fixes a bug as well. When "FileSize:" (p_filesz) is specified and
different from the actual value, the following code probably should not
use PHeader.p_filesz:

if (SHeader->sh_offset == PHeader.p_offset + PHeader.p_filesz)
  PHeader.p_memsz += SHeader->sh_size;

[grimar]

This looks good. Would be nice if someone else also take a look.

lib/ObjectYAML/ELFEmitter.cpp

630 ↗

(On Diff #219031)

When I first saw FileEnd I thought it is somehow related to the end of file.
Maybe FileOffset vs MemOffset is better? (Or some different better names)

[MaskRay]

Support multiple trailing SHT_NOBITS.

lld/ELF/Writer.cpp does not use the trick (two or more SHT_NOBITS are tricky and IIRC, when more than 1 SHF_TLS SHT_NOBITS sections are used, ld.bfd can get addresses wrong). However, since I can do it with 1 fewer line here :) Let me do it.

[MaskRay]

FileEnd -> FileOffset
MemEnd -> MemOffset

[jhenderson]

LGTM, I think.

llvm/trunk/lib/ObjectYAML/ELFEmitter.cpp

if (YamlPhdr.Offset) {
       else
         PHeader.p_offset = 0;
 
       // Find the minimum offset for the program header.
       for (Elf_Shdr *SHeader : Sections)
         PHeader.p_offset = std::min(PHeader.p_offset, SHeader->sh_offset);
     }
 
-    // Find the maximum offset of the end of a section in order to set p_filesz,
-    // if not set explicitly.
-    if (YamlPhdr.FileSize) {
-      PHeader.p_filesz = *YamlPhdr.FileSize;
-    } else {
-      PHeader.p_filesz = 0;
-      for (Elf_Shdr *SHeader : Sections) {
-        uint64_t EndOfSection;
-        if (SHeader->sh_type == llvm::ELF::SHT_NOBITS)
-          EndOfSection = SHeader->sh_offset;
-        else
-          EndOfSection = SHeader->sh_offset + SHeader->sh_size;
-        uint64_t EndOfSegment = PHeader.p_offset + PHeader.p_filesz;
-        EndOfSegment = std::max(EndOfSegment, EndOfSection);
-        PHeader.p_filesz = EndOfSegment - PHeader.p_offset;
-      }
-    }
+    // Find the maximum offset of the end of a section in order to set p_filesz
+    // and p_memsz. When setting p_filesz, trailing SHT_NOBITS sections are not
+    // counted.
+    uint64_t FileOffset = PHeader.p_offset, MemOffset = PHeader.p_offset;
+      uint64_t End = SHeader->sh_offset + SHeader->sh_size;
+      MemOffset = std::max(MemOffset, End);
 
-    // If not set explicitly, find the memory size by adding the size of
-    // sections at the end of the segment. These should be empty (size of zero)
-    // and NOBITS sections.
-    if (YamlPhdr.MemSize) {
-      PHeader.p_memsz = *YamlPhdr.MemSize;
-    } else {
-      PHeader.p_memsz = PHeader.p_filesz;
-      for (Elf_Shdr *SHeader : Sections)
-        if (SHeader->sh_offset == PHeader.p_offset + PHeader.p_filesz)
-          PHeader.p_memsz += SHeader->sh_size;
+      if (SHeader->sh_type != llvm::ELF::SHT_NOBITS)
+        FileOffset = std::max(FileOffset, End);
     }
 
+    // Set the file size and the memory size if not set explicitly.
+    PHeader.p_filesz = YamlPhdr.FileSize ? uint64_t(*YamlPhdr.FileSize)
+                                         : FileOffset - PHeader.p_offset;
+    PHeader.p_memsz = YamlPhdr.MemSize ? uint64_t(*YamlPhdr.MemSize)
+                                       : MemOffset - PHeader.p_offset;
+
     // Set the alignment of the segment to be the same as the maximum alignment
     // of the sections with the same offset so that by default the segment
     // has a valid and sensible alignment.
     if (YamlPhdr.Align) {
       PHeader.p_align = *YamlPhdr.Align;
     } else {
       PHeader.p_align = 1;
       for (Elf_Shdr *SHeader : Sections)

llvm/trunk/test/tools/yaml2obj/program-header-size-offset.yaml

 # Show that yaml2obj properly emits program headers with explicit file size,
 # memory size and offset parameters.
 
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-readobj %t --program-headers | FileCheck %s
 
 # CHECK: ProgramHeaders [
 # CHECK:    Offset: 0x1234
 # CHECK:    FileSize: 1111
 # CHECK:    MemSize: 9999
 
 # CHECK:    Offset: 0x2000
 # CHECK:    FileSize: 6
-# CHECK:    MemSize: 6
+# CHECK:    MemSize: 4
 
 # CHECK:    Offset: 0x2000
 # CHECK:    FileSize: 4
 # CHECK:    MemSize: 6
 
 # CHECK:    Offset: 0x1FFF
 # CHECK:    FileSize: 5
 # CHECK:    MemSize: 5
 
 # CHECK:    Offset: 0xFFE
 # CHECK:    FileSize: 7
 # CHECK:    MemSize: 9
 
 # CHECK:    Offset: 0x3000
 # CHECK:    FileSize: 3
 # CHECK:    MemSize: 2
+
+# CHECK:    Offset: 0x2004
+# CHECK:    FileSize: 4
+# CHECK:    MemSize: 6
 # CHECK: ]
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:
   - Name: .text
     Type: SHT_PROGBITS
     Size: 4
+    ShOffset: 0x1000
     AddressAlign: 0x1000
   - Name: .rodata
     Type: SHT_PROGBITS
     Size: 4
+    ShOffset: 0x2000
     AddressAlign: 0x1000
   - Name: .data
     Type: SHT_PROGBITS
+    ShOffset: 0x2004
     Size: 4
+  - Name: .nobits1
+    Type: SHT_NOBITS
+    ShOffset: 0x2008
+    Size: 1
+  - Name: .nobits2
+    Type: SHT_NOBITS
+    # Intentionally set to 0x2009 though the previous section is SHT_NOBITS.
+    ShOffset: 0x2009
+    Size: 1
 ProgramHeaders:
   # Program header with no sections.
   - Type:     0x6abcdef0 # arbitrary type
     Offset:   0x1234
     FileSize: 1111
     MemSize:  9999
   # Program header with only file size set.
   - Type:     0x6abcdef0
Sections:
       - Section: .text
   # Program header with sections, invalid properties.
   - Type:     0x6abcdef0
     Offset:   0x3000
     FileSize: 3
     MemSize:  2
     Sections:
       - Section: .data
+  # Program header with 2 SHT_NOBITS sections.
+  - Type:     0x6abcdef0
+    Offset:   0x2004
+    Sections:
+      - Section: .data
+      - Section: .nobits1
+      - Section: .nobits2