[PATCH] D43571: [ELF] - Do not remove empty output sections that are explicitly assigned to phdr in script.

[Rafael Avila de Espindola via llvm-commits]

This is a very interesting idea.

It looks like most "very generic" scripts are based on "ld.bfd
--verbose", and that doesn't use explicit phdr assignments.

So it does look like we can keep more output sections and simplify the
code.

LGTM

Cheers,
Rafael

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar created this revision.
> grimar added reviewers: ruiu, espindola.
> Herald added subscribers: arichardson, emaste.
>
> This continues direction shown in https://reviews.llvm.org/D43069.
>
> I suggest to keep sections that are explicitly assigned to segment in script.
> With that code can be simplified (as shown in diff).
>
> Also with that it might be possible to get rid of `Live` flag of output section (if we want).
>
>
> https://reviews.llvm.org/D43571
>
> Files:
>   ELF/LinkerScript.cpp
>   ELF/LinkerScript.h
>   ELF/Writer.cpp
>   test/ELF/linkerscript/implicit-program-header.s
>   test/ELF/linkerscript/orphan-phdrs.s
>
> Index: test/ELF/linkerscript/orphan-phdrs.s
> ===================================================================
> --- test/ELF/linkerscript/orphan-phdrs.s
> +++ test/ELF/linkerscript/orphan-phdrs.s
> @@ -18,6 +18,7 @@
>  # CHECK: Section Headers
>  # CHECK: .text
>  # CHECK-NEXT: .orphan
> +# CHECK-NEXT: .empty
>  # CHECK-NEXT: .rw
>  
>  # CHECK: Segment Sections
> Index: test/ELF/linkerscript/implicit-program-header.s
> ===================================================================
> --- test/ELF/linkerscript/implicit-program-header.s
> +++ test/ELF/linkerscript/implicit-program-header.s
> @@ -15,7 +15,7 @@
>  
>  # CHECK:      Segment Sections...
>  # CHECK-NEXT:   00     .text .dynsym .hash .dynstr .dynamic
> -# CHECK-NEXT:   01     .foo
> +# CHECK-NEXT:   01     .bar .foo
>  
>  .quad 0
>  .section .foo,"ax"
> Index: ELF/Writer.cpp
> ===================================================================
> --- ELF/Writer.cpp
> +++ ELF/Writer.cpp
> @@ -937,8 +937,7 @@
>  
>  static int getRankProximity(OutputSection *A, BaseCommand *B) {
>    if (auto *Sec = dyn_cast<OutputSection>(B))
> -    if (Sec->Live)
> -      return getRankProximityAux(A, Sec);
> +    return getRankProximityAux(A, Sec);
>    return -1;
>  }
>  
> @@ -984,28 +983,24 @@
>    int Proximity = getRankProximity(Sec, *I);
>    for (; I != E; ++I) {
>      auto *CurSec = dyn_cast<OutputSection>(*I);
> -    if (!CurSec || !CurSec->Live)
> +    if (!CurSec)
>        continue;
>      if (getRankProximity(Sec, CurSec) != Proximity ||
>          Sec->SortRank < CurSec->SortRank)
>        break;
>    }
>  
> -  auto IsLiveSection = [](BaseCommand *Cmd) {
> -    auto *OS = dyn_cast<OutputSection>(Cmd);
> -    return OS && OS->Live;
> -  };
> -
> +  auto IsOutputSec = [](BaseCommand *Cmd) { return isa<OutputSection>(Cmd); };
>    auto J = std::find_if(llvm::make_reverse_iterator(I),
> -                        llvm::make_reverse_iterator(B), IsLiveSection);
> +                        llvm::make_reverse_iterator(B), IsOutputSec);
>    I = J.base();
>  
>    // As a special case, if the orphan section is the last section, put
>    // it at the very end, past any other commands.
>    // This matches bfd's behavior and is convenient when the linker script fully
>    // specifies the start of the file, but doesn't care about the end (the non
>    // alloc sections for example).
> -  auto NextSec = std::find_if(I, E, IsLiveSection);
> +  auto NextSec = std::find_if(I, E, IsOutputSec);
>    if (NextSec == E)
>      return E;
>  
> @@ -1077,8 +1072,6 @@
>  
>  static void sortSection(OutputSection *Sec,
>                          const DenseMap<const InputSectionBase *, int> &Order) {
> -  if (!Sec->Live)
> -    return;
>    StringRef Name = Sec->Name;
>  
>    // Sort input sections by section name suffixes for
> @@ -1186,7 +1179,7 @@
>    auto E = Script->SectionCommands.end();
>    auto NonScriptI = std::find_if(I, E, [](BaseCommand *Base) {
>      if (auto *Sec = dyn_cast<OutputSection>(Base))
> -      return Sec->Live && Sec->SectionIndex == INT_MAX;
> +      return Sec->SectionIndex == INT_MAX;
>      return false;
>    });
>  
> @@ -1505,7 +1498,6 @@
>    removeUnusedSyntheticSections();
>  
>    sortSections();
> -  Script->removeEmptyCommands();
>  
>    // Now that we have the final list, create a list of all the
>    // OutputSections for convenience.
> Index: ELF/LinkerScript.h
> ===================================================================
> --- ELF/LinkerScript.h
> +++ ELF/LinkerScript.h
> @@ -254,7 +254,6 @@
>    ExprValue getSymbolValue(StringRef Name, const Twine &Loc);
>  
>    void addOrphanSections();
> -  void removeEmptyCommands();
>    void adjustSectionsBeforeSorting();
>    void adjustSectionsAfterSorting();
>  
> Index: ELF/LinkerScript.cpp
> ===================================================================
> --- ELF/LinkerScript.cpp
> +++ ELF/LinkerScript.cpp
> @@ -752,21 +752,12 @@
>    }
>  }
>  
> -void LinkerScript::removeEmptyCommands() {
> -  // It is common practice to use very generic linker scripts. So for any
> -  // given run some of the output sections in the script will be empty.
> -  // We could create corresponding empty output sections, but that would
> -  // clutter the output.
> -  // We instead remove trivially empty sections. The bfd linker seems even
> -  // more aggressive at removing them.
> -  llvm::erase_if(SectionCommands, [&](BaseCommand *Base) {
> -    if (auto *Sec = dyn_cast<OutputSection>(Base))
> -      return !Sec->Live;
> +static bool isAllSectionDescription(const OutputSection &Cmd) {
> +  // We do not remove empty sections that are explicitly
> +  // assigned to any segment.
> +  if (!Cmd.Phdrs.empty())
>      return false;
> -  });
> -}
>  
> -static bool isAllSectionDescription(const OutputSection &Cmd) {
>    for (BaseCommand *Base : Cmd.SectionCommands)
>      if (!isa<InputSectionDescription>(*Base))
>        return false;
> @@ -796,29 +787,34 @@
>    // the previous sections. Only a few flags are needed to keep the impact low.
>    uint64_t Flags = SHF_ALLOC;
>  
> -  for (BaseCommand *Cmd : SectionCommands) {
> +  for (BaseCommand *&Cmd : SectionCommands) {
>      auto *Sec = dyn_cast<OutputSection>(Cmd);
>      if (!Sec)
>        continue;
>      if (Sec->Live) {
>        Flags = Sec->Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR);
>        continue;
>      }
>  
> -    if (isAllSectionDescription(*Sec))
> -      continue;
> -
> -    Sec->Live = true;
> -    Sec->Flags = Flags;
> +    if (!isAllSectionDescription(*Sec))
> +      Sec->Flags = Flags;
> +    else
> +      Cmd = nullptr;
>    }
> +
> +  // It is common practice to use very generic linker scripts. So for any
> +  // given run some of the output sections in the script will be empty.
> +  // We could create corresponding empty output sections, but that would
> +  // clutter the output.
> +  // We instead remove trivially empty sections. The bfd linker seems even
> +  // more aggressive at removing them.
> +  llvm::erase_if(SectionCommands, [&](BaseCommand *Base) { return !Base; });
>  }
>  
>  void LinkerScript::adjustSectionsAfterSorting() {
>    // Try and find an appropriate memory region to assign offsets in.
>    for (BaseCommand *Base : SectionCommands) {
>      if (auto *Sec = dyn_cast<OutputSection>(Base)) {
> -      if (!Sec->Live)
> -        continue;
>        if (!Sec->LMARegionName.empty()) {
>          if (MemoryRegion *M = MemoryRegions.lookup(Sec->LMARegionName))
>            Sec->LMARegion = M;