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

Types in std::os::raw should be same as libc crate #47027

Open
clarfonthey opened this issue Dec 26, 2017 · 19 comments
Open

Types in std::os::raw should be same as libc crate #47027

clarfonthey opened this issue Dec 26, 2017 · 19 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@clarfonthey
Copy link
Contributor

Right now, there's a bit of a discrepancy between these two, even though there shouldn't be. While the libc crate has very complicated logic to determine how the c_* types are defined, the standard library uses a much simpler logic.

I'm not sure how much these types are desired outside of libc; the only type that I've really seen used across the standard library is c_char, which honestly could just be replaced with an opaque struct with repr(u8) if it weren't already stabilised.

@steveklabnik
Copy link
Member

I feel like this issue is a duplicate, but I'm not sure.

@clarfonthey
Copy link
Contributor Author

It was definitely listed in the original libc RFC as an unresolved question, and I found #36193, but I really didn't find anything that addressed this specific issue.

There are also issues regarding c_void (rust-lang/libc#180) but that's different than this.

@pietroalbini pietroalbini added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 30, 2018
@SimonSapin
Copy link
Contributor

New RFC to propose fixing for c_void specifically: rust-lang/rfcs#2521

Unlike c_void (which is currently an enum) the rest of the c_* types are type aliases, so two separate definitions of e.g. c_long would be compatible in the type system as long as they agree.

Are there cases where std and libc disagree on the definition of a given c_* type for a given target?

@clarfonthey
Copy link
Contributor Author

IMO, the fact that the code disagrees is an indication that that's probably the case. Regardless, I feel that the code for these should be the same, whether this is done by re-exporting or simply copying the code.

@SimonSapin
Copy link
Contributor

To make sure the definitions never diverge I think it would be best to have them in one place, and re-export as needed. So let’s keep this issue open even if rust-lang/rfcs#2521 is implemented.

@kinggoesgaming
Copy link

I noticed that c_char is u8 in std vs i8 in libc yesterday

@SimonSapin
Copy link
Contributor

On what target architecture? This doesn’t seem to be the case on Linux x64 at least:
https://play.rust-lang.org/?gist=1bd21020fc146082988516eb1779e066&version=stable&mode=debug&edition=2015

@retep998
Copy link
Member

retep998 commented Aug 9, 2018

A big example of a difference is SOCKET in winapi is different from std despite both being aliases to primitive numeric types, because std conditionally defines it as u32 or u64 while winapi unconditionally defines it as usize.

@kinggoesgaming
Copy link

Seemingly I was wrong... I was reading through the sources and apparently mixed up the cfgs

@kinggoesgaming
Copy link

Also I seem not the odd one one out here https://users.rust-lang.org/t/crate-types-not-the-same-which-one-should-i-use/19552

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Aug 15, 2018

Genuine question: would it make sense to maybe make all of the c_* types opaque repr(transparent) types?

This way, although they'll work in FFI as expected, Rust code has to use From and TryFrom to actually do conversions. Additionally, the From and TryFrom code can be tailored to only do what's valid in the C standard (e.g. assuming c_int is at least 16 bits, and no more) for portability. 99% of the time, From and co. will be no-ops, but it'll be required to actually do any operations on these types in Rust code.

We could even do this with the inner implementation of c_void, to mask the fact that it's an enum in its implementation. At some point we could potentially make it actually a newtype of ! if we really wanted.

@SimonSapin
Copy link
Contributor

We can add new types like that. But changing the existing std::os::raw::c_* and libc::c_* types would be a breaking change.

@clarfonthey
Copy link
Contributor Author

I think that adding new types is reasonable, if they go into core::ffi like c_void seems to be headed. Putting them in os::raw seems off.

@SimonSapin
Copy link
Contributor

These new types wouldn’t solve this issue’s problem (so they are not quite on topic), and I feel there would be enough details to figure out that they’d be worth their own RFC.

@madsmtm
Copy link
Contributor

madsmtm commented Jun 7, 2022

std::os::raw has recently been moved to core::ffi under an unstable feature flag, so that may be able to solve the duplication issue between libc and std in the future (at some point in the far future when libc drops support for Rust versions that doesn't have this).

Tracking issue here in case you want to provide input: #94501

@joshtriplett
Copy link
Member

libc currently does rustc version detection, so libc could turn these into re-exports when built on a sufficiently new rustc.

@SimonSapin
Copy link
Contributor

Do the definitions agree for all targets? As long as they do there’s no effect for users, for type aliases to primitive integers (as opposed to c_void which is defined as an enum), so choosing to reexport or not might only be a question of what makes maintenance easier. For example when adding support for a new target.

@clarfonthey
Copy link
Contributor Author

Was going through my open issues and was wondering whether this is solved or not. The linked #94501 is merged, and I believe that the types do match now. Should this still be tracked in the standard library, or just as an issue in the libc repo?

@sanmai-NL
Copy link

@joshtriplett What's the current status for this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants