c2rust icon indicating copy to clipboard operation
c2rust copied to clipboard

`long double` varargs won't compile

Open TheDan64 opened this issue 6 years ago • 7 comments

error[E0277]: the trait bound `f128::f128: core::ffi::sealed_trait::VaArgSafe` is not satisfied
   --> src/vfwprintf.rs:789:44
    |
789 |                                         ap.arg::<f128::f128>().to_f64().unwrap()
    |                                            ^^^ the trait `core::ffi::sealed_trait::VaArgSafe` is not implemented for `f128::f128`

error: aborting due to 2 previous errors

Blocked on improved rustc support for arbitrary structs/types, possibly https://github.com/rust-lang/rust/pull/61126

TheDan64 avatar Sep 04 '19 14:09 TheDan64

f128 is implemented in its crate as

#[repr(C)]
#[derive(Clone, Copy)]
pub struct f128(pub(crate) [u8; 16]);

At first I thought the fix was as simple as enabling VaArgSafe for aggregates, but looking at the rustc backend I now realize that va_arg is only implemented for scalars, so the compiler would crash with an ICE for any aggregate. To fix f128, we'd need to implement full aggregate support for va_arg in rustc.

Edit: the problem is that the current variadics implementation in rustc lowers Rust va_arg down to LLVM va_arg, which doesn't support aggregates. clang gets around this by manually implementing va_arg itself using memory operations.

ahomescu avatar Sep 12 '19 20:09 ahomescu

f128 is implemented in its crate as

#[repr(C)]
#[derive(Clone, Copy)]
pub struct f128(pub(crate) [u8; 16]);

Why wouldn't f128 be implemented using u128? Doesn't [u8; 16] have the wrong alignment? bindgen uses u128 for long double (if u128 is supported). If u128 is used as the underlying type, does that fix this issue, as u128 is a scalar?

Also, https://github.com/rust-lang/rust/pull/61126 seems to be about unsized types, but f128 and [u8; 16] aren't unsized.

https://github.com/rust-lang/rfcs/pull/2580 is also now merged, if that was a blocker for https://github.com/rust-lang/rust/pull/61126.

@LegNeato, this may be a reason to not use f128 (#745).

kkysen avatar Nov 28 '22 06:11 kkysen

bindgen uses u128 for long double

From https://github.com/rust-lang/rust-bindgen/issues/1549 it sounds like they're not entirely happy with that either.

Why wouldn't f128 be implemented using u128?

IMHO the real problem is that variadics are part of core, but there is no "official" f128 equivalent in Rust. The language and the standard library would first have to pick one, and then add variadics support for it. The f128 type that C2Rust uses (or used, not sure we do anymore) is from an external crate.

You could define f128 as a newtype over u128, but I'm not sure that would get accepted into core.

ahomescu avatar Nov 28 '22 06:11 ahomescu

Wow, I didn't realize {u,i}128 was not FFI-safe and that LLVM misaligns it to 8 bytes, not 16 (so it would have the wrong alignment for f128, but then so do {u,i}128).

I think the best way to support this would be to get a c_long_double added in libc (hopefully that happens one day), as what we want to do is match the C long double anyways, and long double is not always f128 (__float128 would be different, and would always be f128). But I'm not sure what libc's definition for c_long_double would be.

I also saw that libm is missing the *l functions that operate on long doubles.

kkysen avatar Nov 28 '22 07:11 kkysen

https://github.com/aaronfranke/rfcs/blob/3453-f16-and-f128/text/3453-f16-and-f128.md

LegNeato avatar Oct 05 '23 17:10 LegNeato

https://github.com/rust-lang/rust/issues/116909

LegNeato avatar Nov 02 '23 03:11 LegNeato

https://github.com/rust-lang/rust/issues/116909

The RFC for f{16,128} got accepted? Awesome!

kkysen avatar Nov 02 '23 04:11 kkysen