-
Notifications
You must be signed in to change notification settings - Fork 13.3k
retpoline and retpoline-external-thunk flags (target modifiers) to enable retpoline-related target features #135927
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
base: master
Are you sure you want to change the base?
Conversation
rustbot has assigned @compiler-errors. Use |
Some changes occurred in compiler/rustc_codegen_gcc |
e808e83
to
12b286f
Compare
This comment has been minimized.
This comment has been minimized.
r? compiler |
☔ The latest upstream changes (presumably #136085) made this pull request unmergeable. Please resolve the merge conflicts. |
12b286f
to
f331c60
Compare
This comment has been minimized.
This comment has been minimized.
f331c60
to
434a25f
Compare
This comment has been minimized.
This comment has been minimized.
434a25f
to
945d0f1
Compare
This comment has been minimized.
This comment has been minimized.
r? compiler |
945d0f1
to
0ea46d2
Compare
This comment has been minimized.
This comment has been minimized.
r? compiler |
0ea46d2
to
386c90e
Compare
r? @davidtwco maybe 😅 |
I don't think retpoline actually changes the ABI, so it might not need to be a target modifier. On the other hand, as an exploit mitigation, we do still want to avoid cases where you don't enable the mitigation in every compilation unit. cc @andyhhp do I understand things correctly? |
I read your post in #116852:
That's why I thought it should be a target modifier. |
It's important to distinguish the general retpoline is the specifically transform of an indirect call/jmp instruction into a The need to rewrite the return address on the stack necessitates the use of a GPR, and this is observable call/jmp-ee but any non-parameter register will do, so it doesn't impact the main function call ABI. Even within retpoline itself, there are variations; inline retpolines are an option, but tend to be heavy on the code bloat side, which is why out-of-line retpolines in common sections are often preferred. The alternatives to repoline are As to separate complication units, you can mix and match, but if any one piece of your whole process isn't taking safety precautions, then you've lost. |
Could you, please, recommend the right way to represent this flag according with Should it be |
I would think that the best choice would be to use the same name as clang, which is |
Yeah, personally, I prefer when compiler drivers agree. |
…o enable retpoline-related target features
386c90e
to
597eb26
Compare
Looks like it corresponds to clang flags. |
Some changes occurred in compiler/rustc_codegen_ssa |
} | ||
} | ||
|
||
/// Returns whether the feature may be toggled via `#[target_feature]` or `-Ctarget-feature`. | ||
/// (It might still be nightly-only even if this returns `true`, so make sure to also check | ||
/// `requires_nightly`.) | ||
pub fn toggle_allowed(&self) -> Result<(), &'static str> { |
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 API seems confusing, at the very least change the doc comment.
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.
Per the team documentation, adding a new flag with the intent to later stabilise requires an MCP - https://forge.rust-lang.org/compiler/proposals-and-stabilization.html#compiler-flags
} | ||
} | ||
|
||
/// Returns whether the feature may be toggled via `#[target_feature]` or `-Ctarget-feature`. | ||
/// (It might still be nightly-only even if this returns `true`, so make sure to also check | ||
/// `requires_nightly`.) | ||
pub fn toggle_allowed(&self) -> Result<(), &'static str> { | ||
pub fn toggle_allowed(&self, flag_enabled: impl Fn(&str) -> bool) -> Result<(), &'static str> { |
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.
pre-existing: this name is a little ambiguous, is it toggling something named allowed
, or is toggling allowed? maybe is_toggle_permitted
instead or just is_toggle_allowed
?
@@ -733,7 +733,9 @@ pub(crate) fn global_llvm_features( | |||
sess.dcx().emit_warn(unknown_feature); | |||
} | |||
Some((_, stability, _)) => { | |||
if let Err(reason) = stability.toggle_allowed() { |
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 a bunch of duplication in the code around this between codegen backends and rustc_codegen_ssa
, it's pre-existing, you could sort it if you wanted, but someone ought to.
} | ||
} | ||
|
||
pub fn fill_target_features_by_flags( |
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 doesn't feel like the right place to do this, ideally there would be a query or something that we could add this logic to that would mean that codegen backends just get the features they need to enable and we'd insert this logic into that.
} | ||
} | ||
|
||
/// Returns whether the feature may be toggled via `#[target_feature]` or `-Ctarget-feature`. | ||
/// (It might still be nightly-only even if this returns `true`, so make sure to also check | ||
/// `requires_nightly`.) | ||
pub fn toggle_allowed(&self) -> Result<(), &'static str> { | ||
pub fn toggle_allowed(&self, flag_enabled: impl Fn(&str) -> bool) -> Result<(), &'static str> { |
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 we add a StabilityExt
trait in another crate that just takes a Session
and calls this with the appropriate closure, given its the same closure everywhere?
@@ -34,6 +34,9 @@ pub enum Stability { | |||
/// particular for features are actually ABI configuration flags (not all targets are as nice as | |||
/// RISC-V and have an explicit way to set the ABI separate from target features). | |||
Forbidden { reason: &'static str }, | |||
/// This feature can not be set via `-Ctarget-feature` or `#[target_feature]`, it can only be set | |||
/// by target modifier flag. Target modifier flags are tracked to be consistent in linked modules. | |||
EnabledByTargetModifierFlag { reason: &'static str, flag: &'static str }, |
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.
EnabledByTargetModifierFlag { reason: &'static str, flag: &'static str }, | |
TargetModifierOnly { reason: &'static str, flag: &'static str }, |
@rustbot label F-target_modifiers A-rust-for-linux |
-Zretpoline
and-Zretpoline-external-thunk
flags are target modifiers (tracked to be equal in linked crates).-Zretpoline-external-thunk
:+retpoline-external-thunk
,+retpoline-indirect-branches
,+retpoline-indirect-calls
.-Zretpoline
:+retpoline-indirect-branches
,+retpoline-indirect-calls
.It corresponds to clang -mretpoline & -mretpoline-external-thunk flags.
Also this PR forbids to specify those target features manually (warning).