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

aarch64-softfloat: forbid enabling the neon target feature #135160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 6, 2025

This fixes #134375 in a rather crude way, by making the example not build any more on aarch64-unknown-none-softfloat. That is a breaking change since the "neon" aarch64 target feature is stable, but this is justified as a soundness fix. Note that it's not "neon" which is problematic but "fp-armv8"; however, the two are tied together by rustc.

More work on the LLVM side will be needed before we can let people use neon without impacting the ABI of float values (and, in particular, the ABI used by automatically inserted calls to libm functions, e.g. for int-to-float casts, which rustc has no control over).

Nominating for @rust-lang/lang since it is a breaking change. As-is this PR doesn't have a warning cycle; the hope is that the aarch64-unknown-none-softfloat target is sufficiently niche that there's no huge fallout and we can easily revert if it causes trouble. A warning cycle could be added but would need some dedicated rather hacky check in the target_feature attribute handling logic.

@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2025

r? @Noratrieb

rustbot has assigned @Noratrieb.
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 Jan 6, 2025
@RalfJung
Copy link
Member Author

RalfJung commented Jan 6, 2025

Cc @Darksonn @Amanieu, please chime in if there are any aarch64 stakeholders that need #[target_feature(enable = "neon")] on aarch64-unknown-none-softfloat.

@Darksonn
Copy link
Contributor

Darksonn commented Jan 6, 2025

This change looks good to me. On the off chance that a kernel user comes up in the near future, they can use the "be careful" approach you mentioned in #134375 (comment).

@RalfJung
Copy link
Member Author

RalfJung commented Jan 6, 2025

Yeah, separately building some crates with aarch64-unknown-none-softfloat and some with aarch64-unknown-none and linking them together should work (but I have not tested this, you may need separate sysroots and then link them together as C-style object files). Once we have the target modifier system it should flag this as incompatible, but that can be unsafely overruled.

@traviscross traviscross added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. labels Jan 6, 2025
@tgross35
Copy link
Contributor

tgross35 commented Jan 6, 2025

Once we have the target modifier system it should flag this as incompatible, but that can be unsafely overruled.

Will that mean something like #[unsafe(target_feature(enable = "neon"))] can enable this back?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 6, 2025

I don't think we should allow target features to do that (I was imagining target modifiers would introduce a new flag for overriding ABI-relevant target features, if there's any good use for that), and so far there was no proposal to allow overriding target modifiers on a per-function basis.

The text you quoted referred to -C flags, not attributes.

@RalfJung RalfJung force-pushed the aarch64-softfloat-not-neon branch from 2bd82d7 to bdf1e78 Compare January 6, 2025 23:01
@Amanieu
Copy link
Member

Amanieu commented Jan 7, 2025

I'm happy with this as long as we acknowledge that this is a temporary workaround until LLVM starts handling ABI independently of target features.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 7, 2025

Fully agreed, the ideal end state is that we can use the FPU while sticking to a softfloat ABI (what clang and GCC do for -mfloat-abi=softfp on ARM-32). How temporary this will be depends on how long it takes until someone puts in the required work on the LLVM side.

@RalfJung RalfJung added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 14, 2025
@Noratrieb Noratrieb added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2025
@apiraino
Copy link
Contributor

Was this discussed by T-lang prior to be approved? I assume yes since I see the release notes label, correct?

cc @traviscross probably knows

@RalfJung
Copy link
Member Author

RalfJung commented Jan 29, 2025

We are waiting for t-lang approval, hence the "I-lang-nominated" and "S-waiting-on-team" labels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated Nominated for discussion during a lang team meeting. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aarch64-unknown-none-softfloat: ABI unsoundness when enabling "neon" feature
8 participants