[LLD] Do not overwrite LMAOffset of PT_LOAD header

[kschwarz]

Summary

If more than a single output section is added to a PT_LOAD header, only the first section should set the LMAOffset of the segment. Otherwise, we get a load-address overlap error

[grimar]

ELF/LinkerScript.cpp

794

I think the comment above should be updated to reflect this change.

But also: your test pass if I reduce this to

if (PhdrEntry *L = Ctx->OutSec->PtLoad)
  if (Sec == L->FirstSec)
    L->LMAOffset = Ctx->LMAOffset;

Can you add a test/update it accordingly to check the rest?
(Alternatively, we can go with the reduced code in this patch I think, and leave the rest to follow up patch).

Also, I think that (L->FirstSec == Out::ElfHeader) condition is not correct. Because
FirstSec can be not the ElfHeader, but ProgramHeaders:
https://github.com/llvm-mirror/lld/blob/master/ELF/LinkerScript.cpp#L1073

[kschwarz]

ELF/LinkerScript.cpp

794

The following tests rely on the additional check and fail if it is removed:

lld :: ELF/linkerscript/at-addr.s
lld :: ELF/linkerscript/at.s
lld :: ELF/linkerscript/loadaddr.s
lld :: ELF/linkerscript/merge-header-load.s
lld :: ELF/linkerscript/overlay.test
lld :: ELF/linkerscript/segment-headers.s
lld :: ELF/note-loadaddr.s

The ELF header section is added to the first PT_LOAD header unconditionally here: https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L1807

I think we need to check for both ElfHeader and ProgramHeaders and will try to come up with an additional test case for that.

[grimar]

ELF/LinkerScript.cpp

794

Ah I see, I did not check the other tests.

I think we need to check for both ElfHeader and ProgramHeaders and will try to come up with an additional test case for that.

Yes. I think we might need some little helper function so that it can be reused in places like
https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L1824

The ELF header section is added to the first PT_LOAD header unconditionally here: https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L1807

Yeah, that is for the case when PHDRS command is not used in the linker script. But if it is used, it is not added unconditionally then.

[kschwarz]

ELF/LinkerScript.cpp

794

I don't think the createPhdrs() functions in Writer.cpp and Linkerscript.cpp share a lot of common functionality. What kind of helper function do you have in mind?

With the PHDRS linker script command, the user is in charge of reasonably adding output sections to PT_LOAD segments.
It is possible to create an PT_LOAD header which contains the ProgramHeaders section and multiple other output sections.
Currently we only store the first and last output section of each program header, thus we cannot easily find the first output section after the ELF file header (without iterating over all the section commands once again). A check like (L->FirstSec == Out::ProgramHeaders) && (L->LastSec == Sec) would fail in this case. Is there a better way to find the first section behind elf/program header?

[grimar]

ELF/LinkerScript.cpp

794

I was thinking about something very trivial. Like:

static bool isHeaderSection(InputSection *IS) {
  return IS == Out::ElfHeader || IS == Out::ProgramHeaders;
}

So you could perhaps write:

if (PhdrEntry *L = Ctx->OutSec->PtLoad)
  if ((Sec == L->FirstSec) || (isHeaderSection(L->FirstSec) && (L->LastSec == Sec)))

Does it make sense?

[kschwarz]

ELF/LinkerScript.cpp

794

It would certainly help the readability of the code. I can add that function.

However, this case (FirstSec == Out::ProgramHeaders) can only occur if the PHDRS command is used, and for that case this patch does not provide a solution (see my comment above). Do you have an idea for that problem?

[grimar]

ELF/LinkerScript.cpp

794

I only can think of helper function that would iterate over OutputSections to find the first non-header section for the PT_LOAD given. This should solve the problem, right?

[kschwarz]

ELF/LinkerScript.cpp

794

Yes, that should solve the problem for PHDRS command.

Would you agree to have that in a follow-up patch or should it already go into this one?

[grimar]

ELF/LinkerScript.cpp

794

I would perhaps include it into this one because adding the helper discussed looks like a correct way to go forward here.

I think you can leave the test case as is though (since we already have a number of the tests failing without the new code).

[kschwarz]

Added helper functions.

[grimar]

LGTM, thanks! I'll commit this for you and land a little refactoring change on top I think.

(btw, I think you can request for commit access now if you want/need it).

ELF/LinkerScript.cpp

758

It is taken from LinkerScript.cpp I guess. I think it is ok to have a copy for now since this helper is little.

[grimar]

ELF/LinkerScript.cpp

758

Oops. it is the same file :) Ignore this comment then.

[grimar]

I committed r338699, it removes isHeaderSection.
I think it was my mistake to suggest adding it to this patch.

[kschwarz]

Looks more readable, thanks!

ELF/LinkerScript.cpp

MemoryRegion *LinkerScript::findMemoryRegion(OutputSection *Sec) {
   }
 
   // Otherwise, no suitable region was found.
   if (Sec->Flags & SHF_ALLOC)
     error("no memory region specified for section '" + Sec->Name + "'");
   return nullptr;
 }
 
+static bool isHeaderSection(OutputSection *Sec) {
+  return Sec == Out::ElfHeader || Sec == Out::ProgramHeaders;
+}
+
+static OutputSection *findFirstSection(PhdrEntry *Load) {
+  for (OutputSection *Sec : OutputSections)
+    if (Sec->PtLoad == Load)
+      return Sec;
+  return nullptr;
+}
+
 // This function assigns offsets to input sections and an output section
 // for a single sections command (e.g. ".text { *(.text); }").
 void LinkerScript::assignOffsets(OutputSection *Sec) {
   if (!(Sec->Flags & SHF_ALLOC))
     Dot = 0;
   else if (Sec->AddrExpr)
     setDot(Sec->AddrExpr, Sec->Location, false);
 
void LinkerScript::assignOffsets(OutputSection *Sec) {
 
   if (MemoryRegion *MR = Sec->LMARegion)
     Ctx->LMAOffset = MR->CurPos - Dot;
 
   // 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
+  // This, however, should only be done by the first "non-header" section
+  // in the segment.
   if (PhdrEntry *L = Ctx->OutSec->PtLoad)
+    if ((Sec == L->FirstSec) ||
+        (isHeaderSection(L->FirstSec) && (Sec == findFirstSection(L))))
       L->LMAOffset = Ctx->LMAOffset;
 
   // We can call this method multiple times during the creation of
   // thunks and want to start over calculation each time.
   Sec->Size = 0;
 
   // We visited SectionsCommands from processSectionCommands to
   // layout sections. Now, we visit SectionsCommands again to fix
   // section offsets.
if (Sec->Phdrs.empty()) {
       if (Sec->Flags & SHF_ALLOC)
         Sec->Phdrs = DefPhdrs;
     } else {
       DefPhdrs = Sec->Phdrs;
     }
   }
 }
 
-static OutputSection *findFirstSection(PhdrEntry *Load) {
-  for (OutputSection *Sec : OutputSections)
-    if (Sec->PtLoad == Load)
-      return Sec;
-  return nullptr;
-}
-
 static uint64_t computeBase(uint64_t Min, bool AllocateHeaders) {
   // If there is no SECTIONS or if the linkerscript is explicit about program
   // headers, do our best to allocate them.
   if (!Script->HasSectionsCommand || AllocateHeaders)
     return 0;
   // Otherwise only allocate program headers if that would not add a page.
   return alignDown(Min, Config->MaxPageSize);
 }

test/ELF/linkerscript/Inputs/at8.s

+.section .sec1,"aw",@progbits
+.quad   1
+
+.section .sec2,"aw",@progbits
+.quad   2
+
+.section .sec3,"aw",@progbits
+.quad   3

test/ELF/linkerscript/at8.test

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/at8.s -o %t.o
+# RUN: ld.lld %t.o --script %s -o %t
+# RUN: llvm-readelf -sections -program-headers %t | FileCheck %s
+
+MEMORY {
+  FLASH  : ORIGIN = 0x08000000, LENGTH = 0x100
+  RAM    : ORIGIN = 0x20000000, LENGTH = 0x200
+}
+
+SECTIONS {
+ .text : { *(.text) } > FLASH
+ .sec1 : { *(.sec1) } > RAM AT > FLASH
+ .sec2 : { *(.sec2) } > RAM
+ .sec3 : { *(.sec3) } > RAM AT > FLASH
+}
+
+# Make sure we do not issue a load-address overlap error
+# Previously, .sec3 would overwrite the LMAOffset in the
+# PT_LOAD header.
+
+# CHECK: Name              Type            Address          Off
+# CHECK: .text             PROGBITS        0000000008000000 001000
+# CHECK: .sec1             PROGBITS        0000000020000000 001000
+# CHECK: .sec2             PROGBITS        0000000020000008 001008
+# CHECK: .sec3             PROGBITS        0000000020000010 001010
+
+# CHECK: Program Headers:
+# CHECK:      Type  Offset   VirtAddr           PhysAddr
+# CHECK-NEXT: LOAD  0x001000 0x0000000020000000 0x0000000008000000
+# CHECK-NOT: LOAD