Check #[thread_local] statics correctly in the compiler. #43746

eddyb · 2017-08-08T16:55:52Z

Fixes #43733 by introducing #[allow_internal_unsafe] analogous to #[allow_internal_unstable], for letting a macro expand to unsafe blocks and functions even in #![forbid(unsafe_code)] crates.

Fixes #17954 by not letting references to #[thread_local] statics escape the function they're taken in - we can't just use a magical lifetime because Rust has lifetime parametrism, so if we added the often-proposed 'thread lifetime, we'd have no way to check it in generic code.
To avoid potential edge cases in the compiler, the lifetime is actually that of a temporary at the same position, i.e. &TLS_STATIC has the same lifetime &non_const_fn() would.

Referring to #[thread_local] statics at compile-time is banned now (as per PR discussion).

Additionally, to remove unsafe impl Sync from std::thread::local::fast::Key, #[thread_local] statics are now not required to implement Sync, as they are not shared between threads.

rust-highfive · 2017-08-08T16:56:03Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

eddyb · 2017-08-08T16:56:09Z

cc @rust-lang/libs @rust-lang/compiler

est31 · 2017-08-08T17:56:10Z

If #[allow_internal_unsafe] is ever going to be stabilized, it should imo be forbidden to declare macros with that label in #[forbid(unsafe)] contexts as otherwise the #[forbid(unsafe)] attribute gets meaningless.

eddyb · 2017-08-08T17:57:04Z

@est31 I expect it be replaced by macros 2.0 hygiene.

alexcrichton · 2017-08-09T16:49:57Z

src/librustc/middle/mem_categorization.rs

@@ -643,7 +643,13 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
                Ok(self.cat_rvalue_node(id, span, expr_ty))
          }

-          Def::Static(_, mutbl) => {
+          Def::Static(def_id, mutbl) => {
+            // `#[thread_local]` statics may not outlive the current function.


Looks like 2-space indent here?

The entire file has broken (read: old-skool match rules) indentation, I'm actually on a multiple of 4 and the Def::Static line above is wrong.

Oh I just mentioned this b/c the line just after these modifications is 4-space indented, but yes it looks like the indentation is off in most of this file.

alexcrichton · 2017-08-09T16:51:39Z

Is the intention to get thread-local statics "working" in the sense that it's harder to get them wrong in the compiler? Do you think we should get them on the path to stabilization?

Other programs which work today and "shouldn't" work are:

#![feature(thread_local)]

#[thread_local]
static A: u32 = 1;

static B: &'static u32 = &A;

fn main() {}

but maybe this patch starts to reject that?

eddyb · 2017-08-09T18:46:58Z

@alexcrichton Interesting, I didn't consider that. Does that not ICE? Trigger an LLVM assertion or linker error? That is, I don't know what meaningful codegen LLVM could possibly have for B.

As for the intention, I want people to stop pitching 'thread and/or complain about the feature.
OTOH, if your platform supports it (and we can probably get LLVM to support even more), a Cell in a #[thread_local] static seems pretty good - but I don't have any preference either way.

alexcrichton · 2017-08-09T21:45:37Z

LMRTFY (let me rust that for you)

It works, and I wouldn't expect it to work (I'd expect a rustc error)

eddyb · 2017-08-10T05:56:56Z

Out of minimal reduction, on Release mode, we have this LLVM IR:

@A = thread_local constant i32 1, align 4
@B = local_unnamed_addr constant i32* @A, align 8

And these assembly entries:

	.type	A,@object
	.section	.tdata.A,"awT",@progbits
	.globl	A
	.p2align	2
A:
	.long	1
	.size	A, 4

	.type	B,@object
	.section	.data.rel.ro.B,"aw",@progbits
	.globl	B
	.p2align	3
B:
	.quad	A
	.size	B, 8

What I suspect is happening is B is pointing to the original data, from which each thread gets a copy. This would not be unsound, as it'd behave as if compile-time was another thread.

EDIT: okay, so I was right about the addresses (see playpen) but... changes to the TLS are reflected in the original?! Or is #[thread_local] static mut just completely broken?
I'll attempt the same experiment with internal mutability next.

EDIT2: oh it just looks like the loads from B get aggressively folded to be &A.

EDIT3: fixed up demo - however, because of the aggressive load folding behavior which changes uses of B from pointing to the initializer of A to the runtime TLS slot, we should ban this anyway.

eddyb · 2017-08-10T08:00:45Z

@alexcrichton I've just pushed an updated version of the PR which also checks those cases.

alexcrichton · 2017-08-10T16:29:08Z

@bors: r+

👍

bors · 2017-08-10T16:29:08Z

📌 Commit 44dbbeb has been approved by alexcrichton

bors · 2017-08-11T12:09:47Z

⌛ Testing commit 44dbbeb3da7305b94241901f539034cbfd8150cb with merge 86293a2c4fe7cd45130286b270638779e2ae824f...

bors · 2017-08-11T13:04:55Z

💔 Test failed - status-travis

est31 · 2017-08-11T13:12:34Z

error: /checkout/src/test/compile-fail/thread-local-in-ctfe.rs:16: unexpected "warning": '16:17: 16:18: constant evaluation error: non-constant path in constant expression. This will become a HARD ERROR in the future'

Looks legit

eddyb · 2017-08-11T13:20:53Z

Ah, fallout from #43522, I'll rebase and try to fix it.

eddyb · 2017-08-11T14:36:14Z

@bors r=alexcrichton

bors · 2017-08-11T14:36:15Z

📌 Commit eba7592 has been approved by alexcrichton

bors · 2017-08-11T18:37:35Z

⌛ Testing commit eba7592 with merge e957a91...

Check #[thread_local] statics correctly in the compiler. Fixes #43733 by introducing `#[allow_internal_unsafe]` analogous to `#[allow_internal_unstable]`, for letting a macro expand to `unsafe` blocks and functions even in `#![forbid(unsafe_code)]` crates. Fixes #17954 by not letting references to `#[thread_local]` statics escape the function they're taken in - we can't just use a magical lifetime because Rust has *lifetime parametrism*, so if we added the often-proposed `'thread` lifetime, we'd have no way to check it in generic code. To avoid potential edge cases in the compiler, the lifetime is actually that of a temporary at the same position, i.e. `&TLS_STATIC` has the same lifetime `&non_const_fn()` would. Referring to `#[thread_local]` `static`s at compile-time is banned now (as per PR discussion). Additionally, to remove `unsafe impl Sync` from `std::thread::local::fast::Key`, `#[thread_local]` statics are now not required to implement `Sync`, as they are not shared between threads.

bors · 2017-08-11T19:43:41Z

💔 Test failed - status-appveyor

est31 · 2017-08-11T19:46:00Z

compile-fail/issue-17954.rs:22: unexpected "error": '22:22: 22:27: dummy does not live long enough [E0597]'
[...]
compile-fail/issue-17954.rs:34: expected error not found: borrowed value does not live long enough

looks legit

eddyb · 2017-08-11T19:47:46Z

So the borrow-checker is more clever than I gave it credit for. Maybe I don't need the #[cfg], hmm.

…ros.

eddyb · 2017-08-12T06:53:25Z

@bors r=alexcrichton

bors · 2017-08-12T06:53:26Z

📌 Commit 61ca14f has been approved by alexcrichton

bors · 2017-08-12T08:16:58Z

⌛ Testing commit 61ca14f10f55dac0fbee1525d570fd9e01780431 with merge ffbde4279123b29b6fc73c1e8023751bdbde44ad...

bors · 2017-08-12T09:34:10Z

💔 Test failed - status-travis

kennytm · 2017-08-12T09:52:32Z

arm-android failed. Still complaining about Sync?

[00:59:58] ---- [run-pass] run-pass/thread-local-extern-static.rs stdout ----
[00:59:58] 	
[00:59:58] error: auxiliary build of "/checkout/src/test/run-pass/auxiliary/thread-local-extern-static.rs" failed to compile: 
[00:59:58] status: exit code: 101
[00:59:58] command: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc /checkout/src/test/run-pass/auxiliary/thread-local-extern-static.rs -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass --target=arm-linux-androideabi --crate-type=dylib -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/thread-local-extern-static.stage2-arm-linux-androideabi.run-pass.libaux -C prefer-dynamic --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/thread-local-extern-static.stage2-arm-linux-androideabi.run-pass.libaux -Clinker=/android/ndk/arm-9/bin/arm-linux-androideabi-gcc -Crpath -O -Lnative=/checkout/obj/build/arm-linux-androideabi/native/rust-test-helpers
[00:59:58] stdout:
[00:59:58] ------------------------------------------
[00:59:58] 
[00:59:58] ------------------------------------------
[00:59:58] stderr:
[00:59:58] ------------------------------------------
[00:59:58] error[E0277]: the trait bound `std::cell::Cell<u32>: std::marker::Sync` is not satisfied
[00:59:58]   --> /checkout/src/test/run-pass/auxiliary/thread-local-extern-static.rs:18:1
[00:59:58]    |
[00:59:58] 18 | pub static FOO: Cell<u32> = Cell::new(3);
[00:59:58]    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `std::cell::Cell<u32>` cannot be shared between threads safely
[00:59:58]    |
[00:59:58]    = help: the trait `std::marker::Sync` is not implemented for `std::cell::Cell<u32>`
[00:59:58]    = note: shared static variables must have a type that implements `Sync`
[00:59:58] 
[00:59:58] error: aborting due to previous error
[00:59:58] 
[00:59:58] 
[00:59:58] ------------------------------------------
[00:59:58] 
[00:59:58] thread '[run-pass] run-pass/thread-local-extern-static.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2511:8
[00:59:58] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:59:58] 
[00:59:58] 
[00:59:58] failures:
[00:59:58]     [run-pass] run-pass/thread-local-extern-static.rs
[00:59:58] 
[00:59:58] test result: FAILED. 2707 passed; 1 failed; 20 ignored; 0 measured; 0 filtered out

eddyb · 2017-08-12T09:53:59Z

@kennytm No, I guess no architecture with OS TLS got to the tests before.

eddyb · 2017-08-12T09:58:21Z

@bors r=alexcrichton

bors · 2017-08-12T09:58:21Z

📌 Commit 92892d3 has been approved by alexcrichton

bors · 2017-08-12T11:57:15Z

⌛ Testing commit 92892d3 with merge 4564933...

Check #[thread_local] statics correctly in the compiler. Fixes #43733 by introducing `#[allow_internal_unsafe]` analogous to `#[allow_internal_unstable]`, for letting a macro expand to `unsafe` blocks and functions even in `#![forbid(unsafe_code)]` crates. Fixes #17954 by not letting references to `#[thread_local]` statics escape the function they're taken in - we can't just use a magical lifetime because Rust has *lifetime parametrism*, so if we added the often-proposed `'thread` lifetime, we'd have no way to check it in generic code. To avoid potential edge cases in the compiler, the lifetime is actually that of a temporary at the same position, i.e. `&TLS_STATIC` has the same lifetime `&non_const_fn()` would. Referring to `#[thread_local]` `static`s at compile-time is banned now (as per PR discussion). Additionally, to remove `unsafe impl Sync` from `std::thread::local::fast::Key`, `#[thread_local]` statics are now not required to implement `Sync`, as they are not shared between threads.

bors · 2017-08-12T14:24:35Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 4564933 to master...

- New allocator API - Remove the "allocator" feature, which should be unnecessary due to how the new allocator API works - NonZero no longer implements Deref (rust-lang/rust#41064) - NonZero::new() returns an Option; use NonZero::new_unchecked() - Thread locals are no longer 'static (rust-lang/rust#43746) - Changes to feature flags - Use unsafe to access extern static (rust-lang/rust#36247)

rust-highfive assigned nikomatsakis Aug 8, 2017

eddyb mentioned this pull request Aug 8, 2017

std::thread::local internals allow race conditions in safe but unstable code. #43733

Closed

joshlf mentioned this pull request Aug 8, 2017

std::thread::LocalKeyState: Add state Initializing #43550

Closed

alexcrichton reviewed Aug 9, 2017

View reviewed changes

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 10, 2017

eddyb force-pushed the sound-thread-local branch from 2c2be49 to 44dbbeb Compare August 10, 2017 07:59

eddyb force-pushed the sound-thread-local branch from 44dbbeb to eba7592 Compare August 11, 2017 13:23

syntax: #[allow_internal_unsafe] bypasses the unsafe_code lint in mac…

8a4facc

…ros.

eddyb force-pushed the sound-thread-local branch from eba7592 to 61ca14f Compare August 12, 2017 06:23

Check #[thread_local] statics correctly in the compiler.

92892d3

eddyb force-pushed the sound-thread-local branch from 61ca14f to 92892d3 Compare August 12, 2017 09:58

bors merged commit 92892d3 into rust-lang:master Aug 12, 2017

eddyb deleted the sound-thread-local branch August 12, 2017 14:34

alexcrichton mentioned this pull request Aug 27, 2017

Remove the Sync requirement for #[thread_local] statics. #35035

Closed

cramertj mentioned this pull request Jan 17, 2018

Tracking issue for thread_local stabilization #29594

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check #[thread_local] statics correctly in the compiler. #43746

Check #[thread_local] statics correctly in the compiler. #43746

eddyb commented Aug 8, 2017 •

edited

rust-highfive commented Aug 8, 2017

eddyb commented Aug 8, 2017

est31 commented Aug 8, 2017

eddyb commented Aug 8, 2017

alexcrichton Aug 9, 2017

eddyb Aug 9, 2017

alexcrichton Aug 10, 2017

alexcrichton commented Aug 9, 2017

eddyb commented Aug 9, 2017

alexcrichton commented Aug 9, 2017

eddyb commented Aug 10, 2017 •

edited

eddyb commented Aug 10, 2017

alexcrichton commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 11, 2017

bors commented Aug 11, 2017

est31 commented Aug 11, 2017

eddyb commented Aug 11, 2017

eddyb commented Aug 11, 2017

bors commented Aug 11, 2017

bors commented Aug 11, 2017

bors commented Aug 11, 2017

est31 commented Aug 11, 2017

eddyb commented Aug 11, 2017

eddyb commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017

kennytm commented Aug 12, 2017 •

edited

eddyb commented Aug 12, 2017

eddyb commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017

Check #[thread_local] statics correctly in the compiler. #43746

Check #[thread_local] statics correctly in the compiler. #43746

Conversation

eddyb commented Aug 8, 2017 • edited

rust-highfive commented Aug 8, 2017

eddyb commented Aug 8, 2017

est31 commented Aug 8, 2017

eddyb commented Aug 8, 2017

alexcrichton Aug 9, 2017

Choose a reason for hiding this comment

eddyb Aug 9, 2017

Choose a reason for hiding this comment

alexcrichton Aug 10, 2017

Choose a reason for hiding this comment

alexcrichton commented Aug 9, 2017

eddyb commented Aug 9, 2017

alexcrichton commented Aug 9, 2017

eddyb commented Aug 10, 2017 • edited

eddyb commented Aug 10, 2017

alexcrichton commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 11, 2017

bors commented Aug 11, 2017

est31 commented Aug 11, 2017

eddyb commented Aug 11, 2017

eddyb commented Aug 11, 2017

bors commented Aug 11, 2017

bors commented Aug 11, 2017

bors commented Aug 11, 2017

est31 commented Aug 11, 2017

eddyb commented Aug 11, 2017

eddyb commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017

kennytm commented Aug 12, 2017 • edited

eddyb commented Aug 12, 2017

eddyb commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017

bors commented Aug 12, 2017

eddyb commented Aug 8, 2017 •

edited

eddyb commented Aug 10, 2017 •

edited

kennytm commented Aug 12, 2017 •

edited