forbid toggling x87 and fpregs on hard-float targets #133099

RalfJung · 2024-11-16T09:49:19Z

Part of #116344, follow-up to #129884:

The x87 target feature on x86 and the fpregs target feature on ARM must not be disabled on a hardfloat target, as that would change the float ABI. However, enabling fpregs on ARM is explicitly requested as it seems to be useful. Therefore, we need to refine the distinction of "forbidden" target features and "allowed" target features: all (un)stable target features can determine on a per-target basis whether they should be allowed to be toggled or not. fpregs then checks whether the current target has the soft-float feature, and if yes, fpregs is permitted -- otherwise, it is not. (Same for x87 on x86).

Also fixes #132351. Since fpregs and x87 can be enabled on some builds and disabled on others, it would make sense that one can query it via cfg. Therefore, I made them behave in cfg like any other unstable target feature.

The first commit prepares the infrastructure, but does not change behavior. The second commit then wires up fpregs and x87 with that new infrastructure.

r? @workingjubilee

rustbot · 2024-11-16T09:49:28Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

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

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

bors · 2024-11-28T14:15:52Z

☔ The latest upstream changes (presumably #133568) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2024-11-28T16:50:07Z

tests/ui/check-cfg/mix.stderr

@Urgau do we need these tests enumerating all target features? They make these kinds of PRs more conflict-heavy than they'd have to be, and they almost always add a "PR CI fails" roundtrip when adding new target features.

IMO it'd be better to normalize away the feature list so that the test output doesn't change just because we add a new feature.

The target features in tests/ui/check-cfg/mix.stderr only serves as a convenient cfg with many values so we test test the "and XXX more" part, we could normalize the output or replace it with another cfg.

As for the target_feature cfg values in tests/ui/check-cfg/well-known-values.stderr having the full list is on purpose, they serve as an anti-regression test, i.e. making sure that any modifications to the target features (adding one, removing one, modifying, ...) are intended and reflected in the well known target_feature list.

We could remove it, but we don't currently have any other test that would make sure that the list is coherent with what we should except and that makes me a bit worried that it will drift at some point.

For example in this PR you modified many of the logic in target_features.rs which is the source of truth for the well known target_feature values, with the test I can be confident that every users (through Cargo) of the target_feature cfg won't get a warning, which we might missed without the test.

making sure that any modifications to the target features (adding one, removing one, modifying, ...) are intended

One has to quite explicitly edit the target_features file for that. I don't recall a single incident where this happened accidentally before the addition of this test, so I don't think that's a relevant risk. The test being located in check-cfg also indicates its point is to test check-cfg, not to test the target feature tracking.

OTOH, the extra work caused by having to re-bless this test all the time is quite real.

we don't currently have any other test that would make sure that the list is coherent with what we should except and that makes me a bit worried that it will drift at some point.

The test generally gets blindly --blessed, and the list is way too long to be checked by hand. I don't see how this test can meaningfully check that the list is coherent.

And not to mention the conflicts -- we are generally trying hard to make it so that independent work can land independently, but this test ensures that any two PRs adding a target feature will necessarily conflict.

with the test I can be confident that every users (through Cargo) of the target_feature cfg won't get a warning, which we might missed without the test.

We need tests that target feature cfg are recognized obviously. But we don't want an exhaustive list of all target features in the test, IMO. Certainly not all in one line where it causes a conflict in 100% of the cases.

As Ralf said, it is effectively impossible to accidentally change the list of Rust-recognized target features. I don't find this test particularly relevant.

taiki-e · 2024-11-28T16:59:53Z

tests/ui/target-feature/forbidden-hardfloat-target-feature-attribute.rs

+#[target_feature(enable = "x87")]
+//~^ERROR: cannot be toggled with
+pub unsafe fn my_fun() {}


Why is this unsound? I thought enable = "x87" on cfg(target_feature = "x87") environment is no-op.

Yeah we could allow enabling x87 if it is already enabled. But I think that's wasted effort and a useless risk of getting the logic wrong.

Note that #[target_feature(enable = "x87")] already does not work on stable. So I'd rather wait for someone to show up with a concrete usecase for doing that, and not enable it "just because we can" as part of this PR which is meant to lock down what can be done in terms of target features.

workingjubilee · 2024-12-02T07:18:00Z

compiler/rustc_target/src/target_features.rs

+/// `Stability` where `allow_toggle` has not been computed yet.
+/// Returns `Ok` if the toggle is allowed, `Err` with an explanation of not.
+pub type StabilityUncomputed = Stability<fn(&Target) -> Result<(), &'static str>>;
+/// `Stability` where `allow_toggle` has already been computed.
+pub type StabilityComputed = Stability<Result<(), &'static str>>;


...is this typestate for a ~~lazylock~~ OnceCell?

workingjubilee

hmhm, this OnceCell-ish scheme results in a somewhat mysterious-to-read API.

compiler/rustc_target/src/target_features.rs

workingjubilee · 2024-12-02T07:27:15Z

compiler/rustc_target/src/target_features.rs

-    pub fn is_stable(self) -> bool {
-        matches!(self, Stable)
+impl StabilityUncomputed {
+    pub fn compute(self, target: &Target) -> StabilityComputed {


can this be, like, compute_toggleability?

🤷 sure. Seems like the type aliases should also have a different name then, but StabilityWithComputedToggleability is a bit too much IMO...

compiler/rustc_target/src/target_features.rs

compiler/rustc_codegen_llvm/src/llvm_util.rs

workingjubilee · 2024-12-02T07:48:39Z

I wonder if we ever need to answer the question about toggleability without having access to a &Target? It might be nice to just pass that unconditionally every time and simply answer from cache or not.

If that seems like it'd be worse, then some clearer phrasing will suffice.

RalfJung · 2024-12-02T09:51:46Z

hmhm, this OnceCell-ish scheme results in a somewhat mysterious-to-read API.

Hm, I didn't think of this like OnceCell at all. Note that there's no interior mutability.
Let me make an attempt at clarifying the semantics here:

The thing we want to represent is "when are we allowed to toggle a feature flag". This depends on the current target. We could have each target specify that property, but I'd rather have that logic centralized in one place where it can be more easily reviewed. So, the natural representation of this is a function that takes &Target. However, once we are in a compiler session (i.e., once we have tcx), the target is fixed, so instead of calling that function over and over again, we can just call it once on the actual current target and store the result.

IMO the natural representation for that is a type that is generic in which type is used to represent "can this be toggled", and instantiating it with either the "uncomputed" form (still a function) or the "computed" form (storing the result of the function).

Also, queries cannot return function pointers (they don't have a stable hash), and given that we have a query that returns information about target feature, we have to chose a different representation here -- the "fully computed" representation is a natural choice here since queries require a tcx so the target has been fixed by that time.

I can try to sprinkle more comments about this design in various places if you think that helps? I would appreciate some pointers for which parts you found most confusing so that this comment sprinkling is not entirely unguided. ;)

RalfJung · 2024-12-02T10:04:20Z

I wonder if we ever need to answer the question about toggleability without having access to a &Target?

We better don't as I don't think that's a valid question to ask. Toggleability is target-dependent.

It might be nice to just pass that unconditionally every time and simply answer from cache or not.

I don't understand this sentence, sorry. Pass what unconditionally? Cache things where? The query gets cached, but some of the target feature logic is unfortunately invoked before there is a tcx, so we can't always use that cache. I'd rather not add another global cache though...

workingjubilee · 2024-12-02T16:12:16Z

Oh no, I definitely didn't have a global cache in mind. Will try to cook up a better explanation of what I meant about things in a wee bit.

…cs, r=jieyouxu Reducing `target_feature` check-cfg merge conflicts It was rightfully pointed in rust-lang#133099 (comment) that the expected values for the `target_feature` cfg are regularly updated and unfortunately the check-cfg tests for it are very merge-conflict prone. This PR aims at drastically reducing the likely-hood of those, by normalizing the "and X more" diagnostic, as well as making the full expected list multi-line instead of being on a single one. cc `@RalfJung` r? `@jieyouxu`

Rollup merge of rust-lang#133710 - Urgau:target_feature-merge-conflitcs, r=jieyouxu Reducing `target_feature` check-cfg merge conflicts It was rightfully pointed in rust-lang#133099 (comment) that the expected values for the `target_feature` cfg are regularly updated and unfortunately the check-cfg tests for it are very merge-conflict prone. This PR aims at drastically reducing the likely-hood of those, by normalizing the "and X more" diagnostic, as well as making the full expected list multi-line instead of being on a single one. cc `@RalfJung` r? `@jieyouxu`

bors · 2024-12-03T04:09:09Z

☔ The latest upstream changes (presumably #133770) made this pull request unmergeable. Please resolve the merge conflicts.

workingjubilee · 2024-12-03T05:30:42Z

well, on the upside, you'll have to deal with that particular conflict less in the future

…e can be invalid to toggle Also rename some things for extra clarity

…youxu Reducing `target_feature` check-cfg merge conflicts It was rightfully pointed in rust-lang/rust#133099 (comment) that the expected values for the `target_feature` cfg are regularly updated and unfortunately the check-cfg tests for it are very merge-conflict prone. This PR aims at drastically reducing the likely-hood of those, by normalizing the "and X more" diagnostic, as well as making the full expected list multi-line instead of being on a single one. cc `@RalfJung` r? `@jieyouxu`

rustbot assigned workingjubilee Nov 16, 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 Nov 16, 2024

RalfJung force-pushed the forbidden-hardfloat-features branch from 64c138c to 4017676 Compare November 16, 2024 09:52

This comment has been minimized.

Sign in to view

RalfJung force-pushed the forbidden-hardfloat-features branch 2 times, most recently from 86932a6 to 35bdfe8 Compare November 16, 2024 10:47

This comment has been minimized.

Sign in to view

RalfJung force-pushed the forbidden-hardfloat-features branch from 35bdfe8 to 73bb2e9 Compare November 16, 2024 11:23

RalfJung changed the title ~~Forbidden hardfloat features~~ forbid toggling x87 and fpregs on hard-float targets Nov 16, 2024

This was referenced Nov 16, 2024

Forbid disabling SSE on x86 targets that have SSE in their "baseline" rust-lang/compiler-team#808

Closed

The (stable) neon aarch64 target feature is unsound: it changes the float ABI #131058

Open

RalfJung force-pushed the forbidden-hardfloat-features branch from 73bb2e9 to a43aacb Compare November 24, 2024 11:00

RalfJung mentioned this pull request Nov 24, 2024

reject aarch64 target feature toggling that would change the float ABI #133417

Open

RalfJung commented Nov 28, 2024

View reviewed changes

taiki-e reviewed Nov 28, 2024

View reviewed changes

RalfJung force-pushed the forbidden-hardfloat-features branch from a43aacb to 41ab0fc Compare November 28, 2024 21:27

RalfJung mentioned this pull request Nov 29, 2024

Forbid disabling SSE on x86 targets that have SSE in their "baseline" #133611

Open

Urgau mentioned this pull request Dec 1, 2024

Reducing target_feature check-cfg merge conflicts #133710

Merged

workingjubilee self-requested a review December 2, 2024 07:05

workingjubilee reviewed Dec 2, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the forbidden-hardfloat-features branch from 8a639d4 to bd53b3e Compare December 2, 2024 10:44

RalfJung added 3 commits December 3, 2024 07:33

generalize 'forbidden feature' concept so that even (un)stable featur…

59b7a46

…e can be invalid to toggle Also rename some things for extra clarity

forbid toggling x87 and fpregs on hard-float targets

fb00adb

apply review feedback

0cb8b2b

RalfJung force-pushed the forbidden-hardfloat-features branch from bd53b3e to 0cb8b2b Compare December 3, 2024 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

forbid toggling x87 and fpregs on hard-float targets #133099

forbid toggling x87 and fpregs on hard-float targets #133099

RalfJung commented Nov 16, 2024 •

edited by Noratrieb

Loading

rustbot commented Nov 16, 2024

This comment has been minimized.

This comment has been minimized.

bors commented Nov 28, 2024

RalfJung Nov 28, 2024

Urgau Nov 28, 2024

RalfJung Nov 28, 2024 •

edited

Loading

RalfJung Nov 28, 2024 •

edited

Loading

workingjubilee Nov 29, 2024 •

edited

Loading

taiki-e Nov 28, 2024

RalfJung Nov 28, 2024 •

edited

Loading

workingjubilee Dec 2, 2024 •

edited

Loading

workingjubilee left a comment

workingjubilee Dec 2, 2024

RalfJung Dec 2, 2024

workingjubilee commented Dec 2, 2024 •

edited

Loading

RalfJung commented Dec 2, 2024

RalfJung commented Dec 2, 2024 •

edited

Loading

This comment has been minimized.

workingjubilee commented Dec 2, 2024

bors commented Dec 3, 2024

workingjubilee commented Dec 3, 2024

forbid toggling x87 and fpregs on hard-float targets #133099

Are you sure you want to change the base?

forbid toggling x87 and fpregs on hard-float targets #133099

Conversation

RalfJung commented Nov 16, 2024 • edited by Noratrieb Loading

rustbot commented Nov 16, 2024

This comment has been minimized.

This comment has been minimized.

bors commented Nov 28, 2024

RalfJung Nov 28, 2024

Choose a reason for hiding this comment

Urgau Nov 28, 2024

Choose a reason for hiding this comment

RalfJung Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

workingjubilee Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

taiki-e Nov 28, 2024

Choose a reason for hiding this comment

RalfJung Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

workingjubilee Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee Dec 2, 2024

Choose a reason for hiding this comment

RalfJung Dec 2, 2024

Choose a reason for hiding this comment

workingjubilee commented Dec 2, 2024 • edited Loading

RalfJung commented Dec 2, 2024

RalfJung commented Dec 2, 2024 • edited Loading

This comment has been minimized.

workingjubilee commented Dec 2, 2024

bors commented Dec 3, 2024

workingjubilee commented Dec 3, 2024

RalfJung commented Nov 16, 2024 •

edited by Noratrieb

Loading

RalfJung Nov 28, 2024 •

edited

Loading

RalfJung Nov 28, 2024 •

edited

Loading

workingjubilee Nov 29, 2024 •

edited

Loading

RalfJung Nov 28, 2024 •

edited

Loading

workingjubilee Dec 2, 2024 •

edited

Loading

workingjubilee commented Dec 2, 2024 •

edited

Loading

RalfJung commented Dec 2, 2024 •

edited

Loading