[ELF] Ignore the maximum of input section alignments for two cases

[MaskRay]

Summary

Follow-up for D74286.

Notations:

• alignExpr: the computed ALIGN value
• max_input_align: the maximum of input section alignments

This patch changes the following two cases to match GNU ld:

• When ALIGN is present, GNU ld sets output sh_addr to alignExpr, while lld use max(alignExpr, max_input_align)
• When addrExpr is specified but alignExpr is not, GNU ld sets output sh_addr to addrExpr, while lld uses advance(0, max_input_align)

Note, sh_addralign is still set to max(alignExpr, max_input_align).

lma-align.test is enhanced a bit to check we don't overalign sh_addr.

fixSectionAlignments() sets addrExpr but not alignExpr for the !hasSectionsCommand case.
This patch sets alignExpr as well so that max_input_align will be respected.

[MaskRay]

Adjust linkerscript/section-align2.test for a future change that adds warnings

[grimar]

lld/test/ELF/linkerscript/section-align2.test

2

Can it be x86?

27

The output section is aligned to 16.

But it is aligned to 32 in your output:

# CHECK:      Name         Type     Address          Off    Size   ES Flg Lk Inf Al
...
# CHECK-NEXT: .bss         NOBITS   0000000000020010 020009 000011 00  WA  0   0 32

[grimar]

lld/test/ELF/linkerscript/section-align2.test

2

I've found the answer here: https://reviews.llvm.org/D74741#inline-679844

[MaskRay]

lld/test/ELF/linkerscript/section-align2.test

2

aarch64 is to test thunks. AArch64 uses thunks, while x86 doesn't.

This helps us avoid https://sourceware.org/bugzilla/show_bug.cgi?id=25570

27

Its sh_addr is aligned to 16 though sh_addralign is 32 (max(output section alignment, maximum input section alignment))

This is GNU ld's behavior.

[MaskRay]

Fix another discrepancy

[MaskRay]

lld/ELF/Writer.cpp

2220

I don't want to use early return to change the indentation of the large code block below.

lld/test/ELF/linkerscript/outsections-addr.s

86

I can do a follow-up change to change the llvm-readobj test to use llvm-readelf.

llvm-readobj -S output is very hard to read.

[psmith]

I agree with the specification of the change, I'm not sure why there is a change in Writer.cpp yet, if there is a good reason and it isn't obvious it will be worth a comment.

lld/ELF/Writer.cpp

2219

I'm struggling a bit with this one from the just the context. As far as I can tell there are only two uses of AlignExpr, in switchTo and in
void LinkerScript::adjustSectionsBeforeSorting()

`
    // Handle align (e.g. ".foo : ALIGN(16) { ... }").
    if (sec->alignExpr)
      sec->alignment =
          std::max<uint32_t>(sec->alignment, sec->alignExpr().getValue());

In both cases I can't easily see why adding cmd->alignExpr = [align = cmd->alignment]() { return align; }; would change the calculated alignment. For example sec->alignment should be the same as alignExpr().getValue() and in switchTo

uint32_t align =
        sec->alignExpr ? sec->alignExpr().getValue() : ctx->outSec->alignment;

When there isn't alignExpr isn't ctx->outSec->alignment the same as alignExpr.getValue()?

Can you let me know if I'm missing something?

[MaskRay]

lld/ELF/Writer.cpp

2219

fixSectionAlignments() sets addrExpr but not alignExpr.

In LinkerScript::switchTo, the following branch applies:

if (sec->addrExpr && !sec->alignExpr) {
    // The alignment is ignored.
    ctx->outSec->addr = pos;
}

i.e. max_input_align is not respected.

We need it to take the else branch, thus set alignExpr as well.

[MaskRay]

lld/ELF/Writer.cpp

2219

In both cases I can't easily see why adding cmd->alignExpr = [align = cmd->alignment]() { return align; }; would change the calculated alignment.

cmd->alignExpr = [align = cmd->alignment]() { return align; }; makes LinkerScript::switchTo take the else branch.

sec->alignment = std::max<uint32_t>(sec->alignment, sec->alignExpr().getValue()); sets sh_addralign, but the value is not used for deciding sh_addr.

[psmith]

lld/ELF/LinkerScript.cpp

765

Would it be worth a comment:
// When not using a linker script sec->alignExpr is set to the maximum of input section alignments.

lld/ELF/Writer.cpp

2219

Thanks for the clarification. That part seems easy to miss. I've put in a suggestion for a comment in switchTo.

[MaskRay]

Improve a comment.

[psmith]

Thanks for the update, I'm happy if George is.

[grimar]

lld/ELF/LinkerScript.cpp

767

It is a bit strange to see that expandMemoryRegions is now gone from this path.
Is it correct?

lld/test/ELF/linkerscript/outsections-addr.s

86

I fully agree with "llvm-readobj -S output is very hard to read" here.

[MaskRay]

lld/ELF/LinkerScript.cpp

767

expandMemoryRegions(0) is a no-op.

[grimar]

LGTM

[MaskRay]

Improve description

[leonardchan]

Hi, a bisect seems to show that this causes a kernel panic for an OOM test we have for zircon (https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8887545931153261696/+/steps/failures/0/steps/Linux-_2_/0/steps/attempt_0__fail_/0/steps/failed:_host_x64_oom_tests/0/logs/stdio/0).

We're going to see if this is an us problem or a compiler problem, but just wanted to raise awareness in the meantime.

[MaskRay]

In D74736#1890561, @leonardchan wrote:

Hi, a bisect seems to show that this causes a kernel panic for an OOM test we have for zircon (https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8887545931153261696/+/steps/failures/0/steps/Linux-_2_/0/steps/attempt_0__fail_/0/steps/failed:_host_x64_oom_tests/0/logs/stdio/0).

We're going to see if this is an us problem or a compiler problem, but just wanted to raise awareness in the meantime.

I'm 80% confident that this change is innocent, especially if zircon has never been tested with GNU ld...

It is highly recommended to write portable linker scripts, because lld/GNU ld's linker scripts have some subtle differences. lld's linker script support is catching up, but any improvement may unavoidably cause semantic differences.

lld/ELF/LinkerScript.cpp

void LinkerScript::output(InputSection *s) {
   // SIZEOF works correctly in the case below:
   // .foo { *(.aaa) a = SIZEOF(.foo); *(.bbb) }
   expandOutputSection(pos - before);
 }
 
 void LinkerScript::switchTo(OutputSection *sec) {
   ctx->outSec = sec;
 
-  uint64_t before = advance(0, 1);
-  ctx->outSec->addr = advance(0, ctx->outSec->alignment);
-  expandMemoryRegions(ctx->outSec->addr - before);
+  uint64_t pos = advance(0, 1);
+  if (sec->addrExpr && !sec->alignExpr) {
+    // The alignment is ignored.
+    ctx->outSec->addr = pos;
+  } else {
+    // If ALIGN is specified, advance sh_addr according to ALIGN and ignore the
+    // maximum of input section alignments.
+    uint32_t align =
+        sec->alignExpr ? sec->alignExpr().getValue() : ctx->outSec->alignment;
+    ctx->outSec->addr = advance(0, align);
+    expandMemoryRegions(ctx->outSec->addr - pos);
+  }
 }
 
 // This function searches for a memory region to place the given output
 // section in. If found, a pointer to the appropriate memory region is
 // returned. Otherwise, a nullptr is returned.
 MemoryRegion *LinkerScript::findMemoryRegion(OutputSection *sec) {
   // If a memory region name was specified in the output section command,
   // then try to find that region first.

lld/ELF/Writer.cpp

 
 // Place the first section of each PT_LOAD to a different page (of maxPageSize).
 // This is achieved by assigning an alignment expression to addrExpr of each
 // such section.
 template <class ELFT> void Writer<ELFT>::fixSectionAlignments() {
   const PhdrEntry *prev;
   auto pageAlign = [&](const PhdrEntry *p) {
     OutputSection *cmd = p->firstSec;
-    if (cmd && !cmd->addrExpr) {
+    if (!cmd)
+      return;
+    cmd->alignExpr = [align = cmd->alignment]() { return align; };
+    if (!cmd->addrExpr) {
       // Prefer advancing to align(dot, maxPageSize) + dot%maxPageSize to avoid
       // padding in the file contents.
       //
       // When -z separate-code is used we must not have any overlap in pages
       // between an executable segment and a non-executable segment. We align to
       // the next maximum page size boundary on transitions between executable
       // and non-executable segments.
       //

lld/test/ELF/linkerscript/lma-align.test

 # REQUIRES: x86
-# RUN: echo 'ret; .data.rel.ro; .balign 16; .byte 0; .data; .byte 0; .bss; .byte 0' | \
+# RUN: echo '.globl _start; _start: ret; .data.rel.ro; .balign 16; .byte 0; \
+# RUN:   .data; .balign 32; .byte 0; .bss; .byte 0' | \
 # RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %t.o
 # RUN: ld.lld -T %s %t.o -o %t
 # RUN: llvm-readelf -S -l %t | FileCheck %s
 
 # CHECK:      Name         Type     Address          Off    Size   ES Flg Lk Inf Al
 # CHECK-NEXT:              NULL     0000000000000000 000000 000000 00      0   0  0
 # CHECK-NEXT: .text        PROGBITS 0000000000001000 001000 000001 00  AX  0   0  4
 # CHECK-NEXT: .data.rel.ro PROGBITS 0000000000011000 002000 000001 00  WA  0   0 16
-# CHECK-NEXT: .data        PROGBITS 0000000000011010 002010 000001 00  WA  0   0 16
+# CHECK-NEXT: .data        PROGBITS 0000000000011010 002010 000011 00  WA  0   0 32
 # CHECK-NEXT: .bss         NOBITS   0000000000011040 002040 000001 00  WA  0   0 64
 
 # CHECK:      Type  Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
 # CHECK-NEXT: LOAD  0x001000 0x0000000000001000 0x0000000000001000 0x000001 0x000001 R E 0x1000
 # CHECK-NEXT: LOAD  0x002000 0x0000000000011000 0x0000000000001010 0x000001 0x000001 RW  0x1000
-# CHECK-NEXT: LOAD  0x002010 0x0000000000011010 0x0000000000001020 0x000001 0x000001 RW  0x1000
+# CHECK-NEXT: LOAD  0x002010 0x0000000000011010 0x0000000000001020 0x000011 0x000011 RW  0x1000
 # CHECK-NEXT: LOAD  0x002040 0x0000000000011040 0x0000000000011040 0x000000 0x000001 RW  0x1000
 
 MEMORY {
   ROM : ORIGIN = 0x1000, LENGTH = 1K
   RAM : ORIGIN = 0x11000, LENGTH = 1K
 }
 SECTIONS {
   .text 0x1000 : { *(.text*) } >ROM
   ## Input section alignment decides output section alignment.
   .data.rel.ro 0x11000 : { *(.data.rel.ro) } >RAM AT>ROM
   ## ALIGN decides output section alignment.
   .data . : ALIGN(16) { *(.data*) } >RAM AT>ROM
   ## Start a new PT_LOAD because the LMA region is different from the previous one.
   .bss . : ALIGN(64) { *(.bss*) } >RAM
 }

lld/test/ELF/linkerscript/outsections-addr.s

 #CHECK:  }
 #CHECK:  Section {
 #CHECK:    Index: 4
 #CHECK:    Name: .ddd
 #CHECK:    Type: SHT_PROGBITS
 #CHECK:    Flags [
 #CHECK:      SHF_ALLOC
 #CHECK:    ]
-#CHECK:    Address: 0x5010
-#CHECK:    Offset: 0x4010
-#CHECK:    Size: 8
+#CHECK:    Address: 0x5001
+#CHECK:    Offset: 0x4001
+#CHECK:    Size: 23
 #CHECK:    Link: 0
 #CHECK:    Info: 0
 #CHECK:    AddressAlignment: 16
 #CHECK:    EntrySize: 0
 #CHECK:  }
 
 .globl _start
 _start:

lld/test/ELF/linkerscript/section-align2.test

+# REQUIRES: aarch64
+## Test ALIGN and its interaction with explicit output section addresses.
+
+# RUN: echo '.globl _start; _start: ret; .data.rel.ro; .balign 8; .byte 0; .data; .byte 0; \
+# RUN:   .section .data2,"aw"; .balign 8; .byte 0; .bss; .balign 32; .byte 0' | \
+# RUN:   llvm-mc -filetype=obj -triple=aarch64 - -o %t.o
+# RUN: ld.lld -T %s %t.o -o %t 2>&1 | count 0
+# RUN: llvm-readelf -S %t | FileCheck %s
+
+# CHECK:      Name         Type     Address          Off    Size   ES Flg Lk Inf Al
+# CHECK-NEXT:              NULL     0000000000000000 000000 000000 00      0   0  0
+# CHECK-NEXT: .text        PROGBITS 0000000000010000 010000 000004 00  AX  0   0  4
+# CHECK-NEXT: .data.rel.ro PROGBITS 0000000000010010 010010 000001 00  WA  0   0 16
+# CHECK-NEXT: .data        PROGBITS 0000000000020000 020000 000001 00  WA  0   0  1
+# CHECK-NEXT: .data2       PROGBITS 0000000000020001 020001 000008 00  WA  0   0  8
+# CHECK-NEXT: .bss         NOBITS   0000000000020010 020009 000011 00  WA  0   0 32
+
+SECTIONS {
+  .text 0x10000 : { *(.text) }
+  ## sh_addr is aligned to 16.
+  .data.rel.ro . : ALIGN(16) { *(.data.rel.ro) }
+
+  .data 0x20000 : { *(.data) }
+  ## The output section address is set without ALIGN. sh_addr is set to Dot, ignoring alignment.
+  ## sh_addralign is the maximum of input section alignments, 8.
+  .data2 . : { *(.data2) }
+  ## sh_addr is aligned to 16.
+  ## The input section has a larger alignment and is thus preceded by a gap.
+  .bss . : ALIGN(16) { *(.bss) }
+}