[ELF] - Don't crash when --emit-relocs is used with --gc-sections

[grimar]

Summary

This is PR34512.
We crashed when --emit-relocs was used
and relocated section was collected by GC.

[ruiu]

I don't think this patch addresses the root cause of the issue. All sections passed to addInputSec should be live. If it should be considered dead by gc, it should be marked as such before reaching here. This patch seems to hide the issue instead of fixing it.

[grimar]

In D37561#863647, @ruiu wrote:

I don't think this patch addresses the root cause of the issue. All sections passed to addInputSec should be live. If it should be considered dead by gc, it should be marked as such before reaching here. This patch seems to hide the issue instead of fixing it.

If that should be true, then why first lines of 'addInputSec' returns (and report discarded sections actually) when sections are not live ?

void OutputSectionFactory::addInputSec(InputSectionBase *IS,
                                       StringRef OutsecName,
                                       OutputSection *&Sec) {
  if (!IS->Live) {
    reportDiscarded(IS);
    return;
  }

[grimar]

What about D37735 ? It should allow to move reportDiscarded out or at least
calls it earlier, what should help this patch hopefully.

[ruiu]

I think instead of "fixing" a concrete bug, you want to think more abstractly and focus on an invariant at each stage of linking. Invariants are abstract concepts but important to make program easier to understand. One invariant in lld is that, if some section is not marked as "live" after GC, it should be actually live, and it should be emitted to the output file.

A situation like "this section is not marked as live, but this is actually dead because a section that the section depends on is marked as dead by gc" is not ok. That is a violation of the invariant and needs fixing.

[grimar]

• Addressed review comment.

Note: D37735 should be landed first for allowing this to work properly. Because at next line:
https://github.com/llvm-mirror/lld/blob/master/ELF/OutputSections.cpp#L230
code would try to access to null pointer Out, when it should just report discarded end return at the begining of addSection method.

[ruiu]

ELF/InputSection.cpp

86–88 ↗

(On Diff #115205)

This is too magical that it needs comment.

[grimar]

• Addressed review comment.

ELF/InputSection.cpp

86–88 ↗

(On Diff #115205)

Oh, I am sorry, looks I put = instead of |= at last minute by mistake.
Fixed, added comment.

[ruiu]

ELF/InputSection.cpp

86–88 ↗

(On Diff #115205)

It is better to define a helper function. You can use this.

// Return true if a section with given section flags is live (will never be
// GCed) by default. If a section can be GCed, this function returns false.
static bool isLiveByDefault(uint64_t Flags, uint32_t Type) {
  // If GC is enabled, all memory-mapped sections are subject of GC.
  if (!Config->GcSections)
    return true;
  if (Flags & SHF_ALLOC)
    return false;

  // Besides that, relocation sections can also be GCed because their
  // relocation target sections may be GCed. This doesn't really matter
  // in most cases because the linker usually consumes relocation
  // sections instead of emitting them, but -emit-reloc needs this.
  return Type != SHT_REL && Type != SHT_RELA;
}

[grimar]

• Addressed review comments.

ELF/InputSection.cpp

86–88 ↗

(On Diff #115205)

Used. Thanks !

[ruiu]

LGTM

lld/trunk/ELF/InputSection.cpp

 template <class ELFT>
 static ArrayRef<uint8_t> getSectionContents(ObjFile<ELFT> *File,
                                             const typename ELFT::Shdr *Hdr) {
   if (!File || Hdr->sh_type == SHT_NOBITS)
     return makeArrayRef<uint8_t>(nullptr, Hdr->sh_size);
   return check(File->getObj().getSectionContents(Hdr));
 }
 
+// Return true if a section with given section flags is live (will never be
+// GCed) by default. If a section can be GCed, this function returns false.
+static bool isLiveByDefault(uint64_t Flags, uint32_t Type) {
+  // If GC is enabled, all memory-mapped sections are subject of GC.
+  if (!Config->GcSections)
+    return true;
+  if (Flags & SHF_ALLOC)
+    return false;
+
+  // Besides that, relocation sections can also be GCed because their
+  // relocation target sections may be GCed. This doesn't really matter
+  // in most cases because the linker usually consumes relocation
+  // sections instead of emitting them, but -emit-reloc needs this.
+  return Type != SHT_REL && Type != SHT_RELA;
+}
+
 InputSectionBase::InputSectionBase(InputFile *File, uint64_t Flags,
                                    uint32_t Type, uint64_t Entsize,
                                    uint32_t Link, uint32_t Info,
                                    uint32_t Alignment, ArrayRef<uint8_t> Data,
                                    StringRef Name, Kind SectionKind)
     : SectionBase(SectionKind, Name, Flags, Entsize, Alignment, Type, Info,
                   Link),
       File(File), Data(Data), Repl(this) {
-  Live = !Config->GcSections || !(Flags & SHF_ALLOC);
+  Live = isLiveByDefault(Flags, Type);
   Assigned = false;
   NumRelocations = 0;
   AreRelocsRela = false;
 
   // The ELF spec states that a value of 0 means the section has
   // no alignment constraits.
   uint32_t V = std::max<uint64_t>(Alignment, 1);
   if (!isPowerOf2_64(V))

lld/trunk/test/ELF/emit-relocs-gc.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+
+## Show that we emit .rela.bar when GC is disabled.
+# RUN: ld.lld --emit-relocs %t.o -o %t
+# RUN: llvm-objdump %t -section-headers | FileCheck %s --check-prefix=NOGC
+# NOGC: .rela.bar
+
+## GC collects .bar section and we exclude .rela.bar from output.
+# RUN: ld.lld --gc-sections --emit-relocs --print-gc-sections %t.o -o %t \
+# RUN:   | FileCheck --check-prefix=MSG %s
+# MSG: removing unused section from '.bar' in file
+# MSG: removing unused section from '.rela.bar' in file
+# RUN: llvm-objdump %t -section-headers | FileCheck %s --check-prefix=GC
+# GC-NOT:  rela.bar
+
+.section .bar,"a"
+.quad .bar