search

rust-lang / libc

Raw bindings to platform APIs for Rust
https://docs.rs/libc
Apache License 2.0
2.12k stars 1.04k forks source link

c_char is u8 for uclib on x86_64 #1284

Open gnzlbg opened 5 years ago

gnzlbg commented 5 years ago

Should be i8.

valerius21 commented 5 years ago

It is not compatible with the std::ffi::CStr::from_ptr method, which does take an u8. On Ubuntu 19.04, it is defined as an i8.

gnzlbg commented 5 years ago

That's not really a problem, CStr::from_ptr is not a portable method nor does it intend to be, and CStr is not even FFI safe, so it cannot really be used to interface with C. If you want to handle C strings you need libc::c_char and then you want that to have the correct signedness, which is target dependent.

Amanieu commented 4 years ago

CStr::from_ptr takes a c_char, which is defined in the libc crate. It could be argued that this is a breaking change though, since we are changing the definition of a public type.

gnzlbg commented 4 years ago

Does libstd re-export libc types?

If so, that’s probably a libstd bug.

gnzlbg commented 4 years ago

notice that, eg, for the raw os types like pthreads_t, libstd defines their own, which are different from those in the libc crate, and dont “map” to the same underlying types.

Lokathor commented 4 years ago

std::os::raw makes their own declarations and then has a test that their declarations match the libc declarations.

https://doc.rust-lang.org/src/std/os/raw/mod.rs.html

gnzlbg commented 4 years ago

Sure, for tier-1 targets this test runs, but this test does not run for any of the tier 2 or tier 3 targets, and for example std::os::raw declarations do not match libc for the musl targets, which are quite popular. That is, there are already types for which libc and std types do not match, because std has bugs that would be breaking changes to fix.

The problem here is that the bug is in both, the libc crate, and the std::os::raw types, and while libc can fix this bug, std cannot fix it without breaking backward compatibility. As soon as somebody sends a PR for libc to fix this bug, I'll review it, and there really aren't any good reasons not to merge it for libc. If the standard library think that preserving backward compatibility on the uclib target is something worth pursuing over fixing this bug, they can define their own c_char type that's incorrect for uclibc.

I personally don't think that preserving these bugs on not super-widely used targets is worth doing, but that's something that should be discussed in rust-lang/rust with the libs team. For example, for the musl targets, it was decided not to fix the bugs upstream, which meant a couple of crates broke (rustc, nix, jobserver, rand, ..) when libc fixed the bugs, but we sent PRs to fix them, and essentially those crates are now written in such a way in which changes to these types don't break their build (e.g. instead of using libc::foo(raw::pthreads_t) they now use libc::foo(raw::pthreads_t as _)). This means that it probably would be possible to fix these bugs for musl in libstd if somebody wanted to without any major breakage, but nobody has proposed to do that.

IMO the real underlying problem here is that the standard library stabilizes libc types for some targets without testing them and without manually verifying them, and that's going to lead to bugs that the standard library then cannot fix. The uclibc types, for example, aren't tested anywhere (nor in libstd, nor in libc), so whoever decided to stabilize them for this target, made IMO a mistake. In this particular case, a user has manually verified them, and found a bug. That should have been the minimum procedure required to stabilize anything.