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

Roadmap: move cty into the libc repo #14

Open
gnzlbg opened this issue Feb 20, 2019 · 5 comments
Open

Roadmap: move cty into the libc repo #14

gnzlbg opened this issue Feb 20, 2019 · 5 comments

Comments

@gnzlbg
Copy link

gnzlbg commented Feb 20, 2019

I talked with @japaric on Discord, and we agreed on moving the cty crate into the libc repo. This should get it the same kind of testing as libc, and get some more hands maintaining this crate (e.g. adding support for new targets in a backwards compatible way).

We could stop there.


However, there have been a couple of issues open in the past about addressing the use case of just using C types, without bringing in the whole libc (rust-lang/rust#36193 , rust-lang/rust#47027, rust-lang/libc#881, rust-lang/libc#375 , rust-lang/libc#1244 ).

For libraries doing this, it would be great if these types were also the same C types that libc uses, so that if they expose them in their API, a dependency that does use libc internally can just use them to interface with libc (and std::raw, etc.).

This is something that would require review by the libs team, because:

  • it would "bless" cty as the way to obtain C types,
  • cty would become a dependency of libstd via the libc crate.

Iff we were to do that, there is one aspect of cty's design that differs from libcs and that is worth calling out: libc is empty on unsupported platforms, while cty is not.

The cty crate provides some types on all platforms, past, present, and future ones. This seems like a no brainer for, e.g., int16_t. C implementations do not need to provide this type, so one design would be to only conditionally provide it. But if a C implementation provides this type, there is only one way in which it could be provided AFAICT, and that is type int16_t = i16; [0]. So the design decision of always providing this type, which is what cty does, seems fine to me.

However, cty also provides other C types on all platforms, like ssize_t (#13 ) or intptr_t (see CHERI comments by @gankro and @Amanieu : rust-lang/unsafe-code-guidelines#52 (comment)), where the argument that there is only one single way in which C could provide this is less clear.

If we make cty a dependency of libc, it would be unfortunate to have to perform a breaking change in the cty crate (and therefore libc), if we ever add a platform in which some of these default types are incorrect. We might be able to get away with this breaking change then, because the platform wasn't officially supported before, therefore we would be breaking something that was unstable. But if, for example, we decide to not provide ssize_t by default anymore on unsupported platforms, we will probably end up breaking a lot of people using xargo to build unsupported platforms as a consequence.

The safest design is to make cty empty on unsupported platforms, that is, we would need to manually vetto new platforms into cty support. However, some users want "reasonable" C types in unsupported platforms (e.g. rust-lang/libc#1244). In that PR, my recommendation was to allow that behind some cargo feature. That is, when the user enables a feature in cty, if the platform is unsupported, we would expose some "reasonable" C types (depending on arch, etc.) instead of having the crate be empty.

cc @SimonSapin @denzp


[0] I suppose that if the platform has padding bits or trap representations in the int16_t type, then i16 would probably do so as well - I'm not 100% sure what we guarantee about this in the Rust side.

@Amanieu
Copy link
Contributor

Amanieu commented Feb 20, 2019

Have you considered the alternative of just adding those types to core::ffi like c_void already is?

@gnzlbg
Copy link
Author

gnzlbg commented Feb 20, 2019

Have you considered the alternative of just adding those types

Not much, yet. The bar for adding those to core::ffi is the same as for libc: these types have to be correct on all past, present, and future platforms. Also, because libc supports older Rust toolchains, cty could only re-export these from core::ffi for toolchains for which this is available, and will still need to continue to provide these themselves on older toolchains. That is, we have to get this right for cty/libc anyways.

I suspect that once we get this right for the cty crate, then "moving" these to core::ffi should be easy. All the work making sure that these are correct can be reused in an RFC that proposes that, and we'll already have the experience that "we did it for libc and nothing broke".

Implementation-wise, we probably want to avoid duplicating the maintenance of all of this. So what will happen is that core::ffi will actually depend on a "special" build of cty and re-export some of its types, which then cty gets to use when libcore is available as a dependency. But I'd rather worry about these implementation details later, since all of this still requires having a well functioning cty crate.

@denzp
Copy link
Contributor

denzp commented Feb 21, 2019

What do you think about moving cty into a separate module within libc - let's say libc::generic_cty?

Then whitelisted targets can implicitly reexport (or override some of them) types from the submodule, and users of not-supported targets will have to explicitly specify the types from libc::generic_cty?

Eventually, in order to achieve a complete concistency:

  • make core::ffi reexport the libc::generic_cty::*;
  • make std::os::raw reexport target-specific libc::{c_int, ...} types for the whitelisted targets.

Later we can go deeper and let a user build only "the C types" out of the libc with crate features.

@madsmtm
Copy link

madsmtm commented Jun 7, 2022

std::os::raw has recently been moved/duplicated to core::ffi under an unstable feature flag, tracking issue here: rust-lang/rust#94501

@chrysn
Copy link

chrysn commented Aug 1, 2022

… and has been stabilized aiming for 1.64.

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

5 participants