[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 ↗

(On Diff #171653)

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 ↗

(On Diff #171653)

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 ↗

(On Diff #171653)

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

652 ↗

(On Diff #171653)

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 ↗

(On Diff #171653)

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

lld/trunk/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 faced it in the Linux kernel and have to handle such case
+      // and not to 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

lld/trunk/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

lld/trunk/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