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

Add #[inline] back on __getit (fixes #25088) #50252

Closed
wants to merge 1 commit into from
Closed

Add #[inline] back on __getit (fixes #25088) #50252

wants to merge 1 commit into from

Conversation

nox
Copy link
Contributor

@nox nox commented Apr 26, 2018

No description provided.

@rust-highfive
Copy link
Collaborator

r? @withoutboats

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2018
@eddyb
Copy link
Member

eddyb commented Apr 26, 2018

@bors r+ (let's see if it passes on windows)

@bors
Copy link
Contributor

bors commented Apr 26, 2018

📌 Commit 89f3970 has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2018
@kennytm kennytm mentioned this pull request Apr 27, 2018
Merged
@kennytm
Copy link
Member

kennytm commented Apr 27, 2018 •
edited
Loading

The rollup #50275 failed on x86_64-msvc with an access violation when trying to learn about target-specific information using the stage1 rustc:

Building stage1 std artifacts (x86_64-pc-windows-msvc -> 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 --target x86_64-pc-windows-msvc --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro` (exit code: 3221225477)

Given the previous test result, I think this PR is the cause.

@eddyb
Copy link
Member

eddyb commented Apr 27, 2018

@kennytm Sorry, this should have been excluded from rollup, because we were checking for that failure - is there any way for someone to indicate that a PR is "standalone" yet?

@nox
Copy link
Contributor Author

nox commented Apr 27, 2018

is there any way for someone to indicate that a PR is "standalone" yet?

"(Do not merge)" as a prefix of the PR title would probably signal that this shouldn't go in a rollup.

@eddyb
Copy link
Member

eddyb commented Apr 27, 2018

@nox [WIP] prefix doesn't even let you r+ it. But we want merging, just not rollup.

@alexcrichton
Copy link
Member

If this is claiming to fix #25088 can a codegen test be added to assert such functionality? This has historically been very finnicky and easy to regress.

@bors
Copy link
Contributor

bors commented Apr 29, 2018

⌛ Testing commit 89f3970 with merge ed3ce2ec4df3acb3efcec3d97a0a9876f6aadcf2...

@bors
Copy link
Contributor

bors commented Apr 29, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 29, 2018
@kennytm
Copy link
Member

kennytm commented Apr 29, 2018

Same error as #50252 (comment).

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2018
@shepmaster
Copy link
Member

shepmaster commented May 6, 2018 •
edited
Loading

Ping from triage, @nox ! Will you have time to address the review comments?

I don't believe that fixes #25088 in the PR title works, but I've never tried, honestly. I always put it in the body... Huh TIL.

@pietroalbini
Copy link
Member

Thank you for this PR @nox! Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again!

@pietroalbini pietroalbini added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2018
@rasky rasky mentioned this pull request Jan 4, 2019
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

@withoutboats withoutboats

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nox @rust-highfive @eddyb @bors @kennytm @alexcrichton @shepmaster @pietroalbini @withoutboats