Skip to content

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