reject unsound toggling of RISCV target features #134337

RalfJung · 2024-12-15T11:12:20Z

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

Works towards #132618 (but more remains to be done)
Part of #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.

rustbot · 2024-12-15T11:12:29Z

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

compiler/rustc_target/src/spec/mod.rs

workingjubilee · 2024-12-15T22:21:51Z

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.

@RalfJung So, "Zfinx" is literally

f: "do float operations like those of f0-f31"
in
x: "integer registers x0-x31"

Basically, it's a dedicated extension for "I can't believe it's not soft float!" Hardware that has it must use a soft-float ABI, since it has no float registers, and this means it cannot implement a float ABI.

...but as long as we require that "ilp32f" means you implement "f" then we're fine.

workingjubilee · 2024-12-15T22:55:01Z

compiler/rustc_target/src/target_features.rs

+                    Err("feature is required by ABI")
+                }
+                "ilp32e" if enable => {
+                    // The `d` feature apparently is incompatible with this ABI.


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`

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

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

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.

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

ILP32E and Variadic Arguments riscv-non-isa/riscv-elf-psabi-doc#125

Okay, makes sense.

workingjubilee · 2024-12-15T22:56:50Z

compiler/rustc_target/src/target_features.rs

+            // 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,


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

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.

workingjubilee · 2024-12-15T22:59:46Z

compiler/rustc_target/src/target_features.rs

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


Suggested change

// This is a negative feature, *disabling* it is always okay.

// ilp32e can run on rv32i, treating x16-x31 as caller-save

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.

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

workingjubilee

r=me with comment adjustments or not

beetrees · 2024-12-16T00:21:25Z

Can you check this correctly warns when using -Ctarget-feature=-zicsr with one of the hard-float ABIs (zicsr is required by the f extension, so disabling it automatically disabled the f extension)?

RalfJung · 2024-12-16T06:36:34Z

compiler/rustc_target/src/target_features.rs

+        Stability::Unstable {
+            nightly_feature: sym::riscv_target_feature,
+            allow_toggle: |target, enable| match &*target.llvm_abiname {
+                "ilp32d" | "lp64d" if !enable => {


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

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)

So it should really be I32LP64 then...

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.

RalfJung · 2024-12-16T06:49:35Z

r=me with comment adjustments or not

I attempted to make the comments understandable by people who are not RISCV experts; could you take a look?

RalfJung · 2024-12-16T06:50:52Z

Can you check this correctly warns when using -Ctarget-feature=-zicsr with one of the hard-float ABIs (zicsr is required by the f extension, so disabling it automatically disabled the f extension)?

It will definitely not warn. We don't even have that feature in the known feature list.

workingjubilee · 2024-12-16T10:00:50Z

Yes, those are fine~

RalfJung · 2024-12-16T15:20:41Z

Yes, those are fine~

@bors r=workingjubilee
(I hope I interpreted you correctly.)

bors · 2024-12-16T15:20:44Z

📌 Commit 934ed85 has been approved by workingjubilee

It is now in the queue for this repository.

…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.

…llaumeGomez Rollup of 9 pull requests Successful merges: - rust-lang#132056 (Stabilize `#[diagnostic::do_not_recommend]`) - rust-lang#134124 (CI: use free runners for x86_64-gnu-llvm jobs) - rust-lang#134197 (rustc_mir_build: Clarify that 'mirrored' does not mean 'flipped' or 'reversed') - rust-lang#134260 (Correctly handle comments in attributes in doctests source code) - rust-lang#134277 (rustdoc-search: handle `impl Into<X>` better) - rust-lang#134284 (Keep track of patterns that could have introduced a binding, but didn't) - rust-lang#134337 (reject unsound toggling of RISCV target features) - rust-lang#134385 (tests/ui/asm: Remove uses of rustc_attrs, lang_items, and decl_macro features by using minicore) - rust-lang#134386 (Some trait method vs impl method signature difference diagnostic cleanups) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#134124 (CI: use free runners for x86_64-gnu-llvm jobs) - rust-lang#134197 (rustc_mir_build: Clarify that 'mirrored' does not mean 'flipped' or 'reversed') - rust-lang#134260 (Correctly handle comments in attributes in doctests source code) - rust-lang#134277 (rustdoc-search: handle `impl Into<X>` better) - rust-lang#134284 (Keep track of patterns that could have introduced a binding, but didn't) - rust-lang#134337 (reject unsound toggling of RISCV target features) - rust-lang#134371 (Check for array lengths that aren't actually `usize`) - rust-lang#134385 (tests/ui/asm: Remove uses of rustc_attrs, lang_items, and decl_macro features by using minicore) - rust-lang#134386 (Some trait method vs impl method signature difference diagnostic cleanups) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134337 - RalfJung:riscv-target-features, r=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.

rustbot assigned workingjubilee Dec 15, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 15, 2024

RalfJung force-pushed the riscv-target-features branch from 6111eda to 5dc6e55 Compare December 15, 2024 11:13

RalfJung commented Dec 15, 2024

View reviewed changes

compiler/rustc_target/src/spec/mod.rs Show resolved Hide resolved

RalfJung force-pushed the riscv-target-features branch from 5dc6e55 to 270c9fa Compare December 15, 2024 11:18

reject unsound toggling of RISCV target features

171223e

RalfJung force-pushed the riscv-target-features branch from 270c9fa to 171223e Compare December 15, 2024 19:33

workingjubilee reviewed Dec 15, 2024

View reviewed changes

workingjubilee approved these changes Dec 15, 2024

View reviewed changes

RalfJung commented Dec 16, 2024

View reviewed changes

RalfJung mentioned this pull request Dec 16, 2024

Some -Ctarget-features must be restrained on RISCV #132618

Open

tweak comments

934ed85

RalfJung force-pushed the riscv-target-features branch from 1551bf5 to 934ed85 Compare December 16, 2024 06:56

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2024

GuillaumeGomez mentioned this pull request Dec 16, 2024

Rollup of 9 pull requests #134393

Closed

matthiaskrgr mentioned this pull request Dec 16, 2024

Rollup of 9 pull requests #134395

Merged

bors merged commit dffaad8 into rust-lang:master Dec 16, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 16, 2024

RalfJung deleted the riscv-target-features branch December 17, 2024 06:33

RalfJung mentioned this pull request Dec 26, 2024

Add a notion of "some ABIs require certain target features" #134794

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reject unsound toggling of RISCV target features #134337

reject unsound toggling of RISCV target features #134337

RalfJung commented Dec 15, 2024 •

edited

Loading

rustbot commented Dec 15, 2024

workingjubilee commented Dec 15, 2024

workingjubilee Dec 15, 2024 •

edited

Loading

RalfJung Dec 16, 2024

beetrees Dec 16, 2024

workingjubilee Dec 16, 2024

workingjubilee Dec 16, 2024 •

edited

Loading

RalfJung Dec 16, 2024

workingjubilee Dec 15, 2024

RalfJung Dec 16, 2024

workingjubilee Dec 15, 2024

workingjubilee Dec 15, 2024 •

edited

Loading

RalfJung Dec 16, 2024

workingjubilee left a comment

beetrees commented Dec 16, 2024 •

edited

Loading

RalfJung Dec 16, 2024

taiki-e Dec 16, 2024

RalfJung Dec 16, 2024

workingjubilee Dec 16, 2024

RalfJung commented Dec 16, 2024

RalfJung commented Dec 16, 2024

workingjubilee commented Dec 16, 2024

RalfJung commented Dec 16, 2024

bors commented Dec 16, 2024

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

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

reject unsound toggling of RISCV target features #134337

reject unsound toggling of RISCV target features #134337

Conversation

RalfJung commented Dec 15, 2024 • edited Loading

rustbot commented Dec 15, 2024

workingjubilee commented Dec 15, 2024

workingjubilee Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee left a comment

Choose a reason for hiding this comment

beetrees commented Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Dec 16, 2024

RalfJung commented Dec 16, 2024

workingjubilee commented Dec 16, 2024

RalfJung commented Dec 16, 2024

bors commented Dec 16, 2024

RalfJung commented Dec 15, 2024 •

edited

Loading

workingjubilee Dec 15, 2024 •

edited

Loading

workingjubilee Dec 16, 2024 •

edited

Loading

workingjubilee Dec 15, 2024 •

edited

Loading

beetrees commented Dec 16, 2024 •

edited

Loading