[yaml2obj] Set p_align to the maximum sh_addralign of contained sections

[MaskRay]

Summary

The address difference between two sections in a PT_LOAD is a constant.
Consider a hypothetical case (pagesize can be very small, say, 4).

.text     sh_addralign=4
.text.hot sh_addralign=16

If we set p_align to 4, the PT_LOAD will be loaded to an address which
is a multiple of 4. The address of .text.hot is guaranteed to be a
multiple of 4, but not necessarily a multiple of 16.

This patch deletes the constraint

if (SHeader->sh_offset == PHeader.p_offset)

[grimar]

This LGTM. But please wait a bit to see if others have any comments.

test/tools/yaml2obj/program-header-align.yaml

32

nit: Excessive ":" here and in the comment above?

[MaskRay]

Align: -> Align

Move a comment

[jhenderson]

test/tools/yaml2obj/program-header-align.yaml

2

alignment of containing sections -> alignment of contained sections

7

I think you can just get rid of --check-prefix and use CHECK everywhere, right?

8

Not sure if this is really needed since the yaml2obj below only has one ProgramHeader, but maybe worth considering adding # CHECK-NOT: ProgramHeader after this line, to make sure the Alignment is for the right program header.

33

containing -> contained

36

Delete --check-prefix?

[MaskRay]

containing -> contained

--check-prefix ALIGN-16 -> (empty)

[jhenderson]

LGTM.

lib/ObjectYAML/ELFEmitter.cpp

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;
     }
 
-    // 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.
+    // Set the alignment of the segment to be the maximum alignment of the
+    // sections 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)
-        if (SHeader->sh_offset == PHeader.p_offset)
-          PHeader.p_align = std::max(PHeader.p_align, SHeader->sh_addralign);
     }
   }
 }
 
 template <class ELFT>
 bool ELFState<ELFT>::writeSectionContent(
     Elf_Shdr &SHeader, const ELFYAML::RawContentSection &Section,
     ContiguousBlobAccumulator &CBA) {

test/tools/yaml2obj/program-header-align.yaml

-# RUN: yaml2obj %s -o %t
-# RUN: llvm-readobj -l %t | FileCheck %s
+## Test the p_align field of a program header can be set explicitly or inferred
+## from the maximum alignment of containing sections.
 
-!ELF
+## Explicit Align: has priority over section alignments.
+
+# RUN: yaml2obj --docnum=1 %s -o %t
+# RUN: llvm-readobj -l %t | FileCheck --check-prefix=ALIGN-16 %s
+# ALIGN-16:      ProgramHeader {
+# ALIGN-16:        Alignment: 16
+# ALIGN-16-NEXT: }
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_NONE
+Sections:
+  - Name:         .tdata
+    Type:         SHT_PROGBITS
+    AddressAlign: 4
+  - Name:         .tbss
+    Type:         SHT_NOBITS
+    AddressAlign: 64
+ProgramHeaders:
+  - Type:  PT_TLS
+    Align: 16
+    Sections:
+      - Section: .tdata
+      - Section: .tbss
+
+## If Align: is not specified, p_align is inferred from the maximum alignment
+## of containing sections.
+
+# RUN: yaml2obj --docnum=2 %s -o %t
+# RUN: llvm-readobj -l %t | FileCheck --check-prefix=ALIGN-16 %s
+
+--- !ELF
 FileHeader:
-  Class:           ELFCLASS64
-  Data:            ELFDATA2LSB
-  Type:            ET_EXEC
-  Machine:         EM_X86_64
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_NONE
 Sections:
-  - Name:            .text
-    Type:            SHT_PROGBITS
-    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
-    AddressAlign:    0x0000000000000008
-    Content:         "00000000"
-  - Name:            .data
-    Type:            SHT_PROGBITS
-    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
-    Content:         "00000000"
-    AddressAlign:    0x0000000000000010
+  - Name:         .text
+    Type:         SHT_PROGBITS
+    AddressAlign: 4
+  - Name:         .text.hot
+    Type:         SHT_PROGBITS
+    AddressAlign: 16
 ProgramHeaders:
   - Type: PT_LOAD
-    Flags: [ PF_X, PF_R ]
-    VAddr: 0xAAAA1030
-    PAddr: 0xFFFF1030
-    Align: 0x1000
     Sections:
       - Section: .text
-  - Type: PT_LOAD
-    Flags: [ PF_R ]
-    VAddr: 0xAAAA2040
-    PAddr: 0xFFFF2040
-    Align: 0x1000
-    Sections:
-      - Section: .data
-
-#CHECK:     ProgramHeaders [
-#CHECK-NEXT:   ProgramHeader {
-#CHECK-NEXT:    Type: PT_LOAD
-#CHECK-NEXT:    Offset: 0xB0
-#CHECK-NEXT:    VirtualAddress: 0xAAAA1030
-#CHECK-NEXT:    PhysicalAddress: 0xFFFF1030
-#CHECK-NEXT:    FileSize: 4
-#CHECK-NEXT:    MemSize: 4
-#CHECK-NEXT:    Flags [
-#CHECK-NEXT:      PF_R
-#CHECK-NEXT:      PF_X
-#CHECK-NEXT:    ]
-#CHECK-NEXT:    Alignment: 4096
-#CHECK-NEXT:  }
-#CHECK-NEXT:  ProgramHeader {
-#CHECK-NEXT:    Type: PT_LOAD
-#CHECK-NEXT:    Offset: 0xC0
-#CHECK-NEXT:    VirtualAddress: 0xAAAA2040
-#CHECK-NEXT:    PhysicalAddress: 0xFFFF2040
-#CHECK-NEXT:    FileSize: 4
-#CHECK-NEXT:    MemSize: 4
-#CHECK-NEXT:    Flags [
-#CHECK-NEXT:      PF_R
-#CHECK-NEXT:    ]
-#CHECK-NEXT:    Alignment: 4096
-#CHECK-NEXT:  }
-#CHECK-NEXT:]
+      - Section: .text.hot