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.

Sign up for GitHub

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

Jump to bottom

Make the LocalKey facade of thread_local! inlineable cross-crate. #43931

Merged
merged 1 commit into from
Sep 7, 2017
Merged

Make the LocalKey facade of thread_local! inlineable cross-crate. #43931

merged 1 commit into from
Sep 7, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Aug 17, 2017 •
edited
Loading

Fixes (almost*) #25088 by changing the LocalKey static thread_local! generates to a const.
This can be done because a LocalKey value holds no actual TLS data, only function pointers to get at said data, and it could even be made Copy without any negative consequences.
The recent stabilization of rvalue promotion to 'static allows doing this without changing the API.
r? @alexcrichton

*almost because we can't yet inline __getit because it breaks on MSVC, see #43931 (comment)

@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2017

This doesn't work on stage0 apparently.

@eddyb
Copy link
Member Author

eddyb commented Aug 17, 2017 •
edited
Loading

@oli-obk Fixing.

@eddyb eddyb force-pushed the const-local-key branch 2 times, most recently from 48c6988 to 2040db1 Compare August 17, 2017 11:53
@alexcrichton
Copy link
Member

Awesome, looks great!

One thing I noticed locally was that the __init function isn't inlined. Do you think it'd be worth it to do that? It's definitely overkill for expensive cases but maybe worth it for enough cases to go ahead and inline it as well?

Regardless of that decision r=me, just wanted to see what you thought

@eddyb
Copy link
Member Author

eddyb commented Aug 17, 2017

Ah I didn't think much about it but in the cheap cases it'd definitely help.

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Aug 17, 2017

📌 Commit ee1a970 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 19, 2017

⌛ Testing commit ee1a970513d32d8a748e446d10b093f6d79225ea with merge 414b93e932ec81413dddc3d011c8f0a3f5a6145c...

@bors
Copy link
Contributor

bors commented Aug 19, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Aug 19, 2017 •
edited
Loading

@bors retry

Failed to build rustdoc because rustc failed to execute with error 3221225477, which is 0xc0000005 i.e. STATUS_ACCESS_VIOLATION. Assuming spurious for now.

Building rustdoc for stage2 (x86_64-pc-windows-msvc)
error: failed to run `rustc` to learn about target-specific information
Caused by:
  process didn't exit successfully: `C:\projects\rust\build\bootstrap/debug/rustc - --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --target x86_64-pc-windows-msvc` (exit code: 3221225477)
command did not execute successfully: "C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\stage0/bin\\cargo.exe" "build" "-j" "2" "--target" "x86_64-pc-windows-msvc" "--release" "--locked" "--color" "always" "--manifest-path" "C:\\projects\\rust\\src/tools\\rustdoc\\Cargo.toml"

@bors
Copy link
Contributor

bors commented Aug 19, 2017

⌛ Testing commit ee1a970513d32d8a748e446d10b093f6d79225ea with merge b60970f3ff1d0716c7ff153db0412ce677ea0e9b...

@bors
Copy link
Contributor

bors commented Aug 19, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Aug 19, 2017

Same issue. Legit. @eddyb

@eddyb
Copy link
Member Author

eddyb commented Aug 19, 2017 •
edited
Loading

Same failure. I have no few ideas why this would happen, probably some pre-exiting bug I triggered.

@kennytm
Copy link
Member

kennytm commented Aug 19, 2017

Perhaps a bug in the rvalue promotion to 'static on x86_64-pc-windows-msvc?

@eddyb
Copy link
Member Author

eddyb commented Aug 19, 2017 •
edited
Loading

@kennytm That const is just a facade, I expect OS TLS is broken cross-crate, or at least only in some subtle situations. Plus, rvalue promotion has been around since 1.0, in both old trans and the MIR one, and while there has been a known bug in the old trans implementation, it's pretty solid.

@eddyb
Copy link
Member Author

eddyb commented Aug 19, 2017 •
edited
Loading

(please do not r+) Going to try some builds without fast TLS, to see if the problem is even reproducible on other platforms - as windows would be a massive pain to debug (and the bug triggers in rustc).

EDIT: I'm dumb, MSVC doesn't use OS TLS, so it's the fast TLS that's broken...

@eddyb
Copy link
Member Author

eddyb commented Aug 19, 2017 •
edited
Loading

@alexcrichton Weren't there some oddities ages ago, back in the old runtime, where some of the thread-local accesses had to be in non-#[inline] functions to avoid them breaking cross-crate?

@alexcrichton
Copy link
Member

@eddyb yes, but probably not relevant today.

The MSVC support for #[thread_local] was only turned on very recently, and it segfaulted on all other Windows platforms, so it may be buggy and "accidentally" working, I'm not sure.

@eddyb
Copy link
Member Author

eddyb commented Aug 20, 2017

@alexcrichton Do you think not having #[inline] on __getit on windows is reasonable?

@alexcrichton
Copy link
Member

Personally, not really, but you seem eager to land this so I won't stop you if you'd like to do that. If you do, though, please leave a comment with as much information that you know.

@arielb1 arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 22, 2017
@carols10cents
Copy link
Member

friendly ping @eddyb to keep this on your radar!

@alexcrichton
Copy link
Member

@bors: r-

This got back in the queue...

@eddyb eddyb mentioned this pull request Sep 7, 2017
Open
This was referenced Sep 12, 2017
Closed
Merged
SimonSapin added a commit to servo/servo that referenced this pull request Sep 12, 2017
@SimonSapin
Un-inline some methods initializing thread_local! …
3b26386 
… in order to limit stack frame sizes after extra inlining from
rust-lang/rust#43931

See #18420 (comment)
SimonSapin added a commit to servo/servo that referenced this pull request Sep 12, 2017
@SimonSapin
Un-inline some methods initializing thread_local! …
0d41680 
… in order to limit stack frame sizes after extra inlining from
rust-lang/rust#43931

See #18420 (comment)
@nox nox mentioned this pull request Apr 2, 2018
Closed
@eddyb eddyb deleted the const-local-key branch April 4, 2018 09:21
@kennytm kennytm mentioned this pull request Apr 27, 2018
Closed
@eddyb eddyb mentioned this pull request Dec 3, 2018
Closed
@mati865 mati865 mentioned this pull request Apr 5, 2019
Closed
@alexcrichton alexcrichton mentioned this pull request May 3, 2021
Merged
@alexcrichton alexcrichton mentioned this pull request May 4, 2021
Closed
alexcrichton added a commit to alexcrichton/rust that referenced this pull request May 18, 2021
@alexcrichton
std: Attempt again to inline thread-local-init across crates
641d3b0 
Issue rust-lang#25088 has been part of `thread_local!` for quite some time now.
Historical attempts have been made to add `#[inline]` to `__getit`
in rust-lang#43931, rust-lang#50252, and rust-lang#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 rust-lang#25088
bors added a commit to rust-lang-ci/rust that referenced this pull request May 19, 2021
@bors
Auto merge of rust-lang#84876 - alexcrichton:inline-thread-locals-cro…
3bcaeb0 
…ss-crate, r=Mark-Simulacrum

std: Attempt again to inline thread-local-init across crates

Issue rust-lang#25088 has been part of `thread_local!` for quite some time now.
Historical attempts have been made to add `#[inline]` to `__getit`
in rust-lang#43931, rust-lang#50252, and rust-lang#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 rust-lang#25088
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eddyb @oli-obk @alexcrichton @bors @kennytm @carols10cents @arielb1