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.

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

[experiment] Enable link-time thread local support on MinGW #102243

Closed
wants to merge 2 commits into from

Conversation

joboet
Copy link
Member

@joboet joboet commented Sep 24, 2022

I read somewhere that GCC supports link-time TLS on Windows. That would simplify the TLS handling code in std by a bit and would also hopefully improve performance.

Uses the CI runners for testing as I don't have a Windows machine.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Sep 24, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2022
@ChrisDenton
Copy link
Member

cc @mati865, see also #100917

Note that i686 windows-msvc also does not have thread-local enabled because the last attempt failed (see #95429). This might or might not be related to TLS issues with the Rust compiler.

@mati865
Copy link
Contributor

mati865 commented Sep 24, 2022

I've tried togging it in the past but EmuTLS used by GCC is just too messed up and causes problems for all languages that use TLS.
I don't think it's possible to workaround this problem in Rust but FWIW windows-gnullvm targets that I added not so long ago are working perfectly fine with it (although they are not tested on CI): https://github.com/rust-lang/rust/pull/94872/files#diff-5b82e76d5e73e79c281214a612797bd5ffadd853cc83f1c4e37eef19a1bba35dR48 This is because LLVM uses native Windows TLS by default which is not implemented in GCC.

@rust-log-analyzer
Copy link
Collaborator

The job i686-mingw-2 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Updating files:  98% (37295/38056)
Updating files:  99% (37676/38056)
Updating files: 100% (38056/38056)
Updating files: 100% (38056/38056), done.
Note: switching to 'refs/remotes/pull/102243/merge'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

---
HEAD is now at bae2e926 Merge 618b0546ec6cc5395d4819374110db672aae52f5 into 6580010551063718462f9dfe41c9490d92994d0e
##[endgroup]
[command]"C:\Program Files\Git\bin\git.exe" log -1 --format='%H'
'bae2e926c18de2e5195281bb538ace567fbed892'
##[group]Run echo "[CI_PR_NUMBER=$num]"
echo "[CI_PR_NUMBER=$num]"
env:
  CI_JOB_NAME: i686-mingw-2
  SCCACHE_BUCKET: rust-lang-ci-sccache2
  TOOLSTATE_REPO: https://github.com/rust-lang-nursery/rust-toolstate
---
   Compiling unic-langid-macros-impl v0.9.0
error: could not compile `unic-langid-macros-impl`

Caused by:
  process didn't exit successfully: `D:\a\rust\rust\build\bootstrap\debug\rustc --crate-name unic_langid_macros_impl --edition=2018 C:\Users\runneradmin\.cargo\registry\src\git.luolix.top-1285ae84e5963aae\unic-langid-macros-impl-0.9.0\src\lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type proc-macro --emit=dep-info,link -C prefer-dynamic -C embed-bitcode=no -C debuginfo=0 -Zunstable-options --check-cfg values(feature) --check-cfg names() --check-cfg values() -C metadata=99cdadb944cbfccc -C extra-filename=-99cdadb944cbfccc --out-dir D:\a\rust\rust\build\i686-pc-windows-gnu\stage1-rustc\release\deps -L dependency=D:\a\rust\rust\build\i686-pc-windows-gnu\stage1-rustc\release\deps --extern proc_macro_hack=D:\a\rust\rust\build\i686-pc-windows-gnu\stage1-rustc\release\deps\proc_macro_hack-4b1e6edf13150f97.dll --extern quote=D:\a\rust\rust\build\i686-pc-windows-gnu\stage1-rustc\release\deps\libquote-86fa47d5a22c8465.rlib --extern syn=D:\a\rust\rust\build\i686-pc-windows-gnu\stage1-rustc\release\deps\libsyn-513900d7f78bc0a5.rlib --extern unic_langid_impl=D:\a\rust\rust\build\i686-pc-windows-gnu\stage1-rustc\release\deps\libunic_langid_impl-4f3e471d6f782a3b.rlib --extern proc_macro --cap-lints allow -Z binary-dep-depinfo` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)
Build completed unsuccessfully in 0:16:55
Build completed unsuccessfully in 0:16:55
make: *** [Makefile:85: ci-mingw-subset-2] Error 1

@mati865
Copy link
Contributor

mati865 commented Sep 26, 2022

FYI there are multiple links regarding bugs in GCC's TLS for Windows here: msys2/MINGW-packages#2519

@bjorn3
Copy link
Member

bjorn3 commented Sep 26, 2022

Does MinGW use COFF TLS the same way as MSVC? If not, this will break cg_clif for MinGW.

@mati865
Copy link
Contributor

mati865 commented Sep 26, 2022

MinGW is only the CRT (a bit like glibc or musl on Linux).
TLS is provided by the compiler, in case of GCC it's emuTLS and for Clang it's native Windows TLS (so COFF TLS I suppose?).

@bjorn3
Copy link
Member

bjorn3 commented Sep 26, 2022

Is there any documentation on emuTLS for implementing it in Cranelift?

@mati865
Copy link
Contributor

mati865 commented Sep 26, 2022

I have no idea, I'm not familiar with GCC code.

@Mark-Simulacrum
Copy link
Member

r? compiler

I don't think I have enough context on this platform right now to properly review this.

@fee1-dead
Copy link
Member

r? compiler

@rust-highfive rust-highfive assigned eholk and unassigned fee1-dead Oct 2, 2022
@eholk
Copy link
Contributor

eholk commented Oct 4, 2022

Since this is still a draft I'm going to mark it as waiting on the author.

@rustbot author

@rustbot rustbot 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 Oct 4, 2022
@bors
Copy link
Contributor

bors commented Dec 23, 2022

☔ The latest upstream changes (presumably #106087) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

closing this as it was an experiment

@Dylan-DPC Dylan-DPC closed this May 17, 2023
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.