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

What to do about repr(C, simd)? #47103

Open
hanna-kruppe opened this issue Jan 1, 2018 · 23 comments
Open

What to do about repr(C, simd)? #47103

hanna-kruppe opened this issue Jan 1, 2018 · 23 comments
Labels
A-FFI Area: Foreign function interface (FFI) A-repr Area: the `#[repr(stuff)]` attribute A-SIMD Area: SIMD (Single Instruction Multiple Data) C-enhancement Category: An issue proposing an enhancement or a PR with one. F-simd_ffi `#![feature(simd_ffi)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hanna-kruppe
Copy link
Contributor

Currently, repr(C, simd) warns about "incompatible representation hints", but leaving off the repr(C) triggers the FFI lint when the SIMD type is used in FFI.

Note that #[repr(C)] #[repr(simd)] does not warn only due to #47094. Most uses of SIMD in FFI this in the Rust source tree use two separate repr attributes, which is why they didn't trigger this warning so far.

There's two ways to resolve this:

  1. Require repr(C, simd) for SIMD types used in FFI.
  2. Say repr(simd) is sufficient for FFI (in principle -- there are other concerns that keep SIMD-FFI feature gated at the moment) and keep the warning about repr(C, simd). It could optionally restricted to some simd types that are known to correspond to C types on the platform in question (e.g., permit f32x4 but not f32x3).

cc @alexcrichton

@alexcrichton
Copy link
Member

Oh I was just under the impression we want to unconditionally ban simd types from C APIs. AFAIK the ABI is highly dependent on how C and Rust are compiled (target features and whatnot) and it's very tricky to get them to match up.

@hanna-kruppe
Copy link
Contributor Author

hanna-kruppe commented Jan 1, 2018

Passing vectors as immediates has all these problems and therefore is feature gated separately (feature(simd_ffi)) but if SIMD types are not considered C-compatible, even passing pointers to them will trigger the lint, which I imagine would be very annoying.

@alexcrichton
Copy link
Member

Oh sure yeah, I'd expect &mut u8x32 to work just fine, I'd only expect passing u8x32 as a bare value to "basically never work"

@retep998
Copy link
Member

retep998 commented Jan 2, 2018

Doesn't the alignment of SIMD types depend on which target features are enabled? If so, isn't it bad when a pointer to a SIMD type is passed around when it was allocated with a lower alignment than expected by the receiving end?

@hanna-kruppe
Copy link
Contributor Author

hanna-kruppe commented Jan 2, 2018

On the Rust side, it appears vectors are always naturally aligned (rounded up to the next power of two when needed? not that it matters for types you would see in FFI). The same seems to be true in C.

And now that I've thought about it, I can't really see how it could be any other way – type alignment is a global property, while target_feature can vary by function.

@hanna-kruppe
Copy link
Contributor Author

So in the light of the above discussion I would propose:

  • Declare some SIMD types FFI-safe irrespective of target features, based on architecture and element type and vector length. Which ones exactly is tbd, should probably be based on what C toolchains offer on the respective platform.
  • Keep warning on repr(C, simd).
  • Keep the simd_ffi feature gate which rules out passing SIMD types as immediates (this prevents any ABI mismatches due to different target_features)

hanna-kruppe pushed a commit to hanna-kruppe/rust that referenced this issue Jan 7, 2018
As a side effect, this fixes the warning about repr(C, simd) that has been reported during x86_64 windows builds since rust-lang#47111 (see also: rust-lang#47103)
kennytm added a commit to kennytm/rust that referenced this issue Jan 8, 2018
…richton

Replace empty array hack with repr(align)

As a side effect, this fixes the warning about repr(C, simd) that has been reported during x86_64 windows builds since rust-lang#47111 (see also: rust-lang#47103)

r? @alexcrichton
@pietroalbini pietroalbini added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-FFI Area: Foreign function interface (FFI) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-SIMD Area: SIMD (Single Instruction Multiple Data) labels Jan 30, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 28, 2018

Declare some SIMD types FFI-safe irrespective of target features, based on architecture and element type and vector length. Which ones exactly is tbd, should probably be based on what C toolchains offer on the respective platform.

@rkruppe do you have a concrete example that works? That is, a platform, toolchain, and SIMD type, where passing a SIMD type by pointer to C via FFI works?

Do the target-features of the Rust and C binaries cause problem when passing for example a *mut f32x8 from Rust to C or vice-versa?

@hanna-kruppe
Copy link
Contributor Author

hanna-kruppe commented Mar 28, 2018

I'm not aware of any ABI problems with pointers to vectors -- pointers are pointers are pointers. The only possible problem would be alignment, but that's irrespective of target features as discussed above. The only reason I propose to lint for types like f64x128 is that they don't have a common C equivalent (well, not quite true, there is a little-used GCC extensions for arbitrarily long vector types, but our FFI lint shouldn't care about that sort of thing).

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 28, 2018

@rkruppe I would leave the lint out, at least initially. If somebody uses these types in C FFI, whatever C compiler they are using is already going to lint these for them.

@hanna-kruppe
Copy link
Contributor Author

Since when do C compilers have such lints? (Since when can they even have such lints? C has no concept of a function defintion specifically for FFI, and surely you wouldn't want to warn for functions used by another translation unit of the same program.)

If we don't want to get into the business of defining which vector types are fine for FFI or not, I'd rather unconditionally consider them ffi-unsafe, i.e., always lint on the Rust side. This is closer to what @alexcrichton originally proposed (though a lint, not a hard error, since the ABI concerns are nowhere near as strong).

@hanna-kruppe
Copy link
Contributor Author

But I realize now that some people might be using repr(C, simd) to get a guaranteed layout, not for FFI. For example, to permit type punning in memory. Do we want to support that, and guarantee the layout of vector types? Would be analogous to arrays, I believe.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 28, 2018

@rkruppe

Since when do C compilers have such lints? (Since when can they even have such lints? C has no concept of a function defintion specifically for FFI, and surely you wouldn't want to warn for functions used by another translation unit of the same program.)

What I meant is that when one uses these types on Rust FFI, one would also need to write "some types" in the C side of things. If some types are not supported by the users' C toolchain, then the user would get a compilation or linking error on the C side of things.

@hanna-kruppe
Copy link
Contributor Author

But GCC does support those types as I mentioned. Allowing any and all repr(simd) types in FFI means we're basically comitting to supporting whatever GCC does with those.

@hanna-kruppe
Copy link
Contributor Author

Also the FFI lint is also useful for catching errors where you think you have matching signatures on the Rust and C sides but the Rust signature "can't be right". For example, translating C's char to Rust's char.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 28, 2018

@rkruppe to do this right we need to know which C toolchain the user wants to use, what support does this toolchain have for repr(simd) types, and allow those.

But we can't know this.

@hanna-kruppe
Copy link
Contributor Author

I'm fine with considering all repr(simd) types non-ffi-safe. We probably still need to define the layout, though.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 28, 2018

I would be fine with allowing all repr(simd) types on FFI via pointers, I just think that linting here is hard to do correctly.

If we are calling into a C function from Rust, it is fine to assume that the C compiler supports the repr(simd) type since otherwise no C library can be generated. If the signatures between the Rust and C code do not match, we should obviously produce a good error.

If we are exposing a Rust function to C, if the C compiler does not support the repr(simd) type it is its job to emit a diagnostic because we can't know what the C toolchain does or does not support.

@hanna-kruppe
Copy link
Contributor Author

I don't understand why you'd want to give no warnings at all. If there is uncertainty about or variance in what the C toolchain provides, surely we should err on the side of caution? Otherwise, we wind up implying that we match what the C compiler does, which seems impossible to guarantee if you say that "linting here is hard to do correctly". As another example, i128 is considered ffi-unsafe even though quite a few C toolchains support it.

If the signatures between the Rust and C code do not match, we should obviously produce a good error.

??? we can't. that's the whole problem with FFI.

If we are exposing a Rust function to C, if the C compiler does not support the repr(simd) type it is its job to emit a diagnostic because we can't know what the C toolchain does or does not support.

This lint doesn't even run for Rust-defined functions currently (#19834) but if it did, again, the purpose of the lint is also to warn about bindings that are very dubious on the Rust side, and therefore likely contain a human error.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 28, 2018

I think we are talking past each other (as usual :P). I am not against linting per se, I am against linting against some repr(simd) types but not others, because I cannot think of any good heuristic for how to separate them. Emitting a warning when someone uses any repr(simd) type on C FFI is fine to me.

@hanna-kruppe
Copy link
Contributor Author

OK, cool. (I still don't see how I could've possibly extracted that from your last few comments, but whatever.) That still leaves the issue of using repr(C) to define layout without any intent to do FFI.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 28, 2018

We probably still need to define the layout, though.

A lot of people expect this to work correctly:

union A { data: [f32; 4], vec: f32x4, }
let x: [f32; 4] = unsafe { A { vec: f32x4::new(1., 2., 3., 4.) }.data };
assert_eq!(x[2], 3.);

So I would be fine with defining vector types to have the same layout as arrays (EDIT: preserving the alignment requirements of the vector type).

@Centril Centril added the F-simd_ffi `#![feature(simd_ffi)]` label Jul 28, 2019
@workingjubilee
Copy link
Member

Hm. The 128-bit SIMD types (u8x16, i16x8, f32x4, u64x2) should be passable in registers on x86_64 to the System V AMD64 ABI, as far as I can tell.

__m128 can only be passed by pointer to the Windows "x64 calling convention".

But we can pass some types by register to __vectorcall on x86-64-pc-windows-msvc.

@RalfJung
Copy link
Member

Oh I was just under the impression we want to unconditionally ban simd types from C APIs. AFAIK the ABI is highly dependent on how C and Rust are compiled (target features and whatnot) and it's very tricky to get them to match up.

This is being tackled with #116558 -- we are only allowing calls involving those types when the right target features are present. That should ensure a consistent ABI.

So... allowing repr(C, simd) should be fine then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI) A-repr Area: the `#[repr(stuff)]` attribute A-SIMD Area: SIMD (Single Instruction Multiple Data) C-enhancement Category: An issue proposing an enhancement or a PR with one. F-simd_ffi `#![feature(simd_ffi)]` 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

8 participants