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-unknown-none-softfloat: ABI unsoundness when enabling "neon" feature #134375

Open
RalfJung opened this issue Dec 16, 2024 · 13 comments · May be fixed by #135160
Open

aarch64-unknown-none-softfloat: ABI unsoundness when enabling "neon" feature #134375

RalfJung opened this issue Dec 16, 2024 · 13 comments · May be fixed by #135160
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-AArch64 Armv8-A or later processors in AArch64 mode T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Dec 16, 2024

Enabling the "neon" target feature on the aarch64-unknown-none-softfloat target is taken by LLVM as a sign that we want to use the hardfloat ABI. That is unfortunate as it makes it UB to link such code against code built for aarch64-unknown-none-softfloat without the "neon" target feature.

For Rust-generated functions we work around this by forcing our own ABI, passing floats either indirectly or via integer registers (#133102). However, this does not help for LLVM-generated calls for builtins/intrinsics, as shown in this example by @beetrees.

We don't have target maintainers listed for this target, so maybe that means we can just demote it to tier 3? (See #113739)

LLVM issue: llvm/llvm-project#110632. So far LLVM maintainers seem to not agree that there is a problem here.

@Amanieu unfortunately your proposal for making floats work on that target doesn't quite suffice. :/ We do need some help from LLVM.
@nikic do you have any good ideas for what we could do here? It seems like rejecting enabling "neon" on aarch64-unknown-none-softfloat is the only sound option we have right now, but I worry that may make the Rust-for-Linux folks (among others) unhappy.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 16, 2024
@RalfJung RalfJung added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-AArch64 Armv8-A or later processors in AArch64 mode labels Dec 16, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 16, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-ABI Area: Concerning the application binary interface (ABI) A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 16, 2024
@apiraino
Copy link
Contributor

apiraino commented Dec 16, 2024

We don't have target maintainers listed for this target, so maybe that means we can just demote it to tier 3?

I also skimmed the git history for the https://github.com/rust-lang/rust/blob/master/src/doc/rustc/src/platform-support.md but found nothing. I think it could make sense to demote the target if it's unmaintained. @RalfJung should I mention this question in the next compiler meeting?

Another question: do you think the neon target feature could create undefined behaviours also in other targets? (I don't know how they work)

thanks

EDIT: I feel this comment kind of answers my second question: IIUC the issue is only when the neon target feature is used on compile target with no FPU (softfloats), which by looking at this list it's basically just the one mentioned here.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 16, 2024 via email

@RalfJung
Copy link
Member Author

With #134794 we could fairly easily reject enabling neon specifically on this target. That would fix the ABI issues.

I just wonder, is anyone relying on enabling neon for aarch64-unknown-none-softfloat? Not quite sure whom to ping... @Darksonn for R4L, who else might be using this target?

@ojeda
Copy link
Contributor

ojeda commented Dec 27, 2024

Cc @JamieCunliffe (currently, arm64 in the kernel uses --target=aarch64-unknown-none -Ctarget-feature="-neon").

@Darksonn
Copy link
Contributor

Should we be using --target=aarch64-unknown-none-softfloat instead?

@RalfJung
Copy link
Member Author

If you never need neon, that would be slightly better... but there's still no sound way to enable neon (or fp-armv8) for some of the code. The limiting factor here is LLVM (llvm/llvm-project#110632). The C ecosystem assumes that there is a hard-error when floats are used somewhere that neon is not available, so there's no proper support for a softloat ABI. However Rust assumes float types always exist and there's some reasonable ABI for them.

Probably, someone will have to add a soft-float target feature to LLVM that forces the use of integer registers for passing floats even when fp-armv8 is available.

@RalfJung
Copy link
Member Author

@Darksonn @ojeda does Rust4Linux need to locally enable neon for some parts of the build? Or is that all going to stay on the C side for the foreseeable future?

Right now the only way I can see to fix this issue is to just mark the neon target feature as incompatible with aarch64-unknown-none-softfloat. Some work on the LLVM side is needed to fix that in a way that is compatible with Rust (where we want to always provide the float types and just use softfloats when needed). That said, the C approach of just using different compiler flags for different parts of the code and "being careful" when linking (making sure there are no float types crossing that ABI boundary) would also work in Rust.

@ojeda
Copy link
Contributor

ojeda commented Dec 31, 2024

I don't think the existing C users will be rewritten anytime soon, but who knows what out-of-tree may be doing. Let's see what Jamie says, and I have pinged the arm64 maintainers (Cc @ctmarinas). Alice et al. (Cc @maurer @samitolvanen) can confirm for Android. Also Cc @asahilina for Asahi and @Fabo in case they need it.

If "being careful" (differently-compiled TUs) would still be doable like in C, then I guess it should be fine.

@asahilina
Copy link

asahilina commented Dec 31, 2024 via email

@Darksonn
Copy link
Contributor

Darksonn commented Jan 6, 2025

Android or Panthor does not currently need floats in kernel Rust code.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 6, 2025

#135160 proposes to make #[target_feature(enable = "neon")] a hard error on aarch64-unknown-none-softfloat; please chime in if that would be a problem for anyone.

@briansmith
Copy link
Contributor

I don't think the existing C users will be rewritten anytime soon, but who knows what out-of-tree may be doing.

It is inevitable that some (perhaps third party / out of tree) Rust code will need to use NEON within the Linux kernel, in other kernels, and in similar situations. It is just a matter of time.

In the same way that people are replacing C code with Rust code, people are also trying to replace out-of-line assembly with smaller bits of inline assembly and/or with auto-vectorized code, using patterns like this:

fn foo(safe_to_use_vectorized: bool) {
    // TODO(MSRV): "neon" isn't supported by Rust for target_arch = "arm" yet.
    #[cfg(any(target_arch = "x86", target_arch = "x86_64", target_arch = "aarch64"))]
    if safe_to_use_vectorized {
        #[cfg_attr(any(target_arch = "aarch64"), target_feature(enable="neon"))]
        #[cfg_attr(any(target_arch = "x86", target_arch = "x86_64"), target_feature(enable="sse2"))]
         unsafe fn foo_vectorized() { foo_inner() } // auto-vectorized foo_inner    
     
        return unsafe { foo_vectorized() };
    }
    
    let _ = safe_to_use_vectorized;
    foo_inner()
}

#[inline(always)] // So it can be auto-vectorized within `foo_vectorized()`
fn foo_inner() {
    // auto-vectorizable code goes here.
}

For people writing code like the above, we'd need to change our target_arch = "aarch64" conditions to something so that it can build for aarch64-unknown-none-softfloat. I.e. What is the cfg() expression for 'an aarch64 target that supports target_feature(enable="neon")'?

So, I don't think that disabling target_feature(enable="neon") for these targets isn't a long-terms solution to the ABI issues.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 8, 2025

What is the cfg() expression for 'an aarch64 target that supports target_feature(enable="neon")'?

all(target_arch="aarch64", not(target_abi="softfloat"))

So, I don't think that disabling target_feature(enable="neon") for these targets isn't a long-terms solution to the ABI issues.

Did you intend the double negation? This sentence says you do think that it is a long-term solution...

To be clear, I don't think this is a long-term solution. But it also makes no sense to pretend to support something which our primary backend does not support. Someone has to put in the work on the LLVM side before we can provide this feature (using the FPU on an otherwise softfloat aarch64 target) to our users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-AArch64 Armv8-A or later processors in AArch64 mode T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants