This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Fixed handling R_X86_64_DTPOFF64 relocation relaxation
ClosedPublic

Authored by grimar on Mar 10 2016, 6:14 AM.

Details

Summary

R_X86_64_DTPOFF64 was not handled properly.
Next sample app was impossible to link before this patch:

~/pg/release/bin/clang -target x86_64-pc-linux testthread.cpp -c -g
~/pg/d+a/bin/ld.lld testthread.o
"Unknown TLS optimization" (value was 17)

__thread int x = 0;
void _start() {
}

It works fine now.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 50268.Mar 10 2016, 6:14 AM
grimar retitled this revision from to [ELF] - Fixed handling R_X86_64_DTPOFF64 relocation relaxation.
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
ruiu added inline comments.Mar 10 2016, 9:45 AM
ELF/Target.cpp
958–963 ↗(On Diff #50268)

Can't you reach this piece of code without going through relaxTls, can you?

If that's the case, please move this piece of code to relaxTls.

grimar updated this revision to Diff 50299.Mar 10 2016, 10:19 AM
  • Addressed review comments.
ELF/Target.cpp
961–966 ↗(On Diff #50299)

Done.

ruiu added inline comments.Mar 10 2016, 10:22 AM
ELF/Target.cpp
896–897 ↗(On Diff #50299)

I guess this is the same? Please inline the code here in a follow-up patch.

900 ↗(On Diff #50299)

Does checkInt<64> do anything? A 64-bit integer is always within 64 bits.

grimar updated this revision to Diff 50304.Mar 10 2016, 10:45 AM
  • Addressed review comments
ELF/Target.cpp
896–897 ↗(On Diff #50299)

Yes, looks so, will do that tomorrow.

900 ↗(On Diff #50299)

You right.
Its implementation even has the opt-check for that.

inline bool isInt(int64_t x) {
  return N >= 64 || (-(INT64_C(1)<<(N-1)) <= x && x < (INT64_C(1)<<(N-1)));
}
ruiu accepted this revision.Mar 10 2016, 10:54 AM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Mar 10 2016, 10:54 AM
This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Mar 11 2016, 1:34 AM
ELF/Target.cpp
896–897 ↗(On Diff #50299)

So after closer look, that is a different situation. We can reach R_X86_64_TPOFF32 from code outside relaxTls.
tls.s testcase for example shows it:

movl %fs:a@tpoff, %eax

generates R_X86_64_TPOFF32 and we reach it from common flow: relocate()->relocateOne().

grimar added inline comments.Mar 11 2016, 1:55 AM
ELF/Target.cpp
958–963 ↗(On Diff #50268)

So in this patch - yes, but I found the way to reach R_X86_64_TPOFF64 using the next code:

.global a
.section .tbss,"awT",@nobits
  .align 4
a:
  .long 0

.text
.globl _start
_start:
 .quad	a@tpoff

But that is also not supported by gold:
gold: error: testthread.o: unexpected reloc 18 in object file
(bfd works fine here)

So it seems compilers don't do that and there is no need to support it.