reject unsound toggling of RISCV target features

GuillaumeGomez · Dec 15, 2024 · 171223e · 171223e
1 parent d185062
commit 171223e
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 4 deletions.
diff --git 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
@@ -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), &[]),