This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Handle references to garbage collected common symbols
ClosedPublic

Authored by bd1976llvm on Sep 11 2017, 3:47 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

bd1976llvm created this revision.Sep 11 2017, 3:47 PM
ruiu edited edge metadata.Sep 11 2017, 4:02 PM

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 added a comment.Sep 11 2017, 4:02 PM

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

dmikulin edited edge metadata.Sep 11 2017, 4:16 PM

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

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.

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.

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?

Forgot about debug...
Looks reasonable to me.

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 accepted this revision.Sep 12 2017, 1:50 PM

LGTM

This revision is now accepted and ready to land.Sep 12 2017, 1:50 PM
This revision was automatically updated to reflect the committed changes.

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.)