Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LibELF+LibC: Support loading shared objects with dynamic thread-local storage #19809

Merged
merged 5 commits into from
Aug 18, 2023

Conversation

BertalanD
Copy link
Member

@BertalanD BertalanD commented Jul 5, 2023

This is a prerequisite for upstreaming our LLVM patches, which currently need to hackily force the initial-exec TLS model in the Clang driver.

Currently, our kernel-managed TLS implementation limits us to only having a single block of storage for all thread-local variables that's initialized at load time. Changing that is outside the scope of this PR, but is in my plans, and there have already been design discussions in #clang-toolchain. This PR merely implements the dynamic TLS interface (__tls_get_addr and TLSDESC) on top of our static TLS infrastructure. The limitation about dlopen()ed shared objects with a non-zero-initialized TLS image and having to fit in the static TLS block still stands.

@BertalanD BertalanD marked this pull request as draft July 5, 2023 09:46
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Jul 5, 2023
Userland/Libraries/LibC/tls.cpp Show resolved Hide resolved
Tests/LibELF/DynlibTLS.cpp Outdated Show resolved Hide resolved
Tests/LibELF/TestTLS.cpp Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Jul 28, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@BertalanD

This comment was marked as outdated.

@BertalanD BertalanD force-pushed the global-dynamic branch 2 times, most recently from 3cba3d6 to 727d57b Compare August 16, 2023 09:36
@BertalanD BertalanD marked this pull request as ready for review August 16, 2023 09:37
@BertalanD BertalanD requested a review from timschumi as a code owner August 16, 2023 09:37
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Aug 16, 2023
@BertalanD BertalanD requested a review from ADKaster August 16, 2023 10:05
Copy link
Member

@timschumi timschumi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nitpicks and a build failure, looks good (to the rather untrained observer) otherwise.

Tests/LibELF/CMakeLists.txt Show resolved Hide resolved
Userland/Libraries/LibELF/DynamicLoader.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibC/CMakeLists.txt Outdated Show resolved Hide resolved
Tests/LibELF/CMakeLists.txt Show resolved Hide resolved
These should cover all relocation types we can possibly see in an x86_64
or AArch64 final linked ELF image.
LibC is always guaranteed to be loaded at program start, so its
thread-local variables live in the static TLS block. This permits us to
use the more optimal initial-exec TLS access model.
It's the pointer that should be volatile, not the pointed-to object.
This is a prerequisite for upstreaming our LLVM patches, as our current
hack forcing `-ftls-model=initial-exec` in the Clang driver is not
acceptable upstream.

Currently, our kernel-managed TLS implementation limits us to only
having a single block of storage for all thread-local variables that's
initialized at load time. This PR merely implements the dynamic TLS
interface (`__tls_get_addr` and TLSDESC) on top of our static TLS
infrastructure. The current model's limitations still stand:
- a single static TLS block is reserved at load time, `dlopen()`-ing
  shared libraries that define thread-local variables might cause us to
  run out of space.
- the initial TLS image is not changeable post-load, so `dlopen()`-ing
  libraries with non-zero-initialized TLS variables is not supported.

The way we repurpose `ti_module` to mean "offset within static TLS
block" instead of "module index" is not ABI-compliant.
The setup is a bit peculiar: both the definition and the use site of
these TLS variables have to be in a shared library, otherwise the linker
might relax the global-dynamic access mode to something that doesn't
require a `__tls_get_addr` call.
@timschumi timschumi dismissed their stale review August 17, 2023 10:18

Requested changes were taken care of

@gmta gmta merged commit c63fe0e into SerenityOS:master Aug 18, 2023
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Aug 18, 2023
@BertalanD BertalanD deleted the global-dynamic branch August 18, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants