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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Writer.cpp:1130
+  // have file or program headers.
+  Phdr *Load = Config->OFormatBinary ? nullptr : AddHdr(PT_LOAD, Flags);
+  if (!ScriptConfig->HasSections && !Config->OFormatBinary) {
----------------
grimar wrote:
> emaste wrote:
> > ruiu wrote:
> > > emaste wrote:
> > > > 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);
> > > > }
> > > > ```
> > > > 
> > > 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.
> > That would be even more clear :)
> 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 ?
> 
>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.


https://reviews.llvm.org/D27200