This is an archive of the discontinued LLVM Phabricator instance.

[libc] Setup TLS in x86_64 loader.
ClosedPublic

Authored by sivachandra on Jun 27 2020, 12:16 AM.

Details

Summary

The new code added is still very x86_64 specific. AArch64 support will
be added very soon and refactoring of the loader code will be done as
part of the patches adding it.

Diff Detail

Event Timeline

sivachandra created this revision.Jun 27 2020, 12:16 AM

Add TODO to adjust TLS address for static-pie executables.

MaskRay added inline comments.
libc/loader/linux/x86_64/start.cpp
38

This should be implemented directly in a generic place, instead of adding a TODO and moving all content immediately when implementing aarch64.

46

This formula can be written as: tlsSize = (app.tls.size + app.tls.align) & -app.tls.align
However, there is a larger issue. See below.

51

The size should be 2 words plus sizeof(pthread) plus p_memsz(PT_TLS) plus (headroom left for padding).

Initially dtv[0] = 1 (one module), dtv[1] = DTP offset. The pthread structure is placed at the end of dtv.

60

This can call an internal crash function instead of SYS_exit.

mmapRetVal < 0 is sufficient.

sivachandra marked an inline comment as done.Jul 6 2020, 12:36 PM
sivachandra added inline comments.
libc/loader/linux/x86_64/start.cpp
51

Size of pthread can/will be added once we develop or need an equivalent of that in LLVM-libc. So, without that, and without support for dynamic loading, can you give an example wherein the code as it stands in this patch would fail?

[I will add a note in the commit message and also a comment that we don't yet have dynamic loading support.]

asteinhauser accepted this revision.Aug 7 2020, 11:16 PM
asteinhauser added inline comments.
libc/config/linux/app.h
16

It would be good to add source-code comments about the semantics of those two structs and their fields.

libc/loader/linux/x86_64/start.cpp
25

There should probably be SYS_mmap2 instead of SYS_mmaps2.

46

The existing comment about tlsSize = (app.tls.size + app.tls.align) & -app.tls.align seems relevant to me. It assumes that app.tls.align is a power of two which is a valid assumption, but might be mentioned in libc/config/linux/app.h comments.

59

Why "if (mmapRetVal == MAP_FAILED)" is not a sufficient check?

This revision is now accepted and ready to land.Aug 7 2020, 11:16 PM
  • Rebase
  • Address comments.
  • Along the way, fixed some loader test depenency handling.
sivachandra marked 3 inline comments as done.Aug 7 2020, 11:18 PM
sivachandra added inline comments.
libc/loader/linux/x86_64/start.cpp
25

Good catch! Fixed.

59

Added a comment to explain this.

This revision was landed with ongoing or failed builds.Aug 7 2020, 11:19 PM
This revision was automatically updated to reflect the committed changes.