-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 extern "C"
ABI of SIMD vector types depends on target features (tracking issue for abi_unsupported_vector_types
future-incompatibility lint)
#116558
Comments
For a hard error, I think it probably should go through RFC. I can start drafting that now. As for what it would check, I'd expect a hard error on any time a simd type is passed by value and the appropriate feature is unavailable, even if the specific ABI wouldn't necessarily pass it in registers, including inside a structure, and the lint should probably check the same thing. |
That can have potentially surprising effects, e.g. when a closure captures something of type It also means adding adding a private field that contains by-val |
This would already be the case with replacing several fields with a SIMD type. For example, changing Although thinking about it, because features can be per-crate, this might be a problem on crates that specialize based on target features. Perhaps |
With RISC-V, you have |
But the ABI only changes when So that sounds like whether or not |
No, that is exactly it. Do I understand correctly that you essentially want to do something similar to |
I'm not an expert on target details. 🙈 Usually my purview ends when I have given precise abstract semantics to nasty pieces of unsafe Rust and MIR.^^ But every now and then terrible low-level CPU nightmares creep onto my territory and then I have to learn how to deal with them... So, I don't know what ARM does with eabi vs eabihf. But it sounds like they are considering "hard float" and "soft float" two different ABIs that presumably use different registers for argument passing (so they are indeed different ABIs), and then have corresponding target triples to tell apart the ABIs. So yeah that sounds like what I am suggesting we should do for other targets as well. (We could also consider different ABI strings, like "C" vs "C-softfloat" or so. No idea if that makes any sense.) Obviously we don't actually care whether the functions behind those ABIs use x87/d/f hardware instructions or do some softfloat stuff, we just care how arguments are passed. But sadly it seems that in LLVM, there's no way to say "you have the This may all be completely naive since I'm just coming at this with my principled approach of making everything safe or at least sound and giving it semantics at a high level; I rely on people like @chorman0773 to tell me when my ideas make no sense "in the field". ;) |
If |
Yeah for the Rust ABI we do apply a work-around (it always passes such types in memory), so that call is fine. But the combination of (I updated the example to actually use the Rust ABI for |
Would adding a |
Inlining should be aware of target features, so Yes such an intermediate Rust ABI function works around the problem successfully. But the fundamental problem remains with function pointers: we have to decide whether an |
Ah, the |
The compiler does report |
And that's supposed to be just a warning. We should hard error if we know everything is fucked and there's no salvaging the compilation (whether hard erroring is appropriate here, I don't know yet). |
There's no C code involved here so I would argue it is reasonable to silence the lint for the example. The lint talks about FFI, but the example does no FFI, this is all Rust-to-Rust calls. |
Is there an example that can trigger this with only FFI-safe types though? |
I've actually suppressed that lint for |
I don't think If we did anything with ABI based on |
@briansmith our ctypes lints seem to have enough gaps to be easily circumvented; here's a reproducer that the lint doesn't catch. It's using a closure to pass the value -- closures that capture a single field are newtypes around that field so they get the field's ABI, but the improper-ctypes lint ignores closures. |
More to the point, there's no formal definition of "FFI-safe" and "FFI-unsafe" in the Rust language. As far as I can tell, the actual thing that bears the obligation to be ABI-correct is when you do extern "C" {
fn c_func(arg: SomeType) -> ReturnType;
}
fn rust_func() -> ReturnType {
let arg = SomeType::random();
// here, the Rust caller actually discharges the obligation...
// which is honestly slightly unfortunate:
// in real scenarios, we might not be the author of the `extern "C"` block!
unsafe { c_func(arg) }
} |
Do we know if clang is doing anything clever here? I assume C has exactly the same problem, since their function types don't track target features either. |
clang does not fix this issue. See llvm-project#64706 |
That's a different issue (mismatch with GCC). This here is about mismatch within code compiled by the same compiler, but with different locally set target features. |
…iler-errors Emit warning when calling/declaring functions with unavailable vectors. On some architectures, vector types may have a different ABI depending on whether the relevant target features are enabled. (The ABI when the feature is disabled is often not specified, but LLVM implements some de-facto ABI.) As discussed in rust-lang/lang-team#235, this turns out to very easily lead to unsound code. This commit makes it a post-monomorphization future-incompat warning to declare or call functions using those vector types in a context in which the corresponding target features are disabled, if using an ABI for which the difference is relevant. This ensures that these functions are always called with a consistent ABI. See the [nomination comment](rust-lang#127731 (comment)) for more discussion. Part of rust-lang#116558 r? RalfJung
LoongArch psABI[^1] specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack. This patch addresses the following warnings: ``` warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5 | 3695 | __lsx_vreplgr2vr_b(a) | ^^^^^^^^^^^^^^^^^^^^^ function called here | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#116558 <rust-lang#116558> = note: `#[warn(abi_unsupported_vector_types)]` on by default ``` [^1]: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc
LoongArch psABI[^1] specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack. This patch addresses the following warnings: ``` warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5 | 3695 | __lsx_vreplgr2vr_b(a) | ^^^^^^^^^^^^^^^^^^^^^ function called here | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#116558 <rust-lang#116558> = note: `#[warn(abi_unsupported_vector_types)]` on by default ``` [^1]: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc
…er-errors,uweigand Support s390x z13 vector ABI cc rust-lang#130869 This resolves the following fixmes: - https://github.com/rust-lang/rust/blob/58420a065b68ecb3eec03b942740c761cdadd5c4/compiler/rustc_target/src/abi/call/s390x.rs#L1-L2 - https://github.com/rust-lang/rust/blob/58420a065b68ecb3eec03b942740c761cdadd5c4/compiler/rustc_target/src/spec/targets/s390x_unknown_linux_gnu.rs#L9-L11 Refs: Section 1.2.3 "Parameter Passing" and section 1.2.5 "Return Values" in ELF Application Binary Interface s390x Supplement, Version 1.6.1 (lzsabi_s390x.pdf in https://github.com/IBM/s390x-abi/releases/tag/v1.6.1) This PR extends ~~rust-lang#127731 rust-lang#132173 (merged) 's ABI check to handle cases where `vector` target feature is disabled. If we do not do ABI check, we run into the ABI problems as described in rust-lang#116558 and rust-lang#130869 (comment), and the problem of the compiler generating strange code (rust-lang#131586 (comment)). cc `@uweigand` `@rustbot` label +O-SystemZ +A-ABI
Rollup merge of rust-lang#131586 - taiki-e:s390x-vector-abi, r=compiler-errors,uweigand Support s390x z13 vector ABI cc rust-lang#130869 This resolves the following fixmes: - https://github.com/rust-lang/rust/blob/58420a065b68ecb3eec03b942740c761cdadd5c4/compiler/rustc_target/src/abi/call/s390x.rs#L1-L2 - https://github.com/rust-lang/rust/blob/58420a065b68ecb3eec03b942740c761cdadd5c4/compiler/rustc_target/src/spec/targets/s390x_unknown_linux_gnu.rs#L9-L11 Refs: Section 1.2.3 "Parameter Passing" and section 1.2.5 "Return Values" in ELF Application Binary Interface s390x Supplement, Version 1.6.1 (lzsabi_s390x.pdf in https://github.com/IBM/s390x-abi/releases/tag/v1.6.1) This PR extends ~~rust-lang#127731 rust-lang#132173 (merged) 's ABI check to handle cases where `vector` target feature is disabled. If we do not do ABI check, we run into the ABI problems as described in rust-lang#116558 and rust-lang#130869 (comment), and the problem of the compiler generating strange code (rust-lang#131586 (comment)). cc `@uweigand` `@rustbot` label +O-SystemZ +A-ABI
…pes, r=jieyouxu show abi_unsupported_vector_types lint in future breakage reports The lint is now riding the train to 1.84. Given that crater found no case of this lint triggering at all, IMO it's fine to make it "report in deps" already for 1.85. Part of rust-lang#116558.
…pes, r=jieyouxu show abi_unsupported_vector_types lint in future breakage reports The lint is now riding the train to 1.84. Given that crater found no case of this lint triggering at all, IMO it's fine to make it "report in deps" already for 1.85. Part of rust-lang#116558.
To my great surprise, we knew about this issue since 2019: #64609. We just did nothing about it for >4 years... |
This can replace several old issues marked I-unsound, so let's mark this one as well. |
Rollup merge of rust-lang#133374 - RalfJung:abi_unsupported_vector_types, r=jieyouxu show abi_unsupported_vector_types lint in future breakage reports The lint is now riding the train to 1.84. Given that crater found no case of this lint triggering at all, IMO it's fine to make it "report in deps" already for 1.85. Part of rust-lang#116558.
LoongArch psABI[^1] specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack. This patch addresses the following warnings: ``` warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5 | 3695 | __lsx_vreplgr2vr_b(a) | ^^^^^^^^^^^^^^^^^^^^^ function called here | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#116558 <rust-lang#116558> = note: `#[warn(abi_unsupported_vector_types)]` on by default ``` [^1]: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc
LoongArch psABI[^1] specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack. This patch addresses the following warnings: ``` warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5 | 3695 | __lsx_vreplgr2vr_b(a) | ^^^^^^^^^^^^^^^^^^^^^ function called here | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#116558 <rust-lang#116558> = note: `#[warn(abi_unsupported_vector_types)]` on by default ``` [^1]: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc
…ubilee ABI checks: add support for loongarch LoongArch psABI[^1] specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack. This patch addresses the following warnings: ``` warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5 | 3695 | __lsx_vreplgr2vr_b(a) | ^^^^^^^^^^^^^^^^^^^^^ function called here | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#116558 <rust-lang#116558> = note: `#[warn(abi_unsupported_vector_types)]` on by default ``` [^1]: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc r? `@workingjubilee`
Rollup merge of rust-lang#133249 - heiher:loong-abi-check, r=workingjubilee ABI checks: add support for loongarch LoongArch psABI[^1] specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack. This patch addresses the following warnings: ``` warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5 | 3695 | __lsx_vreplgr2vr_b(a) | ^^^^^^^^^^^^^^^^^^^^^ function called here | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#116558 <rust-lang#116558> = note: `#[warn(abi_unsupported_vector_types)]` on by default ``` [^1]: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc r? `@workingjubilee`
The following program has UB, for very surprising reasons:
The reason is that the ABI of the
__m256
type depends on the set of target features, so the caller (with_target_feature
) and callee (no_target_feature
) do not agree on how the argument should be passed. The result is a vector half-filled with junk. (The same issue also arises in the other direction, where the caller has fewer features than the callee: example here.)Currently, we have no good way to do correct code generation here. See #132865 for a discussion of how we could support such code in the future; it will require some non-trivial work. So instead, the current plan is to reject such code entirely.
This is the tracking issue for the lint that moves us in that direction. The hope is that passing SIMD vectors across a C ABI is sufficiently rare, and most of the cases being rejected have anyway already been broken, that we can reject this without much of an ecosystem impact. Crater showed no regressions.
This is linted against since Rust 1.84, and shown in future breakage reports starting with Rust 1.85. The plan is to move to a hard error with Rust 1.87.
When this becomes a hard error:
Original issue text:
I'm not sure if this is currently even properly documented? We are mentioning it in #115476 but the program above doesn't involve any function pointers so we really cannot expect people to be aware of that part of the docs. We show a general warning about the type not being FFI-compatible, but that warning shows up a lot and anyway in this case both caller and callee are Rust functions!
I think we need to do better here, but backwards compatibility might make that hard. @chorman0773 suggested we should just reject functions like
no_target_feature
that take an AVX type by-val without having declared the AVX feature. That seems reasonable; a crater run would be needed to assess whether it breaks too much code. An alternative might be to have a deny-by-default lint that very clearly explains what is happening.There are also details to work out wrt what exactly the lint should check. Newtypes around
__m256
will have the same problem. What about other larger types that contain__m256
? Behind a ptr indirection it's obviously fine, but what about(__m256, __m256)
? If we apply theScalarPair
optimization this will be passed in registers even on x86.A possible place to put the check could be somewhere around here.
In terms of process, I am not sure if an RFC is required; a t-compiler MCP might be sufficient. Currently we accept code that clearly doesn't do what it looks like it should do.
And finally -- are there any other targets (besides x86 and x86-64) that have target-features that affect the ABI? They should get the same treatment.
Note that this is different from #116344 in two ways:
__m256
by-pointer exactly to work-around this issue.)#[target_feature]
), the other issue comes about when the user disables features (which is only possible on a per-crate level via-C
, and if you're mixing crates with different-C
flags then you're already already on very shaky grounds -- we do that with std but nobody else really gets to do that, I think).Cc @workingjubilee more ABI fun ;)
The text was updated successfully, but these errors were encountered: