Recognize #[thread_local] attr on extern static. Fixes #30795. #30856
Conversation
This will correctly add the thread_local attribute to the external static
variable "errno":
extern {
#[thread_local]
static errno: c_int;
}
Before this commit, the thread_local attribute is ignored.
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
|
Thanks! Could you add a run-pass test for this as well? Something like: // auxiliary/foo.rs
#[thread_local]
pub static FOO: u32 = 3;
// run-pass/foo.rs
extern crate foo;
extern {
#[thread_local]
static FOO: u32;
}
fn main() {
assert_eq!(FOO, 3);
} |
|
It looks like this is adding a new language feature (thread-local externs). Seems like it should get some more eyes. cc @rust-lang/lang Isn't it true that not all platforms support |
|
@brson: AFAIK it's only required (at the moment) to support DragonFly, which defines |
|
To be clear, Platform support is a good point, however, and the test should probably only run on Linux for now to be safe. Either that or it could use the new |
I'm a bit confused. What is the expected behavior on platforms that don't support |
|
For the record, I agree that this feels like a sensible extension of |
|
That's actually a good question about what happens if the target doesn't support From what I understand, support for Note that we have pretty exhaustive tests of |
| @@ -0,0 +1,2 @@ | |||
| #[thread_local] | |||
There was a problem hiding this comment.
These tests will need license header blocks, and I would expect at least that each test would require #![feature(thread_local)], although I suspect the one below may not require it (could you make sure?)
|
@bors r+ |
|
📌 Commit ca76cf1 has been approved by |
|
⌛ Testing commit ca76cf1 with merge b4f697c... |
|
💔 Test failed - auto-mac-64-nopt-t |
|
Error is legit. |
|
@alexcrichton @brson I think those last two commits need to go it to fix running the test case. At least, without the |
|
⌛ Testing commit c2c58a3 with merge 9646537... |
|
💔 Test failed - auto-win-gnu-64-nopt-t |
|
Oh, right! This will probably want to use |
|
@alexcrichton : I am not sure if I understand |
|
Ah yeah to clarify Basically this test won't work on platforms that don't support |
|
@alexcrichton : Understood! You are talking about the |
|
⌛ Testing commit 4ef60a2 with merge cbe4e88... |
|
💔 Test failed - auto-win-msvc-64-opt |
|
@alexcrichton : I wonder why this fails on |
|
That is indeed weird... I guess |
Windows is not #[cfg(target_thread_local)] and as such should link
to the external symbol. But it fails with:
thread '<main>' panicked at 'assertion failed: `(left == right)` (left: `272246271`, right: `3`)', C:/bot/slave/auto-win-msvc-64-opt/build/src/test/run-pass/thread-local-extern-static.rs:24
|
@alexcrichton : now all tests pass :) |
|
⌛ Testing commit 78d9544 with merge 490987e... |
|
💔 Test failed - auto-linux-64-nopt-t |
|
@alexcrichton : This failure doesn't seem to be related to my commits ( |
|
@bors: retry indeed! On Thu, Feb 25, 2016 at 2:06 AM, Michael Neumann notifications@github.com
|
This will correctly add the thread_local attribute to the external static variable ```errno```:
```rust
extern {
#[thread_local]
static errno: c_int;
}
```
Before this commit, the thread_local attribute is ignored. Fixes #30795.
Thanks @alexcrichton for pointing out the solution.
This will correctly add the thread_local attribute to the external static variable
errno:Before this commit, the thread_local attribute is ignored. Fixes #30795.
Thanks @alexcrichton for pointing out the solution.