[LLD][ELF] Revert r318924 Skip over empty sections when checking for contiguous relro

[peter.smith]

Summary

PR35478 https://bugs.llvm.org/show_bug.cgi?id=35478 points out a flaw in the implementation of r318924 from D40364. The implementation depends on the Size field being set or the SyntheticSection::empty() being accurate. These functions are not reliable as some linker script commands that have yet to be processed may affect the results, causing some non-zero size sections to be reported as zero size.

I think the first step is to revert r318924 and come up with a better solution for the underlying problem rather than trying to layer more heuristics onto the zero sized output section.

Chances are I'll be out of office by the time anyone sees this so feel free to commit the revert if you agree with me.

Fixes PR35478

Current thoughts on the underlying problem:
Revisiting the motivation for adding the zero size check in the first place; it was to prevent 0 sized SyntheticSections that a user does not have full control over from needlessly breaking the PT_GNU_RELRO, rather than trying to accommodate arbitrarily complex linker scripts. Looking at the code, it looks like removeUnusedSyntheticSections() should remove zero sized synthetic sections. It does, but it doesn't set the Parent to nullptr, this has the side effect that Sec == InX::BssRelRo->getParent() will make the parent OutputSection of InX::BssRelRo RelRo even if there is no InX::BssRelRo.

I tried a quick experiment with setting the Parent to nullptr and this flushed out a few interesting test failures, it feels like playing Jenga with every change:

• In the isRelroSection() we have to consider the case where there is no .plt and .plt.got but there is a ifunc plt with accompanying (ifunc .got or .plt.got)
• The PPC64 has PltHeaderSize == 0. Unfortunately HeaderSize == 0 is used to choose between the ifunc plt or normal plt. We seem to get away with this at the moment, but tests start to fail when Parent is set to nullptr for the .got.plt.
• The InX::BssRelRo and InX::Bss never get their sizes set and they are always removed by removeUnusedSyntheticSections(), their purpose seems to be as some kind of proxy for add .bss or .bss.relro InputSections into their parent OutputSections, they therefore don't behave like other SyntheticSections anyway.

My thinking is that some work is needed to make sure that the Sec == SyntheticSection->getParent() does a bit more checking before returning true, particularly for InX::BssRelRo as that has special behaviour. I'll hope to post something for review as soon as possible.

[jhenderson]

LGTM, at least from my point of view. I verified that with both this and D38361 all the tests pass.

[peter.smith]

Thanks, if there are no objections from the US timezone, I'll commit on Monday.

ELF/Writer.cpp

 static uint64_t computeFlags(uint64_t Flags) {
   if (Config->Omagic)
     return PF_R | PF_W | PF_X;
   if (Config->SingleRoRx && !(Flags & PF_W))
     return Flags | PF_X;
   return Flags;
 }
 
-// Prior to finalizeContents() an OutputSection containing SyntheticSections
-// may have 0 Size, but contain SyntheticSections that haven't had their size
-// calculated yet. We must use SyntheticSection->empty() for these sections.
-static bool isOutputSectionZeroSize(const OutputSection* Sec) {
-  if (Sec->Size > 0)
-    return false;
-  for (BaseCommand *BC : Sec->SectionCommands) {
-    if (auto *ISD = dyn_cast<InputSectionDescription>(BC))
-      for (InputSection *IS : ISD->Sections)
-        if (SyntheticSection *SS = dyn_cast<SyntheticSection>(IS))
-          if (!SS->empty())
-            return false;
-    }
-  return true;
-}
-
 // Decide which program headers to create and which sections to include in each
 // one.
 template <class ELFT> std::vector<PhdrEntry *> Writer<ELFT>::createPhdrs() {
   std::vector<PhdrEntry *> Ret;
   auto AddHdr = [&](unsigned Type, unsigned Flags) -> PhdrEntry * {
     Ret.push_back(make<PhdrEntry>(Type, Flags));
     return Ret.back();
   };
template <class ELFT> std::vector<PhdrEntry *> Writer<ELFT>::createPhdrs() {
   // PT_GNU_RELRO includes all sections that should be marked as
   // read-only by dynamic linker after proccessing relocations.
   // Current dynamic loaders only support one PT_GNU_RELRO PHDR, give
   // an error message if more than one PT_GNU_RELRO PHDR is required.
   PhdrEntry *RelRo = make<PhdrEntry>(PT_GNU_RELRO, PF_R);
   bool InRelroPhdr = false;
   bool IsRelroFinished = false;
   for (OutputSection *Sec : OutputSections) {
-    if (!needsPtLoad(Sec) || isOutputSectionZeroSize(Sec))
+    if (!needsPtLoad(Sec))
       continue;
     if (isRelroSection(Sec)) {
       InRelroPhdr = true;
       if (!IsRelroFinished)
         RelRo->add(Sec);
       else
         error("section: " + Sec->Name + " is not contiguous with other relro" +
               " sections");

test/ELF/relro-non-contiguous-zerosize.s

-// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/shared.s -o %t.o
-// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/copy-in-shared.s -o %t2.o
-// RUN: ld.lld -shared %t.o %t2.o -o %t.so
-
-// Check that we ignore zero sized non-relro sections that are covered by the
-// range of addresses covered by the PT_GNU_RELRO header.
-// Check that we ignore zero sized relro sections that are disjoint from the
-// range of addresses covered by the PT_GNU_RELRO header.
-// REQUIRES: x86
-
-// RUN: echo "SECTIONS { \
-// RUN: .ctors : { *(.ctors) } \
-// RUN: .large1 : { *(.large1) } \
-// RUN: .dynamic : { *(.dynamic) } \
-// RUN: .zero_size : { *(.zero_size) } \
-// RUN: .jcr : { *(.jcr) } \
-// RUN: .got.plt : { *(.got.plt) } \
-// RUN: .large2 : { *(.large2) } \
-// RUN: .data.rel.ro : { *(.data.rel.ro.*) } \
-// RUN: } " > %t.script
-// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t3.o
-// RUN: ld.lld %t3.o %t.so -o %t --script=%t.script
-// RUN: llvm-readobj -program-headers %t | FileCheck %s
-
-// CHECK: Type: PT_GNU_RELRO
-// CHECK-NEXT: Offset:
-// CHECK-NEXT: VirtualAddress:
-// CHECK-NEXT: PhysicalAddress:
-// CHECK-NEXT: FileSize:
-// CHECK-NEXT: MemSize: 4096
-
-        .section .text, "ax", @progbits
-        .global _start
-        .global bar
-        .global foo
-_start:
-        callq bar
-
-        // page size non-relro sections that would alter PT_GNU_RELRO header
-        // MemSize if counted as part of relro.
-        .section .large1, "aw", @progbits
-        .space 4 * 1024
-
-        .section .large2, "aw", @progbits
-        .space 4 * 1024
-        
-        // empty relro section
-        .section .ctors, "aw", @progbits
-        
-        // non-empty relro section
-        .section .jcr, "aw", @progbits
-        .quad 0
-
-        // empty non-relro section
-        .section .zero_size, "aw", @progbits
-        .global sym
-sym:
-
-        // empty relro section
-        .section .data.rel.ro, "aw", @progbits
-        .global sym2
-sym2: