[PATCH] D34977: [ELF] - Allow moving location counter backward in some cases.

[Rafael Avila de Espindola via llvm-commits]

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:
> +static std::vector<OutputSection *> getSectionsInLoad(OutputSection *First) {
> +  std::vector<OutputSection *> Ret;
> +  for (OutputSectionCommand *Cmd : OutputSectionCommands)
> +    if (Cmd->Sec->FirstInPtLoad == First)
> +      Ret.push_back(Cmd->Sec);
> +  return Ret;
> +}
> +
> +// Using linker script is it possible to assign any virtual addresses
> +// to sections. We currently support only the case when all sections
> +// in a PT_LOAD segment has assending VA order.

s/has/have/

> +static void checkSectionsOrder(OutputSection *FirstInPtLoad) {
> +  std::vector<OutputSection *> V = getSectionsInLoad(FirstInPtLoad);
> +  OutputSection *Prev = nullptr;
> +  for (OutputSection *Sec : V) {
> +    // VA of .tbss is meaningless, it is possible for this sections to
> +    // have VA that violates ascending ordering. Just ignore them.
> +    bool IsTbss = (Sec->Flags & SHF_TLS) && Sec->Type == SHT_NOBITS;
> +    if (IsTbss)
> +      continue;

What test fails if you remove the tbss special case?

> +    if (!(Sec->Flags & SHF_ALLOC))
> +      continue;

This means we support out of order or overlapping non alloc
sections. That is, we accept things like

. = 0x2000
.non_alloc_1 : {...}
. = 0x1000
.non_alloc_2 : {...}

Should we reject that too?

What about using overlapping BYTE commands?

Given that it looks like the current code just works in the saneish
cases, I am tempted to suggest reducing the patch to just:

> Index: ELF/LinkerScript.cpp
> ===================================================================
> --- ELF/LinkerScript.cpp
> +++ ELF/LinkerScript.cpp
> @@ -111,13 +111,9 @@
>  
>  void LinkerScript::setDot(Expr E, const Twine &Loc, bool InSec) {
>    uint64_t Val = E().getValue();
> -  if (Val < Dot) {
> -    if (InSec)
> -      error(Loc + ": unable to move location counter backward for: " +
> -            CurOutSec->Name);
> -    else
> -      error(Loc + ": unable to move location counter backward");
> -  }
> +  if (Val < Dot && InSec)
> +    error(Loc +
> +          ": unable to move location counter backward for: " + CurOutSec->Name);
>    Dot = Val;
>    // Update to location counter means update to section size.
>    if (InSec)

Rui, what do you think?

Cheers,
Rafael