This is an archive of the discontinued LLVM Phabricator instance.

[ELF/GC] Fix pending references to garbage collected sections
ClosedPublic

Authored by davide on Nov 1 2016, 9:54 AM.

Details

Summary

The example reported in https://llvm.org/bugs/show_bug.cgi?id=30793 shows a case where gc reclaims a SHF_TLS section, but it doesn't recliam the section containing the debug info for it. This is, anyway, expected, as we do not reclaim non-alloc sections during the garbage collection phase (and this is not going to change anytime soon, at least this is what I gathered last I talked with Rafael about it).
So, we end up with a pending reference, thinking that the input was invalid (which is not true, as it's GC that removed the SH_TLS section, and therefore didn't create the PT_TLS *segment* for it). In cases like this, just assign a VA of zero at relocaiton time instead of error'ing out (this is what gold does as well, FWIW).

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 76583.Nov 1 2016, 9:54 AM
davide retitled this revision from to [ELF/GC] Fix pending references to garbage collected sections.
davide updated this object.
davide added reviewers: rafael, Bigcheese, phosek.
davide added a subscriber: llvm-commits.
Bigcheese requested changes to this revision.Nov 1 2016, 11:03 AM
Bigcheese edited edge metadata.
Bigcheese added inline comments.
ELF/InputSection.cpp
410–414 ↗(On Diff #76583)

This can be simplified to:

uint64_t SymVA = 0;
if (!Sym.isTls() || Out<ELFT>::TlsPhdr)
  SymVA = SignExtend64<sizeof(uintX_t) * 8>(getSymVA<ELFT>(Type, Addend, AddrLoc, Sym, R_ABS));
test/ELF/gc-debuginfo-tls.s
19–28 ↗(On Diff #76583)

This is a pretty obfuscated way to get a TLS symbol to a gced section. I think this can be simplified to something like:

  .section .tbss,"awT",@nobits
patatino:
  .long 0
  .section .noalloc,""
  .quad patatino
This revision now requires changes to proceed.Nov 1 2016, 11:03 AM
davide added inline comments.Nov 1 2016, 11:08 AM
test/ELF/gc-debuginfo-tls.s
19–28 ↗(On Diff #76583)

This is reduced from the original testcase. But sure, I can add another test case if you want.

Bigcheese added inline comments.Nov 1 2016, 11:10 AM
test/ELF/gc-debuginfo-tls.s
19–28 ↗(On Diff #76583)

Sure, just add a comment to the .cfi* one saying that it was reduced from a testcase and that the debug info directives are generating the reference.

davide updated this revision to Diff 76609.Nov 1 2016, 11:26 AM
davide edited edge metadata.

Addressed Spence's comments.

Bigcheese accepted this revision.Nov 1 2016, 11:27 AM
Bigcheese edited edge metadata.

lgtm

This revision is now accepted and ready to land.Nov 1 2016, 11:27 AM
rafael accepted this revision.Nov 1 2016, 11:59 AM
rafael edited edge metadata.

LGTM

phosek accepted this revision.Nov 1 2016, 12:54 PM
phosek edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.