[ELF] Simplify handling of removed sections

[bd1976llvm]

Summary

Simplify handling of removed sections by testing the Live bit to see if the section was removed.

The code is simpler, and the lack of this test caused confusion (see: https://reviews.llvm.org/D37718).

[ruiu]

I'm not sure if this is an improvement. Essentially, we shouldn't call getSymVA on dead symbols (or, more strictly speaking, symbols pointing to dead sections), and I don't think we want to allow it.

[bd1976llvm]

In D38136#877788, @ruiu wrote:

I'm not sure if this is an improvement. Essentially, we shouldn't call getSymVA on dead symbols (or, more strictly speaking, symbols pointing to dead sections), and I don't think we want to allow it.

I am inclined to agree. In the end we probably want to restrict access to this function so that it can only be called via functions that know how to take dead symbols into account - at that point we can just have an assert in here in order to check that it hasn't been called on a dead symbol. However, at the moment this function is called for dead symbols.

Would you be willing to approve this change as an improvement for now?

• The code is much clearer and we remove the duplication of the .eh_frame comment (the exact same comment is in MarkLive.cpp:198 where we skip setting the Live bit for discarded sections.)

[ruiu]

I am inclined to agree. In the end we probably want to restrict access to this function so that it can only be called via functions that know how to take dead symbols into account - at that point we can just have an assert in here in order to check that it hasn't been called on a dead symbol. However, at the moment this function is called for dead symbols.

How do we call getSymVA on dead symbols? I think I want to fix it first.

[bd1976llvm]

This case commonly arises where the section that is being patched is informational and the linker can't garbage collect it e.g .debug_info.

p.s: challenge goal for the brave - actually a linker could garbage collect these sections, but it would be a lot of work.

[bd1976llvm]

Check out lld/trunk/test/ELF/common-gc3.s for a test case.

[bd1976llvm]

Having thought about this I still think that we want this patch.

Even if we push the check for dead symbols up the stack we still want to test the Live bit to determine if the symbol is dead.

[ruiu]

By doing this, can you simplify other places? If so, it may be worth submitting, but this patch itself seems neutral.

ELF/LinkerScript.cpp

void LinkerScript::processCommands(OutputSectionFactory &Factory) {
   // * SHN_UNDEF
   // * SHN_ABS
   // * Any value meaning a regular section.
   // To handle that, create a dummy aether section that fills the void before
   // the linker scripts switches to another section. It has an index of one
   // which will map to whatever the first actual section is.
   Aether = make<OutputSection>("", 0, SHF_ALLOC);
   Aether->SectionIndex = 1;
+  Aether->Live = true;
   auto State = make_unique<AddressState>(Opt);
   // CurAddressState captures the local AddressState and makes it accessible
   // deliberately. This is needed as there are some cases where we cannot just
   // thread the current state through to a lambda function created by the
   // script parser.
   CurAddressState = State.get();
   CurAddressState->OutSec = Aether;
   Dot = 0;

ELF/Symbols.cpp

 static uint64_t getSymVA(const SymbolBody &Body, int64_t &Addend) {
   switch (Body.kind()) {
   case SymbolBody::DefinedRegularKind: {
     auto &D = cast<DefinedRegular>(Body);
     SectionBase *IS = D.Section;
     if (auto *ISB = dyn_cast_or_null<InputSectionBase>(IS))
       IS = ISB->Repl;
 
-    // According to the ELF spec reference to a local symbol from outside
-    // the group are not allowed. Unfortunately .eh_frame breaks that rule
-    // and must be treated specially. For now we just replace the symbol with
-    // 0.
-    if (IS == &InputSection::Discarded)
-      return 0;
-
     // This is an absolute symbol.
     if (!IS)
       return D.Value;
 
+    if (!IS->Live)
+      return 0;
+
     uint64_t Offset = D.Value;
 
     // An object in an SHF_MERGE section might be referenced via a
     // section symbol (as a hack for reducing the number of local
     // symbols).
     // Depending on the addend, the reference via a section symbol
     // refers to a different object in the merge section.
     // Since the objects in the merge section are not necessarily

ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::createSyntheticSections() {
   InX::DynStrTab = make<StringTableSection>(".dynstr", true);
   InX::Dynamic = make<DynamicSection<ELFT>>();
   In<ELFT>::RelaDyn = make<RelocationSection<ELFT>>(
       Config->IsRela ? ".rela.dyn" : ".rel.dyn", Config->ZCombreloc);
   InX::ShStrTab = make<StringTableSection>(".shstrtab", false);
 
   Out::ElfHeader = make<OutputSection>("", 0, SHF_ALLOC);
   Out::ElfHeader->Size = sizeof(Elf_Ehdr);
+  Out::ElfHeader->Live = true;
   Out::ProgramHeaders = make<OutputSection>("", 0, SHF_ALLOC);
   Out::ProgramHeaders->updateAlignment(Config->Wordsize);
+  Out::ProgramHeaders->Live = true;
 
   if (needsInterpSection()) {
     InX::Interp = createInterpSection();
     Add(InX::Interp);
   } else {
     InX::Interp = nullptr;
   }