[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

810

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

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

714

Use consume

718

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

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

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

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

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

This part of code looks much nicer now.

[grimar]

ELF/LinkerScript.h

223–225

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

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

Removed LMARegion member.

[ruiu]

ELF/LinkerScript.cpp

676

Inline findMemoryRegion and delete that function.

807–810

Rename this findMemoryRegion.

[grimar]

• Addressed review comments.

[grimar]

• Addressed review comments (improved testcase).

[ruiu]

LGTM

ELF/LinkerScript.h

computeInputSections(const InputSectionDescription *,
                        const llvm::DenseMap<SectionBase *, int> &Order);
 
   std::vector<InputSection *>
   createInputSectionList(OutputSection &Cmd,
                          const llvm::DenseMap<SectionBase *, int> &Order);
 
   std::vector<size_t> getPhdrIndices(OutputSection *Sec);
 
-  MemoryRegion *findMemoryRegion(OutputSection *Sec);
+  MemoryRegion *findMemoryRegion(llvm::StringRef Name);
+  MemoryRegion *pickMemoryRegion(OutputSection *Sec);
 
   void switchTo(OutputSection *Sec);
   uint64_t advance(uint64_t Size, unsigned Align);
   void output(InputSection *Sec);
 
   void assignOffsets(OutputSection *Sec);
 
   // Ctx captures the local AddressState and makes it accessible
   // deliberately. This is needed as there are some cases where we cannot just

ELF/LinkerScript.cpp

void LinkerScript::switchTo(OutputSection *Sec) {
   // If neither AT nor AT> is specified for an allocatable section, the linker
   // will set the LMA such that the difference between VMA and LMA for the
   // section is the same as the preceding output section in the same region
   // https://sourceware.org/binutils/docs-2.20/ld/Output-Section-LMA.html
   if (Ctx->LMAOffset)
     Ctx->OutSec->LMAOffset = Ctx->LMAOffset();
 }
 
-// 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()) {
-    auto It = MemoryRegions.find(Sec->MemoryRegionName);
-    if (It != MemoryRegions.end())
-      return It->second;
-    error("memory region '" + Sec->MemoryRegionName + "' not declared");
-    return nullptr;
-  }
+// Function finds memory region by given name.
+MemoryRegion *LinkerScript::findMemoryRegion(StringRef Name) {
+  if (Name.empty())
+    return nullptr;
+  auto It = MemoryRegions.find(Name);
+  error("memory region '" + Name + "' not declared");
 
+// This function tries to pick up a memory region to place the given
+// output section in. At first it searches region by name, then by section
+// flags. Returns pointer to the appropriate memory region or nullptr if no
+// region was found.
+MemoryRegion *LinkerScript::pickMemoryRegion(OutputSection *Sec) {
+  // If a memory region name was specified in the output section command,
+  // then try to find that region first.
+  if (MemoryRegion *MR = findMemoryRegion(Sec->MemoryRegionName))
+    return MR;
+
   // If at least one memory region is defined, all sections must
   // belong to some memory region. Otherwise, we don't need to do
   // anything for memory regions.
   if (MemoryRegions.empty())
     return nullptr;
 
   // See if a region can be found by matching section flags.
   for (auto &Pair : MemoryRegions) {
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->LMARegion) {
+    uint64_t D = Dot;
+    Ctx->LMAOffset = [=] { return Sec->LMARegion->Origin - D; };
+  }
 
   switchTo(Sec);
 
   // We do not support custom layout for compressed debug sectons.
   // At this point we already know their size and have compressed content.
   if (Ctx->OutSec->Flags & SHF_COMPRESSED)
     return;
 

 }
 
 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);
+
+      Sec->MemRegion = pickMemoryRegion(Sec);
+      Sec->LMARegion = findMemoryRegion(Sec->LMARegionName);
+
       // 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,

ELF/OutputSections.h

public:
 
   void addSection(InputSection *IS);
 
   // Location in the output buffer.
   uint8_t *Loc = nullptr;
 
   // The following members are normally only used in linker scripts.
   MemoryRegion *MemRegion = nullptr;
+  MemoryRegion *LMARegion = nullptr;
   Expr AddrExpr;
   Expr AlignExpr;
   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