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

Add emulated TLS support #117873

Merged
merged 1 commit into from
Dec 9, 2023
Merged

Add emulated TLS support #117873

merged 1 commit into from
Dec 9, 2023

Conversation

quininer
Copy link
Contributor

@quininer quininer commented Nov 13, 2023

This is a reopen of #96317 . many android devices still only use 128 pthread keys, so using emutls can be helpful.

Currently LLVM uses emutls by default for some targets (such as android, openbsd), but rust does not use it, because has_thread_local is false.

This commit has some changes to allow users to enable emutls:

  1. add -Zhas-thread-local flag to specify that std uses #[thread_local] instead of pthread key.
  2. when using emutls, decorate symbol names to find thread local symbol correctly.
  3. change -Zforce-emulated-tls to -Ztls-model=emulated to explicitly specify whether to generate emutls.

r? @Amanieu

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 13, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 13, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

@quininer
Copy link
Contributor Author

quininer commented Nov 13, 2023

I noticed that gcc.a with __emutls_get_address is no longer provided after ndk r23 (like libunwind issue #85806 (comment) ), which means that the android target may be forced to link libc++_shared.so.

If this is a problem, then we might provide an flag to allow users to enable emutls, or reimplement emutls in Rust.

Zig has a similar problem, and they implement their emutls. ziglang/zig#7577

I realize compiler-builtins has opened emutls.c at rust-lang/compiler-builtins#458, I'll try again soon.

@quininer quininer marked this pull request as draft November 13, 2023 14:29
@quininer
Copy link
Contributor Author

I still can't get it to work.

LLVM add __emutls_v. prefix for thread local symbols of using emutls , but rustc's exported_symbols still outputs the original symbol name. a linking error occurs when I try to build std.

  = note: ld.lld: error: version script assignment of 'global' to symbol '_ZN3std4hash6random11RandomState3new4KEYS7__getit5__KEY17h67dba5b589f8f44fE' failed: symbol not defined
          ld.lld: error: version script assignment of 'global' to symbol '_ZN3std4sync4mpmc5waker17current_thread_id5DUMMY7__getit5__KEY17h25d816e212fdf2deE' failed: symbol not defined
          ld.lld: error: version script assignment of 'global' to symbol '_ZN3std4sync4mpmc7context7Context4with7CONTEXT7__getit5__KEY17h471bc5da0b12d731E' failed: symbol not defined

@Amanieu
Copy link
Member

Amanieu commented Nov 19, 2023

You'll need to make changes to the symbol handling code in compiler/rustc_codegen_ssa/src/back/symbol_export.rs to add the emutls prefix. I think you can handle this in linking_symbol_name_for_instance_in_crate, but I'm not very familiar with that code.

You'll want to add a field to the target spec to explicitly indicate that a target uses emulated TLS. At the moment I believe this only applies to Android and OpenHarmony (ohos) targets.

@rust-log-analyzer

This comment has been minimized.

@quininer
Copy link
Contributor Author

@Amanieu

I see that force_emulated_tls already exists, so i use that, and if that doesn't make sense i can add another emulated_tls.

I tried modifying linking_symbol_name_for_instance_in_crate but this doesn't work because the inconsistent symbol name in exported_symbols instead of linked_symbols. so i added an exporting_symbol_name_for_instance_in_crate referring to linking_symbol_name_for_instance_in_crate.

@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member

Amanieu commented Nov 20, 2023

It might be worth renaming that target spec option to use_emulated_tls. It makes more sense than "force".

@rust-log-analyzer

This comment has been minimized.

@quininer quininer marked this pull request as ready for review November 20, 2023 07:59
@rustbot
Copy link
Collaborator

rustbot commented Nov 20, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

@Amanieu
Copy link
Member

Amanieu commented Nov 23, 2023

LGTM. Let me quickly double-check with the Android target maintainers: @chriswailes @maurer @mgeisler

@quininer quininer changed the title Set has_thread_local=true for android Use emulated TLS for android Nov 23, 2023
@maurer
Copy link
Contributor

maurer commented Nov 23, 2023

Android hasn't required emutls since android Q (released in Sept 2019). Using emulated TLS has a number of negative effects around dlclose, speed, sanitizer breakage, etc.

While the upstream clang isn't yet using it by default, we do have several places it is turned on in a regular Android build. We haven't turned it on globally mostly due to legacy vendor toolchains and builds which do not use lld.

I agree that enabling this for targeting SDK revisions before Q could be nice, but I would like an option equivalent to -fno-emulated-lts to allow us to use the ELF TLS provided by bionic. Since Rust doesn't have toolchain/linker diversity like C does, it might be good to just use it when targeting Q+.

I'm also pinging people internally who know more about the status of migrating clang to use -fno-emulated-lts for more information.

tl;dr:

  1. Can we control this with a flag? Even a Z flag would be fine for now. Once we learn to change targets by OS versions we could hook it to SDK version instead, but that hasn't happened yet.
  2. Can we have that flag force native TLS rather than emulated?
  3. If you can wait until next week (we're on vacation for Thanksgiving), I can get clarification from the people behind the document I linked above and make sure everything I'm saying is right.

@quininer
Copy link
Contributor Author

@maurer

I agree with add a Z flag, but I need some guidance on how to implement it. i'm not sure how the Z flag is supposed to affect target.

Enjoy your holiday!

@quininer
Copy link
Contributor Author

Another option is to turn off use_emulated_tls and has_thread_local in android target and merge other parts as is. then users who wish to enable emutls can compile it using their own target.

@Amanieu
Copy link
Member

Amanieu commented Nov 28, 2023

For a command-line option, you could extend the existing -Z tls-model option to support emulated TLS as an option. I don't think the standard TLS models apply when using emulated TLS.

@rustbot
Copy link
Collaborator

rustbot commented Nov 29, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@Amanieu
Copy link
Member

Amanieu commented Nov 29, 2023

Looks like a failed rebase?

@quininer
Copy link
Contributor Author

quininer commented Nov 29, 2023

Looks like a failed rebase?

I'll solve this after lunch. :D Fixed

@@ -120,6 +120,7 @@ fn to_llvm_tls_model(tls_model: TlsModel) -> llvm::ThreadLocalMode {
TlsModel::LocalDynamic => llvm::ThreadLocalMode::LocalDynamic,
TlsModel::InitialExec => llvm::ThreadLocalMode::InitialExec,
TlsModel::LocalExec => llvm::ThreadLocalMode::LocalExec,
TlsModel::EmulatedTls => llvm::ThreadLocalMode::GeneralDynamic,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried mapping to NotThreadLocal, but this causes LLVM to not generate thread local symbol.

@@ -20,6 +20,8 @@ loaded at program startup.
The TLS data must not be in a library loaded after startup (via `dlopen`).
- `local-exec` - model usable only if the TLS data is defined directly in the executable,
but not in a shared library, and is accessed only from that executable.
- `emulated-tls` - Uses thread-specific data keys to implement emulated TLS.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this quote from @MaskRay 's post. ❤️

@rfcbot
Copy link

rfcbot commented Dec 4, 2023

@thomcc proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 4, 2023
@rust-log-analyzer

This comment has been minimized.

@quininer
Copy link
Contributor Author

quininer commented Dec 6, 2023

Can we go ahead and merge this? :) @Amanieu

@MaskRay
Copy link
Contributor

MaskRay commented Dec 6, 2023

The first comment (description) can use a better description. I was confused even if I have done a lot of clang driver work and know TLS pretty well...

@quininer
Copy link
Contributor Author

quininer commented Dec 6, 2023

I've added a commit message to explain what this commit does.

Currently LLVM uses emutls by default
for some targets (such as android, openbsd),
but rust does not use it, because `has_thread_local` is false.

This commit has some changes to allow users to enable emutls:

1. add `-Zhas-thread-local` flag to specify
    that std uses `#[thread_local]` instead of pthread key.
2. when using emutls, decorate symbol names
    to find thread local symbol correctly.
3. change `-Zforce-emulated-tls` to `-Ztls-model=emulated`
    to explicitly specify whether to generate emutls.
@Amanieu
Copy link
Member

Amanieu commented Dec 8, 2023

Can you copy the commit message to the PR description? Github will include the PR description in the merge commit.

r=me once the description is updated.

@quininer
Copy link
Contributor Author

quininer commented Dec 9, 2023

@Amanieu done

@Amanieu
Copy link
Member

Amanieu commented Dec 9, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Dec 9, 2023

📌 Commit e5b7689 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2023
@bors
Copy link
Contributor

bors commented Dec 9, 2023

⌛ Testing commit e5b7689 with merge 608f324...

@bors
Copy link
Contributor

bors commented Dec 9, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 608f324 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 9, 2023
@bors bors merged commit 608f324 into rust-lang:master Dec 9, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 9, 2023
@quininer quininer deleted the android-emutls branch December 9, 2023 08:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (608f324): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [0.4%, 2.9%] 4
Regressions ❌
(secondary)
1.9% [1.9%, 1.9%] 1
Improvements ✅
(primary)
-1.5% [-1.5%, -1.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [-1.5%, 2.9%] 5

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 668.465s -> 670.776s (0.35%)
Artifact size: 314.06 MiB -> 314.08 MiB (0.00%)

GuillaumeGomez pushed a commit to GuillaumeGomez/rust that referenced this pull request Feb 21, 2024
Add emulated TLS support

This is a reopen of rust-lang#96317 . many android devices still only use 128 pthread keys, so using emutls can be helpful.

Currently LLVM uses emutls by default for some targets (such as android, openbsd), but rust does not use it, because `has_thread_local` is false.

This commit has some changes to allow users to enable emutls:

1. add `-Zhas-thread-local` flag to specify that std uses `#[thread_local]` instead of pthread key.
2. when using emutls, decorate symbol names to find thread local symbol correctly.
3. change `-Zforce-emulated-tls` to `-Ztls-model=emulated` to explicitly specify whether to generate emutls.

r? `@Amanieu`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.