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

forbid toggling x87 and fpregs on hard-float targets #133099

Merged
merged 3 commits into from
Dec 13, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion compiler/rustc_codegen_cranelift/src/lib.rs
Original file line number Diff line number Diff line change
@@ -175,7 +175,11 @@ impl CodegenBackend for CraneliftCodegenBackend {
}
}

fn target_features(&self, sess: &Session, _allow_unstable: bool) -> Vec<rustc_span::Symbol> {
fn target_features_cfg(
&self,
sess: &Session,
_allow_unstable: bool,
) -> Vec<rustc_span::Symbol> {
// FIXME return the actually used target features. this is necessary for #[cfg(target_feature)]
if sess.target.arch == "x86_64" && sess.target.os != "none" {
// x86_64 mandates SSE2 support
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_gcc/messages.ftl
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@ codegen_gcc_lto_not_supported =
LTO is not supported. You may get a linker error.

codegen_gcc_forbidden_ctarget_feature =
target feature `{$feature}` cannot be toggled with `-Ctarget-feature`
target feature `{$feature}` cannot be toggled with `-Ctarget-feature`: {$reason}

codegen_gcc_unwinding_inline_asm =
GCC backend does not support unwinding from inline asm
1 change: 1 addition & 0 deletions compiler/rustc_codegen_gcc/src/errors.rs
Original file line number Diff line number Diff line change
@@ -28,6 +28,7 @@ pub(crate) struct UnstableCTargetFeature<'a> {
#[diag(codegen_gcc_forbidden_ctarget_feature)]
pub(crate) struct ForbiddenCTargetFeature<'a> {
pub feature: &'a str,
pub reason: &'a str,
}

#[derive(Subdiagnostic)]
20 changes: 12 additions & 8 deletions compiler/rustc_codegen_gcc/src/gcc_util.rs
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@ use rustc_codegen_ssa::errors::TargetFeatureDisableOrEnable;
use rustc_data_structures::fx::FxHashMap;
use rustc_middle::bug;
use rustc_session::Session;
use rustc_target::target_features::{RUSTC_SPECIFIC_FEATURES, Stability};
use rustc_target::target_features::RUSTC_SPECIFIC_FEATURES;
use smallvec::{SmallVec, smallvec};

use crate::errors::{
@@ -94,13 +94,17 @@ pub(crate) fn global_gcc_features(sess: &Session, diagnostics: bool) -> Vec<Stri
};
sess.dcx().emit_warn(unknown_feature);
}
Some((_, Stability::Stable, _)) => {}
Some((_, Stability::Unstable(_), _)) => {
// An unstable feature. Warn about using it.
sess.dcx().emit_warn(UnstableCTargetFeature { feature });
}
Some((_, Stability::Forbidden { .. }, _)) => {
sess.dcx().emit_err(ForbiddenCTargetFeature { feature });
Some((_, stability, _)) => {
if let Err(reason) =
stability.compute_toggleability(&sess.target).allow_toggle()
{
sess.dcx().emit_warn(ForbiddenCTargetFeature { feature, 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 });
}
}
}

11 changes: 6 additions & 5 deletions compiler/rustc_codegen_gcc/src/lib.rs
Original file line number Diff line number Diff line change
@@ -260,8 +260,8 @@ impl CodegenBackend for GccCodegenBackend {
.join(sess)
}

fn target_features(&self, sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
target_features(sess, allow_unstable, &self.target_info)
fn target_features_cfg(&self, sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
target_features_cfg(sess, allow_unstable, &self.target_info)
}
}

@@ -472,7 +472,8 @@ fn to_gcc_opt_level(optlevel: Option<OptLevel>) -> OptimizationLevel {
}
}

pub fn target_features(
/// Returns the features that should be set in `cfg(target_feature)`.
fn target_features_cfg(
sess: &Session,
allow_unstable: bool,
target_info: &LockedTargetInfo,
@@ -481,9 +482,9 @@ pub fn target_features(
sess.target
.rust_target_features()
.iter()
.filter(|(_, gate, _)| gate.is_supported())
.filter(|(_, gate, _)| gate.in_cfg())
.filter_map(|&(feature, gate, _)| {
if sess.is_nightly_build() || allow_unstable || gate.is_stable() {
if sess.is_nightly_build() || allow_unstable || gate.requires_nightly().is_none() {
Some(feature)
} else {
None
6 changes: 3 additions & 3 deletions compiler/rustc_codegen_llvm/src/lib.rs
Original file line number Diff line number Diff line change
@@ -27,7 +27,7 @@ use std::mem::ManuallyDrop;
use back::owned_target_machine::OwnedTargetMachine;
use back::write::{create_informational_target_machine, create_target_machine};
use errors::ParseTargetMachineConfig;
pub use llvm_util::target_features;
pub use llvm_util::target_features_cfg;
use rustc_ast::expand::allocator::AllocatorKind;
use rustc_codegen_ssa::back::lto::{LtoModuleCodegen, SerializedModule, ThinModule};
use rustc_codegen_ssa::back::write::{
@@ -330,8 +330,8 @@ impl CodegenBackend for LlvmCodegenBackend {
llvm_util::print_version();
}

fn target_features(&self, sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
target_features(sess, allow_unstable)
fn target_features_cfg(&self, sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
target_features_cfg(sess, allow_unstable)
}

fn codegen_crate<'tcx>(
30 changes: 17 additions & 13 deletions compiler/rustc_codegen_llvm/src/llvm_util.rs
Original file line number Diff line number Diff line change
@@ -17,7 +17,7 @@ use rustc_session::Session;
use rustc_session::config::{PrintKind, PrintRequest};
use rustc_span::symbol::Symbol;
use rustc_target::spec::{MergeFunctions, PanicStrategy, SmallDataThresholdSupport};
use rustc_target::target_features::{RUSTC_SPECIAL_FEATURES, RUSTC_SPECIFIC_FEATURES, Stability};
use rustc_target::target_features::{RUSTC_SPECIAL_FEATURES, RUSTC_SPECIFIC_FEATURES};

use crate::back::write::create_informational_target_machine;
use crate::errors::{
@@ -300,7 +300,7 @@ pub(crate) fn to_llvm_features<'a>(sess: &Session, s: &'a str) -> Option<LLVMFea
/// Must express features in the way Rust understands them.
///
/// We do not have to worry about RUSTC_SPECIFIC_FEATURES here, those are handled outside codegen.
pub fn target_features(sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
pub fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
let mut features: FxHashSet<Symbol> = Default::default();

// Add base features for the target.
@@ -316,7 +316,7 @@ pub fn target_features(sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
sess.target
.rust_target_features()
.iter()
.filter(|(_, gate, _)| gate.is_supported())
.filter(|(_, gate, _)| gate.in_cfg())
.filter(|(feature, _, _)| {
// skip checking special features, as LLVM may not understand them
if RUSTC_SPECIAL_FEATURES.contains(feature) {
@@ -372,9 +372,9 @@ pub fn target_features(sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
sess.target
.rust_target_features()
.iter()
.filter(|(_, gate, _)| gate.is_supported())
.filter(|(_, gate, _)| gate.in_cfg())
.filter_map(|&(feature, gate, _)| {
if sess.is_nightly_build() || allow_unstable || gate.is_stable() {
if sess.is_nightly_build() || allow_unstable || gate.requires_nightly().is_none() {
Some(feature)
} else {
None
@@ -493,7 +493,7 @@ fn print_target_features(sess: &Session, tm: &llvm::TargetMachine, out: &mut Str
.rust_target_features()
.iter()
.filter_map(|(feature, gate, _implied)| {
if !gate.is_supported() {
if !gate.in_cfg() {
// Only list (experimentally) supported features.
return None;
}
@@ -716,13 +716,17 @@ pub(crate) fn global_llvm_features(
};
sess.dcx().emit_warn(unknown_feature);
}
Some((_, Stability::Stable, _)) => {}
Some((_, Stability::Unstable(_), _)) => {
// An unstable feature. Warn about using it.
sess.dcx().emit_warn(UnstableCTargetFeature { feature });
}
Some((_, Stability::Forbidden { reason }, _)) => {
sess.dcx().emit_warn(ForbiddenCTargetFeature { feature, reason });
Some((_, stability, _)) => {
if let Err(reason) =
stability.compute_toggleability(&sess.target).allow_toggle()
{
sess.dcx().emit_warn(ForbiddenCTargetFeature { feature, 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 });
}
}
}

53 changes: 23 additions & 30 deletions compiler/rustc_codegen_ssa/src/target_features.rs
Original file line number Diff line number Diff line change
@@ -11,7 +11,7 @@ use rustc_middle::ty::TyCtxt;
use rustc_session::parse::feature_err;
use rustc_span::Span;
use rustc_span::symbol::{Symbol, sym};
use rustc_target::target_features::{self, Stability};
use rustc_target::target_features;

use crate::errors;

@@ -20,7 +20,7 @@ use crate::errors;
pub(crate) fn from_target_feature_attr(
tcx: TyCtxt<'_>,
attr: &ast::Attribute,
rust_target_features: &UnordMap<String, target_features::Stability>,
rust_target_features: &UnordMap<String, target_features::StabilityComputed>,
target_features: &mut Vec<TargetFeature>,
) {
let Some(list) = attr.meta_item_list() else { return };
@@ -63,32 +63,24 @@ pub(crate) fn from_target_feature_attr(
return None;
};

// Only allow target features whose feature gates have been enabled.
let allowed = match stability {
Stability::Forbidden { .. } => false,
Stability::Stable => true,
Stability::Unstable(name) => rust_features.enabled(*name),
};
if !allowed {
match stability {
Stability::Stable => unreachable!(),
&Stability::Unstable(lang_feature_name) => {
feature_err(
&tcx.sess,
lang_feature_name,
item.span(),
format!("the target feature `{feature}` is currently unstable"),
)
.emit();
}
Stability::Forbidden { reason } => {
tcx.dcx().emit_err(errors::ForbiddenTargetFeatureAttr {
span: item.span(),
feature,
reason,
});
}
}
// Only allow target features whose feature gates have been enabled
// and which are permitted to be toggled.
if let Err(reason) = stability.allow_toggle() {
tcx.dcx().emit_err(errors::ForbiddenTargetFeatureAttr {
span: item.span(),
feature,
reason,
});
} else if let Some(nightly_feature) = stability.requires_nightly()
&& !rust_features.enabled(nightly_feature)
{
feature_err(
&tcx.sess,
nightly_feature,
item.span(),
format!("the target feature `{feature}` is currently unstable"),
)
.emit();
}
Some(Symbol::intern(feature))
}));
@@ -156,18 +148,19 @@ pub(crate) fn provide(providers: &mut Providers) {
*providers = Providers {
rust_target_features: |tcx, cnum| {
assert_eq!(cnum, LOCAL_CRATE);
let target = &tcx.sess.target;
if tcx.sess.opts.actually_rustdoc {
// rustdoc needs to be able to document functions that use all the features, so
// whitelist them all
rustc_target::target_features::all_rust_features()
.map(|(a, b)| (a.to_string(), b))
.map(|(a, b)| (a.to_string(), b.compute_toggleability(target)))
.collect()
} else {
tcx.sess
.target
.rust_target_features()
.iter()
.map(|&(a, b, _)| (a.to_string(), b))
.map(|&(a, b, _)| (a.to_string(), b.compute_toggleability(target)))
.collect()
}
},
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_ssa/src/traits/backend.rs
Original file line number Diff line number Diff line change
@@ -45,7 +45,9 @@ pub trait CodegenBackend {

fn print(&self, _req: &PrintRequest, _out: &mut String, _sess: &Session) {}

fn target_features(&self, _sess: &Session, _allow_unstable: bool) -> Vec<Symbol> {
/// Returns the features that should be set in `cfg(target_features)`.
/// RUSTC_SPECIFIC_FEATURES should be skipped here, those are handled outside codegen.
fn target_features_cfg(&self, _sess: &Session, _allow_unstable: bool) -> Vec<Symbol> {
vec![]
}

1 change: 1 addition & 0 deletions compiler/rustc_feature/src/unstable.rs
Original file line number Diff line number Diff line change
@@ -342,6 +342,7 @@ declare_features! (
(unstable, sse4a_target_feature, "1.27.0", Some(44839)),
(unstable, tbm_target_feature, "1.27.0", Some(44839)),
(unstable, wasm_target_feature, "1.30.0", Some(44839)),
(unstable, x87_target_feature, "CURRENT_RUSTC_VERSION", Some(44839)),
// !!!! !!!! !!!! !!!! !!!! !!!! !!!! !!!! !!!! !!!! !!!!
// Features are listed in alphabetical order. Tidy will fail if you don't keep it this way.
// !!!! !!!! !!!! !!!! !!!! !!!! !!!! !!!! !!!! !!!! !!!!
4 changes: 2 additions & 2 deletions compiler/rustc_interface/src/util.rs
Original file line number Diff line number Diff line change
@@ -35,10 +35,10 @@ pub type MakeBackendFn = fn() -> Box<dyn CodegenBackend>;
pub fn add_configuration(cfg: &mut Cfg, sess: &mut Session, codegen_backend: &dyn CodegenBackend) {
let tf = sym::target_feature;

let unstable_target_features = codegen_backend.target_features(sess, true);
let unstable_target_features = codegen_backend.target_features_cfg(sess, true);
sess.unstable_target_features.extend(unstable_target_features.iter().cloned());

let target_features = codegen_backend.target_features(sess, false);
let target_features = codegen_backend.target_features_cfg(sess, false);
sess.target_features.extend(target_features.iter().cloned());

cfg.extend(target_features.into_iter().map(|feat| (tf, Some(feat))));
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
@@ -2230,7 +2230,7 @@ rustc_queries! {
}

/// Returns the Rust target features for the current target. These are not always the same as LLVM target features!
query rust_target_features(_: CrateNum) -> &'tcx UnordMap<String, rustc_target::target_features::Stability> {
query rust_target_features(_: CrateNum) -> &'tcx UnordMap<String, rustc_target::target_features::StabilityComputed> {
arena_cache
eval_always
desc { "looking up Rust target features" }
2 changes: 1 addition & 1 deletion compiler/rustc_session/src/config/cfg.rs
Original file line number Diff line number Diff line change
@@ -371,7 +371,7 @@ impl CheckCfg {

ins!(sym::target_feature, empty_values).extend(
rustc_target::target_features::all_rust_features()
.filter(|(_, s)| s.is_supported())
.filter(|(_, s)| s.in_cfg())
.map(|(f, _s)| f)
.chain(rustc_target::target_features::RUSTC_SPECIFIC_FEATURES.iter().cloned())
.map(Symbol::intern),
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
@@ -2200,6 +2200,7 @@ symbols! {
writeln_macro,
x86_amx_intrinsics,
x87_reg,
x87_target_feature,
xer,
xmm_reg,
xop_target_feature,
12 changes: 12 additions & 0 deletions compiler/rustc_target/src/spec/mod.rs
Original file line number Diff line number Diff line change
@@ -2603,6 +2603,18 @@ impl TargetOptions {
.collect();
}
}

pub(crate) fn has_feature(&self, search_feature: &str) -> bool {
self.features.split(',').any(|f| {
if let Some(f) = f.strip_prefix('+')
&& f == search_feature
{
true
} else {
false
}
})
}
}

impl Default for TargetOptions {
778 changes: 441 additions & 337 deletions compiler/rustc_target/src/target_features.rs

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions tests/ui/check-cfg/target_feature.stderr
Original file line number Diff line number Diff line change
@@ -98,6 +98,7 @@ LL | cfg!(target_feature = "_UNEXPECTED_VALUE");
`fp8dot2`
`fp8dot4`
`fp8fma`
`fpregs`
`fpuv2_df`
`fpuv2_sf`
`fpuv3_df`
@@ -261,6 +262,7 @@ LL | cfg!(target_feature = "_UNEXPECTED_VALUE");
`vsx`
`wfxt`
`wide-arithmetic`
`x87`
`xop`
`xsave`
`xsavec`
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//@ compile-flags: --target=x86_64-unknown-none --crate-type=lib
//@ needs-llvm-components: x86
//@ build-pass
#![feature(no_core, lang_items, x87_target_feature)]
#![no_core]

#[lang = "sized"]
pub trait Sized {}

#[target_feature(enable = "x87")]
pub unsafe fn my_fun() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//@ compile-flags: --target=x86_64-unknown-none --crate-type=lib
//@ needs-llvm-components: x86
//@ compile-flags: -Ctarget-feature=-x87
//@ build-pass
#![feature(no_core, lang_items)]
#![no_core]

#[lang = "sized"]
pub trait Sized {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
warning: unstable feature specified for `-Ctarget-feature`: `x87`
|
= note: this feature is not stably supported; its behavior can change in the future

warning: 1 warning emitted

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//@ compile-flags: --target=x86_64-unknown-linux-gnu --crate-type=lib
//@ needs-llvm-components: x86
#![feature(no_core, lang_items)]
#![no_core]

#[lang = "sized"]
pub trait Sized {}

#[target_feature(enable = "x87")]
//~^ERROR: cannot be toggled with
pub unsafe fn my_fun() {}
Comment on lines +9 to +11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this unsound? I thought enable = "x87" on cfg(target_feature = "x87") environment is no-op.

Copy link
Member Author

@RalfJung RalfJung Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we could allow enabling x87 if it is already enabled. But I think that's wasted effort and a useless risk of getting the logic wrong.

Note that #[target_feature(enable = "x87")] already does not work on stable. So I'd rather wait for someone to show up with a concrete usecase for doing that, and not enable it "just because we can" as part of this PR which is meant to lock down what can be done in terms of target features.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: target feature `x87` cannot be toggled with `#[target_feature]`: unsound on hard-float targets because it changes float ABI
--> $DIR/forbidden-hardfloat-target-feature-attribute.rs:9:18
|
LL | #[target_feature(enable = "x87")]
| ^^^^^^^^^^^^^^

error: aborting due to 1 previous error

14 changes: 14 additions & 0 deletions tests/ui/target-feature/forbidden-hardfloat-target-feature-cfg.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//@ compile-flags: --target=x86_64-unknown-linux-gnu --crate-type=lib
//@ needs-llvm-components: x86
//@ check-pass
#![feature(no_core, lang_items)]
#![no_core]
#![allow(unexpected_cfgs)]

#[lang = "sized"]
pub trait Sized {}

// The compile_error macro does not exist, so if the `cfg` evaluates to `true` this
// complains about the missing macro rather than showing the error... but that's good enough.
#[cfg(not(target_feature = "x87"))]
compile_error!("the x87 feature *should* be exposed in `cfg`");
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//@ compile-flags: --target=x86_64-unknown-linux-gnu --crate-type=lib
//@ needs-llvm-components: x86
//@ compile-flags: -Ctarget-feature=-x87
// For now this is just a warning.
//@ build-pass
#![feature(no_core, lang_items)]
#![no_core]

#[lang = "sized"]
pub trait Sized {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
warning: target feature `x87` cannot be toggled with `-Ctarget-feature`: unsound on hard-float targets because it changes float ABI
|
= note: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #116344 <https://github.com/rust-lang/rust/issues/116344>

warning: 1 warning emitted

Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//@ compile-flags: --target=x86_64-unknown-linux-gnu --crate-type=lib
//@ needs-llvm-components: x86
#![feature(no_core, lang_items)]
#![no_std]
#![no_core]

#[lang = "sized"]
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: target feature `soft-float` cannot be toggled with `#[target_feature]`: unsound because it changes float ABI
--> $DIR/forbidden-target-feature-attribute.rs:10:18
--> $DIR/forbidden-target-feature-attribute.rs:9:18
|
LL | #[target_feature(enable = "soft-float")]
| ^^^^^^^^^^^^^^^^^^^^^
1 change: 0 additions & 1 deletion tests/ui/target-feature/forbidden-target-feature-cfg.rs
Original file line number Diff line number Diff line change
@@ -2,7 +2,6 @@
//@ needs-llvm-components: x86
//@ check-pass
#![feature(no_core, lang_items)]
#![no_std]
#![no_core]
#![allow(unexpected_cfgs)]

Original file line number Diff line number Diff line change
@@ -4,7 +4,6 @@
// For now this is just a warning.
//@ build-pass
#![feature(no_core, lang_items)]
#![no_std]
#![no_core]

#[lang = "sized"]
1 change: 0 additions & 1 deletion tests/ui/target-feature/forbidden-target-feature-flag.rs
Original file line number Diff line number Diff line change
@@ -4,7 +4,6 @@
// For now this is just a warning.
//@ build-pass
#![feature(no_core, lang_items)]
#![no_std]
#![no_core]

#[lang = "sized"]
1 change: 1 addition & 0 deletions tests/ui/target-feature/gate.rs
Original file line number Diff line number Diff line change
@@ -24,6 +24,7 @@
// gate-test-prfchw_target_feature
// gate-test-s390x_target_feature
// gate-test-sparc_target_feature
// gate-test-x87_target_feature

#[target_feature(enable = "avx512bw")]
//~^ ERROR: currently unstable
2 changes: 1 addition & 1 deletion tests/ui/target-feature/gate.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0658]: the target feature `avx512bw` is currently unstable
--> $DIR/gate.rs:28:18
--> $DIR/gate.rs:29:18
|
LL | #[target_feature(enable = "avx512bw")]
| ^^^^^^^^^^^^^^^^^^^
Loading