search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
94.88k stars 12.23k forks source link

`#[thread_local]` code gets compiled on targets that don't support it (and code does not work correctly) #106626

Closed flba-eb closed 1 year ago

flba-eb commented 1 year ago

@gh-tr and I have been trying to figure out why the test case tls.rs does not succeed on the QNX/Neutrino operating system. On this system, TLS support is disabled in TargetOptions. The test code compiles but at runtime both threads get the same address for the thread-local value.

After a discussion on zulip with @bjorn3 , @thomcc and @Amanieu my conclusion is that #[thread_local] is only supposed to work on targets that have TLS enabled in their TargetOptions.

If this is correct, then:

Why does the compiler accept it? Is this a bug in the compiler (even though the nightly feature is not stabilized)? Should it reject the code (of the test case on this target)? Or should we just ignore the test case?

We've noticed that when enabling emulated TLS in the compiler for QNX, the issue goes away. Is this the reason this test case succeeds on other targets that don't support thread_local but have emulated TLS enabled in the compiler such as Android? Are there other OS's that don't support thread_local, don't have emulated TLS enabled in the compiler and are passing this test? Should the test be disabled on all targets that don't support TLS, or is this always expected to succeed?

We've seen this for months in different Rust versions (e.g. master from 19th of December 2022 or from 9th of January 2023). Target is x86_64-pc-nto-qnx710.

Any help is appreciated!

Amanieu commented 1 year ago

Currently the has_thread_local target option only controls whether the standard library's thread_local! macro uses #[thread_local] or OS APIs (e.g. pthread_key_t) to implement TLS.

There is no setting in rustc to use emulated TLS (though one could be added). We currently just use LLVM's default setting: https://github.com/llvm/llvm-project/blob/17a19361223d567c154344f1065b315a93ddc4ba/llvm/include/llvm/TargetParser/Triple.h#L953

Given the current state of things (#[thread_local] being unstable), it's fine to just ignore the test for now. The proper way to have thread-local data is to use the thread_local! macro.

I think that the best plan moving forward is to make #[thread_local] follow the behavior of C++'s thread_local on the target, using emulated TLS if that is the default for the target. This enables FFI-compatiblity for extern { #[thread_local] static FOO: ...; }.

flba-eb commented 1 year ago

@Amanieu thanks for the fast response! I will disable the test for QNX/Neutrino.

Would it make sense to prevent users of the nightly feature from running into issues by letting rustc reject usage of #[thread_local] on targets that don't support it (until it gets stabilized)? If not I would close this issue.