Align output segments correctly

[ruiu]

Summary

Previously, segments were aligned according to their first section's
alignment requirements. That was not correct, but segments are also
aligned to a page boundary, and a page boundary is usually much larger
than a section alignment requirement, so no one noticed this bug before.

Now, lld has --nmagic option which sets maxPageSize to 1 to effectively
disable page alignment, which reveals the issue.

Fixes https://bugs.llvm.org/show_bug.cgi?id=43212

[MaskRay]

lld/test/ELF/nmagic.s

4 ↗

(On Diff #218640)

-unknown-linux can be deleted.

5 ↗

(On Diff #218640)

Use -n for a nmagic test.. (-N is omagic which does more)

6 ↗

(On Diff #218640)

-S or --section-headers. The single-dash form -long-option is deprecated in llvm binutils that intend to match GNU binutils.

[MaskRay]

lld/test/ELF/nmagic.s

22 ↗

(On Diff #218640)

If I assemble this file as x86-64, .rodata happens to be aligned without the change. A larger alignment make more likely reveal problems if we have some layout changes.

[peter.smith]

lld/ELF/Writer.cpp

2276 ↗

(On Diff #218640)

Just a thought, could this be reduced to os->ptLoad->p_align? I think config->maxPageSize would already be reflected in ptLoad->p_align. I'll have a check.

[MaskRay]

lld/ELF/Writer.cpp

2276 ↗

(On Diff #218640)

It can't. p_align can be smaller than maxPageSize at this point.
I think this can be reduced if we move the code below

// setPhdrs
    if (p->p_type == PT_LOAD) {
      p->p_align = std::max<uint64_t>(p->p_align, config->maxPageSize);

to the constructor of PhdrEntry.

[ruiu]

lld/test/ELF/nmagic.s

22 ↗

(On Diff #218640)

That is true, but this file should suffice to reveal the bug, and it also captures the real usage, so I want to keep this as-is.

[ruiu]

• address review comments

[MaskRay]

lld/test/ELF/nmagic.s

22 ↗

(On Diff #218640)

Since -Ttext 0 is used, no layout change can affect the addresses. I think keeping it as-is is fine.

lld/trunk/ELF/Writer.cpp

 
 // Compute an in-file position for a given section. The file offset must be the
 // same with its virtual address modulo the page size, so that the loader can
 // load executables without any address adjustment.
 static uint64_t computeFileOffset(OutputSection *os, uint64_t off) {
   // The first section in a PT_LOAD has to have congruent offset and address
   // module the page size.
   if (os->ptLoad && os->ptLoad->firstSec == os) {
-    uint64_t alignment = std::max<uint64_t>(os->alignment, config->maxPageSize);
+    uint64_t alignment =
+        std::max<uint64_t>(os->ptLoad->p_align, config->maxPageSize);
     return alignTo(off, alignment, os->addr);
   }
 
   // File offsets are not significant for .bss sections other than the first one
   // in a PT_LOAD. By convention, we keep section offsets monotonically
   // increasing rather than setting to zero.
    if (os->type == SHT_NOBITS)
      return off;

lld/trunk/test/ELF/nmagic.s

+# REQUIRES: x86
+# Verify that .rodata is aligned to a 8 byte boundary.
+
+# RUN: llvm-mc -filetype=obj -triple=i386 %s -o %t.o
+# RUN: ld.lld %t.o -o %t.exe -n -Ttext 0
+# RUN: llvm-readelf --section-headers %t.exe | FileCheck %s
+
+# CHECK: [ 0]           NULL     00000000 000000 000000 00      0   0  0
+# CHECK: [ 1] .text     PROGBITS 00000000 0000d4 000001 00  AX  0   0  4
+# CHECK: [ 2] .rodata   PROGBITS 00000008 0000d8 000008 00   A  0   0  8
+# CHECK: [ 3] .comment  PROGBITS 00000000 0000e0 000008 01  MS  0   0  1
+# CHECK: [ 4] .symtab   SYMTAB   00000000 0000e8 000020 10      6   1  4
+# CHECK: [ 5] .shstrtab STRTAB   00000000 000108 000032 00      0   0  1
+# CHECK: [ 6] .strtab   STRTAB   00000000 00013a 000008 00      0   0  1
+
+.globl _start
+.text
+_start:
+  ret
+
+.rodata
+.align 8
+.quad 42