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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

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 features soft-float and x87 are on that list. We'll have to make some effort to collect similar features from other targets, but that can happen after the basic infrastructure for this landed.

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2024

Some changes occurred in tests/ui/check-cfg

cc @Urgau

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

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

@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
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. :)

@RalfJung RalfJung added S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Sep 12, 2024
@bors
Copy link
Contributor

bors commented Sep 17, 2024

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 17, 2024
@RalfJung
Copy link
Member Author

MCP in rust-lang/compiler-team#780 has passed. The PR now does the conservative thing of emitting a warning rather than a hard error, pointing to #116344.

@petrochenkov this is ready for review.

@RalfJung
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 27, 2024
@RalfJung RalfJung added relnotes Marks issues that should be documented in the release notes of the next release. and removed S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. labels Sep 27, 2024
Turns out fpregs is sometimes needed on ARM when -Ctarget-cpu enables too many things on a soft-float target,
and we need to cut things back down to not have the FPU. This is sound as the target is +soft-float so toggling
fpregs does not change the ABI -- but toggling fpregs on an eabihf target is not sound!
@RalfJung
Copy link
Member Author

Turns out ARM fpregs is sometimes needed on ARM when -Ctarget-cpu enables too many things on a soft-float target, and one needs to cut things back down to not have the FPU. This is sound as the target is +soft-float so toggling fpregs does not change the ABI -- but toggling fpregs on an eabihf target is not sound!

So we'll need to find a way to allow fpregs on some targets but disallow it on others. But that can be done in a next step, this PR is now restricted to the soft-float target feature which we do not want to be set either way for any target.

@bors
Copy link
Contributor

bors commented Oct 10, 2024

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

@RalfJung
Copy link
Member Author

Since we haven't heard from the reviewer in a while
r? compiler

@rustbot rustbot assigned nnethercote and unassigned petrochenkov Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.