[ELF] - keep alive all non-text sections referenced by .eh_frame

[grimar]

Summary

Patch implements one of suggestions from Rafael Ávila de Espíndola,
to fix segfault after section that contains personality being
garbage collected.

Suggestion was just to keep alive all non executable sections
referenced by .eh_frame.

This fixes PR27529.

[compnerd]

ELF/MarkLive.cpp

102 ↗

(On Diff #55408)

Unused typedef.

[compnerd]

ELF/MarkLive.cpp

190 ↗

(On Diff #55408)

Nit: you misspelt referred.

194 ↗

(On Diff #55408)

Is there a reason to not just merge this into the loops above? It seems unnecessary to re-iterate the objects and sections within the objects.

test/ELF/eh-frame-gc.s

21 ↗

(On Diff #55408)

Might be able to drop the .long 0 as well. Just thought of that, haven't had a chance to try it.

[ruiu]

This patch seems to be pretty effective for large executables with debug info. r266158 (Rafael's patch to change the way how we apply relocations) caused a temporary performance degradation for such executables, but this patch makes it even faster than before.

Time to link clang with debug info (output size is 1070 MB):

before r266158: 15.312 seconds (0%)
r266158:        17.301 seconds (+13.0%)
Head:           16.484 seconds (+7.7%)
w/patch:        13.166 seconds (-14.0%)

[ruiu]

Oh, I'm sorry, I added a comment to an unrelated patch. Ignore the above message.

[compnerd]

This introduces a new crash on x86_64. Ive not had a chance to reduce this to a test case that is easy to work with, but the following seems to demonstrate the issue:

$ cat /tmp/reduced.cc
struct S { static inline long m() { return 0; } };

#if defined(TU_A)
void f(void) { S::m(); }
#elif defined(TU_B)
void g(void) { S::m(); }
#endif

$ clang++ -target x86_64-unknown-linux-gnu -DTU_A -c /tmp/reduced.cc -o /tmp/reduced-0.o
$ clang++ -target x86_64-unknown-linux-gnu -DTU_B -c /tmp/reduced.cc -o /tmp/reduced-1.o
$ lld -flavor gnu --eh-frame-hdr -m elf_x86_64 -shared -o /tmp/reduced.so --gc-sections /tmp/reduced-0.o /tmp/reduced-1.o

[grimar]

In D19656#415910, @compnerd wrote:

This introduces a new crash on x86_64. Ive not had a chance to reduce this to a test case that is easy to work with, but the following seems to demonstrate the issue:

Reproduced, looking at.

ELF/MarkLive.cpp

190 ↗

(On Diff #55408)

Thanks !

194 ↗

(On Diff #55408)

I tried both ways when wrote that and finally decided to split eh_frame processing as special case just to emphasize its exceptional nature. Have no real preference here.

[grimar]

In D19656#415910, @compnerd wrote:

This introduces a new crash on x86_64. Ive not had a chance to reduce this to a test case that is easy to work with, but the following seems to demonstrate the issue:

$ cat /tmp/reduced.cc
struct S { static inline long m() { return 0; } };

#if defined(TU_A)
void f(void) { S::m(); }
#elif defined(TU_B)
void g(void) { S::m(); }
#endif

$ clang++ -target x86_64-unknown-linux-gnu -DTU_A -c /tmp/reduced.cc -o /tmp/reduced-0.o
$ clang++ -target x86_64-unknown-linux-gnu -DTU_B -c /tmp/reduced.cc -o /tmp/reduced-1.o
$ lld -flavor gnu --eh-frame-hdr -m elf_x86_64 -shared -o /tmp/reduced.so --gc-sections /tmp/reduced-0.o /tmp/reduced-1.o

Just in case, this can be reduced to:
reduced-0.s:

.section .text,"G",@progbits,foo
.type foo,@function
foo:

reduced-1.s:

.section .text,"G",@progbits,foo
.type foo,@function
foo:
 .cfi_startproc
 .cfi_endproc

llvm-mc -filetype=obj -triple=x86_64-pc-linux reduced-0.s -o reduced0.o
llvm-mc -filetype=obj -triple=x86_64-pc-linux reduced-1.s -o reduced1.o

[grimar]

• Fixed segfault case reported.
• Addressed review comments.
• Slightly updated testcase (NFC).

ELF/MarkLive.cpp

102 ↗

(On Diff #55408)

Fixed.

194 ↗

(On Diff #55408)

After revisiting this, merged into one loop to avoid additional iteration.

test/ELF/eh-frame-gc.s

21 ↗

(On Diff #55408)

Done.

[• rafael]

LGTM with nits.

I now remember that eh can point to LSDA, but lets get this fixed first.

ELF/MarkLive.cpp

78 ↗

(On Diff #55609)

Please use std::function<void(ResolvedReloc<ELFT>)> to mach the declaration of forEachSuccessor.

79 ↗

(On Diff #55609)

These now have a single use each, please inline.

94 ↗

(On Diff #55609)

Single use typedef.

test/ELF/eh-frame-gc.s

9 ↗

(On Diff #55609)

Give the section a more unique name like test_personality_section.

[grimar]

ELF/MarkLive.cpp

78 ↗

(On Diff #55609)

Done.

79 ↗

(On Diff #55609)

Done.

94 ↗

(On Diff #55609)

It was single use, I removed typedef (like was in run()).

test/ELF/eh-frame-gc.s

9 ↗

(On Diff #55609)

Done.

lld/trunk/ELF/MarkLive.cpp

 
 template <class ELFT>
 static typename ELFT::uint getAddend(InputSectionBase<ELFT> *Sec,
                                      const typename ELFT::Rela &Rel) {
   return Rel.r_addend;
 }
 
 template <class ELFT, class RelT>
-static ResolvedReloc<ELFT> resolveReloc(InputSection<ELFT> *Sec, RelT &Rel) {
+static ResolvedReloc<ELFT> resolveReloc(InputSectionBase<ELFT> *Sec,
+                                        RelT &Rel) {
   SymbolBody &B = Sec->getFile()->getRelocTargetSym(Rel);
   auto *D = dyn_cast<DefinedRegular<ELFT>>(&B);
   if (!D || !D->Section)
     return {nullptr, 0};
   typename ELFT::uint Offset = D->Value;
   if (D->isSection())
     Offset += getAddend(Sec, Rel);
   return {D->Section->Repl, Offset};
 }
 
-// Calls Fn for each section that Sec refers to via relocations.
-template <class ELFT>
-static void forEachSuccessor(InputSection<ELFT> *Sec,
-                             std::function<void(ResolvedReloc<ELFT>)> Fn) {
-  typedef typename ELFT::Rel Elf_Rel;
-  typedef typename ELFT::Rela Elf_Rela;
-  typedef typename ELFT::Shdr Elf_Shdr;
-
-  ELFFile<ELFT> &Obj = Sec->getFile()->getObj();
-  for (const Elf_Shdr *RelSec : Sec->RelocSections) {
-    if (RelSec->sh_type == SHT_RELA) {
-      for (const Elf_Rela &RI : Obj.relas(RelSec))
-        Fn(resolveReloc(Sec, RI));
-    } else {
-      for (const Elf_Rel &RI : Obj.rels(RelSec))
-        Fn(resolveReloc(Sec, RI));
-    }
-  }
+template <class ELFT, class Elf_Shdr>
+static void run(ELFFile<ELFT> &Obj, InputSectionBase<ELFT> *Sec,
+                Elf_Shdr *RelSec, std::function<void(ResolvedReloc<ELFT>)> Fn) {
+    for (const typename ELFT::Rela &RI : Obj.relas(RelSec))
+    for (const typename ELFT::Rel &RI : Obj.rels(RelSec))
+
+// Calls Fn for each section that Sec refers to via relocations.
+template <class ELFT>
+static void forEachSuccessor(InputSection<ELFT> *Sec,
+                             std::function<void(ResolvedReloc<ELFT>)> Fn) {
+  ELFFile<ELFT> &Obj = Sec->getFile()->getObj();
+  for (const typename ELFT::Shdr *RelSec : Sec->RelocSections)
+    run(Obj, Sec, RelSec, Fn);
+}
+
+template <class ELFT> static void scanEhFrameSection(EHInputSection<ELFT> &EH) {
+  if (!EH.RelocSection)
+    return;
+  ELFFile<ELFT> &EObj = EH.getFile()->getObj();
+  run<ELFT>(EObj, &EH, EH.RelocSection, [&](ResolvedReloc<ELFT> R) {
+    if (!R.Sec || R.Sec == &InputSection<ELFT>::Discarded)
+      return;
+    if (R.Sec->getSectionHdr()->sh_flags & SHF_EXECINSTR)
+      return;
+    R.Sec->Live = true;
+  });
 }
 
 // Sections listed below are special because they are used by the loader
 // just by being in an ELF file. They should not be garbage-collected.
 template <class ELFT> static bool isReserved(InputSectionBase<ELFT> *Sec) {
   switch (Sec->getSectionHdr()->sh_type) {
   case SHT_FINI_ARRAY:
   case SHT_INIT_ARRAY:
template <class ELFT> void elf::markLive(SymbolTable<ELFT> *Symtab) {
   for (const Symbol *S : Symtab->getSymbols())
     if (S->includeInDynsym())
       MarkSymbol(S->body());
 
   // Preserve special sections and those which are specified in linker
   // script KEEP command.
   for (const std::unique_ptr<ObjectFile<ELFT>> &F : Symtab->getObjectFiles())
     for (InputSectionBase<ELFT> *Sec : F->getSections())
-      if (Sec && Sec != &InputSection<ELFT>::Discarded)
+      if (Sec && Sec != &InputSection<ELFT>::Discarded) {
+        // .eh_frame is always marked as live now, but also it can reference to
+        // sections that contain personality. We preserve all non-text sections
+        // referred by .eh_frame here.
+        if (auto *EH = dyn_cast_or_null<EHInputSection<ELFT>>(Sec))
+          scanEhFrameSection<ELFT>(*EH);
         if (isReserved(Sec) || Script<ELFT>::X->shouldKeep(Sec))
           Enqueue({Sec, 0});
+      }
 
   // Mark all reachable sections.
   while (!Q.empty())
     forEachSuccessor<ELFT>(Q.pop_back_val(), Enqueue);
 }
 
 template void elf::markLive<ELF32LE>(SymbolTable<ELF32LE> *);
 template void elf::markLive<ELF32BE>(SymbolTable<ELF32BE> *);
 template void elf::markLive<ELF64LE>(SymbolTable<ELF64LE> *);
 template void elf::markLive<ELF64BE>(SymbolTable<ELF64BE> *);

lld/trunk/test/ELF/eh-frame-gc.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=i686-pc-linux %s -o %t.o
+# RUN: ld.lld -shared --gc-sections %t.o -o %t
+# RUN: llvm-readobj  -s %t | FileCheck %s
+
+## Check that section containing personality is
+## not garbage collected.
+# CHECK: Sections [
+# CHECK: Name: .test_personality_section
+
+.text
+.globl foo
+.type foo,@function
+foo:
+ .cfi_startproc
+ .cfi_personality 155, DW.ref.__gxx_personality_v0
+ .cfi_endproc
+
+.section .test_personality_section
+DW.ref.__gxx_personality_v0: