[ELF] - Do not crash when -r output uses linker script with `/DISCARD/`

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=39493.

We crashed previously because did not handle /DISCARD/ properly
when -r was used. I think it is uncommon to use scripts with -r, though I see
nothing wrong to handle the /DISCARD/ so that we will not crash at least.

[grimar]

• Simplify checks in the test.

[peter.smith]

I can confirm that this change does fix the problem observed in pr39493.

ELF/InputFiles.cpp

648–656

This bit of code looks like it could be merged with if (Config->EmitRelocs) on line 692

By doing so we wouldn't be skipping the code at 672 for Config->Relocatable I don't think that this code has any side-effects that would affect a discarded section though.

652

My understanding is that it is all Linux kernel modules use that fragment of linker script and not just arm64.

[grimar]

Thanks, Peter!

• Rewrote the comment.

ELF/InputFiles.cpp

648–656

I would prefer keeping early return, that is a bit simpler IMO. Let's see what others think.

652

Maybe it is just better to omit the name completely. It does not seem to be important that we observed this in Linux kernel, it could be anywhere.

[ruiu]

ELF/InputFiles.cpp

652

I think it is actually important to mention Linux kernel module as a use case of linker script + the -r flag, because using -r with a linker script sounds like a really bad idea (perhaps it is though).

[grimar]

• Mentioned the Linux kernel as a user of -r + /DISCARD/ in the comment.

[ruiu]

LGTM

ELF/InputFiles.cpp

case SHT_REL: {
     // relocation sections.
     InputSectionBase *Target = getRelocTarget(Sec);
     if (!Target)
       return nullptr;
 
     // This section contains relocation information.
     // If -r is given, we do not interpret or apply relocation
     // but just copy relocation sections to output.
-    if (Config->Relocatable)
-      return make<InputSection>(*this, Sec, Name);
+    if (Config->Relocatable) {
+      InputSection *RelocSec = make<InputSection>(*this, Sec, Name);
+      // We want to add a dependency to target, similar like we do for
+      // -emit-relocs below. This is useful for the case when linker script
+      // contains the "/DISCARD/". It is perhaps uncommon to use a script
+      // with -r, but we still have to handle such case and do not crash.
+      Target->DependentSections.push_back(RelocSec);
+      return RelocSec;
+    }
 
     if (Target->FirstRelocation)
       fatal(toString(this) +
             ": multiple relocation sections to one section are not supported");
 
     // ELF spec allows mergeable sections with relocations, but they are
     // rare, and it is in practice hard to merge such sections by contents,
     // because applying relocations at end of linking changes section

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.
       auto *D = dyn_cast<Defined>(&Sym);
       if (!D) {
         error("STT_SECTION symbol should be defined");
         continue;
       }
-      SectionBase *Section = D->Section;
-      if (Section == &InputSection::Discarded) {
+      SectionBase *Section = D->Section->Repl;
+      if (!Section->Live) {
         P->setSymbolAndType(0, 0, false);
         continue;
       }
 
       int64_t Addend = getAddend<ELFT>(Rel);
       const uint8_t *BufLoc = Sec->data().begin() + Rel.r_offset;
       if (!RelTy::IsRela)
         Addend = Target->getImplicitAddend(BufLoc, Type);
if (Sym.Type == STT_SECTION) {
         // not calculate relocations in this mode and loses
         // individual "gp" values used by each input object file.
         // As a workaround we add the "gp" value to the relocation
         // addend and save it back to the file.
         Addend += Sec->getFile<ELFT>()->MipsGp0;
       }
 
       if (RelTy::IsRela)
-        P->r_addend = Sym.getVA(Addend) - Section->Repl->getOutputSection()->Addr;
+        P->r_addend = Sym.getVA(Addend) - Section->getOutputSection()->Addr;
       else if (Config->Relocatable)
         Sec->Relocations.push_back({R_ABS, Type, Rel.r_offset, Addend, &Sym});
     }
   }
 }
 
 // The ARM and AArch64 ABI handle pc-relative relocations to undefined weak
 // references specially. The general rule is that the value of the symbol in

test/ELF/linkerscript/relocatable-discard.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "SECTIONS { /DISCARD/ : { *(.discard.*) }}" > %t.script
+# RUN: ld.lld -o %t --script %t.script -r %t.o
+# RUN: llvm-readobj -sections %t | FileCheck %s
+
+## Test shows that we do not crash after discarding the .discard.foo with -r.
+## Previously it happened because of 2 reasons:
+## 1) .rela.discard.foo was not handled properly and was not discarded.
+##    Remaining reference was invalid and caused the crash.
+## 2) Third-party section .debug_info referencing discarded section
+##    did not handle this case properly and tried to apply the
+##    relocation instead of ignoring it.
+
+# CHECK-NOT: .discard
+
+.section .discard.foo,"ax"
+callq fn@PLT
+
+.section .debug_info,"",@progbits
+.long .discard.foo