[ELF] Align file offset for .bss if first section in a PT_LOAD

[jrtc27]

Summary

If a linker script requests additional alignment for .bss, and it is the
first section in a PT_LOAD, we must ensure to apply this alignment to
the file offset as well, as with other sections, to ensure the file
offset and virtual address are congruent modulo the page size.

[jrtc27]

Updated affected tests.

[ruiu]

LGTM

[MaskRay]

lld/ELF/Writer.cpp

2221

when not the first section in a PT_LOAD -> not in a PT_LOAD?

[jrtc27]

lld/ELF/Writer.cpp

2221

not in a PT_LOAD is encompassed by that, and this means it also covers the case below that has a reference to this comment.

lld/test/ELF/basic-ppc64.s

166 ↗

(On Diff #195420)

Technically, in this case, we could not align the section in the file, since the entire PT_LOAD it gets put into is empty and thus omitted, but that's complexity for optimising a rare edge-case.

[MaskRay]

lld/ELF/Writer.cpp

2224

What happens if you delete if (OS->Type == SHT_NOBITS) return Off; here?

[MaskRay]

lld/ELF/Writer.cpp

2224

I think we probably just need to delete if (OS->Type == SHT_NOBITS) return Off; from the original code.

.tss in tls-offset.s is affected but that offset is not significant.

[jrtc27]

lld/ELF/Writer.cpp

2224

Then you potentially unnecessarily over-align a non-loaded nobits section in the file. It's not wrong, it just deviates more from the current output.

2239

This might actually be wrong; consider .bss being followed (stupidly) by .data in the same segment. In that case, .bss will be filled with zeroes and thus have a non-zero filesz, but we need to make sure its file offset is still the same as its address modulo the page size (ie by using the standard formula below), otherwise it will appear shifted in the segment. I'll see if this is true in practice with another simple test case.

[jrtc27]

Also handle .bss where the zeroes are written to the file.

[jrtc27]

Ping?

[MaskRay]

Sorry I didn't compare the results carefully, but I ran into a similar issue with PPC32 BSS PLT.

Have you tried this diff?

-  if (OS->Type == SHT_NOBITS)
+  if (OS->Type == SHT_NOBITS && !(OS->PtLoad && OS->PtLoad->FirstSec == OS))
     return Off;

  // If the section is not in a PT_LOAD, we just have to align it.
  if (!OS->PtLoad)
    return alignTo(Off, OS->Alignment);

[MaskRay]

lld/test/ELF/linkerscript/bss-middle-align.test

4 ↗

(On Diff #195758)

For llvm-readelf, --long-option is now more common (GNU readelf parses -long-option as a list of short options).

8 ↗

(On Diff #195758)

I think most other linkerscript tests write this on one line: .text : { *(.text) }. It is concise and more readable in my opinion.

[jrtc27]

Fixed slightly differently by D66658 which landed over a month ago.

lld/ELF/Writer.cpp

if (needsPtLoad(Cmd))
       PageAlign(Cmd);
   }
 }
 
 // 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) {
-  // File offsets are not significant for .bss sections. By convention, we keep
-  // section offsets monotonically increasing rather than setting to zero.
+  // If the section is not in a PT_LOAD, we just have to align it.
+  if (!OS->PtLoad) {
+    // File offsets are not significant for .bss sections when not the first
+    // section in a PT_LOAD. By convention, we keep section offsets
+    // monotonically increasing rather than setting to zero.
     if (OS->Type == SHT_NOBITS)
       return Off;
 
-  // If the section is not in a PT_LOAD, we just have to align it.
-  if (!OS->PtLoad)
     return alignTo(Off, OS->Alignment);
+  }
 
   // The first section in a PT_LOAD has to have congruent offset and address
   // module the page size.
   OutputSection *First = OS->PtLoad->FirstSec;
   if (OS == First) {
     uint64_t Alignment = std::max<uint64_t>(OS->Alignment, Config->MaxPageSize);
     return alignTo(Off, Alignment, OS->Addr);
   }
 
+  // This is a .bss section that isn't first in its PT_LOAD; see above.
+  if (OS->Type == SHT_NOBITS)
+    return Off;
+
   // If two sections share the same PT_LOAD the file offset is calculated
   // using this formula: Off2 = Off1 + (VA2 - VA1).
   return First->Offset + OS->Addr - First->Addr;
 }
 
 // Set an in-file position to a given section and returns the end position of
 // the section.
 static uint64_t setFileOffset(OutputSection *OS, uint64_t Off) {

lld/test/ELF/linkerscript/Inputs/bss-first-align.s

+.text
+.globl _start
+_start:
+ nop
+
+.bss
+.short 0

lld/test/ELF/linkerscript/bss-first-align.test

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/bss-first-align.s -o %t.o
+# RUN: ld.lld -o %t.exe --script %s %t.o
+# RUN: llvm-readelf -program-headers %t.exe | FileCheck %s
+
+SECTIONS {
+  .text : {
+    *(.text)
+  }
+  .bss ALIGN(0x20): {
+    *(.bss)
+  }
+}
+
+# CHECK: Program Headers:
+# CHECK-NEXT: Type  Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
+# CHECK-NEXT: LOAD  0x001000 0x0000000000000000 0x0000000000000000 0x000001 0x000001 R E 0x1000
+# CHECK-NEXT: LOAD  0x001020 0x0000000000000020 0x0000000000000020 0x000000 0x000002 RW  0x1000
+# CHECK-NOT: LOAD