[LLD][ELF] Always use Script::assignAddresses()

[peter.smith]

Summary

This change fabricates linker script commands for the case where there is no linker script SECTIONS to control address assignment. This permits us to have a single Script->assignAddresses() function.

There is a small change in user-visible-behavior with respect to the handling of .tbss SHT_NOBITS, SHF_TLS as the Script->assignAddresses() requires setDot() to be called with monotically increasing addresses. The tls-offset.s test has been updated so that the script and non-script results match. ld.bfd and ld.gold behave similarly to lld with a linker script in that dot is always monotonically increasing.

[ruiu]

ELF/LinkerScript.cpp

411 ↗

(On Diff #94673)

You are not using Phdrs?

412 ↗

(On Diff #94673)

MaxPageSize can always be obtained by Config->MaxPageSize, so no need to pass it as an argument.

420 ↗

(On Diff #94673)

Add a blank line before comment.

427–429 ↗

(On Diff #94673)

This can be

Commands.push_back(make<SymbolAssignment>(".", [=] { return StartAddr; }, ""));

437–442 ↗

(On Diff #94673)

This can be something like

if (uint64_t Addr = Config->SectionStartMap.lookup(Sec->Name))
  Commands.push_back(make<SymbolAssignment>(".", [=] { return Addr; }, ""));

[peter.smith]

Updated diff to address review comments.
Apologies for leaving PHDRS in, I had originally attempted to merge fixSectionAlignments() into FabricateSectionAlignments(), but decided against it.

I've left in:

auto I = Config->SectionStartMap.find(Sec->Name);
if (I != Config->SectionStartMap.end())

Rather than use lookup(). If my understanding is correct, lookup() will return 0 if there is no entry in SectionStart which is a valid address that can be set with -section-start.

[ruiu]

LGTM

I don't think this is by itself an improvement, but I can see that this could make it easier to implement thunk support.

[peter.smith]

Thanks for the review. I'll hold off on committing this till I get back from the UK Easter public holidays just in case this causes problems outside of the tests and I'm not around to fix them.

I've also made some progress on what the next steps would need to be to make thunks work with InputSectionDescriptions. I'll summarise the findings in the llvm-dev thread later today.

[peter.smith]

Now committed r300687.

[dmikulin]

Can this review be re-opened?
There's an inconsistency in the arg passing to fabricateDefaultCommands(). It's declared as taking "bool AllocateHeader" parameter, but called with "Config->MaxPageSize" as an argument, so AllocateHeader is pretty much always true. This creates a problem when headers are not to be allocated, as is the case on our platform. I'm still trying to figure out how to deal with it, since fixing the argument does not fix the problem: second call to allocateHeaders(), from assignAddresses() moves the starting address below MinVA.

[peter.smith]

I've been having a bit of trouble trying to reproduce the problem from the source code with enough confidence to know it is the same problem you are seeing. I've definitely found one other problem which I have a fix for but I don't think it is the same thing.

• The first thing I did was to fix the problem you mention and pass AllocateHeader to FabricateDefaultCommands(). I don't see any test failures but that doesn't mean there isn't a problem.
• From what I can see of AllocateHeader = allocateHeaders(Phdrs, OutputSections, Min) is that it returns false only when there are no PT_LOAD program headers or if HeaderSize > Min.
• Min is set to either -1 (Unsigned so Wrap to max) or the minimum VA of the sections in the SectionStartMap. These are AFAIK only set with -Ttext=addr or --section-start .text=addr
  • Min is set regardless of whether the section is SHF_ALLOC or not
• If I'm correct, I assume that your target is using some kind of SectionStartMap, or you have some downstream modification that I don't know about.
  • Can you let me know how you achieve AllocateHeader=false?
• If AllocateHeader=false then the first call to allocateHeaders terminates before do alignDown(Min - HeaderSize)
• There is a problem in Script::fabricateDefaultCommands() in that it uses the first section in the SectionStartMap to find the minimum address instead of iterating through it and find the lowest so we can get the wrong value for lowest and get an error message from setDot(). This is trivial to fix by looping through and finding the minimum.
  • As you don't mention an error I'm guessing that this isn't the problem that you are seeing.
• The second call to allocateHeaders() in assignAddresses() sets the Min from the lowest SHF_ALLOC section address.
  • As non SHF_ALLOC sections are ignored we can enter assignAddresses() with a different value of Min from the first call as I can do --section-start non_alloc_section=0x0 which isn't ignored for the first call, but is for the second. This means that the second call to assignAddresses() can in theory get to alignDown(Min - HeaderSize)
  • As the SHF_ALLOC sections are given addresses from the SectionStartMap they should get the same address from the script so if HeaderSize < Min in the first call it should also be the case in the second.

Does this match anything that you are seeing? I think that there are some potential holes here, but I'm not confident that they are actually causing your problem. Would you be able to send some more information, and send a reproducer?

I'll pick this back up on Tuesday.

[bd1976llvm]

I *really* like this change. There should be only one function that assigns addresses.

I am also maintaining support for a target that does not allocate headers. Here are the modifications that I have made in case they are useful to someone else:

1. Change the call to fabricateDefaultCommands() to take false.
2. Remove the call to allocateHeaders() at the end of assignAddresses().
3. Modify allocateHeaders() to return early after doing:

Out::ElfHeader->Addr = Min;
Out::ProgramHeaders->Addr = Min + Out::ElfHeader->Size;

but before doing:

if (FirstPTLoad->First)
  for (OutputSection *Sec : OutputSections)
    if (Sec->FirstInPtLoad == FirstPTLoad->First)
      Sec->FirstInPtLoad = Out::ElfHeader;
FirstPTLoad->First = Out::ElfHeader;
if (!FirstPTLoad->Last)
  FirstPTLoad->Last = Out::ProgramHeaders;
return true;

The question I am struggling with is: Why is allocateHeaders called twice? Could the function fixHeaders() be removed?

There is a comment in Writer.cpp claiming that:

// The headers have to be created before finalize as that can influence the image base and the dynamic section on mips includes the image base.

Is this actually a hard requirement? Surely, given that this is a linker (where one of its principle jobs is to write addresses into holes after the program has been laid out) we can fix up this section later on?

If there is someone who knows this area of the code it would be very much appreciated if they could comment on this.

Thanks in advance!

lld/trunk/ELF/LinkerScript.h

public:
   OutputSection *getOutputSection(const Twine &Loc, StringRef S);
   uint64_t getOutputSectionSize(StringRef S);
   void discard(ArrayRef<InputSectionBase *> V);
 
   ExprValue getSymbolValue(const Twine &Loc, StringRef S);
   bool isDefined(StringRef S);
 
   std::vector<OutputSection *> *OutputSections;
+  void fabricateDefaultCommands(bool AllocateHeader);
   void addOrphanSections(OutputSectionFactory &Factory);
   void removeEmptyCommands();
   void adjustSectionsBeforeSorting();
   void adjustSectionsAfterSorting();
 
   std::vector<PhdrEntry> createPhdrs();
   bool ignoreInterpSection();
 

lld/trunk/ELF/LinkerScript.cpp

if (auto *Cmd = dyn_cast<OutputSectionCommand>(Opt.Commands[I])) {
       // Add input sections to an output section.
       for (InputSectionBase *S : V)
         Factory.addInputSec(S, Cmd->Name);
     }
   }
   CurOutSec = nullptr;
 }
 
+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);
+  }
+  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 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)
+      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;
+      }
+    }
+  }
+  // SECTIONS commands run before other non SECTIONS commands
+  Commands.insert(Commands.end(), Opt.Commands.begin(), Opt.Commands.end());
+  Opt.Commands = std::move(Commands);
+}
+
 // Add sections that didn't match any sections command.
 void LinkerScript::addOrphanSections(OutputSectionFactory &Factory) {
   for (InputSectionBase *S : InputSections)
     if (S->Live && !S->OutSec)
       Factory.addInputSec(S, getOutputSectionName(S->Name));
 }
 
 static bool isTbss(OutputSection *Sec) {

lld/trunk/ELF/Writer.cpp

private:
   void forEachRelSec(std::function<void(InputSectionBase &)> Fn);
   void sortSections();
   void finalizeSections();
   void addPredefinedSections();
 
   std::vector<PhdrEntry> createPhdrs();
   void removeEmptyPTLoad();
   void addPtArmExid(std::vector<PhdrEntry> &Phdrs);
-  void assignAddresses();
   void assignFileOffsets();
   void assignFileOffsetsBinary();
   void setPhdrs();
   void fixHeaders();
   void fixSectionAlignments();
   void fixPredefinedSymbols();
   void openFile();
   void writeHeader();
template <class ELFT> void Writer<ELFT>::run() {
   // finalizeSections does that.
   finalizeSections();
   if (ErrorCount)
     return;
 
   if (Config->Relocatable) {
     assignFileOffsets();
   } else {
-    if (Script->Opt.HasSections) {
-      Script->assignAddresses(Phdrs);
-    } else {
+    if (!Script->Opt.HasSections) {
       fixSectionAlignments();
-      assignAddresses();
-      Script->processNonSectionCommands();
+      Script->fabricateDefaultCommands(Config->MaxPageSize);
     }
+    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();
 
     if (!Config->OFormatBinary)
       assignFileOffsets();
template <class ELFT> void Writer<ELFT>::fixHeaders() {
   uint64_t Min = -1;
   if (!Config->SectionStartMap.empty())
     for (const auto &P : Config->SectionStartMap)
       Min = std::min(Min, P.second);
 
   AllocateHeader = allocateHeaders(Phdrs, OutputSections, Min);
 }
 
-// Assign VAs (addresses at run-time) to output sections.
-template <class ELFT> void Writer<ELFT>::assignAddresses() {
-  uint64_t VA = Config->ImageBase;
-  uint64_t ThreadBssOffset = 0;
-
-  if (AllocateHeader)
-    VA += getHeaderSize();
-
-  for (OutputSection *Sec : OutputSections) {
-    uint32_t Alignment = Sec->Alignment;
-    if (Sec->PageAlign)
-      Alignment = std::max<uint32_t>(Alignment, Config->MaxPageSize);
-
-    auto I = Config->SectionStartMap.find(Sec->Name);
-    if (I != Config->SectionStartMap.end())
-      VA = I->second;
-
-    // We only assign VAs to allocated sections.
-    if (needsPtLoad(Sec)) {
-      VA = alignTo(VA, Alignment);
-      Sec->Addr = VA;
-      VA += Sec->Size;
-    } else if (Sec->Flags & SHF_TLS && Sec->Type == SHT_NOBITS) {
-      uint64_t TVA = VA + ThreadBssOffset;
-      TVA = alignTo(TVA, Alignment);
-      Sec->Addr = TVA;
-      ThreadBssOffset = TVA - VA + Sec->Size;
-    }
-  }
-}
-
 // Adjusts the file alignment for a given output section and returns
 // its new file offset. The file offset must be the same with its
 // virtual address (modulo the page size) so that the loader can load
 // executables without any address adjustment.
 static uint64_t getFileAlignment(uint64_t Off, OutputSection *Sec) {
   OutputSection *First = Sec->FirstInPtLoad;
   // If the section is not in a PT_LOAD, we just have to align it.
   if (!First)

lld/trunk/test/ELF/tls-offset.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 // RUN: ld.lld %t -o %tout
 // RUN: llvm-readobj -s %tout | FileCheck %s
-
+// RUN: echo "SECTIONS { \
+// RUN:   . = 0x201000; \
+// RUN:   .text : { *(.text) } \
+// RUN:   . = 0x202000; \
+// RUN:   .tdata : { *(.tdata) } \
+// RUN:   .tbss : { *(.tbss) } \
+// RUN:   .data.rel.ro : { *(.data.rel.ro) } \
+// RUN: }" > %t.script
+        // RUN: ld.lld -T %t.script %t -o %tout2
+// RUN: echo SCRIPT
+// RUN: llvm-readobj -s %tout2 | FileCheck %s
         .global _start
 _start:
         retq
 
         .section        .tdata,"awT",@progbits
         .align  4
         .long   42
 

 // CHECK-NEXT: Size: 16
 
 // CHECK:      Name: .data.rel.ro
 // CHECK-NEXT: Type: SHT_PROGBITS
 // CHECK-NEXT: Flags [
 // CHECK-NEXT:   SHF_ALLOC
 // CHECK-NEXT:   SHF_WRITE
 // CHECK-NEXT: ]
-// CHECK-NEXT: Address: 0x202004
-// CHECK-NEXT: Offset: 0x2004
+// CHECK-NEXT: Address: 0x202010
+// CHECK-NEXT: Offset: 0x2010
 // CHECK-NEXT: Size: 4