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

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

compiler/rustc_interface/src/util.rs

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

compiler/rustc_codegen_gcc/src/gcc_util.rs

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

compiler/rustc_codegen_llvm/src/llvm_util.rs

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

tests/ui/target-feature/feature-hierarchy.aarch64-sve2.stderr

@@ -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``

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: make SSE2 required for i686 targets and use it to pass SIMD types #135408

Draft

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

Open

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
6 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RalfJung commented Jan 27, 2025 •

edited

Loading

rustbot commented Jan 27, 2025

rustbot commented Jan 27, 2025

This comment has been minimized.

rustbot commented Jan 27, 2025

This comment has been minimized.

rustbot commented Jan 28, 2025

This comment has been minimized.

workingjubilee left a comment

workingjubilee Jan 28, 2025

RalfJung Jan 28, 2025

workingjubilee Jan 28, 2025 •

edited

Loading

workingjubilee Jan 28, 2025

workingjubilee Jan 28, 2025

workingjubilee commented Jan 28, 2025

RalfJung commented Jan 28, 2025 •

edited

Loading

RalfJung Jan 28, 2025

workingjubilee commented Jan 28, 2025

workingjubilee commented Jan 28, 2025

bors commented Jan 28, 2025

RalfJung commented Jan 28, 2025

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

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

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

Conversation

RalfJung commented Jan 27, 2025 • edited Loading

rustbot commented Jan 27, 2025

rustbot commented Jan 27, 2025

This comment has been minimized.

rustbot commented Jan 27, 2025

This comment has been minimized.

rustbot commented Jan 28, 2025

This comment has been minimized.

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee Jan 28, 2025

Choose a reason for hiding this comment

RalfJung Jan 28, 2025

Choose a reason for hiding this comment

workingjubilee Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

workingjubilee Jan 28, 2025

Choose a reason for hiding this comment

workingjubilee Jan 28, 2025

Choose a reason for hiding this comment

workingjubilee commented Jan 28, 2025

RalfJung commented Jan 28, 2025 • edited Loading

RalfJung Jan 28, 2025

Choose a reason for hiding this comment

workingjubilee commented Jan 28, 2025

workingjubilee commented Jan 28, 2025

bors commented Jan 28, 2025

RalfJung commented Jan 28, 2025

RalfJung commented Jan 27, 2025 •

edited

Loading

workingjubilee Jan 28, 2025 •

edited

Loading

RalfJung commented Jan 28, 2025 •

edited

Loading