Skip to content

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

Merged
merged 1 commit into from
May 30, 2025

Conversation

Berrysoft
Copy link
Contributor

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

@rustbot
Copy link
Collaborator

rustbot commented May 29, 2025

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 29, 2025

These commits modify compiler targets.
(See the Target Tier Policy.)

@jieyouxu jieyouxu added the O-cygwin Target: *-pc-cygwin label May 29, 2025
@mati865
Copy link
Member

mati865 commented May 29, 2025

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)?

@rustbot rustbot assigned mati865 and unassigned BoxyUwU May 29, 2025
@Berrysoft
Copy link
Contributor Author

I'll run it later.

@ChrisDenton
Copy link
Member

Might it be worth doing some try jobs for the affected targets?

@jieyouxu
Copy link
Member

jieyouxu commented May 29, 2025

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 🤔

@ChrisDenton
Copy link
Member

Oh right. Shame.

@Berrysoft
Copy link
Contributor Author

Berrysoft commented May 29, 2025

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?

@jieyouxu
Copy link
Member

A small question: why ui tests?

Some ui tests do exercise thread locals, probably a quick check in case something immediately blows up?

@Berrysoft
Copy link
Contributor Author

I have tried tests in tests/ui/thread-local and they passes either with has_thread_local being true or false.

Is it better to make it true?

@mati865
Copy link
Member

mati865 commented May 29, 2025

Why do we still implement our own thread_local instead of delegating all of them to LLVM?

If you are aware of LLVM API that would cover it all, please let us know 😉

The current PR affects nothing. I'm just asking the possibility to make cygwin target have thread_local attr.

has_thread_local: false is already false by the default, so there is no change here.

A small question: why ui tests?

I've found them to fail spectacularly (or typically if you are used to C ecosystem) when tinkering with TLS on windows-gnu.
Also see #102243

Some ui tests do exercise thread locals, probably a quick check in case something immediately blows up?

Not directly, but yeah, they use code paths from libstd that involve TLS.

I have tried tests in tests/ui/thread-local and they passes either with has_thread_local being true or false.

Thanks, I also did some tests, and indeed, it works fine:

has_thread_local: false
test result: FAILED. 18727 passed; 62 failed; 268 ignored; 0 measured; 0 filtered out; finished in 519.12s

has_thread_local: true
test result: FAILED. 18727 passed; 62 failed; 268 ignored; 0 measured; 0 filtered out; finished in 493.64s

(for others: failures are not related to this PR, this target is just not fully working yet)

Also, tested analogous change with x86_64-pc-windows-gnu and it passes all the tests locally (at least with the latest GCC and Binutils). Which is a good indication that these options will be safe now.

Is it better to make it true?

Yes, please amend or squash it into a single commit.

@Berrysoft Berrysoft changed the title Add tls_model for cygwin target Add tls_model for cygwin and enable has_thread_local May 29, 2025
@Berrysoft
Copy link
Contributor Author

@mati865 Thanks for your explanations! I've set it to true.

Copy link
Member

@mati865 mati865 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@mati865
Copy link
Member

mati865 commented May 29, 2025

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 29, 2025

📌 Commit 9281958 has been approved by mati865

It is now in the queue for this repository.

@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 May 29, 2025
@rust-log-analyzer

This comment was marked as outdated.

@mati865
Copy link
Member

mati865 commented May 29, 2025

^ network issues on GHA, don't worry about it.

bors added a commit that referenced this pull request May 30, 2025
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
@bors bors merged commit c61a1e0 into rust-lang:master May 30, 2025
16 of 17 checks passed
@rustbot rustbot added this to the 1.89.0 milestone May 30, 2025
@Berrysoft Berrysoft deleted the cygwin-tls-model branch May 30, 2025 15:31
rust-timer added a commit that referenced this pull request May 30, 2025
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`
@mati865
Copy link
Member

mati865 commented May 30, 2025

FYI broken TLS on Windows fails like this: #141794 (comment)

@bjorn3
Copy link
Member

bjorn3 commented May 31, 2025

This is still in the queue.

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 31, 2025
@jeremyd2019
Copy link
Contributor

I got this error on the last nightly:

 error: linking with `D:/cygwin/home/runneradmin/bin/x86_64-pc-cygwin-gcc.exe` failed: exit code: 1
    |
    = note: "D:/cygwin/home/runneradmin/bin/x86_64-pc-cygwin-gcc.exe" "-Wl,D:\\cygwin\\tmp\\rustcQ9hF9o\\list.def" "-Wl,--disable-dynamicbase" "-Wl,--enable-auto-image-base" "-m64" "D:\\cygwin\\tmp\\rustcQ9hF9o\\symbols.o" "<1366 object files omitted>" "<sysroot>-rustc\\x86_64-pc-cygwin\\release\\deps\\rustc_driver-7abd146cd83e6b94.d4paeqbw9pwnl4qoky0i0cv1s.rcgu.rmeta" "-Wl,-Bstatic" "D:\\cygwin\\tmp\\rustcQ9hF9o/{librustc_codegen_llvm-533350d348060472.rlib,librustc_llvm-37fa5b9b755d34f8.rlib,libblake3-f4d335051fe5a7ec.rlib,libpsm-8b74d93d37888c5b.rlib}.rlib" "<sysroot>\\lib\\rustlib\\x86_64-pc-cygwin\\lib/{libcompiler_builtins-*}.rlib" "-Wl,-Bdynamic" "-lz" "-lLLVM-20" "-lstdc++" "-lpthread" "-lkernel32" "-lutil" "-lrt" "-lpthread" "-lm" "-ldl" "-lc" "-lcygwin" "-lgcc" "-lcygwin" "-luser32" "-lkernel32" "-lgcc_s" "-Wl,--nxcompat" "-L" "<sysroot>-rustc\\x86_64-pc-cygwin\\release\\build\\psm-4e409ee52966cf15\\out" "-L" "<sysroot>-rustc\\x86_64-pc-cygwin\\release\\build\\blake3-b6c16d70e52b484d\\out" "-L" "<sysroot>-rustc\\x86_64-pc-cygwin\\release\\build\\blake3-b6c16d70e52b484d\\out" "-L" "<sysroot>-rustc\\x86_64-pc-cygwin\\release\\build\\rustc_llvm-4fe3dd582faa86cd\\out" "-L" "D:/cygwin/lib" "-o" "<sysroot>-rustc\\x86_64-pc-cygwin\\release\\deps\\rustc_driver-7abd146cd83e6b94.dll" "-shared" "-Wl,--out-implib=<sysroot>-rustc\\x86_64-pc-cygwin\\release\\deps\\librustc_driver-7abd146cd83e6b94.dll.a" "-Wl,-O1" "-nodefaultlibs"
    = note: some arguments are omitted. use `--verbose` to show all linker arguments
    = note: /usr/lib/gcc/x86_64-pc-cygwin/12/../../../../x86_64-pc-cygwin/bin/ld: /home/runneradmin/rustc-nightly-src/build-Cygwin/x86_64-pc-windows-gnu/stage1-rustc/x86_64-pc-cygwin/release/deps/rustc_driver-7abd146cd83e6b94.annotate_snippets-e12afcabe723ef9f.annotate_snippets.b65e84f0d73c7580-cgu.00.rcgu.o.rcgu.o:annotate_snippets.b65e84f0d73c7580-cgu.00:(.text+0xacb8): undefined reference to `_tls_index'
            /usr/lib/gcc/x86_64-pc-cygwin/12/../../../../x86_64-pc-cygwin/bin/ld: /home/runneradmin/rustc-nightly-src/build-Cygwin/x86_64-pc-windows-gnu/stage1-rustc/x86_64-pc-cygwin/release/deps/rustc_driver-7abd146cd83e6b94.cc-590791bad2240aa9.cc.d99e1fef9923402-cgu.08.rcgu.o.rcgu.o:cc.d99e1fef9923402-cgu.08:(.text+0x8e3): undefined reference to `_tls_index'
            /usr/lib/gcc/x86_64-pc-cygwin/12/../../../../x86_64-pc-cygwin/bin/ld: /home/runneradmin/rustc-nightly-src/build-Cygwin/x86_64-pc-windows-gnu/stage1-rustc/x86_64-pc-cygwin/release/deps/rustc_driver-7abd146cd83e6b94.cc-590791bad2240aa9.cc.d99e1fef9923402-cgu.12.rcgu.o.rcgu.o:cc.d99e1fef9923402-cgu.12:(.text+0x1046): undefined reference to `_tls_index'
            /usr/lib/gcc/x86_64-pc-cygwin/12/../../../../x86_64-pc-cygwin/bin/ld: /home/runneradmin/rustc-nightly-src/build-Cygwin/x86_64-pc-windows-gnu/stage1-rustc/x86_64-pc-cygwin/release/deps/rustc_driver-7abd146cd83e6b94.crossbeam_utils-7579fc9ec7da6027.crossbeam_utils.e8ac82be4c62a686-cgu.4.rcgu.o.rcgu.o:crossbeam_utils.e8ac82be4c62a686-cgu.4:(.text+0x1b2): undefined reference to `_tls_index'
            /usr/lib/gcc/x86_64-pc-cygwin/12/../../../../x86_64-pc-cygwin/bin/ld: /home/runneradmin/rustc-nightly-src/build-Cygwin/x86_64-pc-windows-gnu/stage1-rustc/x86_64-pc-cygwin/release/deps/rustc_driver-7abd146cd83e6b94.crossbeam_utils-7579fc9ec7da6027.crossbeam_utils.e8ac82be4c62a686-cgu.4.rcgu.o.rcgu.o:crossbeam_utils.e8ac82be4c62a686-cgu.4:(.text+0x272): undefined reference to `_tls_index'
            /usr/lib/gcc/x86_64-pc-cygwin/12/../../../../x86_64-pc-cygwin/bin/ld: /home/runneradmin/rustc-nightly-src/build-Cygwin/x86_64-pc-windows-gnu/stage1-rustc/x86_64-pc-cygwin/release/deps/rustc_driver-7abd146cd83e6b94.fastrand-3578f6ff8747b8ce.fastrand.5807741945a13650-cgu.0.rcgu.o.rcgu.o:fastrand.5807741945a13650-cgu.0:(.text+0x20d): more undefined references to `_tls_index' follow
            collect2: error: ld returned 1 exit status
            
    = note: some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified
    = note: use the `-l` flag to specify native libraries to link
    = note: use the `cargo:rustc-link-lib` directive to specify the native libraries to link with Cargo (see https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-link-lib)
  
  error: could not compile `rustc_driver` (lib) due to 1 previous error
  Build completed unsuccessfully in 1:04:08

https://github.com/jeremyd2019/cygwin-rust-bootstrap/actions/runs/15360769471/job/43227887473

@Berrysoft
Copy link
Contributor Author

Oh, no. Why didn't it show up in the tests...

@Berrysoft
Copy link
Contributor Author

if !mode.must_support_dlopen() && !target.triple.starts_with("powerpc-") {
cargo.env("RUSTC_TLS_MODEL_INITIAL_EXEC", "1");
}

I think we should disable it for cygwin.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 1, 2025
…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 :(
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 1, 2025
…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 :(
rust-timer added a commit that referenced this pull request Jun 1, 2025
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 :(
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-cygwin Target: *-pc-cygwin S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants