rust-num / num

A collection of numeric types and traits for Rust.
Apache License 2.0
1.05k stars 145 forks source link

Complex struct should be #repr(C) #79

Closed awelkie closed 7 years ago

awelkie commented 9 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

hauleth commented 8 years ago

@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 commented 8 years ago

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

cuviper commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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