This is an archive of the discontinued LLVM Phabricator instance.

[lld][elf2] Generate PT_TLS.
ClosedPublic

Authored by Bigcheese on Oct 28 2015, 5:48 PM.

Diff Detail

Event Timeline

Bigcheese updated this revision to Diff 38701.Oct 28 2015, 5:48 PM
Bigcheese retitled this revision from to [lld][elf2] Generate PT_TLS..
Bigcheese updated this object.
Bigcheese added reviewers: rafael, ruiu.
Bigcheese added a subscriber: llvm-commits.
grimar added a subscriber: grimar.Oct 29 2015, 12:18 AM
ruiu added inline comments.Oct 29 2015, 7:19 AM
ELF/Writer.cpp
675

ThreadBSSOffset -> TbssOffset since .tbss is a section name.

710–711
ThreadBSSOffset = TVA - VA + Sec->getSize();
727

Can you make TlsPhdr a Elf_Phdr (instead of Elf_Phdr *) and check if its validity using its members? If TlsPhdr.p_vaddr > 0, for example, it must have a valid value.

Bigcheese updated this revision to Diff 38994.Nov 2 2015, 3:30 PM
Bigcheese marked 2 inline comments as done.

Address review comments.

ELF/Writer.cpp
675

.tbss is not the only possible section name. And this variable exists because of that. It handles the case of multiple SHF_ALLOC | SHF_WRITE | SHF_TLS, SHT_NOBITS output sections.

ruiu added inline comments.Nov 2 2015, 3:37 PM
ELF/Writer.cpp
674

Do we need this? Isn't an Elf_Phdr zero-initialized?

Bigcheese added inline comments.Nov 2 2015, 3:46 PM
ELF/Writer.cpp
674

There's no explicit zero init, and I'm pretty sure default-initialization here does nothing.

ruiu accepted this revision.Nov 2 2015, 3:50 PM
ruiu edited edge metadata.

LGTM

ELF/Writer.cpp
674

I'd zero out all fields using memset.

This revision is now accepted and ready to land.Nov 2 2015, 3:50 PM
Closed by commit rL251872: [elf2] Generate PT_TLS. (authored by mspencer). · Explain WhyNov 2 2015, 4:37 PM
This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Nov 3 2015, 12:02 AM
ELF/Writer.cpp
674

What about next ?

Elf_Phdr TlsPhdr = {};
ruiu added inline comments.Nov 3 2015, 11:15 AM
ELF/Writer.cpp
674

Seems like a good idea. I was not completely sure if the empty initializer is allowed to initialize a struct, but it seems to be accepted.