[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

1

Can it be x86?

26

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

1

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

[MaskRay]

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

1

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

26

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 ↗

(On Diff #245258)

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 ↗

(On Diff #245258)

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 ↗

(On Diff #245258)

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 ↗

(On Diff #245258)

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 ↗

(On Diff #245258)

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

769

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 ↗

(On Diff #245258)

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

771

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 ↗

(On Diff #245258)

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

[MaskRay]

lld/ELF/LinkerScript.cpp

771

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) {
   // .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);
+  uint32_t align =
+      sec->alignExpr ? sec->alignExpr().getValue() : ctx->outSec->alignment;
+  ctx->outSec->addr = advance(0, align);
   expandMemoryRegions(ctx->outSec->addr - before);
 }
 
 // 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.
   if (!sec->memoryRegionName.empty()) {
     if (MemoryRegion *m = memoryRegions.lookup(sec->memoryRegionName))
       return m;

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/section-align2.test

+# REQUIRES: aarch64
+# 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 0000000000020008 020008 000001 00  WA  0   0  8
+# CHECK-NEXT: .bss         NOBITS   0000000000020010 020009 000011 00  WA  0   0 32
+
+SECTIONS {
+  .text 0x10000 : {}
+  ## The output section is aligned to 16.
+  .data.rel.ro . : ALIGN(16) {}
+
+  .data 0x20000 : {}
+  ## The output section is aligned to 8.
+  .data2 . : { *(.data2) }
+  ## The output section is aligned to 16.
+  ## The input section has a larger alignment and is thus preceded by a gap.
+  .bss . : ALIGN(16) {}
+}