Skip to content

Commit

Permalink
Rollup merge of rust-lang#134337 - RalfJung:riscv-target-features, r=…
Browse files Browse the repository at this point in the history
…workingjubilee

reject unsound toggling of RISCV target features

~~Stacked on top of rust-lang#133417, only the last commit is new.~~

Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment)))
Part of rust-lang#116344

Cc `@beetrees` I hope I got everything.  I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks.
r? `@workingjubilee`

Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.
  • Loading branch information
GuillaumeGomez authored Dec 16, 2024
2 parents bcc184d + 934ed85 commit 362b68d
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 5 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_target/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
}
Expand Down
75 changes: 71 additions & 4 deletions compiler/rustc_target/src/target_features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pub enum Stability<Toggleability> {
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 },
}
Expand Down Expand Up @@ -590,9 +590,76 @@ 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 => {
// ilp32e is incompatible with features that need aligned load/stores > 32 bits,
// like `d`.
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 => {
// 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" => {
// 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), &[]),
Expand Down

0 comments on commit 362b68d

Please sign in to comment.