Enable `#[thread_local]` for all windows-msvc targets #92042

ChrisDenton · 2021-12-17T15:59:09Z

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

rust-highfive · 2021-12-17T15:59:12Z

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2021-12-17T15:59:13Z

⚠️ Warning ⚠️

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

nagisa · 2021-12-17T19:33:54Z

compiler/rustc_target/src/spec/windows_msvc_base.rs

@@ -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:

rust/compiler/rustc_session/src/config.rs

Lines 883 to 885 in c5ecc15

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).

nagisa · 2021-12-17T19:39:24Z

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?

ChrisDenton · 2021-12-17T20:40:21Z

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.

bjorn3 · 2021-12-18T16:20:01Z

compiler/rustc_target/src/spec/mod.rs

-    /// 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_local also 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…

nagisa · 2021-12-18T21:55:38Z

@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.

bors · 2021-12-18T21:55:40Z

📌 Commit 391332c has been approved by nagisa

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#91141 (Revert "Temporarily rename int_roundings functions to avoid conflicts") - rust-lang#91984 (Remove `in_band_lifetimes` from `rustc_middle`) - rust-lang#92028 (Sync portable-simd to fix libcore build for AVX-512 enabled targets) - rust-lang#92042 (Enable `#[thread_local]` for all windows-msvc targets) - rust-lang#92071 (Update example code for Vec::splice to change the length) - rust-lang#92077 (rustdoc: Remove unused `collapsed` field) - rust-lang#92081 (rustdoc: Remove unnecessary `need_backline` function) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

ChrisDenton · 2021-12-19T16:05:48Z

Should the naming discussion be continued on the original issue?

nagisa · 2021-12-19T16:12:50Z

Seems reasonable to do so. Or in a new issue entirely.

quininer · 2022-03-08T11:32:56Z

I noticed this has a regression on i686-pc-windows-msvc + windows7. This results in an access violation when the program initializes tls.

Unfortunately I can't provide a minimal reproducible example, but I'm sure turning off has_thread_local fixes the problem. I've tried using rustc before that commit or compiling rustc with has_thread_local turned off, and they don't crash.

ChrisDenton · 2022-03-10T21:44:06Z

@quininer oh dear, that means our CI tests aren't able to tell when this regresses. This will likely need to be reverted again.

But first would it be at all possible for you test this PR? No worries if not but it'd be nice to know if it helps your case or not.

quininer · 2022-03-11T03:46:32Z

@ChrisDenton Sorry, due to COVID-19 I need to WFH, so there is no windows7 device can to test at the moment.

Enable #[thread_local] for all windows-msvc targets

9ca26f1

rust-highfive assigned nagisa Dec 17, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 17, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2021

nagisa reviewed Dec 17, 2021

View reviewed changes

nagisa added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2021

Rename has_elf_tls to has_thread_local

391332c

bjorn3 reviewed Dec 18, 2021

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 18, 2021

matthiaskrgr mentioned this pull request Dec 19, 2021

Rollup of 7 pull requests #92099

Merged

bors merged commit 4dbe966 into rust-lang:master Dec 19, 2021

rustbot added this to the 1.59.0 milestone Dec 19, 2021

ChrisDenton deleted the msvc-static-tls branch December 19, 2021 16:05

csmoe mentioned this pull request Mar 29, 2022

#[thread_local] on i686 windows msvc causes suspicious crash in production #95429

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `#[thread_local]` for all windows-msvc targets #92042

Enable `#[thread_local]` for all windows-msvc targets #92042

ChrisDenton commented Dec 17, 2021

rust-highfive commented Dec 17, 2021

rust-highfive commented Dec 17, 2021

nagisa Dec 17, 2021 •

edited

Loading

ChrisDenton Dec 17, 2021

Aaron1011 Dec 17, 2021

ChrisDenton Dec 17, 2021

nagisa commented Dec 17, 2021

ChrisDenton commented Dec 17, 2021 •

edited

Loading

bjorn3 Dec 18, 2021

bjorn3 Dec 18, 2021

nagisa Dec 18, 2021

nagisa commented Dec 18, 2021

bors commented Dec 18, 2021

ChrisDenton commented Dec 19, 2021

nagisa commented Dec 19, 2021

quininer commented Mar 8, 2022

ChrisDenton commented Mar 10, 2022

quininer commented Mar 11, 2022

	if sess.target.has_elf_tls {
	ret.insert((sym::target_thread_local, None));
	}

Enable #[thread_local] for all windows-msvc targets #92042

Enable #[thread_local] for all windows-msvc targets #92042

Conversation

ChrisDenton commented Dec 17, 2021

rust-highfive commented Dec 17, 2021

rust-highfive commented Dec 17, 2021

nagisa Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

ChrisDenton Dec 17, 2021

Choose a reason for hiding this comment

Aaron1011 Dec 17, 2021

Choose a reason for hiding this comment

ChrisDenton Dec 17, 2021

Choose a reason for hiding this comment

nagisa commented Dec 17, 2021

ChrisDenton commented Dec 17, 2021 • edited Loading

bjorn3 Dec 18, 2021

Choose a reason for hiding this comment

bjorn3 Dec 18, 2021

Choose a reason for hiding this comment

nagisa Dec 18, 2021

Choose a reason for hiding this comment

nagisa commented Dec 18, 2021

bors commented Dec 18, 2021

ChrisDenton commented Dec 19, 2021

nagisa commented Dec 19, 2021

quininer commented Mar 8, 2022

ChrisDenton commented Mar 10, 2022

quininer commented Mar 11, 2022

Enable `#[thread_local]` for all windows-msvc targets #92042

Enable `#[thread_local]` for all windows-msvc targets #92042

nagisa Dec 17, 2021 •

edited

Loading

ChrisDenton commented Dec 17, 2021 •

edited

Loading