Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ELF TLS detection on OSX #30417

Merged
merged 3 commits into from
Dec 22, 2015

Conversation

alexcrichton
Copy link
Member

Currently a compiler can be built with the --disable-elf-tls option for compatibility with OSX 10.6 which doesn't have ELF TLS. This is unfortunate, however, as a whole new compiler must be generated which can take some time. These commits add a new (feature gated) cfg(target_thread_local) annotation set by the compiler which indicates whether #[thread_local] is available for use. The compiler now interprets MACOSX_DEPLOYMENT_TARGET (a standard environment variable) to set this flag on OSX. With this we may want to start compiling our OSX nightlies with MACOSX_DEPLOYMENT_TARGET set to 10.6 which would allow the compiler out-of-the-box to generate 10.6-compatible binaries.

For now the compiler still by default targets OSX 10.7 by allowing ELF TLS by default (e.g. if MACOSX_DEPLOYMENT_TARGET isn't set).

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@alexcrichton
Copy link
Member Author

cc @brson, @rillian

This is primarily motivated by Gecko which requires 10.6 compatibility but is currently having to compile their own OSX toolchain (with --disable-elf-tls). The primary benefit for this to them is if we ourselves start compiling our nightlies to run on 10.6, which means the standard library won't use ELF TLS. External consumers of the standard library, however, will continue to use ELF TLS by default. This seems like an ok balance between compatibility and speed to me, but I'm curious what others think!

@rillian
Copy link
Contributor

rillian commented Dec 16, 2015

Thanks, @alexcrichton! For gecko we actually build on 10.7, so the compiler itself can use ELF TLS, we just need it to be able to produce binaries which work on 10.6 (and so libstd must not use ELF TLS) when MACOSX_DEPLOYMENT_TARGET is < 10.7.

If I understand the patch properly, that case is still covered by building nightlies with MACOSX_DEPLOYMENT_TARGET=10.6, but I wanted to be sure the requirement is clear.

@alexcrichton
Copy link
Member Author

Ah yeah thanks for clarifying, right now the build which produces standard libraries for OSX also produces compilers, so for us they're one and the same. I could imagine, however, that we start providing alternate compilation profiles of the standard library and this could certainly be one of them!

@bors
Copy link
Contributor

bors commented Dec 18, 2015

☔ The latest upstream changes (presumably #30457) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 19, 2015

☔ The latest upstream changes (presumably #30184) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member Author

Ah and I finally got around to doing some benchmarking, and of this simple benchmark:

#![feature(test)]
#![feature(static_mutex, const_fn)]

extern crate test;

use std::sync::StaticMutex;

pub fn doit() {
    static L: StaticMutex = StaticMutex::new();
    drop(L.lock());
}

#[bench]
fn bench_a(bh: &mut test::Bencher) {
    bh.iter(|| { doit() });
}

The timings look like:

$ ./use-elf-tls --bench

running 1 test
test bench_a ... bench:          22 ns/iter (+/- 13)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured

$ ./use-os-tls --bench

running 1 test
test bench_a ... bench:          24 ns/iter (+/- 9)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured

So we may actually be able to turn on 10.6 support by default in nightlies with little-to-no performance impact

@alexcrichton
Copy link
Member Author

Ah and a similar benchmark for just raw TLS access times:

#![feature(test)]
#![feature(const_fn)]

extern crate test;

use std::cell::Cell;

#[bench]
fn bench_a(bh: &mut test::Bencher) {
    thread_local!(static C: Cell<usize> = Cell::new(0));
    bh.iter(|| {
        C.with(|c| {
            let r = c.get();
            c.set(r + 1);
            r
        })
    })
}

Shows a difference of 4ns/iter vs 5ns/iter for ELF vs OS tls (respectively), so it may actually be that libstd is barely affected at all in terms of perf.

@aturon
Copy link
Member

aturon commented Dec 21, 2015

I'm 👍 on this change, and likely for changing the default target in our release artifacts as well.

@brson
Copy link
Contributor

brson commented Dec 22, 2015

@bors r+

@bors
Copy link
Contributor

bors commented Dec 22, 2015

📌 Commit c211a78 has been approved by brson

@bors
Copy link
Contributor

bors commented Dec 22, 2015

⌛ Testing commit c211a78 with merge b2b8f4e...

bors added a commit that referenced this pull request Dec 22, 2015
Currently a compiler can be built with the `--disable-elf-tls` option for compatibility with OSX 10.6 which doesn't have ELF TLS. This is unfortunate, however, as a whole new compiler must be generated which can take some time. These commits add a new (feature gated) `cfg(target_thread_local)` annotation set by the compiler which indicates whether `#[thread_local]` is available for use. The compiler now interprets `MACOSX_DEPLOYMENT_TARGET` (a standard environment variable) to set this flag on OSX. With this we may want to start compiling our OSX nightlies with `MACOSX_DEPLOYMENT_TARGET` set to 10.6 which would allow the compiler out-of-the-box to generate 10.6-compatible binaries.

For now the compiler still by default targets OSX 10.7 by allowing ELF TLS by default (e.g. if `MACOSX_DEPLOYMENT_TARGET` isn't set).
@bors
Copy link
Contributor

bors commented Dec 22, 2015

💔 Test failed - auto-linux-64-x-android-t

Currently the standard library has some pretty complicated logic to detect
whether #[thread_local] should be used or whether it's supported. This is also
unfortunately not quite true for OSX where not all versions support
the #[thread_local] attribute (only 10.7+ does). Compiling code for OSX 10.6 is
typically requested via the MACOSX_DEPLOYMENT_TARGET environment variable (e.g.
the linker recognizes this), but the standard library unfortunately does not
respect this.

This commit updates the compiler to add a `target_thread_local` cfg annotation
if the platform being targeted supports the `#[thread_local]` attribute. This is
feature gated for now, and it is only true on non-aarch64 Linux and 10.7+ OSX
(e.g. what the module already does today). Logic has also been added to parse
the deployment target environment variable.
This change modifies the feature gating of special `#[cfg]` attributes to not
require a `#![feature]` directive in the crate-of-use if the source of the macro
was declared with `#[allow_internal_unstable]`. This enables the standard
library's macro for `thread_local!` to make use of the
`#[cfg(target_thread_local)]` attribute despite it being feature gated (e.g.
it's a hidden implementation detail).
This transitions the standard library's `thread_local!` macro to use the
freshly-added and gated `#[cfg(target_thread_local)]` attribute. This greatly
simplifies the `#[cfg]` logic in play here, but requires that the standard
library expose both the OS and ELF TLS implementation modules as unstable
implementation details.

The implementation details were shuffled around a bit but end up generally
compiling to the same thing.

Closes rust-lang#26581 (this supersedes the need for the option)
Closes rust-lang#27057 (this also starts ignoring the option)
@alexcrichton
Copy link
Member Author

@bors: r=brson 617a7af

@bors
Copy link
Contributor

bors commented Dec 22, 2015

🙀 617a7af is not a valid commit SHA. Please try again with cd74364.

@alexcrichton
Copy link
Member Author

@bors: r+ cd74364

@bors
Copy link
Contributor

bors commented Dec 22, 2015

⌛ Testing commit cd74364 with merge 42c3ef8...

bors added a commit that referenced this pull request Dec 22, 2015
…hton

Currently a compiler can be built with the `--disable-elf-tls` option for compatibility with OSX 10.6 which doesn't have ELF TLS. This is unfortunate, however, as a whole new compiler must be generated which can take some time. These commits add a new (feature gated) `cfg(target_thread_local)` annotation set by the compiler which indicates whether `#[thread_local]` is available for use. The compiler now interprets `MACOSX_DEPLOYMENT_TARGET` (a standard environment variable) to set this flag on OSX. With this we may want to start compiling our OSX nightlies with `MACOSX_DEPLOYMENT_TARGET` set to 10.6 which would allow the compiler out-of-the-box to generate 10.6-compatible binaries.

For now the compiler still by default targets OSX 10.7 by allowing ELF TLS by default (e.g. if `MACOSX_DEPLOYMENT_TARGET` isn't set).
@bors bors merged commit cd74364 into rust-lang:master Dec 22, 2015
@alexcrichton alexcrichton deleted the better-detect-elf-tls branch January 21, 2016 01:30
rillian added a commit to rillian/rust that referenced this pull request May 2, 2017
Support for disabling ELF-style thread local storage in
the standard library at configure time was removed in
pulls rust-lang#30417 and rust-lang#30678, in favour of a member in
the TargetOptions database. The new method respects
MACOSX_DEPLOYMENT_TARGET on macOS, addressing the
original use case for this configure option.

However, those commits left the configure option itself
in place. It's no longer referenced anywhere and can
be removed.
bors added a commit that referenced this pull request May 4, 2017
Remove obsolete --disable-elf-tls configure switch.

Support for disabling ELF-style thread local storage in
the standard library at configure time was removed in
pulls #30417 and #30678, in favour of a member in
the TargetOptions database. The new mentod respects
MACOSX_DEPLOYMENT_TARGET on macOS, addressing the
original use case for this configure optionl

However, those commits left the configure option itself
in place. It's no longer referenced anywhere and can
be removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants