Page MenuHomePhabricator

[ELF] - fixed not properly handled @GOTTPOFF relocation against local symbols.
ClosedPublic

Authored by grimar on Dec 16 2015, 10:00 AM.

Details

Summary

This patch changes sequence of applying relocations, moving tls optimized relocation handling code before code for other locals.
Without that change relocation @GOTTPOFF against local symbol caused runtime error ("unrecognized reloc ...").
That change also should fix other tls optimized relocations, but I did not check them, thats a field for another patch.

R_X86_64_GOTTPOFF relocations agains locals can be found when linking against libc.a(malloc.o):
000000000036 000600000016 R_X86_64_GOTTPOFF 0000000000000000 libc_tsd_MALLOC - 4
000000000131 000600000016 R_X86_64_GOTTPOFF 0000000000000000
libc_tsd_MALLOC - 4
...

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 43024.Dec 16 2015, 10:00 AM
grimar retitled this revision from to [ELF] - fixed not properly handled @GOTTPOFF relocation against local symbols..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
emaste added a subscriber: emaste.Dec 16 2015, 10:15 AM
ruiu added inline comments.Dec 16 2015, 10:46 AM
ELF/InputSection.cpp
187 ↗(On Diff #43024)

This is an unrelated change to the reordering of TLS optimization code, no? If so, please split this patch into two patches.

grimar added inline comments.Dec 17 2015, 12:54 AM
ELF/InputSection.cpp
187 ↗(On Diff #43024)

Actually it is related. For local symbols getLocalRelTarget() is used to obtain SymVA. Previously it calculated addend inside and always used it for result. But for this relocation addend should not be taken in account as it is optimized in relocateTlsOptimize().
Thats why I had to add addend as argument here.

ruiu accepted this revision.Dec 19 2015, 12:21 AM
ruiu edited edge metadata.

LGTM

ELF/InputSection.cpp
193 ↗(On Diff #43024)

Can you remove variable Body and use *PBody instead? It now feels a bit odd to define a new variable here just to dereference PBody.

This revision is now accepted and ready to land.Dec 19 2015, 12:21 AM
This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Dec 21 2015, 2:41 AM
ELF/InputSection.cpp
193 ↗(On Diff #43024)

Ok, I did it because of 2 reasons:

  1. Code that used Body assumed that it is not null, therefore reference object is the best for it.
  2. Using of *PBody introduced lot of changes, almost each line was affected what was not very reasonable because of first reason for me.

But having another variable for the same object also not very good, here I agree. I renamed PBody to Body then, since PBody name appeared because Body was busy, and its also consistent with other code which uses Body name for pointers to SymbolBody.