[ELF] Handle references to garbage collected common symbols

[bd1976llvm]

Summary

https://reviews.llvm.org/rL312796 meant that references to garbage collected common symbols would cause a segfault.

This change fixes the behaviour for references to stripped common symbols.

[ruiu]

I don't remember why we don't have this check in getSymVA() for DefinedRegularKind. Should we actually be returning 0 for DefinedRegular if sections are dead too?

[ruiu]

(I'm not suggesting you make that change. I'm just trying to understand.)

[dmikulin]

Why is there a relocation for "unused" if it's marked not live?
Relocation section '.rela.noalloc' at offset 0x98 contains 1 entries:

Offset          Info           Type           Sym. Value    Sym. Name + Addend

000000000000 000200000001 R_X86_64_64 0000000000000004 unused + 0

[bd1976llvm]

In D37718#867246, @ruiu wrote:

I don't remember why we don't have this check in getSymVA() for DefinedRegularKind. Should we actually be returning 0 for DefinedRegular if sections are dead too?

Definitely. I also think that we should change the code to treat regular and common symbols in the same way when it is possible.

[bd1976llvm]

In D37718#867277, @dmikulin wrote:

Why is there a relocation for "unused" if it's marked not live?
Relocation section '.rela.noalloc' at offset 0x98 contains 1 entries:

Offset          Info           Type           Sym. Value    Sym. Name + Addend

000000000000 000200000001 R_X86_64_64 0000000000000004 unused + 0

The target of the relocation is "unused". The relocation patches a section that remains live.

This case commonly arises where the section that is being patched is informational and the linker can't garbage collect it e.g .debug_info.

p.s: strech goal for the brave - actually a linker could garbage collect these sections but it would be a lot of work.

[bd1976llvm]

In D37718#867427, @bd1976llvm wrote:

In D37718#867246, @ruiu wrote:

I don't remember why we don't have this check in getSymVA() for DefinedRegularKind. Should we actually be returning 0 for DefinedRegular if sections are dead too?

Definitely. I also think that we should change the code to treat regular and common symbols in the same way when it is possible.

I have a patch for what you are suggesting - is it ok to get this patch in first and put the improvements in as a later patch?

[dmikulin]

Forgot about debug...
Looks reasonable to me.

[smeenai]

I'm seeing breakages caused by this as well. Thanks for the fix!

I'm also supportive of getting this patch in ASAP and looking into further improvements as a follow-up, since it fixes a linker crash.

[ruiu]

LGTM

[bd1976llvm]

I posted this to the mailing list (copy here):

Thanks - I now plan to submit a series of patches to clean up this area of the code.
The basic plan is:

1. During write we should be able to use the Live bit to treat discarded and gc'd sections identically.
2. Before garbage collection we should replace all common symbols with equivalent bss symbols. These can then be treated like any other Defined Regular after that point.

I have finally got around to doing this improvement work, please take a look at:

1. https://reviews.llvm.org/D38136 (During write we should be able to use the Live bit to treat discarded and gc'd sections identically.)
2. https://reviews.llvm.org/D38137 (replace all common symbols with equivalent bss symbols.)

ELF/Symbols.cpp

if (D.isTls() && !Config->Relocatable) {
       return VA - Out::TlsPhdr->p_vaddr;
     }
     return VA;
   }
   case SymbolBody::DefinedCommonKind: {
     if (!Config->DefineCommon)
       return 0;
     auto DC = cast<DefinedCommon>(Body);
+    if (!DC.Live)
+      return 0;
     return DC.Section->getParent()->Addr + DC.Section->OutSecOff +
            DC.Offset;
   }
   case SymbolBody::SharedKind: {
     auto &SS = cast<SharedSymbol>(Body);
     if (SS.CopyRelSec)
       return SS.CopyRelSec->getParent()->Addr + SS.CopyRelSec->OutSecOff +
              SS.CopyRelSecOff;

test/ELF/common-gc3.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: ld.lld %t.o -o %t1 --gc-sections
+# RUN: llvm-objdump -s %t1 | FileCheck %s
+
+# CHECK:      Contents of section .noalloc:
+# 0000 00000000 00000000                    ........
+
+  .section  .text._start,"ax",@progbits
+  .globl  _start
+_start:
+  retq
+
+  .type unused,@object
+  .comm unused,4,4
+
+  .section  .noalloc,"",@progbits
+  .quad unused