[ELF] - Fix for ld.lld does not accept "AT" syntax for declaring LMA region

[grimar]

Summary

AT> lma_region expression allows to specify the memory region
for section load address.

Should fix PR35684.

[grimar]

• Fixed initial logic (all tests pass now).

[ruiu]

ELF/LinkerScript.cpp

802 ↗

(On Diff #127854)

This function call is not desirable because we know that Sec is not used by the function. Please improve, split or whatever you can do to findMemoryRegion so that its semantics becomes more clear.

ELF/ScriptParser.cpp

711 ↗

(On Diff #127854)

This seems unnecessarily complicated. Why don't you just not allow a space between "AT" and ">"?

714 ↗

(On Diff #127854)

Use consume

718 ↗

(On Diff #127854)

This order isn't easy to understand because it is not obvious what drop_front drops (which is ">", but you need to carefully read the code to find it out.) Please write this way

if (consume(">"))
else if (peek().startswith(">"))
else

[grimar]

ELF/ScriptParser.cpp

711 ↗

(On Diff #127854)

I checked that gnu linkers support all 4 forms:

AT>REGION
AT> REGION
AT >REGION
AT > REGION

PR mentions 2 ways: "AT> syntax" and "AT > syntax".
Spec mentions third: "AT>lma_region" (https://www.eecs.umich.edu/courses/eecs373/readings/Linker.pdf, p.48).

I guess we just want to support all of them then.

[ruiu]

ELF/ScriptParser.cpp

711 ↗

(On Diff #127854)

It's a little bit pathetic. If you want to do that, please try changing the lexer so that ">" is always parsed as a single-character token instead of a regular token character

[grimar]

ELF/ScriptParser.cpp

711 ↗

(On Diff #127854)

Changing lexer feels a bit overkill for this feature for me, though looks it probably the most correct way.
I'll try it, thanks.

[ruiu]

I don't think it's overkill. Please take a look at https://reviews.llvm.org/D41577

[grimar]

• Rebased, addressed review comments.

[ruiu]

ELF/LinkerScript.h

223–225 ↗

(On Diff #128176)

Well, I think you already knew what I would be saying, but these names are not good. I think you just chose "pick" because "find" is already in use. That's not a good way of naming a function.

ELF/ScriptParser.cpp

711 ↗

(On Diff #127854)

This part of code looks much nicer now.

[grimar]

ELF/LinkerScript.h

223–225 ↗

(On Diff #128176)

I had to search different wording because find is already in use, that is obvious.
At first I tried findNamedMemoryRegion and findMemoryRegion, but did not like it. I chose pick because in compare with
findMemoryRegion which simply searches region by name, pickMemoryRegion uses more complex rules,
including heuristic to select the appropriate memory region for section.

After your comment I experimented with this code again, but found that I like what these methods do, so
I think problem is with naming and not with their logic/how they were splitted. Do you agree here ?

If so, what do you think about following naming ?

MemoryRegion *findMemoryRegion(llvm::StringRef Name);
MemoryRegion *matchMemoryRegion(OutputSection *Sec);

or

MemoryRegion *findMemoryRegion(llvm::StringRef Name);
MemoryRegion *selectMemoryRegion(OutputSection *Sec);

[ruiu]

ELF/LinkerScript.cpp

676 ↗

(On Diff #128176)

Looks like you don't even need this LMARegion member in the first place, as you can just check if LMARegionName is empty here.

[grimar]

• Addressed review comment.

ELF/LinkerScript.cpp

676 ↗

(On Diff #128176)

Removed LMARegion member.

[ruiu]

ELF/LinkerScript.cpp

676 ↗

(On Diff #129061)

Inline findMemoryRegion and delete that function.

802 ↗

(On Diff #129061)

Rename this findMemoryRegion.

[grimar]

• Addressed review comments.

[grimar]

• Addressed review comments (improved testcase).

[ruiu]

LGTM

lld/trunk/ELF/LinkerScript.cpp

void LinkerScript::assignOffsets(OutputSection *Sec) {
   if (Ctx->MemRegion)
     Dot = Ctx->MemRegionOffset[Ctx->MemRegion];
 
   if (Sec->LMAExpr) {
     uint64_t D = Dot;
     Ctx->LMAOffset = [=] { return Sec->LMAExpr().getValue() - D; };
   }
 
+  if (!Sec->LMARegionName.empty()) {
+    if (MemoryRegion *MR = MemoryRegions.lookup(Sec->LMARegionName)) {
+      uint64_t Offset = MR->Origin - Dot;
+      Ctx->LMAOffset = [=] { return Offset; };
+    } else {
+      error("memory region '" + Sec->LMARegionName + "' not declared");
+    }
+  }
+
   switchTo(Sec);
 
   // The Size previously denoted how many InputSections had been added to this
   // section, and was used for sorting SHF_LINK_ORDER sections. Reset it to
   // compute the actual size value.
   Sec->Size = 0;
 
   // We visited SectionsCommands from processSectionCommands to

lld/trunk/ELF/OutputSections.h

public:
   Expr LMAExpr;
   Expr SubalignExpr;
   std::vector<BaseCommand *> SectionCommands;
   std::vector<StringRef> Phdrs;
   llvm::Optional<uint32_t> Filler;
   ConstraintKind Constraint = ConstraintKind::NoConstraint;
   std::string Location;
   std::string MemoryRegionName;
+  std::string LMARegionName;
   bool Noload = false;
 
   template <class ELFT> void finalize();
   template <class ELFT> void writeTo(uint8_t *Buf);
   template <class ELFT> void maybeCompress();
 
   void sort(std::function<int(InputSectionBase *S)> Order);
   void sortInitFini();

lld/trunk/ELF/ScriptParser.cpp

while (!errorCount() && !consume("}")) {
     } else {
       setError("unknown command " + Tok);
     }
   }
 
   if (consume(">"))
     Cmd->MemoryRegionName = next();
 
+  if (consume("AT")) {
+    expect(">");
+    Cmd->LMARegionName = next();
+  }
+
+  if (Cmd->LMAExpr && !Cmd->LMARegionName.empty())
+    error("section can't have both LMA and a load region");
+
   Cmd->Phdrs = readOutputSectionPhdrs();
 
   if (consume("="))
     Cmd->Filler = parseFill(next());
   else if (peek().startswith("="))
     Cmd->Filler = parseFill(next().drop_front());
 
   // Consume optional comma following output section command.

lld/trunk/test/ELF/linkerscript/at2.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: echo "MEMORY {                                   \
+# RUN:   AX (ax)   : ORIGIN = 0x2000, LENGTH = 0x100   \
+# RUN:   AW (aw)   : ORIGIN = 0x3000, LENGTH = 0x100   \
+# RUN:   FLASH (ax) : ORIGIN = 0x6000, LENGTH = 0x100   \
+# RUN:   RAM (aw)   : ORIGIN = 0x7000, LENGTH = 0x100 } \
+# RUN: SECTIONS {                                       \
+# RUN:  .foo1 : { *(.foo1) } > AX AT>FLASH             \
+# RUN:  .foo2 : { *(.foo2) } > AX                      \
+# RUN:  .bar1 : { *(.bar1) } > AW AT> RAM              \
+# RUN:  .bar2 : { *(.bar2) } > AW AT > RAM             \
+# RUN:  .bar3 : { *(.bar3) } > AW AT >RAM              \
+# RUN: }" > %t.script
+# RUN: ld.lld %t --script %t.script -o %t2
+# RUN: llvm-readobj -program-headers %t2 | FileCheck %s
+# RUN: llvm-objdump -section-headers %t2 | FileCheck %s --check-prefix=SECTIONS
+
+# CHECK:      ProgramHeaders [
+# CHECK-NEXT:   ProgramHeader {
+# CHECK-NEXT:     Type: PT_LOAD
+# CHECK-NEXT:     Offset: 0x1000
+# CHECK-NEXT:     VirtualAddress: 0x2000
+# CHECK-NEXT:     PhysicalAddress: 0x6000
+# CHECK-NEXT:     FileSize: 16
+# CHECK-NEXT:     MemSize: 16
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       PF_R
+# CHECK-NEXT:       PF_X
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Alignment:
+# CHECK-NEXT:   }
+# CHECK-NEXT:   ProgramHeader {
+# CHECK-NEXT:     Type: PT_LOAD
+# CHECK-NEXT:     Offset: 0x2000
+# CHECK-NEXT:     VirtualAddress: 0x3000
+# CHECK-NEXT:     PhysicalAddress: 0x7000
+# CHECK-NEXT:     FileSize: 24
+# CHECK-NEXT:     MemSize: 24
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       PF_R
+# CHECK-NEXT:       PF_W
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Alignment: 4096
+# CHECK-NEXT:   }
+
+# SECTIONS:      Sections:
+# SECTIONS-NEXT: Idx Name          Size      Address
+# SECTIONS-NEXT:   0               00000000 0000000000000000
+# SECTIONS-NEXT:   1 .foo1         00000008 0000000000002000
+# SECTIONS-NEXT:   2 .foo2         00000008 0000000000002008
+# SECTIONS-NEXT:   3 .text         00000000 0000000000002010
+# SECTIONS-NEXT:   4 .bar1         00000008 0000000000003000
+# SECTIONS-NEXT:   5 .bar2         00000008 0000000000003008
+# SECTIONS-NEXT:   6 .bar3         00000008 0000000000003010
+  
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: echo "MEMORY {                                            \
+# RUN:   FLASH (ax) : ORIGIN = 0x2000, LENGTH = 0x100            \
+# RUN:   RAM (aw)   : ORIGIN = 0x5000, LENGTH = 0x100 }          \
+# RUN: SECTIONS {                                                \
+# RUN:  .foo1 : AT(0x500) { *(.foo1) } > FLASH AT>FLASH          \
+# RUN: }" > %t2.script
+# RUN: not ld.lld %t --script %t2.script -o %t2 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=ERR
+# ERR: error: section can't have both LMA and a load region
+
+.section .foo1, "ax"
+.quad 0
+
+.section .foo2, "ax"
+.quad 0
+
+.section .bar1, "aw"
+.quad 0
+
+.section .bar2, "aw"
+.quad 0
+
+.section .bar3, "aw"
+.quad 0