From 171223e01bbf46a8ab61a418871c2385dedaa926 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 15 Dec 2024 12:08:50 +0100 Subject: [PATCH 1/2] reject unsound toggling of RISCV target features --- compiler/rustc_target/src/spec/mod.rs | 2 +- compiler/rustc_target/src/target_features.rs | 69 +++++++++++++++++++- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_target/src/spec/mod.rs b/compiler/rustc_target/src/spec/mod.rs index a44d2af6b9086..f7706f094af66 100644 --- a/compiler/rustc_target/src/spec/mod.rs +++ b/compiler/rustc_target/src/spec/mod.rs @@ -3166,7 +3166,7 @@ impl Target { // Note that the `lp64e` is still unstable as it's not (yet) part of the ELF psABI. check_matches!( &*self.llvm_abiname, - "lp64" | "lp64f" | "lp64d" | "lp64q" | "lp64e", + "lp64" | "lp64f" | "lp64d" | "lp64e", "invalid RISC-V ABI name" ); } diff --git a/compiler/rustc_target/src/target_features.rs b/compiler/rustc_target/src/target_features.rs index 713b5dc70d7ad..b649610b3917c 100644 --- a/compiler/rustc_target/src/target_features.rs +++ b/compiler/rustc_target/src/target_features.rs @@ -590,9 +590,72 @@ const RISCV_FEATURES: &[(&str, StabilityUncomputed, ImpliedFeatures)] = &[ // tidy-alphabetical-start ("a", STABLE, &["zaamo", "zalrsc"]), ("c", STABLE, &[]), - ("d", unstable(sym::riscv_target_feature), &["f"]), - ("e", unstable(sym::riscv_target_feature), &[]), - ("f", unstable(sym::riscv_target_feature), &[]), + ( + "d", + Stability::Unstable { + nightly_feature: sym::riscv_target_feature, + allow_toggle: |target, enable| match &*target.llvm_abiname { + "ilp32d" | "lp64d" if !enable => { + // The ABI requires the `d` feature, so it cannot be disabled. + Err("feature is required by ABI") + } + "ilp32e" if enable => { + // The `d` feature apparently is incompatible with this ABI. + Err("feature is incompatible with ABI") + } + _ => Ok(()), + }, + }, + &["f"], + ), + ( + "e", + Stability::Unstable { + // Given that this is a negative feature, consider this before stabilizing: + // does it really make sense to enable this feature in an individual + // function with `#[target_feature]`? + nightly_feature: sym::riscv_target_feature, + allow_toggle: |target, enable| { + match &*target.llvm_abiname { + _ if !enable => { + // This is a negative feature, *disabling* it is always okay. + Ok(()) + } + "ilp32e" | "lp64e" => { + // Embedded ABIs should already have the feature anyway, it's fine to enable + // it again from an ABI perspective. + Ok(()) + } + _ => { + // *Not* an embedded ABI. Enabling `e` is invalid. + Err("feature is incompatible with ABI") + } + } + }, + }, + &[], + ), + ( + "f", + Stability::Unstable { + nightly_feature: sym::riscv_target_feature, + allow_toggle: |target, enable| { + match &*target.llvm_abiname { + "ilp32f" | "ilp32d" | "lp64f" | "lp64d" if !enable => { + // The ABI requires the `f` feature, so it cannot be disabled. + Err("feature is required by ABI") + } + _ => Ok(()), + } + }, + }, + &[], + ), + ( + "forced-atomics", + Stability::Forbidden { reason: "unsound because it changes the ABI of atomic operations" }, + &[], + ), ("m", STABLE, &[]), ("relax", unstable(sym::riscv_target_feature), &[]), ("unaligned-scalar-mem", unstable(sym::riscv_target_feature), &[]), From 934ed85e79c315d745ec9f443daed89a4defacbd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 16 Dec 2024 07:48:27 +0100 Subject: [PATCH 2/2] tweak comments --- compiler/rustc_target/src/target_features.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_target/src/target_features.rs b/compiler/rustc_target/src/target_features.rs index b649610b3917c..a313baf606693 100644 --- a/compiler/rustc_target/src/target_features.rs +++ b/compiler/rustc_target/src/target_features.rs @@ -39,7 +39,7 @@ pub enum Stability { allow_toggle: Toggleability, }, /// This feature can not be set via `-Ctarget-feature` or `#[target_feature]`, it can only be - /// set in the basic target definition. It is never set in `cfg(target_feature)`. Used in + /// set in the target spec. It is never set in `cfg(target_feature)`. Used in /// particular for features that change the floating-point ABI. Forbidden { reason: &'static str }, } @@ -600,7 +600,8 @@ const RISCV_FEATURES: &[(&str, StabilityUncomputed, ImpliedFeatures)] = &[ Err("feature is required by ABI") } "ilp32e" if enable => { - // The `d` feature apparently is incompatible with this ABI. + // ilp32e is incompatible with features that need aligned load/stores > 32 bits, + // like `d`. Err("feature is incompatible with ABI") } _ => Ok(()), @@ -618,7 +619,10 @@ const RISCV_FEATURES: &[(&str, StabilityUncomputed, ImpliedFeatures)] = &[ allow_toggle: |target, enable| { match &*target.llvm_abiname { _ if !enable => { - // This is a negative feature, *disabling* it is always okay. + // Disabling this feature means we can use more registers (x16-x31). + // The "e" ABIs treat them as caller-save, so it is safe to use them only + // in some parts of a program while the rest doesn't know they even exist. + // On other ABIs, the feature is already disabled anyway. Ok(()) } "ilp32e" | "lp64e" => {