[ELF] - Do not create 4gb output when -obinary -Ttext and -omagic used together.

[grimar]

Summary

This fixed PR31196,

problem is that ElfHeader address was taken in account when we calculated offsets:

static uintX_t getFileAlignment(uintX_t Off, OutputSectionBase *Sec) {
...
  return First->Offset + Sec->Addr - First->Addr;

Sec->Addr is 600 here.
First->Addr is 0x10000 (First here is Out<ELFT>::ElfHeader->Addr == ImageBase).

Binary output does not have headers, we do not need to rely on their address to
calculate file offset.

[grimar]

Alternative solution is https://reviews.llvm.org/D27201

[emaste]

For me at least this approach (avoiding adding an empty PT_LOAD) seems to be nicer than D27201, although I find the ternary operator and different condition on the following line somewhat puzzling. I'm curious what @ruiu or @rafael think.

ELF/Writer.cpp

1120

Ah. Why do we add Load for the !ScriptConfig->HasSections case?

For me at least this seems to make it more clear:

Phdr *Load = nullptr;
if (!ScriptConfig->HasSections && !Config->OFormatBinary) {
    Load = AddHdr(PT_LOAD, Flags);
    Load->add(Out<ELFT>::ElfHeader);
    Load->add(Out<ELFT>::ProgramHeaders);
}

[ruiu]

Why are we creating program headers for --oformat=binary in the first place?

[grimar]

In D27200#607883, @ruiu wrote:

Why are we creating program headers for --oformat=binary in the first place?

I think we always create them.
That true for obinary or case when we exclude them later with script:

Out<ELFT>::ElfHeader = make<OutputSectionBase>("", 0, SHF_ALLOC);
Out<ELFT>::ElfHeader->Size = sizeof(Elf_Ehdr);
Out<ELFT>::ProgramHeaders = make<OutputSectionBase>("", 0, SHF_ALLOC);
Out<ELFT>::ProgramHeaders->updateAlignment(sizeof(uintX_t));

We just do not always add them to output sections.

[grimar]

In D27200#607883, @ruiu wrote:

Why are we creating program headers for --oformat=binary in the first place?

And that patch (and D27201) as variation stops adding these headers as output sections,
like it supposed to be I think.

[ruiu]

The way how this patch fixes the issue is a bit puzzling and too subtle (the important condition is buried inside createPhdrs which later affects file offset computation in an nonobvious). I believe there's a better way. Let me take a closer look.

[grimar]

In D27200#607902, @ruiu wrote:

The way how this patch fixes the issue is a bit puzzling and too subtle (the important condition is buried inside createPhdrs which later affects file offset computation in an nonobvious). I believe there's a better way. Let me take a closer look.

The fact we are setting offsets basing on First section in a load actually does not seems too subtle for me:

static uintX_t getFileAlignment(uintX_t Off, OutputSectionBase *Sec) {
...
  return First->Offset + Sec->Addr - First->Addr;

-Ttext in this case sets address of .text below the 0x10000 what is ImageBase (to 0x600).
We set image base address to headers:

uintX_t BaseVA = Config->ImageBase;
Out<ELFT>::ElfHeader->Addr = BaseVA;
Out<ELFT>::ProgramHeaders->Addr = BaseVA + Out<ELFT>::ElfHeader->Size;

one of solutions probably could be set some different BaseVA here in that case (0x600 - X),
but I found that much more puzling and wrong to rely on headers addresses for offsets for binary output,
since they should absent and even PT_LOAD can be excessive and probably also should not be created (like this patch do).

[ruiu]

ELF/Writer.cpp

1120

Agreed. But furthermore, can we avoid calling createPhdrs() at all if OFormatBinary is true? If it's true, all data created in this function will be just ignored later.

[emaste]

ELF/Writer.cpp

1120

That would be even more clear :)

[grimar]

ELF/Writer.cpp

1120

Well, if we will avoid calling createPhdrs() that means that we will be not able to pagealign .data/.text.
Because page aligning as perfomed basing on loads logic (what is intentional):

for (const Phdr &P : Phdrs)
  if (P.H.p_type == PT_LOAD)
    P.First->PageAlign = true;

As a result our output will be different from ld.

Though looking on PR31196, it creates single load. Also I suppose binary output is used mostly with linkerscripts
to control internal layout and much probably do not need auto pagealign we provide for regular link.
So far I suppose that should be probably OK.

But that also affects on file offsets calculation. Which is currently performed as:

OutputSectionBase<ELFT> *First = Sec->FirstInPtLoad;
// If two sections share the same PT_LOAD the file offset is calculated using
// this formula: Off2 = Off1 + (VA2 - VA1).
if (Sec == First)
  return alignTo(Off, Target->MaxPageSize, Sec->Addr);
return First->Offset + Sec->Addr - First->Addr;

(Introduced in D25014).

where FirstInPtLoad set from:

template <class ELFT> void PhdrEntry<ELFT>::add(OutputSectionBase *Sec) {
..
  if (H.p_type == PT_LOAD)
    Sec->FirstInPtLoad = First;
}

And I think we want to keep the effect from this logic.

So I guess technically we can stop calling createPhdrs(), but that involves much more changes than this patch do, do we want it ?

[grimar]

ELF/Writer.cpp

1120

Ah. Why do we add Load for the !ScriptConfig->HasSections case?

It is used for elf and program headers. I investigated if it possible to do suggested change.
At first it looks good since allows to remove the next code:

if (!FirstPTLoad->First) {
  // Sometimes the very first PT_LOAD segment can be empty.
  // This happens if (all conditions met):
  //  - Linker script is used
  //  - First section in ELF image is not RO
  //  - Not enough space for program headers.
  // The code below removes empty PT_LOAD segment and updates
  // program headers size.
  Phdrs.erase(FirstPTLoad);
  Out<ELFT>::ProgramHeaders->Size =
      sizeof(typename ELFT::Phdr) * Phdrs.size();
}

But that breaks linkerscript\at.s test and logic.

# RUN: echo "SECTIONS { \
# RUN:  . = 0x1000; \
# RUN:  .aaa : AT(0x2000) \
# RUN:  { \
# RUN:   *(.aaa) \
# RUN:  } \

It relies on the fact that we create new LOAD for each section that specified AT.
With suggested change headers are placed to load containing .aaa,
that breaks calculation of LMA finally:

if (!P.HasLMA)
  H.p_paddr = First->getLMA();

So I think it is easier to leave them in created load for now than handle such specific case separatelly.

[grimar]

• Updated.

[grimar]

I'll post different solution.

ELF/Writer.cpp

template <class ELFT> std::vector<PhdrEntry<ELFT>> Writer<ELFT>::createPhdrs() {
   // PT_INTERP must be the second entry if exists.
   if (OutputSectionBase *Sec = findSection(".interp")) {
     Phdr &Hdr = *AddHdr(PT_INTERP, Sec->getPhdrFlags());
     Hdr.add(Sec);
   }
 
   // Add the first PT_LOAD segment for regular output sections.
   uintX_t Flags = computeFlags<ELFT>(PF_R);
-  Phdr *Load = AddHdr(PT_LOAD, Flags);
-  if (!ScriptConfig->HasSections) {
+
+  // Do not create PT_LOAD segment for binary output, it does not
+  // have file or program headers.
+  Phdr *Load = nullptr;
+  if (!Config->OFormatBinary)
+    Load = AddHdr(PT_LOAD, Flags);
+  // We do not place headers when have SECTIONS command here, because it is
+  // possible to have no place for them. We do it later when script assign
+  // sections addressed. But we still need to create this load for them for case
+  // when first section uses AT command and requires own PT_LOAD.
+  if (Load && !ScriptConfig->HasSections) {
     Load->add(Out<ELFT>::ElfHeader);
     Load->add(Out<ELFT>::ProgramHeaders);
   }
 
   Phdr TlsHdr(PT_TLS, PF_R);
   Phdr RelRo(PT_GNU_RELRO, PF_R);
   Phdr Note(PT_NOTE, PF_R);
   for (OutputSectionBase *Sec : OutputSections) {
if (!needsPtLoad<ELFT>(Sec))
       continue;
 
     // Segments are contiguous memory regions that has the same attributes
     // (e.g. executable or writable). There is one phdr for each segment.
     // Therefore, we need to create a new phdr when the next section has
     // different flags or is loaded at a discontiguous address using AT linker
     // script command.
     uintX_t NewFlags = computeFlags<ELFT>(Sec->getPhdrFlags());
-    if (Script<ELFT>::X->hasLMA(Sec->getName()) || Flags != NewFlags) {
+    if (!Load || Script<ELFT>::X->hasLMA(Sec->getName()) || Flags != NewFlags) {
       Load = AddHdr(PT_LOAD, NewFlags);
       Flags = NewFlags;
     }
 
     Load->add(Sec);
 
     if (isRelroSection<ELFT>(Sec))
       RelRo.add(Sec);

test/ELF/oformat-binary-ttext.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+
+# RUN: ld.lld -N -Ttext 0x100 -o %t.out %t --oformat binary
+# RUN: od -t x1 -v %t.out | FileCheck %s --check-prefix=BIN
+
+# BIN:      0000000 90 00 00 00 00 00 00 00
+# BIN-NEXT: 0000010
+# BIN-NOT:  0000020
+
+## The same but without OMAGIC.
+# RUN: ld.lld -Ttext 0x100 -o %t.out %t --oformat binary
+# RUN: od -t x1 -v %t.out | FileCheck %s --check-prefix=BIN
+
+.text
+.globl _start
+_start:
+ nop