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

The ABI of float types can be changed by -Ctarget-feature #116344

Open
RalfJung opened this issue Oct 2, 2023 · 37 comments
Open

The ABI of float types can be changed by -Ctarget-feature #116344

RalfJung opened this issue Oct 2, 2023 · 37 comments
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-floating-point Area: Floating point numbers and arithmetic A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 2, 2023

A function that returns an f32/f64 is not ABI-compatible with other functions that have the same signature on i686 when certain target features differ. It looks like one can disable the x87 feature or enable the soft-float and then it will use different ways of passing floating-point arguments.

This is unsound as code calling methods from the standard library would now use the wrong registers to return results. In other words, setting -Ctarget-feature=-x87 or -Ctarget-feature=+soft-float can introduce UB unless the standard library is rebuilt with the same flags. We therefore should reject these flags, to avoid the UB. This issue tracks that problem, and transitioning it to a hard error.

(SIMD types have a similar problem, but we are dealing with that differently. See #116558.)

Current status:

  • It is a hard error to toggle some features with #[target_feature] (will be shipped in 1.84) -- not a breaking change since those features were not allowed in #[target_feature] before either, only the error message changed
  • It is a warning to toggle them with -Ctarget-feature announcing that this will become a hard error in the future (will be shipped in 1.84)
  • Not all relevant features are properly detected yet, not even on tier 1 targets
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 2, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Oct 2, 2023

Cc @rust-lang/opsem

@chorman0773
Copy link
Contributor

FTR, I think the -x87,+softfp and -x87,+sse codegens are wrong for at least the C abi, because Sys-V (and msabi) do prescribe that float/double are returned in st(0) and provide no other alternative - so I think rustc should reject this code in particular.

I actually wanted a similar prohibition retroactively on the simd types, but the x86_64-psabi list did not accept that request.

@workingjubilee
Copy link
Member

workingjubilee commented Oct 2, 2023

If I understand @chorman0773 correctly #115476 (comment), then a function that takes/returns an f32/f64 is not ABI-compatible with other functions that have the same signature on i686 when certain target features differ. It looks like one can disable the x87 feature and then it will use different ways of passing floating-point arguments.

This is correct.

So IMO we should consider certain features to be in the required baseline for i686 targets and just error out when they get disabled (or force-enable them, or refuse to codegen things involving floats, or something like that) -- in particular, x87 and sse2.

I believe I have vocalized that this is my desired solution as well.

@chorman0773
Copy link
Contributor

chorman0773 commented Oct 2, 2023

Demonstrating the 3 different ways that rustc returns floats on x86: https://rust.godbolt.org/z/r83MbYh5n.

Although it seems f64 specifically is spared on sse and softfp (not between +x87 and -x87 though). Both cases it's returned in edx:eax (which is weird, I'd expect f64 to get returned in an xmm register otherwise).

@chorman0773
Copy link
Contributor

chorman0773 commented Oct 2, 2023

So IMO we should consider certain features to be in the required baseline for i686 targets and just error out when they get disabled (or force-enable them, or refuse to codegen things involving floats, or something like that) -- in particular, x87 and sse2. Currently it may seem like --target i686-unknown-linux-gnu -C target-feature=-sse2,-sse is a tier 1 target but really it isn't.

Force enabling them (or blanket erroring) on i686-wide would affect kernel mode code that typically disables the FPU and vector extensions to avoid having to save that state every context switch. Refusing to codegen floats is a reasonable alternative, though. For sse in particular, llvm really loves to copy data arround using xmm registers, so this will either cause a #GP(0) when llvm starts putting movupss everywhere, or worse, silently clobber xmm registers when the kernel does something even cleverer with cr4.OSFXSAVE/cr4.OSXSAVE enabled.

@workingjubilee
Copy link
Member

Kernel-friendly targets need to be handled specially as always.

@saethlin saethlin added O-x86 O-x86_64 Target: x86-64 processors (like x86_64-*) A-ABI Area: Concerning the application binary interface (ABI) T-opsem Relevant to the opsem team 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 Oct 2, 2023
@chorman0773
Copy link
Contributor

I was about to say this didn't need O-x86_64, but...
https://rust.godbolt.org/z/EzMhdsqx9

@RalfJung
Copy link
Member Author

RalfJung commented Oct 2, 2023

I just realized that with #[target_feature] we don't allow disabling features at all. That makes me quite surprised that we allow disabling features on stable with -C target-feature... was that a deliberate mismatch?


Force enabling them (or blanket erroring) on i686-wide would affect kernel mode code that typically disables the FPU and vector extensions to avoid having to save that state every context switch.

Force enabling them wouldn't affect that code if it doesn't use any floats. :)

@chorman0773
Copy link
Contributor

chorman0773 commented Oct 2, 2023

Force enabling them wouldn't affect that code if it doesn't use any floats. :)

It does though, especially sse as mentioned.
llvm will happily fold a 16 byte 4 dword mov into movupss, which will #GP(0) if cr4.OSFXSAVE=0.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 2, 2023

It does though, especially sse as mentioned.

It does?

llvm will happily fold a 16 byte 4 dword mov into movupss, which will #GP(0) if cr4.OSFXSAVE=0.

Bless you? To me it looks like you put the output of pwgen into the editor. ;) Can you explain this in higher-level terms?

@chorman0773
Copy link
Contributor

chorman0773 commented Oct 2, 2023

If you tell llvm that it can use sse instructions, it will completely decide to fold scalar bytewise copies into sse copies and cause a general protection exception in kernel code that isn't configured to allow those instructions.

This is why it's considered undefined behaviour to merely enter code with an unavailable feature available.

@chorman0773
Copy link
Contributor

Example of llvm using movups rather than scalar copies with sse enabled: https://rust.godbolt.org/z/jW8W54sc9

@RalfJung
Copy link
Member Author

RalfJung commented Oct 2, 2023

If you tell llvm that it can use sse instructions, it will completely decide to fold scalar bytewise copies into sse copies and cause a general protection exception in kernel code that isn't configured to allow those instructions.

Ah, bummer.

That sounds like we want -softfloat/-nofloat targets then. But disabling target features seems to have all sorts of bad side-effects and I wish we never allowed it -- and I wonder to what extend we can take it back...

@chorman0773
Copy link
Contributor

Disabling target features is incredibly useful when writing all kinds of code. Kernel and driver code especially, but I write a lot of "Low-level user mode code" that also somestimes requires finagling with -C target-feature and -C target-cpu.

And sometimes you live before the kernel. A bootloader gratuitously opting arbitrary kernels into cr4.OSXFSAVE is even worse, because the instructions won't trap, just silently clobber user mode state.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 2, 2023

You are describing a good motivation for a -nofloat/-softfloat target. In fact, we have some -softlofat targets.

You are not describing why we should offer the ability to disable target features, when perfectly valid alternatives exist; alternatives that do not also eat your kitttens. "It is useful" applies to many things that we very deliberately do not let people do because they just cause too many issues.

@workingjubilee
Copy link
Member

It is still possible to obtain what is desired for those by switching to an enable-only process, or virtually so (I realize that softfloat is technically a feature one must often disable to get correct codegen).

@chorman0773
Copy link
Contributor

Note: #115919 would make this apply by toggling sse and not x87, which can be done without disabling any features on i586 targets.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 3, 2023

#115919 could be adjusted to only kick in when the baseline features of the target include SSE. If we do that, does enabling SSE ever affect the ABI of f32/f64? If the answer is "no" then I think the i586 targets are good, right?

For softfloat targets, we'd have to ensure their f32/f64 ABI is unaffected by enabling x87 or SSE, or we have to reject enabling those features. The former should actually be possible, right? I would assume f32 is passed much like i32 and f64 like i64 on those targets, so we can tell LLVM to pass floats as i32/i64 and then we don't have to worry about target features at all?

@RalfJung
Copy link
Member Author

I made this issue about tracking the unsoundness caused by -Ctarget-issue (so it will be referenced by warnings once #129884 lands), and opened #131799 for gathering the data for all targets about which features affect ABI.

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 22, 2024
…ngjubilee

Always specify `llvm_abiname` for RISC-V targets

For RISC-V targets, when `llvm_abiname` is not specified LLVM will infer the ABI from the target features, causing rust-lang#116344 to occur. This PR adds the correct `llvm_abiname` to all RISC-V targets where it is missing (which are all soft-float targets), and adds a test to prevent future RISC-V targets from accidentally omitting `llvm_abiname`. The only affect of this PR is that `-Ctarget-feature=+f` (or similar) will no longer affect the ABI on the modified targets.

<!-- homu-ignore:start -->
r? `@RalfJung`
<!--- homu-ignore:end -->
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 22, 2024
Rollup merge of rust-lang#131807 - beetrees:riscv-target-abi, r=workingjubilee

Always specify `llvm_abiname` for RISC-V targets

For RISC-V targets, when `llvm_abiname` is not specified LLVM will infer the ABI from the target features, causing rust-lang#116344 to occur. This PR adds the correct `llvm_abiname` to all RISC-V targets where it is missing (which are all soft-float targets), and adds a test to prevent future RISC-V targets from accidentally omitting `llvm_abiname`. The only affect of this PR is that `-Ctarget-feature=+f` (or similar) will no longer affect the ABI on the modified targets.

<!-- homu-ignore:start -->
r? `@RalfJung`
<!--- homu-ignore:end -->
@workingjubilee workingjubilee changed the title The ABI of float types on can be changed by -Ctarget-feature The ABI of float types can be changed by -Ctarget-feature Oct 23, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this issue 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 issue 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 added a commit to rust-lang-ci/rust that referenced this issue 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
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Nov 9, 2024
…jubilee

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

The context for this is rust-lang/rust#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/rust#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
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Nov 28, 2024
…jubilee

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

The context for this is rust-lang/rust#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/rust#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
antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this issue Dec 11, 2024
…jubilee

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

The context for this is rust-lang/rust#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/rust#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 issue 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 issue 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`
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 15, 2024
…gjubilee

reject aarch64 target feature toggling that would change the float ABI

~~Stacked on top of rust-lang#133099. Only the last two commits are new.~~

The first new commit lays the groundwork for separately controlling whether a feature may be enabled or disabled. The second commit uses that to make it illegal to *disable* the `neon` feature (which is only possible via `-Ctarget-feature`, and so the new check just adds a warning). Enabling the `neon` feature remains allowed on targets that don't disable `neon` or `fp-armv8`, which is all our built-in targets. This way, the entire PR is not a breaking change.

Fixes rust-lang#131058 for hardfloat targets (together with rust-lang#133102 which fixed it for softfloat targets).

Part of rust-lang#116344.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Dec 16, 2024
…workingjubilee

reject unsound toggling of RISCV target features

~~Stacked on top of rust-lang#133417, only the last commit is new.~~

Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment)))
Part of rust-lang#116344

Cc `@beetrees` I hope I got everything.  I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks.
r? `@workingjubilee`

Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 16, 2024
…workingjubilee

reject unsound toggling of RISCV target features

~~Stacked on top of rust-lang#133417, only the last commit is new.~~

Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment)))
Part of rust-lang#116344

Cc ``@beetrees`` I hope I got everything.  I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks.
r? ``@workingjubilee``

Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 16, 2024
Rollup merge of rust-lang#134337 - RalfJung:riscv-target-features, r=workingjubilee

reject unsound toggling of RISCV target features

~~Stacked on top of rust-lang#133417, only the last commit is new.~~

Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment)))
Part of rust-lang#116344

Cc ``@beetrees`` I hope I got everything.  I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks.
r? ``@workingjubilee``

Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.
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-floating-point Area: Floating point numbers and arithmetic A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Development

No branches or pull requests

10 participants