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

-Csoft-float flag is unsound #129893

Open
RalfJung opened this issue Sep 2, 2024 · 20 comments
Open

-Csoft-float flag is unsound #129893

RalfJung opened this issue Sep 2, 2024 · 20 comments
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-codegen Area: Code generation 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-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 2, 2024

Current status: all use of the flag causes a warning (will be shipped in 1.83), announcing that it will become a hard error in the future.


@bjorn3 just made me aware of this amazing flag:

    -C               soft-float=val -- use soft float ABI (*eabihf targets only) (default: no)

This is quite unsound: if code gets compiled with -Csoft-float and calls code from the standard library that uses the hard float ABI, we have UB. Generally we need different target triples for softfloat vs hardfloat ABIs, since (as per the discussion in rust-lang/lang-team#235) code within a single target should be ABI compatible. Cargo even (unstably) allows overwriting RUSTFLAGS on a per-crate basis, so we better make sure crates compiled with different flags can be linked with each other.

This was added a looooong time ago in #9617. I couldn't find any discussion regarding its soundness.

We have e.g. arm-unknown-linux-musleabi and arm-unknown-linux-musleabihf, so using the *hf target but with -Csoft-float also seems kind of unnecessary. (But I have not checked whether all eabihf targets have a corresponding eabi target.)

According to the documentation, this can only be used by ARM targets. So paging in some ARM folks -- is this used in practice, and if yes, how do people avoid the soundness problems?
@rustbot ping arm

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state labels Sep 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2024

Hey ARM Group! This bug has been identified as a good "ARM candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @adamgemmell @hug-dev @jacobbramley @JamieCunliffe @joaopaulocarreiro @raw-bin @Stammark

@RalfJung RalfJung added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Sep 2, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 2, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Sep 2, 2024

Rustc just always forwards this flag to LLVM, so it seems possible that the *eabihf targets only part of the docs is not correct. We're certainly seeing a bunch of people out there setting this on non-ARM targets (e.g. here are some uses on x86 and riscv).

Cc @parched who added this part to the docs in #36261. Does LLVM document anywhere what this flag does, and which targets it affects?

@Dirbaio
Copy link
Contributor

Dirbaio commented Sep 2, 2024

(hi from from the embedded side. i'm mostly familiar with the thumb*eabi(hf)? targets, i'm not sure about others)

One use of the flag is to allow using the FPU while still using the soft-float ABI for compatibility with other code. However, this can already be done by using the -eabi targets and then telling the compiler to use the FPU anyway with some -Ctarget-feature, which seems a much less dangerous way of doing it.

I can't think of any other uses, so I wouldn't oppose deprecating/removing it.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 2, 2024

Seems like this flag is called -mfloat-abi in GCC/clang and indeed only exists for ARM. So most uses out there are bogus. A first step might be for us to show a warning when the flag is used on a target where it has no effect.

One use of the flag is to allow using the FPU while still using the soft-float ABI for compatibility with other code. However, this can already be done by using the -eabi targets and then telling the compiler to use the FPU anyway with some -Ctarget-feature, which seems a much less dangerous way of doing it.

Yes indeed, if there's a way to tell LLVM "use soft-float ABI but also use FPU", in a way that doesn't affect the ABI at all as is entirely link-compatible with fully soft-float code, then that's what should be done.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 2, 2024

Hm, either I am misunderstanding the assembly or this feature does not work as advertised? https://godbolt.org/z/53W75s3fc seems to show that -Csoft-float does not actually change the ABI?

EDIT: Ah no I just can't read assembly. This still uses hard-float operations but first moves the data from rN to sN, i.e. it expects a soft-float ABI. Never mind.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 2, 2024

Here are the eabihf targets that do not have a corresponding eabi target, and thus might rely on -Csoft-float to get a soft-float ABI:

  • thumbv7neon-unknown-linux-gnueabihf (tier 2)
  • armv6-unknown-netbsd-eabihf (tier 3)
  • armv7-sony-vita-newlibeabihf (tier 3)
  • armv7-unknown-netbsd-eabihf (tier 3)
  • armv7-wrs-vxworks-eabihf (tier 3)
  • armv8r-none-eabihf (tier 3)
  • thumbv7neon-unknown-linux-musleabihf (tier 3)

So, only one tier 2 target is affected. And it's a tier 2 target without listed maintainers.

@bjorn3
Copy link
Member

bjorn3 commented Sep 2, 2024

armv7-sony-vita-newlibeabihf is meant to be used with a single cpu only which supports floats, so there is probably no need for soft-float support for that target.

@thejpster
Copy link
Contributor

Turning off the hard float ABI on a hard float target sounds like a "why do we even have that lever" kind of deal. I can't imagine why anyone would want to and I expect it would end badly if they tried.

As others have said, adding FPU instructions into a soft-float build is a entirely different matter, as is enabling Helium (MVE) or anything else that affects the juicy centre of a function but not its hard outer shell.

@thejpster
Copy link
Contributor

On the Cortex-R52, the FPU is mandatory and if anyone wants compatibility with older soft-float code they can either use armv7r-none-eabi, or they can come and make a compelling case for the v8-R version (kernel developers? idk). I'm not minded to add it on a whim because it seems pretty niche.

@workingjubilee
Copy link
Member

This flag has a very "from the days when there was an OABI" vibe to it.

@workingjubilee
Copy link
Member

I think we should start by warning when this flag is used, maybe that will get someone to explain their hypothetical use-case that cannot possibly be replaced by the -Ctarget-feature route.

@lolbinarycat lolbinarycat added A-codegen Area: Code generation 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 3, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2024

I think we should start by warning when this flag is used, maybe that will get someone to explain their hypothetical use-case that cannot possibly be replaced by the -Ctarget-feature route.

All right, I made #129897 do exactly that now.

@apiraino
Copy link
Contributor

apiraino commented Sep 3, 2024

WG-prioritization assigning priority on Zulip.

Also, (related discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Sep 15, 2024
deprecate -Csoft-float because it is unsound (and not fixable)

See  rust-lang#129893 for details. The general sentiment there seems to be that this flag has no use and sound alternatives exist, so let's add this warning and see if anyone out there disagrees.

Also show a different warning on targets where it does nothing (as documented since rust-lang#36261): it seems to correspond to `-mfloat-abi` in GCC/clang, which is an ARM-specific option. To be really sure it does nothing, only forward the flag to LLVM for eabihf targets. This should not change behavior but makes me sleep better ;)
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 15, 2024
Rollup merge of rust-lang#129897 - RalfJung:soft-float-ignored, r=Urgau

deprecate -Csoft-float because it is unsound (and not fixable)

See  rust-lang#129893 for details. The general sentiment there seems to be that this flag has no use and sound alternatives exist, so let's add this warning and see if anyone out there disagrees.

Also show a different warning on targets where it does nothing (as documented since rust-lang#36261): it seems to correspond to `-mfloat-abi` in GCC/clang, which is an ARM-specific option. To be really sure it does nothing, only forward the flag to LLVM for eabihf targets. This should not change behavior but makes me sleep better ;)
@skade
Copy link
Contributor

skade commented Sep 30, 2024

On the Cortex-R52, the FPU is mandatory and if anyone wants compatibility with older soft-float code they can either use armv7r-none-eabi, or they can come and make a compelling case for the v8-R version (kernel developers? idk). I'm not minded to add it on a whim because it seems pretty niche.

A good soft/no-float story is highly desired by Rust for Linux (and other kernel developers): Rust-for-Linux/linux#514

There's the clippy lint to avoid floats, but it runs into the same issue as this flag: lints are crate per crate, while this should be binary by binary.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 30, 2024

A good soft/no-float story is highly desired by Rust for Linux (and other kernel developers): Rust-for-Linux/linux#514

That will almost surely evolve around -Ctarget-feature=+soft-float, or some new way of exposing that target feature, not around -Csoft-float. The -C flag is mostly a red herring and confusing. See the discussion above for more details.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 13, 2024

@jamesmunns you brought this issue up in wg-embedded discussions. Is this flag something that embedded code relies on today? Note that this is only about -Csoft-float, not about using -Ctarget-feature to toggle between soft-floats and hard-floats.

Basically I am trying to figure out which timeline we can go for here in terms of making -Csoft-float a hard error. Is anyone currently relying on that flag? Do we have to wait for some project to migrate to a better option? Do we have to wait for a better option to be made available?

This warning will hit stable with the 1.83 release in 2 weeks. Then we should probably wait a release or two for people to notice this warning and get in touch with us if they rely on this flag. So I would hope to make this a hard error in nightly around February 2025, to hit stable either with 1.86 or 1.87.

OTOH, this flag could also become part of the target modifiers system (Cc @Darksonn), in which case the timeline would be "whenever mismatching target modifiers without an explicit override become a hard error". Though I think even then we should make it a hard error (without offering an override) to set this flag on any target where it doesn't actually do anything. Also, we already have *hf and non-hf targets on 32bit ARM, so it's a bit unclear whether we really want that and this flag.

@jamesmunns
Copy link
Member

I don't believe that anyone in the wg-embedded discussions was using -Csoft-float. I can ping again, but as mentioned above, most embedded folks just use the soft-float targets (non-hf). We mostly deal with bare metal/RTOS systems where you are compiling everything at once. I can't remember it being mentioned at all prior to this issue.

@jamesmunns
Copy link
Member

Ah, I did managed to find some usage of this in Redox: https://grep.app/search?q=-C%20soft-float

CC @jackpot51

@RalfJung
Copy link
Member Author

RalfJung commented Nov 13, 2024

Redox should have been fixed by https://gitlab.redox-os.org/redox-os/cookbook/-/merge_requests/392. Or does it use the flag in more than one place?

From these search results you linked, none of them seems to be on 32bit ARM, so the flag is anyway already a NOP.

@RalfJung
Copy link
Member Author

Long-term we could also consider making this flag actually work on all targets (probably with a warning on targets that are already soft-float targets). But that will require gathering some data to figure out which target features and other things need to be set on any given target to make it a valid soft-float target.

crawfxrd added a commit to system76/firmware-update that referenced this issue Nov 13, 2024
`-Csoft-float` is deprecated and will be removed. Soft float is enabled
by the target spec.

    $ rustc -Z unstable-options --target=x86_64-unknown-uefi --print target-spec-json | grep features
    "features": "-mmx,-sse,+soft-float",

Ref: rust-lang/rust#129893
Signed-off-by: Tim Crawford <[email protected]>
crawfxrd added a commit to system76/firmware-update that referenced this issue Nov 13, 2024
`-Csoft-float` is deprecated. Soft float is enabled by the target spec.

    $ rustc -Z unstable-options --target=x86_64-unknown-uefi --print target-spec-json | grep features
    "features": "-mmx,-sse,+soft-float",

Ref: rust-lang/rust#129893
Signed-off-by: Tim Crawford <[email protected]>
crawfxrd added a commit to system76/firmware-update that referenced this issue Nov 14, 2024
`-Csoft-float` is deprecated. Soft float is enabled by the target spec.

    $ rustc -Z unstable-options --target=x86_64-unknown-uefi --print target-spec-json | grep features
    "features": "-mmx,-sse,+soft-float",

Ref: rust-lang/rust#129893
Signed-off-by: Tim Crawford <[email protected]>
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-codegen Area: Code generation 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-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants