Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature #129884

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 2, 2024

The context for this is #116344: some target features change the way floats are passed between functions. Changing those target features is unsound as code compiled for the same target may now use different ABIs.

So this introduces a new concept of "forbidden" target features (on top of the existing "stable " and "unstable" categories), and makes it a hard error to (un)set such a target feature. For now, the x86 and ARM feature soft-float is on that list. We'll have to make some effort to collect more relevant features, and similar features from other targets, but that can happen after the basic infrastructure for this landed. (These features are being collected in #131799.)

I've made this a warning for now to give people some time to speak up if this would break something.

MCP: rust-lang/compiler-team#780

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
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 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 Sep 2, 2024
@RalfJung RalfJung force-pushed the forbidden-target-features branch from f8424ee to 191ef36 Compare September 2, 2024 10:14
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the forbidden-target-features branch from df74d6b to a340e78 Compare September 2, 2024 13:04
@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2024

Some changes occurred in tests/ui/check-cfg

cc @Urgau

@RalfJung RalfJung force-pushed the forbidden-target-features branch from bbf47ab to 319aafa Compare September 2, 2024 13:48
@rust-log-analyzer

This comment was marked as outdated.

@@ -328,6 +355,7 @@ const X86_ALLOWED_FEATURES: &[(&str, Stability, ImpliedFeatures)] = &[
("sha512", Unstable(sym::sha512_sm_x86), &["avx2"]),
("sm3", Unstable(sym::sha512_sm_x86), &["avx"]),
("sm4", Unstable(sym::sha512_sm_x86), &["avx2"]),
("soft-float", Forbidden, &[]), // changes float ABI
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARM targets also have a soft-float target feature. Aarch64 does not, though -- so we can't just add this for all targets, unfortunately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aarch64 has abi: "softfloat":

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like it cannot be changed with -Ctarget-feature, which is good news.

@rust-log-analyzer

This comment was marked as resolved.

@RalfJung RalfJung force-pushed the forbidden-target-features branch from 0ae9f0e to b67a656 Compare September 2, 2024 15:02
@VorpalBlade
Copy link

How does this affect kernel / bare metal builds on x86 where you don't enable any floating point support whatsoever? I know the Linux kernel uses that. I'm not sure what the status for Redox, etc is.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 2, 2024

How does this affect kernel / bare metal builds on x86 where you don't enable any floating point support whatsoever? I know the Linux kernel uses that. I'm not sure what the status for Redox, etc is.

We have a x86_64-unknown-none soft-float target for them. I sure hope they don't use some other target and -Ctarget-features=+soft-float.

@RalfJung RalfJung force-pushed the forbidden-target-features branch from b67a656 to 3e44287 Compare September 2, 2024 15:50
@bjorn3
Copy link
Member

bjorn3 commented Sep 2, 2024

We have a x86_64-unknown-none soft-float target for them. I sure hope they don't use some other target and -Ctarget-features=+soft-float.

The kernel currently has a script generating a target spec for x86: https://github.com/Rust-for-Linux/linux/blob/rust-next/scripts/generate_rust_target.rs Arm64, riscv and loongarch use builtin bare-metal targets though.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 2, 2024

Yeah anyone using custom targets can still set these target features in the target spec. I assume it is generally understood that code using different custom target specs cannot be linked together, so the same concerns do not apply there.

@RalfJung RalfJung changed the title mark some target features as 'forbidden' so they cannot be (un)set mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature Sep 2, 2024
@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Sep 2, 2024

I assume it is generally understood that code using different custom target specs cannot be linked together

In fact rustc already checks that the content of the target spec file matches for custom target specs ever since #98225.

@beetrees
Copy link
Contributor

beetrees commented Sep 2, 2024

The one exception is that it may be the case that on a target with +soft-float, it is okay to enable x87 as floats will still be returned in regular registers... I haven't checked this, so I am not sure. I would say this seems sufficiently niche that it's okay to reject this for now, and if the need really comes up we'll have to add a special case.

AFAICT, on x86(-64), soft-float completely disables use of floating-point registers and always uses software floating point operations regardless of which other target features are enabled, meaning that it's safe to enable the x87 feature (and other floating-point features) but completely useless (e.g. if you enable the sse feature and try to use an xmm_reg input/output in inline ASM, LLVM will emit rustc-LLVM ERROR: Do not know how to soften this operator's operand!).

@RalfJung RalfJung force-pushed the forbidden-target-features branch from 3e44287 to 098469e Compare September 2, 2024 16:50
@RalfJung
Copy link
Member Author

RalfJung commented Sep 2, 2024

AFAICT, on x86(-64), soft-float completely disables use of floating-point registers and always uses software floating point operations regardless of which other target features are enabled, meaning that it's safe to enable the x87 feature (and other floating-point features) but completely useless (e.g. if you enable the sse feature and try to use an xmm_reg input/output in inline ASM, LLVM will emit rustc-LLVM ERROR: Do not know how to soften this operator's operand!).

The problematic case is -x87 on what is usually a hardfloat target -- that will change ABI. So we have to make x87 "forbidden".

We could make it "forbidden to disable but not forbidden to enable", but if you're saying that enabling it is useless anyway then I suggest we go with "forbidden" now. :)

@bors 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 Nov 4, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 4, 2024
…, r=workingjubilee

mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature

The context for this is rust-lang#116344: some target features change the way floats are passed between functions. Changing those target features is unsound as code compiled for the same target may now use different ABIs.

So this introduces a new concept of "forbidden" target features (on top of the existing "stable " and "unstable" categories), and makes it a hard error to (un)set such a target feature. For now, the x86 and ARM feature `soft-float` is on that list. We'll have to make some effort to collect more relevant features, and similar features from other targets, but that can happen after the basic infrastructure for this landed. (These features are being collected in rust-lang#131799.)

I've made this a warning for now to give people some time to speak up if this would break something.

MCP: rust-lang/compiler-team#780
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#129884 (mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature)
 - rust-lang#132153 (Stabilise `const_char_encode_utf16`.)
 - rust-lang#132473 ([core/fmt] Replace checked slice indexing by unchecked to support panic-free code)
 - rust-lang#132571 (add const_eval_select macro to reduce redundancy)
 - rust-lang#132587 (Revert "Avoid nested replacement ranges" from rust-lang#129346.)
 - rust-lang#132596 ([rustdoc] Fix `--show-coverage` when JSON output format is used)
 - rust-lang#132598 (Clippy: Move some attribute lints to be early pass (post expansion))
 - rust-lang#132601 (Update books)
 - rust-lang#132606 (Improve example of `impl Pattern for &[char]`)
 - rust-lang#132609 (docs: fix grammar in doc comment at unix/process.rs)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2024
…r=workingjubilee

mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature

The context for this is rust-lang#116344: some target features change the way floats are passed between functions. Changing those target features is unsound as code compiled for the same target may now use different ABIs.

So this introduces a new concept of "forbidden" target features (on top of the existing "stable " and "unstable" categories), and makes it a hard error to (un)set such a target feature. For now, the x86 and ARM feature `soft-float` is on that list. We'll have to make some effort to collect more relevant features, and similar features from other targets, but that can happen after the basic infrastructure for this landed. (These features are being collected in rust-lang#131799.)

I've made this a warning for now to give people some time to speak up if this would break something.

MCP: rust-lang/compiler-team#780
@bors
Copy link
Contributor

bors commented Nov 5, 2024

⌛ Testing commit ffad9aa with merge 4990a69...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Nov 5, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 5, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Nov 5, 2024

The musl job timed out?

@bors retry

@bors 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 Nov 5, 2024
@workingjubilee
Copy link
Member

@bors rollup=iffy

@bors
Copy link
Contributor

bors commented Nov 5, 2024

⌛ Testing commit ffad9aa with merge e8c698b...

@bors
Copy link
Contributor

bors commented Nov 5, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 5, 2024
@bors bors merged commit e8c698b into rust-lang:master Nov 5, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 5, 2024
@RalfJung RalfJung deleted the forbidden-target-features branch November 5, 2024 19:04
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e8c698b): 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)
1.3% [1.3%, 1.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -0.9%, secondary -1.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)
- - 0
Improvements ✅
(primary)
-0.9% [-0.9%, -0.9%] 1
Improvements ✅
(secondary)
-1.5% [-2.0%, -1.1%] 6
All ❌✅ (primary) -0.9% [-0.9%, -0.9%] 1

Cycles

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

Binary size

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

Bootstrap: 780.065s -> 779.96s (-0.01%)
Artifact size: 335.31 MiB -> 335.30 MiB (-0.00%)

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 13, 2024
…s, r=workingjubilee

forbid toggling x87 and fpregs on hard-float targets

Part of rust-lang#116344, follow-up to rust-lang#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](rust-lang#130988) 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 rust-lang#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`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 15, 2024
…ingjubilee

forbid toggling x87 and fpregs on hard-float targets

Part of rust-lang/rust#116344, follow-up to rust-lang/rust#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](rust-lang/rust#130988) 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 rust-lang/rust#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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.