Enable `#[thread_local]` for all windows-msvc targets by ChrisDenton · Pull Requ...
source link: https://github.com/rust-lang/rust/pull/92042
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
As it stands, #[thread_local] is enabled haphazardly for msvc. It seems all 64-bit targets have it enabled, but not 32-bit targets unless they're also UWP targets (perhaps because UWP was added more recently?). So this PR simply enables it for 32-bit targets as well. I can't think of a reason not to and I've confirmed by running tests locally which pass.
See also #91659
r? @nagisa
(rust-highfive has picked a reviewer for you, use r? to override)
Warning
- These commits modify compiler targets. (See the Target Tier Policy.)
@@ -27,6 +27,7 @@ pub fn opts() -> TargetOptions {
// linking some libraries which require a specific agreement, so it may
// not ever be possible for us to pass this flag.
no_default_libraries: false,
has_elf_tls: true,
I think this field wants a rename to something like has_thread_local or something of a similar nature?
EDIT: reading the issue, yeah it is fine to rename target fields.
has_thread_local sounds ok to me! If anybody wants to bikeshed the name I don't mind changing it again but it's probably not worth arguing about it too much 
.
Are we sure that this field is never used in some ELF-specific way?
Hm, well if it is then it was wrong to use it for msvc targets. But I did do a search and the only place it's used (other than specs) is here:
if sess.target.has_elf_tls {
ret.insert((sym::target_thread_local, None));
}
As far as I understand it, Rust defers to LLVM for actually generating the TLS access (it uses the LLVM IR thread_local).
Hm, one thing that's curious to me is that we have a test ui/thread-local/tls.rs which only ignores emscripten targets and uses #[thread_local] unconditionally.
Can you confirm this test is run on Windows?
Yes it runs. But it succeeds regardless of if has_elf_tls is true or false. I'm confused. As a test I forced it to fail by appending assert!(false) so it's definitely being run.
EDIT EDIT: Hm, I think #[thread_local] always works but the associated cfg isn't enabled.
/// this target.
pub has_elf_tls: bool,
/// Flag indicating whether #[thread_local] is available for this target.
pub has_thread_local: bool,
Maybe call it has_object_thread_local? This indicates whether thread locals are supported by the used object file format, not whether thread locals in general are supported. thread_local!{} works even if this is false.
has_target_thread_local also makes sense. This matches the #[cfg(target_thread_local)] attribute whose value it controls.
has_target_thread_localalso makes sense.
This is already in the context of Target though and many other fields don't have this prefix in target spec but has one as the cfg variable.
Some other options: has_static_thread_local (as in “static initailizers”), has_native_thread_local…
@bors r+
I think it is fine if we rename the field again if has_thread_local turns out to be too ambiguous. At the very least it is no longer incorrect.
Commit 391332c has been approved by nagisa
Should the naming discussion be continued on the original issue?
Seems reasonable to do so. Or in a new issue entirely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK