-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Add tls_model for cygwin and enable has_thread_local #141719
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
Conversation
These commits modify compiler targets. |
r? mati865 This change should be fine these days but in the past it didn't work well with windows-gnu. Did you run testsuite (ui tests are important here)? |
I'll run it later. |
Might it be worth doing some try jobs for the affected targets? |
Looking the changes here, cygwin is a Tier 3 target, do we even run that in CI? AFAICT we don't 🤔 |
Oh right. Shame. |
The current PR affects nothing. I'm just asking the possibility to make cygwin target have thread_local attr. A small question: why ui tests? |
Some ui tests do exercise thread locals, probably a quick check in case something immediately blows up? |
I have tried tests in Is it better to make it |
If you are aware of LLVM API that would cover it all, please let us know 😉
I've found them to fail spectacularly (or typically if you are used to C ecosystem) when tinkering with TLS on windows-gnu.
Not directly, but yeah, they use code paths from libstd that involve TLS.
Thanks, I also did some tests, and indeed, it works fine:
(for others: failures are not related to this PR, this target is just not fully working yet) Also, tested analogous change with
Yes, please amend or squash it into a single commit. |
6523b9a
to
9281958
Compare
@mati865 Thanks for your explanations! I've set it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
@bors r+ rollup |
This comment was marked as outdated.
This comment was marked as outdated.
^ network issues on GHA, don't worry about it. |
Rollup of 5 pull requests Successful merges: - #141703 (Structurally normalize types as needed in `projection_ty_core`) - #141719 (Add tls_model for cygwin and enable has_thread_local) - #141736 (resolve stage0 sysroot from rustc) - #141746 (Rework `#[doc(cfg(..))]` checks as distinct pass in rustdoc) - #141749 (Remove RUSTC_RETRY_LINKER_ON_SEGFAULT hack) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #141719 - Berrysoft:cygwin-tls-model, r=mati865 Add tls_model for cygwin and enable has_thread_local I've also tried to set `has_thread_local` to `true` and found it works actually. Why do we still implement our own `thread_local` instead of delegating all of them to LLVM? cc: `@jeremyd2019`
FYI broken TLS on Windows fails like this: #141794 (comment) |
This is still in the queue. @bors r- |
I got this error on the last nightly:
https://github.com/jeremyd2019/cygwin-rust-bootstrap/actions/runs/15360769471/job/43227887473 |
Oh, no. Why didn't it show up in the tests... |
rust/src/bootstrap/src/core/builder/cargo.rs Lines 1007 to 1009 in 337c11e
I think we should disable it for cygwin. |
…mati865 Fix TLS model on bootstrap for cygwin There aren't other targets that both use emutls and enable `has_thread_local`, so cygwin triggers this bug first. r? mati865 See: rust-lang#141719 (comment) `@jeremyd2019` Could you check if this PR fixes the issue? I just found my pre-built stage-0 rustc was too old to build the current rustc :(
…mati865 Fix TLS model on bootstrap for cygwin There aren't other targets that both use emutls and enable `has_thread_local`, so cygwin triggers this bug first. r? mati865 See: rust-lang#141719 (comment) ``@jeremyd2019`` Could you check if this PR fixes the issue? I just found my pre-built stage-0 rustc was too old to build the current rustc :(
Rollup merge of #141846 - Berrysoft:cygwin-bootstrap-tls, r=mati865 Fix TLS model on bootstrap for cygwin There aren't other targets that both use emutls and enable `has_thread_local`, so cygwin triggers this bug first. r? mati865 See: #141719 (comment) ``@jeremyd2019`` Could you check if this PR fixes the issue? I just found my pre-built stage-0 rustc was too old to build the current rustc :(
I've also tried to set
has_thread_local
totrue
and found it works actually. Why do we still implement our ownthread_local
instead of delegating all of them to LLVM?cc: @jeremyd2019