From 641d3b09f41b441f2c2618de32983ad3d13ea3f8 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 3 May 2021 11:39:10 -0700 Subject: [PATCH] std: Attempt again to inline thread-local-init across crates Issue #25088 has been part of `thread_local!` for quite some time now. Historical attempts have been made to add `#[inline]` to `__getit` in #43931, #50252, and #59720, but these attempts ended up not landing at the time due to segfaults on Windows. In the interim though with `const`-initialized thread locals AFAIK this is the only remaining bug which is why you might want to use `#[thread_local]` over `thread_local!`. As a result I figured it was time to resubmit this and see how it fares on CI and if I can help debugging any issues that crop up. Closes #25088 --- library/std/src/thread/local.rs | 24 +++++++++ .../codegen/auxiliary/thread_local_aux.rs | 6 +++ src/test/codegen/thread-local.rs | 50 +++++++++++++++++++ 3 files changed, 80 insertions(+) create mode 100644 src/test/codegen/auxiliary/thread_local_aux.rs create mode 100644 src/test/codegen/thread-local.rs diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index 1f5f26e3e1449..c2c7ab918985b 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -162,6 +162,7 @@ macro_rules! thread_local { macro_rules! __thread_local_inner { // used to generate the `LocalKey` value for const-initialized thread locals (@key $t:ty, const $init:expr) => {{ + #[cfg_attr(not(target_env = "msvc"), inline)] // see comments below unsafe fn __getit() -> $crate::option::Option<&'static $t> { const _REQUIRE_UNSTABLE: () = $crate::thread::require_unstable_const_init_thread_local(); @@ -260,6 +261,29 @@ macro_rules! __thread_local_inner { #[inline] fn __init() -> $t { $init } + // When reading this function you might ask "why is this inlined + // everywhere other than MSVC?", and that's a very reasonable + // question to ask. The short story is that it segfaults rustc if + // this function is inlined. The longer story is that MSVC looks to + // not support `extern` references to thread locals across DLL + // boundaries. This appears to at least not be supported in the ABI + // that LLVM implements. + // + // Because of this we never inline on MVSC, but we do inline on + // other platforms (where external references to thread locals + // across DLLs are supported). A better fix for this would be to + // inline this function on MSVC, but only for "statically linked" + // components. For example if two separately compiled rlibs end up + // getting linked into a DLL then it's fine to inline this function + // across that boundary. It's only not fine to inline this function + // across a DLL boundary. Unfortunately rustc doesn't currently have + // this sort of logic available in an attribute, and it's not clear + // that rustc is even equipped to answer this (it's more of a Cargo + // question kinda). This means that, unfortunately, MSVC gets the + // pessimistic path for now where it's never inlined. + // + // The issue of "should enable on MSVC sometimes" is #84933 + #[cfg_attr(not(target_env = "msvc"), inline)] unsafe fn __getit() -> $crate::option::Option<&'static $t> { #[cfg(all(target_arch = "wasm32", not(target_feature = "atomics")))] static __KEY: $crate::thread::__StaticLocalKeyInner<$t> = diff --git a/src/test/codegen/auxiliary/thread_local_aux.rs b/src/test/codegen/auxiliary/thread_local_aux.rs new file mode 100644 index 0000000000000..29b5e3ca244bf --- /dev/null +++ b/src/test/codegen/auxiliary/thread_local_aux.rs @@ -0,0 +1,6 @@ +#![crate_type = "lib"] +#![feature(thread_local_const_init)] + +use std::cell::Cell; + +thread_local!(pub static A: Cell = const { Cell::new(0) }); diff --git a/src/test/codegen/thread-local.rs b/src/test/codegen/thread-local.rs new file mode 100644 index 0000000000000..f14368e39902d --- /dev/null +++ b/src/test/codegen/thread-local.rs @@ -0,0 +1,50 @@ +// compile-flags: -O +// aux-build:thread_local_aux.rs +// ignore-windows FIXME(#84933) +// ignore-wasm globals are used instead of thread locals +// ignore-emscripten globals are used instead of thread locals +// ignore-android does not use #[thread_local] + +#![crate_type = "lib"] +#![feature(thread_local_const_init)] + +extern crate thread_local_aux as aux; + +use std::cell::Cell; + +thread_local!(static A: Cell = const { Cell::new(1) }); + +// CHECK: [[TLS_AUX:@.+]] = external thread_local local_unnamed_addr global i64 +// CHECK: [[TLS:@.+]] = internal thread_local unnamed_addr global + +// CHECK-LABEL: @get +#[no_mangle] +fn get() -> u32 { + // CHECK: %0 = load i32, i32* bitcast ({{.*}} [[TLS]] to i32*) + // CHECK-NEXT: ret i32 %0 + A.with(|a| a.get()) +} + +// CHECK-LABEL: @set +#[no_mangle] +fn set(v: u32) { + // CHECK: store i32 %0, i32* bitcast ({{.*}} [[TLS]] to i32*) + // CHECK-NEXT: ret void + A.with(|a| a.set(v)) +} + +// CHECK-LABEL: @get_aux +#[no_mangle] +fn get_aux() -> u64 { + // CHECK: %0 = load i64, i64* [[TLS_AUX]] + // CHECK-NEXT: ret i64 %0 + aux::A.with(|a| a.get()) +} + +// CHECK-LABEL: @set_aux +#[no_mangle] +fn set_aux(v: u64) { + // CHECK: store i64 %0, i64* [[TLS_AUX]] + // CHECK-NEXT: ret void + aux::A.with(|a| a.set(v)) +}