[ELF] Fix a null pointer dereference when --emit-relocs and --strip-debug are used together

[MaskRay]

Summary

Fixes https://bugs.llvm.org//show_bug.cgi?id=44878

When --strip-debug is specified, .debug* are removed from inputSections
while .rel[a].debug* (incorrectly) remain.

LinkerScript::addOrphanSections() requires the output section of a relocated
InputSectionBase to be created first.

.debug* are not in inputSections -> output sections .debug* are not
created -> getOutputSectionName(.rel[a].debug*) dereferences a null
pointer.

Fix the null pointer dereference by deleting .rel[a].debug* from inputSections as well.

[MaskRay]

lld/test/ELF/emit-relocs-debug.s

4

I'm a bit reluctant to add another test for the obsoleted feature .zdebug*

[nickdesaulniers]

lld/ELF/Driver.cpp

1947

will this fit on one line?

if (isDebugSection(isec->getRelocationSection())`

[MaskRay]

lld/ELF/Driver.cpp

1947

rel may be nullptr, if sh_info=0. Maybe I should make isDebugSection take a reference to emphasize that the argument cannot be nullptr.

[MaskRay]

Change isDebugSection() to take a const reference.

[grimar]

LGTM.

lld/test/ELF/emit-relocs-debug.s

4

We support it though. I have no strong opinion on this as I do not know if it is used in the wild.
Covering all the features can be good when collecting the code coverage. I had experiments
with it before: http://lists.llvm.org/pipermail/llvm-dev/2018-April/122782.html

May be we might think about reporting an error if an object has .zdebug and see if people will complain?
We have some places in the linker where we had to introduce some sort of temporary hacks.
E.g:

// The linkonce feature is a sort of proto-comdat. Some glibc i386 object
// files contain definitions of symbol "__x86.get_pc_thunk.bx" in linkonce
// sections. Drop those sections to avoid duplicate symbol errors.
// FIXME: This is glibc PR20543, we should remove this hack once that has been
// fixed for a while.
if (name == ".gnu.linkonce.t.__x86.get_pc_thunk.bx" ||
    name == ".gnu.linkonce.t.__i686.get_pc_thunk.bx")
  return &InputSection::discarded;

I guess one day we might want to start removing such things. Perhaps .zdebug is a good candidate
for one of the next LLD releases.

[MaskRay]

lld/test/ELF/emit-relocs-debug.s

4

My clang D61689 was reverted. I think our internal toolchain is still not ready (-no-integrated-as uses an old GNU as). I will try fixing the remaining issues so that our internal toolchain will not block .zdebug removal. I will start a llvm-dev notification when I want to delete .zdebug input support.

Regarding .gnu.linkonce*: I also want to delete them. I left a comment on https://sourceware.org/bugzilla/show_bug.cgi?id=20543 half a year ago.

[nickdesaulniers]

Regarding .gnu.linkonce*: I also want to delete them. I left a comment on https://sourceware.org/bugzilla/show_bug.cgi?id=20543 half a year ago.

Removing support for .gnu.linkonce will break module loading in the Linux kernel.
See also: https://github.com/ClangBuiltLinux/linux/issues/432

[MaskRay]

In D74510#1874665, @nickdesaulniers wrote:

Regarding .gnu.linkonce*: I also want to delete them. I left a comment on https://sourceware.org/bugzilla/show_bug.cgi?id=20543 half a year ago.

Removing support for .gnu.linkonce will break module loading in the Linux kernel.
See also: https://github.com/ClangBuiltLinux/linux/issues/432

What lld does is to simply discard .gnu.linkonce.t.__x86.get_pc_thunk.bx and .gnu.linkonce.t.__i386.get_pc_thunk.bx input sections from object files.
It never supports the semantics of .gnu.linkonce. Other .gnu.linkonce. prefixed section names (e.g. .gnu.link_once.this_module) are not recognized by lld.
So if something currently works, after removing the code below, it will continue working.

// The linkonce feature is a sort of proto-comdat. Some glibc i386 object
// files contain definitions of symbol "__x86.get_pc_thunk.bx" in linkonce
// sections. Drop those sections to avoid duplicate symbol errors.
// FIXME: This is glibc PR20543, we should remove this hack once that has been
// fixed for a while.
if (name == ".gnu.linkonce.t.__x86.get_pc_thunk.bx" ||
    name == ".gnu.linkonce.t.__i686.get_pc_thunk.bx")
  return &InputSection::discarded;

[kees]

Thank you for the quick fix! I can confirm my builds with --string-debug work now. :)

lld/ELF/Driver.cpp

template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
   llvm::erase_if(inputSections, [](InputSectionBase *s) {
     if (s->type == SHT_LLVM_SYMPART) {
       readSymbolPartitionSection<ELFT>(s);
       return true;
     }
 
     // We do not want to emit debug sections if --strip-all
     // or -strip-debug are given.
-    return config->strip != StripPolicy::None &&
-           (s->name.startswith(".debug") || s->name.startswith(".zdebug"));
+    if (config->strip == StripPolicy::None)
+      return false;
+
+    if (isDebugSection(*s))
+      return true;
+    if (auto *isec = dyn_cast<InputSection>(s))
+      if (InputSectionBase *rel = isec->getRelocatedSection())
+        if (isDebugSection(*rel))
+          return true;
+
+    return false;
   });
 
   // Now that the number of partitions is fixed, save a pointer to the main
   // partition.
   mainPart = &partitions[0];
 
   // Read .note.gnu.property sections from input object files which
   // contain a hint to tweak linker's and loader's behaviors.

lld/ELF/InputSection.h

 
 private:
   template <class ELFT, class RelTy>
   void copyRelocations(uint8_t *buf, llvm::ArrayRef<RelTy> rels);
 
   template <class ELFT> void copyShtGroup(uint8_t *buf);
 };
 
+inline bool isDebugSection(const InputSectionBase &sec) {
+  return sec.name.startswith(".debug") || sec.name.startswith(".zdebug");
+}
+
 // The list of all input sections.
 extern std::vector<InputSectionBase *> inputSections;
 
 } // namespace elf
 
 std::string toString(const elf::InputSectionBase *);
 } // namespace lld
 
 #endif

lld/ELF/InputSection.cpp

if (sym.type == STT_SECTION) {
       // avoid having to parse and recreate .eh_frame, we just replace any
       // relocation in it pointing to discarded sections with R_*_NONE, which
       // hopefully creates a frame that is ignored at runtime. Also, don't warn
       // on .gcc_except_table and debug sections.
       //
       // See the comment in maybeReportUndefined for PPC64 .toc .
       auto *d = dyn_cast<Defined>(&sym);
       if (!d) {
-        if (!sec->name.startswith(".debug") &&
-            !sec->name.startswith(".zdebug") && sec->name != ".eh_frame" &&
+        if (!isDebugSection(*sec) && sec->name != ".eh_frame" &&
             sec->name != ".gcc_except_table" && sec->name != ".toc") {
           uint32_t secIdx = cast<Undefined>(sym).discardedSecIdx;
           Elf_Shdr_Impl<ELFT> sec =
               CHECK(file->getObj().sections(), file)[secIdx];
           warn("relocation refers to a discarded section: " +
                CHECK(file->getObj().getSectionName(&sec), file) +
                "\n>>> referenced by " + getObjMsg(p->r_offset));
         }

lld/test/ELF/emit-relocs-debug.s

+# REQUIRES: x86
+## Test --emit-relocs handles .debug*
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: ld.lld --emit-relocs %t.o -o %t
+# RUN: llvm-readobj -r %t | FileCheck %s
+# RUN: ld.lld --emit-relocs --strip-debug %t.o -o %t.no
+# RUN: llvm-readobj -r %t.no | FileCheck --check-prefix=NO %s
+
+# CHECK:      Section {{.*}} .rela.debug_info {
+# CHECK-NEXT:   R_X86_64_64 .text 0x0
+# CHECK-NEXT: }
+
+# NO:      Relocations [
+# NO-NEXT: ]
+
+foo:
+
+.section .debug_info
+.quad foo