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

Yeah I think we can leave the nouns as-is, I just want this particular verb to be a bit clearer.

It's renamed in the latest diff.

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.

bors · 2024-12-11T08:24:46Z

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

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

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

RalfJung · 2024-12-11T21:13:59Z

Something is odd with the tests/ui/check-cfg/target_feature.stderr file, git doesn't resolve conflicts in there properly as it says "Cannot merge binary files". @Urgau any idea what is up with that?

RalfJung · 2024-12-11T21:15:10Z

Ah, we have this in .gitattributes:

*.stderr -merge

That means that conflicts due to this check-cfg test are still painful. :(

Urgau · 2024-12-11T21:53:26Z

Ah, we have this in .gitattributes

😩 ; seems to be a consequence of the quite recent #133851 (cc @oli-obk)

idk if it's possible but we may want to add a special .gitattributes under tests/ui/check-cfg/ to override that default

oli-obk · 2024-12-11T22:09:49Z

Ah... damn... my goal was to avoid conflict markers, but conflict free merges should be fine. Let's revert that

RalfJung · 2024-12-12T06:22:07Z

Agreed: #134199

workingjubilee · 2024-12-12T11:33:49Z

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

Hmm. I reread everything and I understand better now. Apologies for the delay. With the verbiage change and trying to absorb it again apparently something crossed a threshold and it clicked.

@bors r+

bors · 2024-12-12T11:33:52Z

📌 Commit 60eca2c has been approved by workingjubilee

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#130060 (Autodiff Upstreaming - rustc_codegen_llvm changes) - rust-lang#132038 (Add lint rule for `#[deprecated]` on re-exports) - rust-lang#133937 (Keep track of parse errors in `mod`s and don't emit resolve errors for paths involving them) - rust-lang#133942 (Clarify how to use `black_box()`) - rust-lang#134081 (Try to evaluate constants in legacy mangling) - rust-lang#134192 (Remove `Lexer`'s dependency on `Parser`.) - rust-lang#134209 (validate `--skip` and `--exclude` paths) Failed merges: - rust-lang#133099 (forbid toggling x87 and fpregs on hard-float targets) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 9 pull requests Successful merges: - rust-lang#130060 (Autodiff Upstreaming - rustc_codegen_llvm changes) - rust-lang#132038 (Add lint rule for `#[deprecated]` on re-exports) - rust-lang#132150 (Fix powerpc64 big-endian FreeBSD ABI) - rust-lang#133633 (don't show the full linker args unless `--verbose` is passed) - rust-lang#133942 (Clarify how to use `black_box()`) - rust-lang#134081 (Try to evaluate constants in legacy mangling) - rust-lang#134192 (Remove `Lexer`'s dependency on `Parser`.) - rust-lang#134208 (coverage: Tidy up creation of covmap and covfun records) - rust-lang#134211 (On Neutrino QNX, reduce the need to set archiver via environment variables) Failed merges: - rust-lang#133099 (forbid toggling x87 and fpregs on hard-float targets) r? `@ghost` `@rustbot` modify labels: rollup

workingjubilee · 2024-12-13T17:01:31Z

@bors rollup=iffy p=1

bors · 2024-12-13T19:43:04Z

⌛ Testing commit 60eca2c with merge 327c7ee...

bors · 2024-12-13T23:08:08Z

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

rust-timer · 2024-12-14T00:25:53Z

Finished benchmarking commit (327c7ee): 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.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 2.7%, secondary 0.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)	2.7%	[1.3%, 4.1%]	2
Regressions ❌ (secondary)	1.9%	[0.9%, 3.9%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-5.2%, -2.0%]	3
All ❌✅ (primary)	2.7%	[1.3%, 4.1%]	2

Cycles

Results (secondary 0.5%)

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)	2.5%	[1.9%, 3.4%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-7.4%	[-8.1%, -7.0%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 769.384s -> 769.449s (0.01%)
Artifact size: 331.02 MiB -> 331.04 MiB (0.01%)

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

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

2d887a5

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

RalfJung added 2 commits December 11, 2024 22:18

forbid toggling x87 and fpregs on hard-float targets

d6ddc73

apply review feedback

60eca2c

RalfJung force-pushed the forbidden-hardfloat-features branch from 0cb8b2b to 60eca2c Compare December 11, 2024 21:18

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 12, 2024

matthiaskrgr mentioned this pull request Dec 12, 2024

Rollup of 7 pull requests #134235

Closed

Zalathar mentioned this pull request Dec 13, 2024

Rollup of 9 pull requests #134245

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2024

bors merged commit 327c7ee into rust-lang:master Dec 13, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Dec 13, 2024

This was referenced Dec 13, 2024

Autodiff Upstreaming - rustc_codegen_llvm changes #130060

Open

Hir attributes #131808

Open

Re-export more rustc_span::symbol things from rustc_span. #134243

Open

RalfJung deleted the forbidden-hardfloat-features branch December 14, 2024 07:24

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 Dec 7, 2024

RalfJung Dec 7, 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 11, 2024

RalfJung commented Dec 11, 2024

RalfJung commented Dec 11, 2024 •

edited

Loading

Urgau commented Dec 11, 2024

oli-obk commented Dec 11, 2024

RalfJung commented Dec 12, 2024

workingjubilee commented Dec 12, 2024

bors commented Dec 12, 2024

workingjubilee commented Dec 13, 2024

bors commented Dec 13, 2024

bors commented Dec 13, 2024

rust-timer commented Dec 14, 2024

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

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

Choose a reason for hiding this comment

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

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

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 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 11, 2024

RalfJung commented Dec 11, 2024

RalfJung commented Dec 11, 2024 • edited Loading

Urgau commented Dec 11, 2024

oli-obk commented Dec 11, 2024

RalfJung commented Dec 12, 2024

workingjubilee commented Dec 12, 2024

bors commented Dec 12, 2024

workingjubilee commented Dec 13, 2024

bors commented Dec 13, 2024

bors commented Dec 13, 2024

rust-timer commented Dec 14, 2024

Overall result: ❌ regressions - no action needed

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

RalfJung commented Dec 11, 2024 •

edited

Loading