[LLD] Improve handling of AT> linker script commands

[kschwarz]

Summary

The condition to create a new phdr must also check the usage of "AT>" linker script command, and create a new PT_LOAD header if a new LMARegion is used.

This fixes PR38307

[grimar]

Seems several independent bugs are fixed here. I would suggest splitting this patch to independent reviews.

(Also, an uploaded patch has no context. Please include it next time (https://llvm.org/docs/Phabricator.html#id3).)

ELF/LinkerScript.cpp

120 ↗

(On Diff #158215)

Can you split this change and the corresponding test case (at8.test) to the different patch?

test/ELF/linkerscript/at8.test

10 ↗

(On Diff #158215)

Do you really need an alias?

13 ↗

(On Diff #158215)

Please remove excessive empty lines here and below.

[kschwarz]

ELF/LinkerScript.cpp

120 ↗

(On Diff #158215)

Done in https://reviews.llvm.org/D50065

test/ELF/linkerscript/at8.test

10 ↗

(On Diff #158215)

I used the REGION_ALIAS feature to illustrate how this scenario might occur in real world linker scripts.

[kschwarz]

Split one of the fixes into its own patch.

[grimar]

Can this be split into 2 different patches?

[kschwarz]

Split patch into seperate patches (https://reviews.llvm.org/D50133)

[grimar]

I think it is fine (with minor nit). I would wait for the landing of the D50133 first though. It might affect the code of this patch perhaps I think.

ELF/Writer.cpp

1826 ↗

(On Diff #158476)

The comment above needs an update.

[grimar]

On the second look, this change is more independent from D50133 than I thought initially.
So, LGTM with the comment updated.

[kschwarz]

Update comment

[grimar]

LGTM

[grimar]

do you have a commit access?

[kschwarz]

No, could you commit for me? Thanks!

[ruiu]

Could you give me a few days to review code since we are working in different time zones?

lld/trunk/ELF/Writer.cpp

1823

It feels to me that the condition is now too complicated. The code for the linker script is basically too complicated, and I don't like to add too much complexity.

[grimar]

In D50052#1186364, @ruiu wrote:

Could you give me a few days to review code since we are working in different time zones?

Could you comment D49374 and D49136, please. If the issue is different time zones.

lld/trunk/ELF/Writer.cpp

for (OutputSection *Sec : OutputSections) {
     if (!(Sec->Flags & SHF_ALLOC))
       break;
     if (!needsPtLoad(Sec))
       continue;
 
     // Segments are contiguous memory regions that has the same attributes
     // (e.g. executable or writable). There is one phdr for each segment.
     // Therefore, we need to create a new phdr when the next section has
-    // different flags or is loaded at a discontiguous address using AT linker
-    // script command. At the same time, we don't want to create a separate
-    // load segment for the headers, even if the first output section has
-    // an AT attribute.
+    // different flags or is loaded at a discontiguous address or memory
+    // region using AT or AT> linker script command, respectively. At the same
+    // time, we don't want to create a separate load segment for the headers,
+    // even if the first output section has an AT or AT> attribute.
     uint64_t NewFlags = computeFlags(Sec->getPhdrFlags());
-    if ((Sec->LMAExpr && Load->LastSec != Out::ProgramHeaders) ||
+    if (((Sec->LMAExpr ||
+          (Sec->LMARegion && (Sec->LMARegion != Load->FirstSec->LMARegion))) &&
+         Load->LastSec != Out::ProgramHeaders) ||
         Sec->MemRegion != Load->FirstSec->MemRegion || Flags != NewFlags) {
 
       Load = AddHdr(PT_LOAD, NewFlags);
       Flags = NewFlags;
     }
 
     Load->add(Sec);
   }

lld/trunk/test/ELF/linkerscript/Inputs/at6.s

+.global _start
+.text
+_start:
+nop
+
+.section .sec1,"aw",@progbits
+.long 1
+
+.section .sec2,"aw",@progbits
+.long 2
+

lld/trunk/test/ELF/linkerscript/at6.test

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/at6.s -o %t.o
+# RUN: ld.lld %t.o --script %s -o %t
+# RUN: llvm-readelf -sections -program-headers %t | FileCheck %s
+
+MEMORY {
+  FLASH : ORIGIN = 0x08000000, LENGTH = 0x100
+  RAM   : ORIGIN = 0x20000000, LENGTH = 0x200
+}
+
+SECTIONS {
+ .text : { *(.text) } > FLASH
+ .sec1 : { *(.sec1) } > RAM
+ .sec2 : { *(.sec2) } > RAM AT > FLASH
+}
+
+# Make sure we create a separate PT_LOAD entry for .sec2. Previously,
+# it was added to the PT_LOAD entry of .sec1
+
+# CHECK: Name              Type            Address          Off
+# CHECK: .text             PROGBITS        0000000008000000 001000
+# CHECK: .sec1             PROGBITS        0000000020000000 002000
+# CHECK: .sec2             PROGBITS        0000000020000004 002004
+
+# CHECK: Program Headers:
+# CHECK:      Type  Offset   VirtAddr           PhysAddr
+# CHECK-NEXT: LOAD  0x001000 0x0000000008000000 0x0000000008000000
+# CHECK-NEXT: LOAD  0x002000 0x0000000020000000 0x0000000020000000
+# CHECK-NEXT: LOAD  0x002004 0x0000000020000004 0x0000000008000001
+# CHECK-NOT: LOAD