-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Support TLS access into dylibs on Windows #108089
Conversation
r? @Nilstrieb (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt These commits modify compiler targets. Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
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.
I took a look over it, looks great, thanks for finally solving this!
I am not familiar enough with this sort of code to approve it, so I'll hand it off to someone else.
r? @wesleywiser, I hope you can review this :D, assigning you directly since compiler
includes plenty of people who wouldn't review this code either
ccing the people involved in #101368, @thomcc @eddyb @ChrisDenton
is_cleanup: false, | ||
}); | ||
|
||
let ret_ty = Rvalue::ThreadLocalRef(def_id).ty(&IndexVec::new(), tcx); |
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.
Could you factor out the "type of tls ref" logic into a helper function somewhere and use it here (and in the other places where you need it, including the impl of Rvalue::ty
)?
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.
I created a tcx.thread_local_ptr_ty
method for that. It's suspiciously similar to tcx.static_ptr_ty
which normalizes and erases regions. It's possible thread_local_ptr_ty
should call that.
This comment has been minimized.
This comment has been minimized.
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
I've added untested cranelift support. I tried to test using
|
On Windows panic_unwind imports Line 722 in eebdfb5
|
☔ The latest upstream changes (presumably #108340) made this pull request unmergeable. Please resolve the merge conflicts. |
Support TLS access into dylibs on Windows This allows access to `#[thread_local]` in upstream dylibs on Windows by introducing a MIR shim to return the address of the thread local. Accesses that go into an upstream dylib will call the MIR shim to get the address of it. `convert_tls_rvalues` is introduced in `rustc_codegen_ssa` which rewrites MIR TLS accesses to dummy calls which are replaced with calls to the MIR shims when the dummy calls are lowered to backend calls. A new `dll_tls_export` target option enables this behavior with a `false` value which is set for Windows platforms. This fixes rust-lang#84933.
Support TLS access into dylibs on Windows This allows access to `#[thread_local]` in upstream dylibs on Windows by introducing a MIR shim to return the address of the thread local. Accesses that go into an upstream dylib will call the MIR shim to get the address of it. `convert_tls_rvalues` is introduced in `rustc_codegen_ssa` which rewrites MIR TLS accesses to dummy calls which are replaced with calls to the MIR shims when the dummy calls are lowered to backend calls. A new `dll_tls_export` target option enables this behavior with a `false` value which is set for Windows platforms. This fixes rust-lang#84933.
@bors r+ |
⌛ Testing commit 3019a34 with merge 55acf51dcaf91ffa64f7a2aea4fc0615720b28b9... |
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (f98598c): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. |
Workaround rust-lang#109797 on windows-gnu The addition of `#[inline]` here in rust-lang#108089 caused an unrelated linking issue (rust-lang#109797). This PR removes this attribute again on Windows to avoid regressions.
Workaround #109797 on windows-gnu The addition of `#[inline]` here in rust-lang/rust#108089 caused an unrelated linking issue (rust-lang/rust#109797). This PR removes this attribute again on Windows to avoid regressions.
This allows access to
#[thread_local]
in upstream dylibs on Windows by introducing a MIR shim to return the address of the thread local. Accesses that go into an upstream dylib will call the MIR shim to get the address of it.convert_tls_rvalues
is introduced inrustc_codegen_ssa
which rewrites MIR TLS accesses to dummy calls which are replaced with calls to the MIR shims when the dummy calls are lowered to backend calls.A new
dll_tls_export
target option enables this behavior with afalse
value which is set for Windows platforms.This fixes #84933.