-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
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 |
---|---|---|
|
@@ -16,15 +16,15 @@ use rustc_fs_util::path_to_c_string; | |
use rustc_middle::bug; | ||
use rustc_session::Session; | ||
use rustc_session::config::{PrintKind, PrintRequest}; | ||
use rustc_session::features::{StabilityExt, retpoline_features_by_flags}; | ||
use rustc_span::Symbol; | ||
use rustc_target::spec::{MergeFunctions, PanicStrategy, SmallDataThresholdSupport}; | ||
use rustc_target::target_features::{RUSTC_SPECIAL_FEATURES, RUSTC_SPECIFIC_FEATURES}; | ||
use smallvec::{SmallVec, smallvec}; | ||
|
||
use crate::back::write::create_informational_target_machine; | ||
use crate::errors::{ | ||
FixedX18InvalidArch, ForbiddenCTargetFeature, PossibleFeature, UnknownCTargetFeature, | ||
UnknownCTargetFeaturePrefix, UnstableCTargetFeature, | ||
FixedX18InvalidArch, PossibleFeature, UnknownCTargetFeature, UnknownCTargetFeaturePrefix, | ||
}; | ||
use crate::llvm; | ||
|
||
|
@@ -707,6 +707,12 @@ pub(crate) fn target_cpu(sess: &Session) -> &str { | |
handle_native(cpu_name) | ||
} | ||
|
||
fn llvm_features_by_flags(sess: &Session) -> Vec<&str> { | ||
let mut features: Vec<&str> = Vec::new(); | ||
retpoline_features_by_flags(sess, &mut features); | ||
features | ||
} | ||
|
||
/// The list of LLVM features computed from CLI flags (`-Ctarget-cpu`, `-Ctarget-feature`, | ||
/// `--target` and similar). | ||
pub(crate) fn global_llvm_features( | ||
|
@@ -787,7 +793,7 @@ pub(crate) fn global_llvm_features( | |
|
||
// Compute implied features | ||
let mut all_rust_features = vec![]; | ||
for feature in sess.opts.cg.target_feature.split(',') { | ||
for feature in sess.opts.cg.target_feature.split(',').chain(llvm_features_by_flags(sess)) { | ||
if let Some(feature) = feature.strip_prefix('+') { | ||
all_rust_features.extend( | ||
UnordSet::from(sess.target.implied_target_features(feature)) | ||
|
@@ -840,18 +846,7 @@ 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 commentThe 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 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. Moved into 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 was cleaning up all this duplication in #140920, and doing the same in this PR is now causing super painful conflicts :( |
||
sess.dcx().emit_warn(ForbiddenCTargetFeature { | ||
feature, | ||
enabled: if enable { "enabled" } else { "disabled" }, | ||
reason, | ||
}); | ||
} else if stability.requires_nightly().is_some() { | ||
// An unstable feature. Warn about using it. It makes little sense | ||
// to hard-error here since we just warn about fully unknown | ||
// features above. | ||
sess.dcx().emit_warn(UnstableCTargetFeature { feature }); | ||
} | ||
stability.verify_feature_enabled_by_flag(sess, enable, feature); | ||
} | ||
} | ||
|
||
|
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. As discussed in #140920, codegen_ssa is probably a much better place for this target feature handling. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
use rustc_target::target_features::Stability; | ||
|
||
use crate::Session; | ||
use crate::errors::{ForbiddenCTargetFeature, UnstableCTargetFeature}; | ||
|
||
pub trait StabilityExt { | ||
/// Returns whether the feature may be toggled via `#[target_feature]` or `-Ctarget-feature`. | ||
/// Otherwise, some features also may only be enabled by flag (target modifier). | ||
/// (It might still be nightly-only even if this returns `true`, so make sure to also check | ||
/// `requires_nightly`.) | ||
fn is_toggle_permitted(&self, sess: &Session) -> Result<(), &'static str>; | ||
|
||
/// Check that feature is correctly enabled/disabled by command line flag (emits warnings) | ||
fn verify_feature_enabled_by_flag(&self, sess: &Session, enable: bool, feature: &str); | ||
} | ||
|
||
impl StabilityExt for Stability { | ||
fn is_toggle_permitted(&self, sess: &Session) -> Result<(), &'static str> { | ||
match self { | ||
Stability::Forbidden { reason } => Err(reason), | ||
Stability::TargetModifierOnly { reason, flag } => { | ||
if !sess.opts.target_feature_flag_enabled(*flag) { Err(reason) } else { Ok(()) } | ||
} | ||
Comment on lines
+21
to
+23
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 don't understand this logic. Somewhere it says these target features are not allowed in 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. Oh, you are taking the named I think it's a bad idea to ever mix these. The code paths should be entirely separate all the way until actually generating backend target features. Please let's not make the target feature code even more spaghetti than it already was. 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. If I understand this PR correctly, then once I set |
||
_ => Ok(()), | ||
} | ||
} | ||
fn verify_feature_enabled_by_flag(&self, sess: &Session, enable: bool, feature: &str) { | ||
if let Err(reason) = self.is_toggle_permitted(sess) { | ||
sess.dcx().emit_warn(ForbiddenCTargetFeature { | ||
feature, | ||
enabled: if enable { "enabled" } else { "disabled" }, | ||
reason, | ||
}); | ||
} else if self.requires_nightly().is_some() { | ||
// An unstable feature. Warn about using it. It makes little sense | ||
// to hard-error here since we just warn about fully unknown | ||
// features above. | ||
sess.dcx().emit_warn(UnstableCTargetFeature { feature }); | ||
} | ||
} | ||
} | ||
|
||
pub fn retpoline_features_by_flags(sess: &Session, features: &mut Vec<&str>) { | ||
// -Zretpoline without -Zretpoline-external-thunk enables | ||
// retpoline-indirect-branches and retpoline-indirect-calls target features | ||
let unstable_opts = &sess.opts.unstable_opts; | ||
if unstable_opts.retpoline && !unstable_opts.retpoline_external_thunk { | ||
features.push("+retpoline-indirect-branches"); | ||
features.push("+retpoline-indirect-calls"); | ||
} | ||
// -Zretpoline-external-thunk (maybe, with -Zretpoline too) enables | ||
// retpoline-external-thunk, retpoline-indirect-branches and | ||
// retpoline-indirect-calls target features | ||
if unstable_opts.retpoline_external_thunk { | ||
features.push("+retpoline-external-thunk"); | ||
features.push("+retpoline-indirect-branches"); | ||
features.push("+retpoline-indirect-calls"); | ||
} | ||
} |
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.
The handling for
fixed_x18
should probably be merged with this. (That would also have been an easier model to follow for adding the new flags.)