-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Allow specifying the ELF TLS ABI #132480
base: master
Are you sure you want to change the base?
Allow specifying the ELF TLS ABI #132480
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ use rustc_span::{RealFileName, SourceFileHashAlgorithm}; | |
use rustc_target::spec::{ | ||
CodeModel, FramePointer, LinkerFlavorCli, MergeFunctions, OnBrokenPipe, PanicStrategy, | ||
RelocModel, RelroLevel, SanitizerSet, SplitDebuginfo, StackProtector, SymbolVisibility, | ||
TargetTriple, TlsModel, WasmCAbi, | ||
TargetTriple, TlsDialect, TlsModel, WasmCAbi, | ||
}; | ||
|
||
use crate::config::*; | ||
|
@@ -427,6 +427,8 @@ mod desc { | |
"one of supported code models (`rustc --print code-models`)"; | ||
pub(crate) const parse_tls_model: &str = | ||
"one of supported TLS models (`rustc --print tls-models`)"; | ||
pub(crate) const parse_tls_dialect: &str = | ||
"one of supported TLS dialects (`rustc --print tls-dialect`)"; | ||
pub(crate) const parse_target_feature: &str = parse_string; | ||
pub(crate) const parse_terminal_url: &str = | ||
"either a boolean (`yes`, `no`, `on`, `off`, etc), or `auto`"; | ||
|
@@ -1244,6 +1246,13 @@ mod parse { | |
} | ||
true | ||
} | ||
pub(crate) fn parse_tls_dialect(slot: &mut Option<TlsDialect>, v: Option<&str>) -> bool { | ||
match v.and_then(|s| TlsDialect::from_str(s).ok()) { | ||
Some(tls_dialect) => *slot = Some(tls_dialect), | ||
_ => return false, | ||
} | ||
true | ||
} | ||
|
||
pub(crate) fn parse_terminal_url(slot: &mut TerminalUrl, v: Option<&str>) -> bool { | ||
*slot = match v { | ||
|
@@ -2114,6 +2123,9 @@ written to standard error output)"), | |
#[rustc_lint_opt_deny_field_access("use `Session::tls_model` instead of this field")] | ||
tls_model: Option<TlsModel> = (None, parse_tls_model, [TRACKED], | ||
"choose the TLS model to use (`rustc --print tls-models` for details)"), | ||
#[rustc_lint_opt_deny_field_access("use `Session::tls_model` instead of this field")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. *tls_model |
||
tls_dialect: Option<TlsDialect> = (None, parse_tls_dialect, [TRACKED], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe name this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm OK w/ that, but I think its usually better to support an existing spelling when there are existing conventions in other compilers. We'd have to check w/ folks doing the GCC backend work, but its possible the variants would be easier to integrate for them. We've talked about adding support for a 3 GOT slot variant of TLSDESC. That would require support from the linker and libc. There are advantages to doing it that way, like avoiding a level of indirection to read the descriptor for the dynamic cases, but I'm not sure how likely that is to ever happen. I'd like to see it, and was hoping that would be the ABI for RISC-V, but it would be a win for any architecture. But I can change this to a boolean if you'd like. |
||
"choose the TLS dialect to use (`rustc --print tls-dialect` for details)"), | ||
trace_macros: bool = (false, parse_bool, [UNTRACKED], | ||
"for every macro invocation, print its name and arguments (default: no)"), | ||
track_diagnostics: bool = (false, parse_bool, [UNTRACKED], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
#![crate_type = "lib"] | ||
|
||
use std::cell::Cell; | ||
|
||
thread_local! { | ||
#[no_mangle] | ||
pub static A: Cell<u64> = const { Cell::new(0) }; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
// Verifies that setting the tls-dialect result sin the correct set of assembly | ||
// instructions. | ||
// | ||
// Note: tls-dialect flags have no changes to LLVM IR, and only affect which | ||
// instruction sequences are emitted by the LLVM backend. Checking the assembly | ||
// output is how we test the lowering in LLVM, and is the only way a frontend | ||
// can determine if its code generation flags are set correctly. | ||
// | ||
//@ revisions: x64 x64-trad x64-desc | ||
// | ||
//@[x64] compile-flags: --target=x86_64-unknown-linux-gnu | ||
//@[x64-trad] compile-flags: --target=x86_64-unknown-linux-gnu -Z tls-dialect=trad | ||
//@[x64-desc] compile-flags: --target=x86_64-unknown-linux-gnu -Z tls-dialect=desc | ||
// | ||
//@ assembly-output: emit-asm | ||
//@ aux-build:tlsdesc_aux.rs | ||
|
||
#![crate_type = "lib"] | ||
|
||
extern crate tlsdesc_aux as aux; | ||
|
||
#[no_mangle] | ||
fn get_aux() -> u64 { | ||
// x64: __tls_get_addr | ||
// x64-trad: __tls_get_addr | ||
// x64-desc: tlsdesc | ||
// x64-desc: tlscall | ||
aux::A.with(|a| a.get()) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
error: unknown print request: `yyyy` | ||
| | ||
= help: valid print requests are: `all-target-specs-json`, `calling-conventions`, `cfg`, `check-cfg`, `code-models`, `crate-name`, `deployment-target`, `file-names`, `link-args`, `native-static-libs`, `relocation-models`, `split-debuginfo`, `stack-protector-strategies`, `sysroot`, `target-cpus`, `target-features`, `target-libdir`, `target-list`, `target-spec-json`, `tls-models` | ||
= help: valid print requests are: `all-target-specs-json`, `calling-conventions`, `cfg`, `check-cfg`, `code-models`, `crate-name`, `deployment-target`, `file-names`, `link-args`, `native-static-libs`, `relocation-models`, `split-debuginfo`, `stack-protector-strategies`, `sysroot`, `target-cpus`, `target-features`, `target-libdir`, `target-list`, `target-spec-json`, `tls-dialect`, `tls-models` | ||
|
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.
Why is this added? Do you have any actual need to know which tls dialect will ne used by rustc on a given target?
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.
Also it should probably be made available on nightly only so we can remove it again once the traditional tls dialect is no longer used anywhere.
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 followed the example for
TlsModels
, since they seem of equivalent diagnostic value to me. For Android RISC-V targets TLSDESC is the default, and I think it may be a requirement.I'm not sure that will ever happen. TLSDESC requires support in the dynamic linker, so its imaginable that not all libcs will support that. For instance I've seen embedded targets that implement something like a loader service that support
__tls_get_addr
with overlays, but do not support TLSDESC.At any rate, I'm not sure we will ever drop support for traditional TLS in LLVM. It's been discussed, but I think its unlikely until GNU toolchains drop support.
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.
It can be the default on targets that do support it and not having an option to enable it on targets that don't support it.
I mean rustc dropping support for configuring it to anything other than the default for the target.
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'm not sure how you can make that determination, since you can't know which libc someone is using ahead of time. Being able to configure and opt in/out is necessary, IMO.
Failing to expose a code generation option doesn't sound right to me. There are valid reasons someone may want to select a particular variant of an option differently than the default. Until LLVM (and I guess all backends?) drops support altogether, I'm not sure its a good idea for
rustc
to stop exposing that option altogether.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.
AFAIK, LLVM has no plans to drop support. Its hard to say never, but as a mater of practicality, I doubt it will be removed any time soon. If it does, we'd probably have a long period where its listed as "deprecated". Even then its possible it wouldn't actually be removed. There is precedent for that.
In general, passing
-mllvm
options often fails for LTO builds, because frontends tend to fail to pass those along correctly to the linker via-Wl,-mllvm,-option
. This was particularly painful for RISC-V targets whose target features were not propagated to the linker correctly, and required extra build plumbing to solve. It's arguably a failing of LLVM for not encoding those options in the IR. How to solve this class of problems has been being discussed for a long time now, and I don't think we'll arrive at a solution any time soon.First, this comment was about compiler options in general. Defaults are usually chosen to make sense for the most common software. Projects may want to leverage knowledge they have about their system to improve performance, security, reduce size, etc. It's quite hard to guess what requirements people may have, and failing to expose options that allows them to deviate from the default feels like the wrong approach for a compiler.
For TLS dialects, I'd say performance is the most obvious motivation. Another thing to consider is that you may want to support different configurations for the same target triple. For instance, Android after some particular SDK version may want to drop TLSDESC support, but you may need to retain the ability to target earlier SDKs that predate TLSDESC support in bionic. Similarly, we know that Fuchsia's driver ABI will only support TLSDESC, but the rest of the system will likely be more flexible about supporting both TLSDESC and traditional TLS.
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.
Is Android planning to drop support for not using TLSDESC before the last LTS NDK that doesn't support TLSDESC is no longer supported? We only support the most recent LTS NDK.
If Fuchsia has a different ABI between regular userspace and drivers, it would make sense to have separate targets, right? Does anyone care about supporting Fuchsia versions from before TLSDESC got introduced in the first place?
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.
This argument generalizes too much. It could mean that every single decision the compiler could make should have a user-facing option.
Compiler flags are part of our user interface. Which means they need testing, we need to consider compatibility, ABI unsoundness, terrible linker errors and so on. So adding one should come with a stronger motivation than "it'd be nice to have a knob, even though there's no immediate need".
As bjorn says it's possible that this decision is better left to a combination of target specifications and the compiler checking if it's supported (e.g. based on LLVM version) and determining the correct setting based on that.
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 doubt it, but I'm not an Android maintainer and the precise details are outside of my area of expertise. I mentioned this, since similar concerns were brought up on the LLVM side w.r.t. emulated TLS and the NDK's need to maintain support for it until the NDK versions that used it were no longer supported. Those time horizons were typically long, IIRC. If Rust has a support story here tied to the NDK then maybe the concern I raised is moot.
Drivers in Fuchsia are still user-level programs, and I'm fairly sure we don't want to introduce another triple for them.
Fuchsia was brought up as an example of a scenario where its useful to have such knobs. I don't think the focus should be on Fuchsia specifically. FWIW, though, we do certainly support older versions of our SDK, though those are usually tied to a specific toolchain and set of libraries.
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.
There's an argument that it should, even if its not easy to access. Not all software is created equal, and not all compiler decisions are appropriate everywhere. I'm not suggesting that be made the case, just that in many cases not leaving a escape hatch for an optimization leads to worse performance, code size, or a security problem that has to be worked around.
I don't view this as "nice to have," I view it as a requirement. Given the history of these options, it seems irrational to fail to give developers the choice of what to do if their needs differ from the default. I'm fine to leave the disagreement there, but I'm not doing this work because projects I support don't need it. Clearly the fact that I'm implementing it suggests some level of need.
I disagree. IMO, setting defaults based on the triple is sensible, but locking users in is not, especially when the backend supports both.