This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Implement Local Dynamic style TLSDESC for x86-64
ClosedPublic

Authored by MaskRay on May 29 2019, 3:34 AM.

Details

Summary

For the Local Dynamic case of TLSDESC, _TLS_MODULE_BASE_ is defined as a
special TLS symbol that makes:

  1. With LD->LE relaxation: _TLS_MODULE_BASE_@tpoff = 0 (lowest address in

the TLS block).

  1. Without relaxation: it produces a dynamic TLSDESC relocation that

computes 0.

A Local Dynamic symbol can be accessed by adding an offset to tpoff (LE)
or dtpoff (LD) of _TLS_MODULE_BASE_. For the Local Dynamic case, this
saves GOT slots as otherwise we would create a dynamic TLSDESC
relocation and reserve two GOT slots for each symbol.

Add ElfSym::TlsModuleBase and change the signature of getTlsTpOffset()
to special case _TLS_MODULE_BASE_.

Event Timeline

MaskRay created this revision.May 29 2019, 3:34 AM
grimar added inline comments.May 29 2019, 3:48 AM
ELF/InputSection.cpp
595

I searched for _TLS_MODULE_BASE_ in LLVM code and found it is mentioned, but only for AARCH64.
Should it be created for this target too?

ELF/Writer.cpp
1609

i.e. how much reasonable this check is?

MaskRay marked an inline comment as done.May 29 2019, 4:17 AM
MaskRay added inline comments.
ELF/Writer.cpp
1609

In GNU linkers, _TLS_MODULE_BASE_ is defined on demand on i386/x86-64/arm/aarch64/s390. Many targets don't have an ABI for TLSDESC. If you feel we don't have to special case the TLSDESC targets that support TLSDESC, I can make this unconditional.

MaskRay updated this revision to Diff 201860.May 29 2019, 4:24 AM
MaskRay edited the summary of this revision. (Show Details)

Synthesize _TLS_MODULE_BASE_ on all targets for simplicity.

MaskRay updated this revision to Diff 201864.May 29 2019, 4:30 AM

Special case _TLS_MODULE_BASE_ on x86/x86-64 currently. We may extend this to
ARM/AArch64 in the future. Other targets don't have ABI for TLSDESC.

grimar added inline comments.May 29 2019, 5:14 AM
ELF/Writer.cpp
1609

I am not sure. Let see what Rui think. Perhaps whitelisting was fine. Without any conditions, I think arm/aarch64 will be just broken with this change (i.e. LLD will produce broken output instead of link error it seems).

1613

tlsBase -> TlsBase

test/ELF/x86-64-tlsdesc-ld.s
5

I would suggest adding a short description.

ruiu added inline comments.May 29 2019, 5:23 AM
ELF/InputSection.cpp
604

This line can be expensive if we have a lot of thread-local local symbols. getName() computes symbol names lazily, so if you call it on a local symbol, the function can be expensive. Maybe you should store TLSModuleBase symbol somewhere and compare S with the pointer?

grimar added inline comments.May 29 2019, 5:27 AM
ELF/InputSection.cpp
604

I thought about that too, but supposed that TLS symbols are not that common. Am I wrong?

grimar added inline comments.May 29 2019, 5:31 AM
ELF/InputSection.cpp
604

Also we can probably avoid this check if create the symbol with value of alignTo(Out::TlsPhdr->p_memsz, Out::TlsPhdr->p_align) it seems.

I.e. I am thinking about using original getTlsTpOffset() helper and creating a symbol with the value of -getTlsTpOffset().
Will this work?

ruiu added inline comments.May 29 2019, 5:38 AM
ELF/InputSection.cpp
604

Maybe it's not a big deal in practice. But identifying a symbol by name always rings a bell, so it'd probably easier to identify by a pointer comparison than explaining why this is not a big deal.

MaskRay updated this revision to Diff 201879.May 29 2019, 5:45 AM
MaskRay marked 3 inline comments as done.

Store _TLS_MODULE_BASE as SymbolTable::TlsBase

MaskRay updated this revision to Diff 201885.May 29 2019, 5:54 AM
MaskRay marked an inline comment as done.

Add comments to the test

MaskRay marked an inline comment as done.May 29 2019, 5:55 AM
MaskRay added inline comments.
ELF/InputSection.cpp
604

As a R_X86_64_TLSDESC dynamic relocation, the formula is: Sym->getVA(Addend) (in DynamicReloc::computeAddend())
As @tpoff, the formula is: Sym->getVA(A) + getTlsTpOffset()

Both must equal to 0. So there is a conflict. At least one place has to be a special case.

I feel the most straightforward approach is to special case the @tpoff computation here.

MaskRay updated this revision to Diff 201887.May 29 2019, 6:03 AM
MaskRay edited the summary of this revision. (Show Details)

Switch back to the original whitelist approach

I have no more comments, this looks good to me.

MaskRay updated this revision to Diff 202099.May 29 2019, 7:12 PM

Rename TlsBase to TlsModuleBase, elaborate the comment, and place the whitelist only in Writer::finalizeSections().

grimar added inline comments.May 30 2019, 1:50 AM
ELF/Writer.cpp
1626

Should this assignment be under if (S && S->isUndefined()) check?

I am not sure how much is real (I guess not at all honestly). but if user defines his own _TLS_MODULE_BASE_ symbol, then
with the current code getTlsTpOffset would return 0 for it. It is probably not user would expect.

grimar added inline comments.May 30 2019, 1:53 AM
ELF/Writer.cpp
1626

i.e.

if (S && S->isUndefined()) {
  S->resolve(Defined{/*File=*/nullptr, S->getName(), STB_GLOBAL, STV_HIDDEN,
                         STT_TLS, /*Value=*/0, 0,
                         /*Section=*/nullptr});
  Symtab->TlsModuleBase = S;
}
ruiu accepted this revision.May 30 2019, 2:10 AM

LGTM with this change.

ELF/SymbolTable.h
63 ↗(On Diff #202099)

We store this kind of symbol to ElfSym defined in Symbols.h. Please move.

This revision is now accepted and ready to land.May 30 2019, 2:10 AM
MaskRay updated this revision to Diff 202143.May 30 2019, 2:46 AM
MaskRay marked 4 inline comments as done.
MaskRay retitled this revision from [ELF] Support Local Dynamic style TLSDESC for x86-64 to [ELF] Implement Local Dynamic style TLSDESC for x86-64.
MaskRay edited the summary of this revision. (Show Details)

Move SymTable::TlsModuleBase to ElfSym::TlsModuleBase and
move the assignment under the if (S && S->isUndefined()) check as grimar suggested.

MaskRay marked 4 inline comments as done.May 30 2019, 2:54 AM
This revision was automatically updated to reflect the committed changes.