ABI-required target features: warn when they are missing in base CPU by RalfJung · Pull Request #136147 · rust-lang/rust

RalfJung · 2025-01-27T17:37:49Z

Part of #135408:
instead of adding ABI-required features to the target we build for LLVM, check that they are already there. Crucially we check this after applying -Ctarget-cpu and -Ctarget-feature, by reading sess.unstable_target_features. This means we can tweak the ABI target feature check without changing the behavior for any existing user; they will get warnings but the target features behave as before.

The test changes here show that we are un-doing the "add all required target features" part. Without the full #135408, there is no way to take a way an ABI-required target feature with -Ctarget-cpu, so we cannot yet test that part.

Cc @workingjubilee

rustbot · 2025-01-27T17:38:00Z

r? @oli-obk

rustbot has assigned @oli-obk.
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 · 2025-01-27T17:38:01Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

rustbot · 2025-01-27T19:10:48Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

rustbot · 2025-01-28T01:43:51Z

The Miri subtree was changed

cc @rust-lang/miri

…(rather than silently enabling them)

workingjubilee

This looks correct.

One question though, who are we adding new warnings to, exactly?

workingjubilee · 2025-01-28T07:24:50Z

+/// Ensures that all target features required by the ABI are present.
+/// Must be called after `unstable_target_features` has been populated!


I do not love ordering sensitivity, but it is tolerable in small amounts, and it seems we are localizing all our various similarly-shaped checks to this one spot, right?

This is now part of early session bringup, which is quite order-dependent. I didn't have any better idea for where to put it... I could put it somewhere in run_compiler?

Then I think we should worry about fixing this after we fix the ordering dependency of session bringup (even if just to make it more state-machine-y)

workingjubilee · 2025-01-28T07:24:58Z

-            // Ensure that the features we enable/disable are compatible with the ABI.
-            if enable {
-                if abi_incompatible_set.contains(feature) {
-                    sess.dcx().emit_warn(ForbiddenCTargetFeature {
-                        feature,
-                        enabled: "enabled",
-                        reason: "this feature is incompatible with the target ABI",
-                    });
-                }
-            } else {
-                // FIXME: we have to request implied features here since
-                // negative features do not handle implied features above.
-                for &required in abi_feature_constraints.required.iter() {
-                    let implied = sess.target.implied_target_features(std::iter::once(required));
-                    if implied.contains(feature) {
-                        sess.dcx().emit_warn(ForbiddenCTargetFeature {
-                            feature,
-                            enabled: "disabled",
-                            reason: "this feature is required by the target ABI",
-                        });
-                    }
-                }
-            }
-


workingjubilee · 2025-01-28T07:25:24Z

-        // To be sure the ABI-relevant features are all in the right state, we explicitly
-        // (un)set them here. This means if the target spec sets those features wrong,
-        // we will silently correct them rather than silently producing wrong code.
-        // (The target sanity check tries to catch this, but we can't know which features are
-        // enabled in LLVM by default so we can't be fully sure about that check.)
-        // We add these at the beginning of the list so that `-Ctarget-features` can
-        // still override it... that's unsound, but more compatible with past behavior.
-        all_rust_features.splice(
-            0..0,
-            abi_feature_constraints
-                .required
-                .iter()
-                .map(|&f| (true, f))
-                .chain(abi_feature_constraints.incompatible.iter().map(|&f| (false, f))),
-        );
-


love to see it

workingjubilee · 2025-01-28T07:32:06Z

and to be clear: if the answer to my question is "currently, no one" then r=me otherwise I'll think about it after I sleep on it.

RalfJung · 2025-01-28T10:17:35Z

One question though, who are we adding new warnings to, exactly?

If an ABI-required feature is missing from the base target spec (or an ABI-incompatible feature is present), this would now warn rather than just enabling the feature anyway.

All built-in targets should already be setting the right target features, so there's no new warnings here. (Ideally we'd have a test asserting this for all our targets, but that seems non-trivial.)

RalfJung · 2025-01-28T12:07:10Z

@@ -1,7 +0,0 @@
-warning: target feature `neon` cannot be disabled with `-Ctarget-feature`: this feature is required by the target ABI


This warning disappears because sve2 implies neon, so -neon,+sve2 actually doesn't have adverse ABI effects.

workingjubilee · 2025-01-28T18:05:23Z

If an ABI-required feature is missing from the base target spec (or an ABI-incompatible feature is present), this would now warn rather than just enabling the feature anyway.

Okay, so basically this is only shipping warnings to custom specs, or preventing us from shipping a broken spec? Sounds good to me, then.

workingjubilee · 2025-01-28T18:05:37Z

@bors r+

bors · 2025-01-28T18:05:40Z

📌 Commit 3f6ffa1 has been approved by workingjubilee

It is now in the queue for this repository.

RalfJung · 2025-01-28T18:12:35Z

The specific form in which it prevents us from shipping a broken spec is "it shows a warning and we'd see that in CI (for the major targets) / someone would surely complain". It's not great, but it's a start.

@bors p=1
(there's a follow-up PR waiting for this one)

…check-not-add, r=workingjubilee ABI-required target features: warn when they are missing in base CPU Part of rust-lang#135408: instead of adding ABI-required features to the target we build for LLVM, check that they are already there. Crucially we check this after applying `-Ctarget-cpu` and `-Ctarget-feature`, by reading `sess.unstable_target_features`. This means we can tweak the ABI target feature check without changing the behavior for any existing user; they will get warnings but the target features behave as before. The test changes here show that we are un-doing the "add all required target features" part. Without the full rust-lang#135408, there is no way to take a way an ABI-required target feature with `-Ctarget-cpu`, so we cannot yet test that part. Cc `@workingjubilee`

Rollup of 7 pull requests Successful merges: - rust-lang#135625 ([cfg_match] Document the use of expressions.) - rust-lang#135902 (Do not consider child bound assumptions for rigid alias) - rust-lang#135943 (Rename `Piece::String` to `Piece::Lit`) - rust-lang#136104 (Add mermaid graphs of NLL regions and SCCs to polonius MIR dump) - rust-lang#136143 (Update books) - rust-lang#136147 (ABI-required target features: warn when they are missing in base CPU) - rust-lang#136164 (Refactor FnKind variant to hold &Fn) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#136147 - RalfJung:required-target-features-check-not-add, r=workingjubilee ABI-required target features: warn when they are missing in base CPU Part of rust-lang#135408: instead of adding ABI-required features to the target we build for LLVM, check that they are already there. Crucially we check this after applying `-Ctarget-cpu` and `-Ctarget-feature`, by reading `sess.unstable_target_features`. This means we can tweak the ABI target feature check without changing the behavior for any existing user; they will get warnings but the target features behave as before. The test changes here show that we are un-doing the "add all required target features" part. Without the full rust-lang#135408, there is no way to take a way an ABI-required target feature with `-Ctarget-cpu`, so we cannot yet test that part. Cc ``@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 oli-obk Jan 27, 2025

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 Jan 27, 2025

RalfJung mentioned this pull request Jan 27, 2025

x86: use SSE2 to pass float and SIMD types #135408

Merged

This comment has been minimized.

Sign in to view

RalfJung force-pushed the required-target-features-check-not-add branch from e40d565 to 968974d Compare January 27, 2025 19:10

This comment has been minimized.

Sign in to view

RalfJung force-pushed the required-target-features-check-not-add branch from 27885ad to b793ec0 Compare January 28, 2025 01:43

This comment has been minimized.

Sign in to view

RalfJung force-pushed the required-target-features-check-not-add branch 2 times, most recently from 4188395 to d63382d Compare January 28, 2025 02:35

RalfJung mentioned this pull request Jan 28, 2025

Explicitly choose x86 softfloat/hardfloat ABI #136146

Merged

RalfJung added 2 commits January 28, 2025 04:40

ABI-required target features: warn when they are missing in base CPU …

93ee180

…(rather than silently enabling them)

update comments

3f6ffa1

RalfJung force-pushed the required-target-features-check-not-add branch from d63382d to 3f6ffa1 Compare January 28, 2025 03:40

workingjubilee self-assigned this Jan 28, 2025

workingjubilee approved these changes Jan 28, 2025

View reviewed changes

RalfJung commented Jan 28, 2025

View reviewed changes

oli-obk removed their assignment Jan 28, 2025

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 Jan 28, 2025

RalfJung mentioned this pull request Jan 28, 2025

Add a test ensuring all our targets set the features required by their ABI. #136208

Open

Zalathar mentioned this pull request Jan 29, 2025

Rollup of 8 pull requests #136222

Closed

fmease mentioned this pull request Jan 29, 2025

Rollup of 7 pull requests #136225

Merged

bors merged commit 7e123e4 into rust-lang:master Jan 29, 2025

rustbot added this to the 1.86.0 milestone Jan 29, 2025

RalfJung deleted the required-target-features-check-not-add branch January 29, 2025 10:13

RalfJung mentioned this pull request Aug 18, 2026

make target feature ABI check a hard error on ARM #161280

Merged

		/// Ensures that all target features required by the ABI are present.
		/// Must be called after `unstable_target_features` has been populated!

		@@ -1,7 +0,0 @@
		warning: target feature `neon` cannot be disabled with `-Ctarget-feature`: this feature is required by the target ABI

Uh oh!

Uh oh!

Conversation

RalfJung commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jan 27, 2025

Uh oh!

rustbot commented Jan 27, 2025

Uh oh!

This comment has been minimized.

rustbot commented Jan 27, 2025

Uh oh!

This comment has been minimized.

rustbot commented Jan 28, 2025

Uh oh!

This comment has been minimized.

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

RalfJung Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee commented Jan 28, 2025

Uh oh!

RalfJung commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee commented Jan 28, 2025

Uh oh!

workingjubilee commented Jan 28, 2025

Uh oh!

bors commented Jan 28, 2025

Uh oh!

RalfJung commented Jan 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RalfJung commented Jan 27, 2025 •

edited

Loading

workingjubilee Jan 28, 2025 •

edited

Loading

RalfJung commented Jan 28, 2025 •

edited

Loading