Add a notion of "some ABIs require certain target features" by RalfJung · Pull Request #134794 · rust-lang/rust

RalfJung · 2024-12-26T18:23:53Z

I think I finally found the right shape for the data and checks that I recently added in #133099, #133417, #134337: we have a notion of "this ABI requires the following list of target features, and it is incompatible with the following list of target features". Both -Ctarget-feature and #[target_feature] are updated to ensure we follow the rules of the ABI. This removes all the "toggleability" stuff introduced before, though we do keep the notion of a fully "forbidden" target feature -- this is needed to deal with target features that are actual ABI switches, and hence are needed to even compute the list of required target features.

We always explicitly (un)set all required and in-conflict features, just to avoid potential trouble caused by the default features of whatever the base CPU is. We do this before applying -Ctarget-feature to maintain backward compatibility; this poses a slight risk of missing some implicit feature dependencies in LLVM but has the advantage of not breaking users that deliberately toggle ABI-relevant target features. They get a warning but the feature does get toggled the way they requested.

For now, our logic supports x86, ARM, and RISC-V (just like the previous logic did). Unsurprisingly, RISC-V is the nicest. ;)

As a side-effect this also (unstably) allows enabling x87 when that is harmless. I used the opportunity to mark SSE2 as required on x86-64, to better match the actual logic in LLVM and because all x86-64 chips do have SSE2. This infrastructure also prepares us for requiring SSE on x86-32 when we want to use that for our ABI (and for float semantics sanity), see #133611, but no such change is happening in this PR.

r? @workingjubilee

rustbot · 2024-12-26T18:23:56Z

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

rustbot · 2024-12-26T18:24:01Z

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-12-26T18:24:03Z

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

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

workingjubilee · 2024-12-26T22:13:08Z

I will get around to this, I just put a freeze on my queue because I was accumulating enough reviews in addition to ongoing patch series. :P

taiki-e · 2024-12-27T01:28:07Z

+                        // Embedded ABI, requires e.
+                        (&["e"], &[])
+                    }
+                    "ilp32" | "lp64" => NOTHING,


Suggested change

"ilp32" | "lp64" => NOTHING,

"ilp32" | "lp64" => {

// Incompatible with e.

(&[], &["e"])

}

Ah right the old logic treated all the non-e ABIs the same, we should keep that.

The old logic also allowed disabling e when using an e ABI, so we should probably keep that, too.

RalfJung · 2024-12-27T14:57:49Z

@ojeda wrote

(currently, arm64 in the kernel uses --target=aarch64-unknown-none -Ctarget-feature="-neon").

That no longer works after this PR (neon is a required ABI feature so it cannot be disabled with -Ctarget-feature)... so I probably need to at least slightly adjust my strategy here.

RalfJung · 2024-12-27T20:40:04Z

+                // We support 2 ABIs, hardfloat (default) and softfloat.
+                if self.has_feature("soft-float") {
+                    NOTHING


It seems like this might be wrong, and we should be checking self.abi instead... but I am not sure yet.

RalfJung · 2024-12-27T20:46:04Z

+        for feature in abi_enable {
+            all_rust_features.push((true, feature));
+        }
+        for feature in abi_disable {
+            all_rust_features.push((false, feature));
+        }


This is the part where we explicitly set the ABI-relevant features. The concern here is on the one hand not knowing the default target features, and also not being entirely sure that we correctly model all "implied features" on the LLVM side.

Currently this adds the ABI-relevant features after the ones from -Ctarget-features, i.e. we are overriding the user here. That's a massive change in behavior. I feel I should probably swap the order, but then we are not protected against unexpected feature implications any more. Still, we probably want the target modifier story to be more solidified before we do any behavior changes here.

RalfJung · 2024-12-27T21:32:17Z

That no longer works after this PR (neon is a required ABI feature so it cannot be disabled with -Ctarget-feature)... so I probably need to at least slightly adjust my strategy here.

I changed this to add the ABI features before the user-defined features, so things will behave as before with -Ctarget-feature=-neon on ARM targets. However, a warning is shown since this is not a supported combination -- long-term we want to forbid disabling neon on targets that need float registers for their ABI.

tgross35 · 2024-12-27T23:02:57Z

I changed this to add the ABI features before the user-defined features, so things will behave as before with -Ctarget-feature=-neon on ARM targets. However, a warning is shown since this is not a supported combination -- long-term we want to forbid disabling neon on targets that need float registers for their ABI.

Could we instead add a -nofloat version of these targets? Or provide a compiler way to disable all use of float types with a build-std config to go with it.

RalfJung · 2024-12-31T11:48:24Z

Rebased now that #134932 landed; the ARM code here now is very clean. We don't even have to mark soft-float as a "forbidden feature" any more. X86 is okay from a soundness perspective (by refusing to ever toggle the soft-float feature); aarch64 is a sad mess that I think we can only fix by refusing to toggle neon on our softfloat target -- this PR doesn't do that as I want this PR to not be a breaking change.

workingjubilee

thank you! this does look a lot nicer overall. only a small nit or three (as most of the "review" of "how even to proceed?" happened on Zulip already)

workingjubilee · 2025-01-05T21:15:56Z

Wonderful! @bors r+

bors · 2025-01-05T21:15:59Z

📌 Commit 2e64b53 has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2025-01-05T23:21:09Z

⌛ Testing commit 2e64b53 with merge feb32c6...

bors · 2025-01-06T02:04:56Z

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing feb32c6 to master...

rust-timer · 2025-01-06T03:22:01Z

Finished benchmarking commit (feb32c6): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -1.7%, secondary 0.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.3%	[0.5%, 2.7%]	6
Improvements ✅ (primary)	-1.7%	[-2.3%, -1.2%]	8
Improvements ✅ (secondary)	-1.7%	[-1.7%, -1.7%]	2
All ❌✅ (primary)	-1.7%	[-2.3%, -1.2%]	8

Cycles

Results (secondary 1.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	4
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	4

Bootstrap: 763.545s -> 762.625s (-0.12%)
Artifact size: 325.56 MiB -> 325.60 MiB (0.01%)

…s, r=workingjubilee Add a notion of "some ABIs require certain target features" I think I finally found the right shape for the data and checks that I recently added in rust-lang#133099, rust-lang#133417, rust-lang#134337: we have a notion of "this ABI requires the following list of target features, and it is incompatible with the following list of target features". Both `-Ctarget-feature` and `#[target_feature]` are updated to ensure we follow the rules of the ABI. This removes all the "toggleability" stuff introduced before, though we do keep the notion of a fully "forbidden" target feature -- this is needed to deal with target features that are actual ABI switches, and hence are needed to even compute the list of required target features. We always explicitly (un)set all required and in-conflict features, just to avoid potential trouble caused by the default features of whatever the base CPU is. We do this *before* applying `-Ctarget-feature` to maintain backward compatibility; this poses a slight risk of missing some implicit feature dependencies in LLVM but has the advantage of not breaking users that deliberately toggle ABI-relevant target features. They get a warning but the feature does get toggled the way they requested. For now, our logic supports x86, ARM, and RISC-V (just like the previous logic did). Unsurprisingly, RISC-V is the nicest. ;) As a side-effect this also (unstably) allows *enabling* `x87` when that is harmless. I used the opportunity to mark SSE2 as required on x86-64, to better match the actual logic in LLVM and because all x86-64 chips do have SSE2. This infrastructure also prepares us for requiring SSE on x86-32 when we want to use that for our ABI (and for float semantics sanity), see rust-lang#133611, but no such change is happening in this PR. r? `@workingjubilee`

@davidtwco

…re-arm, r=davidtwco make target feature ABI check a hard error on ARM With LLVM 24, we now (finally) get hard errors from the backend for "you want ABI X but the target features required for that ABI are missing", at least on the ARM backend: ``` error: <unknown>:0:0: in function _RNvXsc_Cs4Af2OiBEA1T_8minicoreiNtB5_3Add3add i32 (i32, i32): calling convention is hard-float, but floating-point registers are unavailable ``` That's not a pretty error, so let's make rust emit a hard error before we even get there -- just on ARM for now, matching LLVM. We have emitted a future-compat error for this since Rust 1.86 (rust-lang#136147, rust-lang#134794). I'm not making it a hard error everywhere since for other targets we are still figuring out the exact things we have to check, and it's better to do that without risking new hard errors. For ARM, the exact check we are doing is: - every target must set `llvm_floatabi` to either "soft" or "hard" - on softfloat targets, no check is needed - on hardfloat targets, the "fpregs" target feature is *required* and the the "soft-float" target feature is *forbidden* Since this is a new hard error, this probably requires FCP. I am not sure for which team -- @rust-lang/lang is often involved for target feature things, but this is mostly about rejecting invalid `-C` flags which are handled by t-compiler (@davidtwco @BoxyUwU). Fixes rust-lang#161276 Tracking issue rust-lang#116344 Cc @TimNN

@davidtwco

…re-arm, r=davidtwco make target feature ABI check a hard error on ARM With LLVM 24, we now (finally) get hard errors from the backend for "you want ABI X but the target features required for that ABI are missing", at least on the ARM backend: ``` error: <unknown>:0:0: in function _RNvXsc_Cs4Af2OiBEA1T_8minicoreiNtB5_3Add3add i32 (i32, i32): calling convention is hard-float, but floating-point registers are unavailable ``` That's not a pretty error, so let's make rust emit a hard error before we even get there -- just on ARM for now, matching LLVM. We have emitted a future-compat error for this since Rust 1.86 (rust-lang#136147, rust-lang#134794). I'm not making it a hard error everywhere since for other targets we are still figuring out the exact things we have to check, and it's better to do that without risking new hard errors. For ARM, the exact check we are doing is: - every target must set `llvm_floatabi` to either "soft" or "hard" - on softfloat targets, no check is needed - on hardfloat targets, the "fpregs" target feature is *required* and the the "soft-float" target feature is *forbidden* Since this is a new hard error, this probably requires FCP. I am not sure for which team -- @rust-lang/lang is often involved for target feature things, but this is mostly about rejecting invalid `-C` flags which are handled by t-compiler (@davidtwco @BoxyUwU). Fixes rust-lang#161276 Tracking issue rust-lang#116344 Cc @TimNN

@davidtwco

…re-arm, r=davidtwco make target feature ABI check a hard error on ARM With LLVM 24, we now (finally) get hard errors from the backend for "you want ABI X but the target features required for that ABI are missing", at least on the ARM backend: ``` error: <unknown>:0:0: in function _RNvXsc_Cs4Af2OiBEA1T_8minicoreiNtB5_3Add3add i32 (i32, i32): calling convention is hard-float, but floating-point registers are unavailable ``` That's not a pretty error, so let's make rust emit a hard error before we even get there -- just on ARM for now, matching LLVM. We have emitted a future-compat error for this since Rust 1.86 (rust-lang#136147, rust-lang#134794). I'm not making it a hard error everywhere since for other targets we are still figuring out the exact things we have to check, and it's better to do that without risking new hard errors. For ARM, the exact check we are doing is: - every target must set `llvm_floatabi` to either "soft" or "hard" - on softfloat targets, no check is needed - on hardfloat targets, the "fpregs" target feature is *required* and the the "soft-float" target feature is *forbidden* Since this is a new hard error, this probably requires FCP. I am not sure for which team -- @rust-lang/lang is often involved for target feature things, but this is mostly about rejecting invalid `-C` flags which are handled by t-compiler (@davidtwco @BoxyUwU). Fixes rust-lang#161276 Tracking issue rust-lang#116344 Cc @TimNN

@davidtwco

Rollup merge of #161280 - RalfJung:abi-required-target-feature-arm, r=davidtwco make target feature ABI check a hard error on ARM With LLVM 24, we now (finally) get hard errors from the backend for "you want ABI X but the target features required for that ABI are missing", at least on the ARM backend: ``` error: <unknown>:0:0: in function _RNvXsc_Cs4Af2OiBEA1T_8minicoreiNtB5_3Add3add i32 (i32, i32): calling convention is hard-float, but floating-point registers are unavailable ``` That's not a pretty error, so let's make rust emit a hard error before we even get there -- just on ARM for now, matching LLVM. We have emitted a future-compat error for this since Rust 1.86 (#136147, #134794). I'm not making it a hard error everywhere since for other targets we are still figuring out the exact things we have to check, and it's better to do that without risking new hard errors. For ARM, the exact check we are doing is: - every target must set `llvm_floatabi` to either "soft" or "hard" - on softfloat targets, no check is needed - on hardfloat targets, the "fpregs" target feature is *required* and the the "soft-float" target feature is *forbidden* Since this is a new hard error, this probably requires FCP. I am not sure for which team -- @rust-lang/lang is often involved for target feature things, but this is mostly about rejecting invalid `-C` flags which are handled by t-compiler (@davidtwco @BoxyUwU). Fixes #161276 Tracking issue #116344 Cc @TimNN

rustbot assigned davidtwco Dec 26, 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 26, 2024

RalfJung mentioned this pull request Dec 26, 2024

Figure out which target features are required/incompatible for which ABI #131799

Open

17 tasks