[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 ↗

(On Diff #219037)

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

[MaskRay]

Align: -> Align

Move a comment

[jhenderson]

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

2 ↗

(On Diff #219051)

alignment of containing sections -> alignment of contained sections

7 ↗

(On Diff #219051)

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

8 ↗

(On Diff #219051)

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 ↗

(On Diff #219051)

containing -> contained

36 ↗

(On Diff #219051)

Delete --check-prefix?

[MaskRay]

containing -> contained

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

[jhenderson]

LGTM.

llvm/trunk/lib/ObjectYAML/ELFEmitter.cpp

for (auto &YamlPhdr : Doc.ProgramHeaders) {
     }
 
     // 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 {
+      // 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.
       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>
 void ELFState<ELFT>::writeSectionContent(
     Elf_Shdr &SHeader, const ELFYAML::RawContentSection &Section,
     ContiguousBlobAccumulator &CBA) {

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

-# RUN: yaml2obj %s -o %t
+## Test the p_align field of a program header can be set explicitly or inferred
+## from the maximum alignment of contained sections.
+
+## Explicit Align has priority over section alignments.
+
+# RUN: yaml2obj --docnum=1 %s -o %t
+# RUN: llvm-readobj -l %t | FileCheck %s
+# CHECK:      ProgramHeader {
+# CHECK-NOT:  ProgramHeader {
+# CHECK:        Alignment: 16
+# CHECK-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 contained sections.
+
+# RUN: yaml2obj --docnum=2 %s -o %t
 # RUN: llvm-readobj -l %t | FileCheck %s
 
-!ELF
+--- !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