[LLD][ELF] Fix problems with fabricateDefaultCommands() and --section-start

[peter.smith]

Summary

The --section-start <name>=<address> needs to be translated into equivalent linker script commands. There are a couple of problems with the existing implementation:

• The --section-start with the lowest address is assumed to be at the start of the map. This assumption is incorrect, we have to iterate through the SectionStartMap to find the lowest address.
• The addresses in --section-start were being over-aligned when the sections were marked as PageAlign. This is inconsistent with the use of SectionStartMap in fixHeaders(), and can cause problems when the PageAlign causes an "unable to move location counter backward" error when the --section-start section with PageAlign is aligned to an address higher than the next --section-start without PageAlign. The ld.bfd and ld.gold seem to be more consistent with this interpretation but this is not a well specified area.

This change fixes the problems above and also corrects a typo in which fabricateDefaultCommands() is called with the wrong parameter, it should be called with AllocateHeader not Config->MaxPageSize.

The problems were found while investigating dmikulin's comments in D31888 , however I don't think that these changes are directly related.

[ruiu]

LGTM

ELF/LinkerScript.cpp

431–433 ↗

(On Diff #97445)

Can you add full stops to this comment?

434 ↗

(On Diff #97445)

You don't need to check if it is empty, as the following for-loop loops 0 times if it is empty.

lld/trunk/ELF/LinkerScript.cpp

 void LinkerScript::fabricateDefaultCommands(bool AllocateHeader) {
   std::vector<BaseCommand *> Commands;
 
   // Define start address
   uint64_t StartAddr = Config->ImageBase;
   if (AllocateHeader)
     StartAddr += elf::getHeaderSize();
 
-  // The Sections with -T<section> are sorted in order of ascending address
-  // we must use this if it is lower than StartAddr as calls to setDot() must
-  // be monotonically increasing
-  if (!Config->SectionStartMap.empty()) {
-    uint64_t LowestSecStart = Config->SectionStartMap.begin()->second;
-    StartAddr = std::min(StartAddr, LowestSecStart);
-  }
+  // The Sections with -T<section> have been sorted in order of ascending
+  // address. We must lower StartAddr if the lowest -T<section address> as
+  // calls to setDot() must be monotonically increasing.
+  for (auto& KV : Config->SectionStartMap)
+    StartAddr = std::min(StartAddr, KV.second);
+
   Commands.push_back(
       make<SymbolAssignment>(".", [=] { return StartAddr; }, ""));
 
   // For each OutputSection that needs a VA fabricate an OutputSectionCommand
   // with an InputSectionDescription describing the InputSections
   for (OutputSection *Sec : *OutputSections) {
     if (!(Sec->Flags & SHF_ALLOC))
       continue;
 
+    auto *OSCmd = make<OutputSectionCommand>(Sec->Name);
+    OSCmd->Sec = Sec;
+
+    // Prefer user supplied address over additional alignment constraint
     auto I = Config->SectionStartMap.find(Sec->Name);
     if (I != Config->SectionStartMap.end())
       Commands.push_back(
           make<SymbolAssignment>(".", [=] { return I->second; }, ""));
-
-    auto *OSCmd = make<OutputSectionCommand>(Sec->Name);
-    OSCmd->Sec = Sec;
-    if (Sec->PageAlign)
+    else if (Sec->PageAlign)
       OSCmd->AddrExpr = [=] {
         return alignTo(Script->getDot(), Config->MaxPageSize);
       };
+
     Commands.push_back(OSCmd);
     if (Sec->Sections.size()) {
       auto *ISD = make<InputSectionDescription>("");
       OSCmd->Commands.push_back(ISD);
       for (InputSection *ISec : Sec->Sections) {
         ISD->Sections.push_back(ISec);
         ISec->Assigned = true;
       }

lld/trunk/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::run() {
   if (ErrorCount)
     return;
 
   if (Config->Relocatable) {
     assignFileOffsets();
   } else {
     if (!Script->Opt.HasSections) {
       fixSectionAlignments();
-      Script->fabricateDefaultCommands(Config->MaxPageSize);
+      Script->fabricateDefaultCommands(AllocateHeader);
     }
     Script->synchronize();
     Script->assignAddresses(Phdrs);
 
     // Remove empty PT_LOAD to avoid causing the dynamic linker to try to mmap a
     // 0 sized region. This has to be done late since only after assignAddresses
     // we know the size of the sections.
     removeEmptyPTLoad();

lld/trunk/test/ELF/sectionstart-noallochdr.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld %t.o --section-start .data=0x20 \
+# RUN: --section-start .bss=0x30 --section-start .text=0x10 -o %t1
+# RUN: llvm-objdump -section-headers %t1 | FileCheck %s
+
+# CHECK:      Sections:
+# CHECK-NEXT:  Idx Name          Size      Address          Type
+# CHECK-NEXT:    0               00000000 0000000000000000
+# CHECK-NEXT:    1 .text         00000001 0000000000000010 TEXT DATA
+# CHECK-NEXT:    2 .data         00000004 0000000000000020 DATA
+# CHECK-NEXT:    3 .bss          00000004 0000000000000030 BSS
+
+.text
+.globl _start
+_start:
+ nop
+
+.data
+.long 0
+
+.bss
+.zero 4