[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

803

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

712

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

715

Use consume

719

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

712

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

712

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

712

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

712

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

669

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

669

Removed LMARegion member.

[ruiu]

ELF/LinkerScript.cpp

669

Inline findMemoryRegion and delete that function.

800

Rename this findMemoryRegion.

[grimar]

• Addressed review comments.

[grimar]

• Addressed review comments (improved testcase).

[ruiu]

LGTM

ELF/LinkerScript.cpp

void LinkerScript::assignOffsets(OutputSection *Sec) {
   Ctx->MemRegion = Sec->MemRegion;
   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

 }
 
 void LinkerScript::adjustSectionsAfterSorting() {
   // Try and find an appropriate memory region to assign offsets in.
   for (BaseCommand *Base : SectionCommands) {
     if (auto *Sec = dyn_cast<OutputSection>(Base)) {
       if (!Sec->Live)
         continue;
       Sec->MemRegion = findMemoryRegion(Sec);
       // Handle align (e.g. ".foo : ALIGN(16) { ... }").
       if (Sec->AlignExpr)
         Sec->Alignment =
             std::max<uint32_t>(Sec->Alignment, Sec->AlignExpr().getValue());
     }
   }
 
   // If output section command doesn't specify any segments,
   // and we haven't previously assigned any section to segment,
   // then we simply assign section to the very first load segment.
   // Below is an example of such linker script:

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();

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.

test/ELF/linkerscript/at2.s

+# REQUIRES: x86
+# 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 : { *(.foo1) } > FLASH AT>FLASH           \
+# RUN:  .foo2 : { *(.foo2) } > FLASH                    \
+# RUN:  .bar1 : { *(.bar1) } > RAM AT> RAM              \
+# RUN:  .bar2 : { *(.bar2) } > RAM AT > RAM             \
+# RUN:  .bar3 : { *(.bar3) } > RAM AT >RAM              \
+# RUN: }" > %t.script
+# RUN: ld.lld %t --script %t.script -o %t2
+# RUN: llvm-readobj -program-headers %t2 | FileCheck %s
+
+# 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
+
+# CHECK:      ProgramHeaders [
+# CHECK-NEXT:   ProgramHeader {
+# CHECK-NEXT:     Type: PT_LOAD
+# CHECK-NEXT:     Offset: 0x1000
+# CHECK-NEXT:     VirtualAddress: 0x2000
+# CHECK-NEXT:     PhysicalAddress: 0x2000
+# CHECK-NEXT:     FileSize:
+# CHECK-NEXT:     MemSize:
+# 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: 0x5000
+# CHECK-NEXT:     PhysicalAddress: 0x5000
+# CHECK-NEXT:     FileSize:
+# CHECK-NEXT:     MemSize:
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       PF_R
+# CHECK-NEXT:       PF_W
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Alignment: 4096
+# CHECK-NEXT:   }
+
+.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