Page MenuHomePhabricator

[ELF] Set Out::TlsPhdr earlier for encoding packed reloc tables
ClosedPublic

Authored by rprichard on Sep 4 2018, 9:32 PM.

Details

Summary

For --pack-dyn-relocs=android, finalizeSections calls
LinkerScript::assignAddresses and
AndroidPackedRelocationSection::updateAllocSize in a loop,
where assignAddresses lays out the ELF image, then updateAllocSize
determines the size of the Android packed relocation table by encoding it.
Encoding the table requires knowing the values of relocation addends.

To get the addend of a TLS relocation, updateAllocSize can call getSymVA
on a TLS symbol before setPhdrs has initialized Out::TlsPhdr, producing an
error:

<file> has an STT_TLS symbol but doesn't have an SHF_TLS section

Fix the problem by initializing Out::TlsPhdr immediately after the program
headers are created. The segment's p_vaddr field isn't initialized until
setPhdrs, so use FirstSec->Addr, which is what setPhdrs would use.
FirstSec will typically refer to the .tdata or .tbss output section, whose
(tentative) address was computed by assignAddresses.

Android currently avoids this problem because it uses emutls and doesn't
support ELF TLS. This problem doesn't apply to --pack-dyn-relocs=relr
because SHR_RELR only handles relative relocations without explicit addends
or info.

Fixes https://bugs.llvm.org/show_bug.cgi?id=37841.

Event Timeline

rprichard created this revision.Sep 4 2018, 9:32 PM

Thanks for the detailed explanations. I only had one minor nitpick.

ELF/Writer.cpp
1654

const?

rprichard added inline comments.Sep 5 2018, 2:25 AM
ELF/Writer.cpp
1654

We'd also have to make Out::TlsPhdr point to a const PhdrEntry. Do we want to do that? The rest of the section fields in InX and Out aren't const.

srhines added inline comments.Sep 5 2018, 2:34 AM
ELF/Writer.cpp
1654

That should be decided by the main authors of LLD, but it seems like const would work fine on the declaration today for all of the existing uses.

rprichard updated this revision to Diff 164112.Sep 5 2018, 3:04 PM

Make Out::TlsPhdr a pointer to const.

rprichard marked an inline comment as done.Sep 5 2018, 3:04 PM

Thanks for the update. I'll let the project maintainers give a final LGTM, but I'm happy with it.

ruiu added inline comments.Sep 13 2018, 4:54 PM
ELF/OutputSections.h
132 ↗(On Diff #164112)

Seems like an unrelated change.

ELF/Writer.cpp
1654

We normally don't add const; instead we keep variable scope as short as possible to make it obviously const. Please drop it.

1654

For consistency, add {}. We do

for (...)
  if (...)
    ...;

but don't do this:

for (...)
  if (...) {
    ...
  }

But perhaps the best way of writing this code is

for (...) {
  if (!condition)
    continue;
  ...;
  break;
}
1654

It needs comments. Essentially, all nontrivial code needs comments so that the code can be read and understood by a first-time reader.

rprichard updated this revision to Diff 165636.Sep 14 2018, 9:46 PM

I removed const from Out::TlsPhdr and added comments.

I simplified the TLS segment search loop. It could break out of the loop once it finds the segment, which I suppose could be faster, but it doesn't worth the extra lines of code.

ruiu accepted this revision.Sep 15 2018, 3:46 AM

LGTM

This revision is now accepted and ready to land.Sep 15 2018, 3:46 AM
This revision was automatically updated to reflect the committed changes.