Skip to content

reject unsound toggling of RISCV target features #134337

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 16, 2024
Merged

reject unsound toggling of RISCV target features #134337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
171223e
reject unsound toggling of RISCV target features
RalfJung Dec 15, 2024
934ed85
tweak comments
RalfJung Dec 16, 2024
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
2 changes: 1 addition & 1 deletion compiler/rustc_target/src/spec/mod.rs
View file
Open in desktop
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
69 changes: 66 additions & 3 deletions compiler/rustc_target/src/target_features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Btw do you know why the 32bit ABIs start with "ilp" but the 64bit ABIs start with "lp"?

Copy link
Member

Choose a reason for hiding this comment

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

  • ILP32: Int, Long, and Pointer are 32-bit
  • LP64: Long and Pointer are 64-bit (int is 32-bit)

(see also "Data models" section in https://en.cppreference.com/w/cpp/language/types)

Copy link
Member Author

Choose a reason for hiding this comment

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

So it should really be I32LP64 then...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but a convention at some point developed that

  • we omit the "c8s16" prefix... types that are the smallest possible value
  • we all quietly pretend the smallest possible value of int is 32 bits and not 16

names deeply fermented in tradition.

// 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.
Copy link
Member

@workingjubilee workingjubilee Dec 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
// The `d` feature apparently is incompatible with this ABI.
// ilp32e is incompatible with features that need aligned load/stores > 32 bits, like `d`

Copy link
Member Author

Choose a reason for hiding this comment

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

I am confused: Rust lets you always define types that need >32bit alignment, so that can't be a problem in itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct AFAIK that this isn't actually a problem, however the RISC-V ELF psABI for the ILP32E ABI states:

The ILP32E calling convention is not compatible with ISAs that have registers that require load and store alignments of more than 32 bits. In particular, this calling convention must not be used with the D ISA extension.

Correspondingly, LLVM emits an error when there is an attempt to use the D extension with the ILP32E ABI:

rustc-LLVM ERROR: ILP32E cannot be used with the D ISA extension

Copy link
Member

Choose a reason for hiding this comment

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

ack, I didn't mean to confuse.

yeah, it's about the actual CPU-internal alignment requirements imposed by the way the hardware moves things around, as opposed to our language requirements.

Copy link
Member

@workingjubilee workingjubilee Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

oh actually it's kinda weirder, sorry, here we go:

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, makes sense.

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,
Comment on lines +615 to +618
Copy link
Member

Choose a reason for hiding this comment

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

not really, but it does allow #[cfg(target_feature = "e")], unfortunately, but that may not be correct if it tries to be dependent on the ABI

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like we may want to mark this feature as "forbidden", i.e., "must be fixed by target spec and cannot be queried by cfg", and have a separate way to query the ABI.

But anyway it's a nightly-only feature so that can be done in a future PR.

allow_toggle: |target, enable| {
match &*target.llvm_abiname {
_ if !enable => {
// This is a negative feature, *disabling* it is always okay.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// This is a negative feature, *disabling* it is always okay.
// ilp32e can run on rv32i, treating x16-x31 as caller-save

Copy link
Member

@workingjubilee workingjubilee Dec 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

reason for explicit reasoning: "negative features" have a weird tendency to become "not" at some point (although there's reservations which suggest that shouldn't happen in this case), so having a clear justification for why is preferable in case we have to revisit it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately now the comment has so much jargon that I don't understand it...

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
Loading