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

Complex struct should be #repr(C) #79

Closed
awelkie opened this issue Apr 5, 2015 · 16 comments
Closed

Complex struct should be #repr(C) #79

awelkie opened this issue Apr 5, 2015 · 16 comments

Comments

@awelkie
Copy link

awelkie commented Apr 5, 2015

The memory layout is probably already identical to C's complex type or std::complex<T>, but it'd be nice to make it explicit. This would help with C interop, and things like real-only FFTs that need to cast real-only vectors into vectors of alternating real/imaginary.

@bluss
Copy link
Contributor

bluss commented Dec 24, 2015

It's more complicated than that, C's complex type needs its own handling in the C ABI and rust can't do that yet. See RFC issue here: rust-lang/rfcs/issues/793

That said, the representation is the same when it comes to accessing complex numbers in memory, so for example BLAS bindings still work with it as it is today: it's only complex by-value function parameters and return values that are not c-compatible as it is now.

Due to being usable with ffi when accessed behind a pointer, I think it would be good to mark it #[repr(C)]? but it needs a highlighted doc to explain the limitations.

@harpocrates
Copy link

Any chance of this happening? Even when numbers are passed by value, AFAIC tell most modern platforms also use this representation.

Would you need a PR?

@cuviper
Copy link
Member

cuviper commented Aug 8, 2016

I'm really hesitant to add this with such documented limitations. People are prone to ignoring that sort of thing until it blows up in their face. Maybe it could be added just for the known-good architectures with cfg_attr. Even then it's a little weird to declare repr(C) for all generic Complex<T>, when it only makes sense for f32 and f64.

Another possibility is to solve it in the hypothetical libm crate, which would present it's own ABI-compatible versions of complex types. We could then add From implementations in both directions for the Complex type here. But I wonder how common it would be to straddle these worlds -- I expect most people would either stay in pure-Rust here or use purely libm.

@clamydo
Copy link

clamydo commented Aug 9, 2016

But I wonder how common it would be to straddle these worlds -- I expect most people would either stay in pure-Rust here or use purely libm.

I'd disagree. When interfacing with certain numeric centric libraries with C-ABI, interfacing with _Complex independent of libm can be necessary. For example when calling to FFTW3, it expects an array of double[2]; which is guaranteed to be represented the exactly as _Complex in memory:

505 Each complex type has the same representation and alignment requirements as an array type containing exactly two elements of the corresponding real type;

This does not involve the libm, but nonetheless uses a _Complex representation. I worked around it by defining my own complex type as a tuple struct struct Complex<T>([T; 2]);, which is represented most likely the same as [T; 2] (although AFAIK it is not guaranteed by rustc).

It would be super nice, if we could interface directly with the C _Complex from rust. But I'm not sure, if a two field struct (as in num) with #[repr(C)] meets the requirements of being aligned identically with a double[2]/_Complex in memory. Does anybody has references about this?

@cuviper
Copy link
Member

cuviper commented Aug 9, 2016

OK, if libm is too specific, then it probably belongs in libc itself. Something like c_complex_float and c_complex_double which are C-ABI compatible, and num's Complex can cheaply convert. But I'm still really hesitant to make any repr(C) promises in num itself.

@hauleth
Copy link
Member

hauleth commented Aug 10, 2016

Could anyone link the section of C99 standard that describes memory layout of _Complex? Because I think that this is implementation defined and if so interop between num_complex::Complex and _Complex would be impossible as it would be easy way to blow up everything.

@clamydo
Copy link

clamydo commented Aug 10, 2016

@hauleth See my post above.
505 in http://c0x.coding-guidelines.com/6.2.5.html

@hauleth
Copy link
Member

hauleth commented Aug 10, 2016

@fkjogu ok. So now we need to be sure that our struct with #[repr(C)] will have the same memory layout as float[2] or double[2].

@cuviper
Copy link
Member

cuviper commented Aug 10, 2016

The memory layout is one thing, but the ABI for complex parameters is certainly implementation defined.

@cuviper
Copy link
Member

cuviper commented Aug 11, 2016

FWIW, it's easy to approximate this, if you want to deal with ABI on your own:

#[repr(C)]
struct c_complex_double([f64; 2]);

impl From<Complex64> for c_complex_double {
    fn from(c: Complex64) -> Self {
        c_complex_double([c.re, c.im])
    }
}

impl From<c_complex_double> for Complex64 {
    fn from(c: c_complex_double) -> Self {
        Complex64::new(c.0[0], c.0[1])
    }
}

@clamydo
Copy link

clamydo commented Aug 11, 2016

@cuviper Good point. But I'm not sure, what #[repr(C)] actually means for a tuple struct. Does handle Rust tuple structs completely transparent? Is this guaranteed?

Also I'm never sure what consequences this type of copy conversation has on performance in real applications. Definitely more, than just passing a pointer, for some amount of more.

@cuviper
Copy link
Member

cuviper commented Aug 11, 2016

I don't know about guarantees, but tuple structs should be transparent as long as you don't implement something like Drop, which adds a hidden drop flag (although that should eventually go away). There's even a warning for this:

warning: implementing Drop adds hidden state to types, possibly conflicting with `#[repr(C)]`, #[warn(drop_with_repr_extern)] on by default

Besides, you can only attach #[repr(C)] to structs or enums in the first place. An array like [f64;2] is surely already C-compatible, but I'm not sure whether that is guaranteed.

As for the performance of copy conversions, I'd expect it to have negligible overhead. This is completely transparent to the optimizer, so in some cases the copies might be avoided entirely. I would avoid conversions back and forth in the loop of a math kernel though.

@bluss
Copy link
Contributor

bluss commented Nov 11, 2016

If it does not have #[repr(C)], then crates will need to migrate off using num's complex. It would be unsound (strictly speaking, but in practice not a problem) for numerical projects to use num's complex when integrating with complex-consuming libraries like BLAS.

@cuviper
Copy link
Member

cuviper commented Nov 11, 2016

If it does not have #[repr(C)], then crates will need to migrate off using num's complex.

I think that's overstated. It just means crates just shouldn't use num::Complex directly for FFI is all. I think we really need something in libc for this use case.

It would be unsound (strictly speaking, but in practice not a problem) for numerical projects to use num's complex when integrating with complex-consuming libraries like BLAS.

Adding #[repr(C)] still doesn't correct the parameter ABI, as you stated up front. That's my major gripe here, that this wouldn't actually be solving the whole issue to make this FFI compatible.


That said, I think I can relent. It doesn't look like #[repr(C)] shows up directly in the docs anyway, so it's only discoverable insofar as we describe it. If you want to open a PR with a cautious explanation in the documentation, I'll accept it. FFI is inherently unsafe, so folks always need to be careful, but make sure it's clear what the caveats are in this case.

@bluss
Copy link
Contributor

bluss commented Nov 11, 2016

Great. Using a different type for ffi would mean using a different type all the time. (Converting an ndarray of thousands of entries is not done for free, can't do that just to call a function.)

@IvanUkhov
Copy link
Contributor

Regarding libc, I just want to link this issue to that one.

@homu homu closed this as completed in #275 Apr 4, 2017
homu added a commit that referenced this issue Apr 4, 2017
Complex: Use repr(C) and add documentation for what it means

Here's an ambitious proposal that puts currently practiced ffi usage of `Complex<f32/f64>` on sound footing.

Fixes  #79

Current users appear to be:

- https://crates.io/crates/blas
  + Their use is not about Complex<f64> in an extern function's signature, but it is explicitly using that it is memory layout compatible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants