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

ssize_t is not a standard C type? #13

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

ssize_t is not a standard C type? #13

gnzlbg opened this issue Feb 20, 2019 · 13 comments

Comments

@gnzlbg
Copy link

gnzlbg commented Feb 20, 2019

cc @retep998

The VC CRT does not provide ssize_t and libc bases its ssize_t on SSIZE_T from Windows API. I'd really rather not have ssize_t declared a truly universal type.

@Amanieu
Copy link
Contributor

Amanieu commented Feb 20, 2019

It is a type defined by POSIX. It is not in the C standard.

The standard equivalent is ptrdiff_t.

@Amanieu
Copy link
Contributor

Amanieu commented Feb 20, 2019

IMO we should just remove ssize_t from cty.

@japaric
Copy link
Owner

japaric commented Feb 20, 2019

Note that cty is not a crate with "type aliases to standard C types"; it's a crate with "Type aliases to C types like c_int for use with bindgen" so I would not remove any type alias that bindgen uses as that's the main use case. Whether that's the case for ssize_t with today's bindgen I do not know -- I haven't used bindgen in a while.

@gnzlbg
Copy link
Author

gnzlbg commented Feb 20, 2019

Does bindgen generate a libc::ssize_t or cty::ssize_t ? I thought it always translated ssize_t to isize. cc @emilio could you comment on that ?

IMO we should just remove ssize_t from cty.

So for supported targets, we do know whether the target has an ssize_t type, and if that's the case, what type does it map to (e.g. isize). So we can always expose it reliably.

One question is whether ssize_t should be available in targets that do not have it like some windows targets. If you are targeting a target without ssize_t, I expect that you won't need ssize_t while using bindgen because the headers for this target wouldn't use it either. Otherwise, something is really messed up, e.g., the headers one is feeding to bindgen correspond to a different target.

Another question is what shall we do if the user tries to use cty on a target that we don't know anything about? Should we always assume that such targets always have an ssize_t = isize ? Or shall we allow users to customize this behavior, e.g., via a cargo feature, etc. ?

@emilio
Copy link

emilio commented Feb 21, 2019

Bindgen always translates ssize_t and ptrdiff_t to isize, yeah. I'm not quite sure on the reason why, if any, if you want that changed and have a reasonable argument I'm happy to take patches for that.

@gnzlbg
Copy link
Author

gnzlbg commented Feb 21, 2019

I think we should tell bindgen to use the "<ctypes_crate>" here (whether from libc or the user specified crate / path) and I can send a PR for that.

Whether we should provide ssize_t here, I think, with the inclusion in libc in mind, that we can do that on platforms that support it, but we should not provide it otherwise. If someone complains that it is not available, we can explore what the issue is, and figure out how to solve it (e.g. we can add an optional cargo feature that enables these unconditionally or something).

How does that sound ?

@emilio
Copy link

emilio commented Feb 21, 2019

Sounds fine, but note that bindgen supports both libc and std. Does std provide some ssize_t alternative?

@gnzlbg
Copy link
Author

gnzlbg commented Feb 21, 2019

We can make bindgen use libc::ssize_t when the user wants to use the libc / cty types, and isize otherwise for the time being.

I don't know if libstd provides ssize_t, but if it does, it would probably be in the neighborhood of std::os::raw.

@emilio
Copy link

emilio commented Feb 22, 2019

We can make bindgen use libc::ssize_t when the user wants to use the libc / cty types, and isize otherwise for the time being.

What's the point of that, if cty::ssize_t is always isize? Is there a case where ssize_t wouldn't match isize?

@gnzlbg
Copy link
Author

gnzlbg commented Feb 22, 2019

@emilio We know that all targets that Rust currently supports, if they do provide ssize_t (some windows targets do not), then it is equal to isize.

I do not know if this is the case for all future targets that Rust will support (maybe somebody else knows what POSIX says about ssize_t?).

If we ever support a target for which, isize is 128-bit wide, but ssize_t is only 64-bit wide, then we will need to do a backwards incompatible change to cty, or libc, or both, and that was very painful for the whole ecosystem last time.

This doesn't impact bindgen much, as in, if bindgen chooses to always use isize, then if a new target pops up for which this does not hold, then we upgrade bindgen and call it a day. I don't expect that to be painful. But if we have to release a backwards incompatible version of libc because of this, then that would be very unfortunate.

@gnzlbg
Copy link
Author

gnzlbg commented Feb 22, 2019

@japaric it appears that, at least for the time being, bindgen doesn't require ssize_t at all. So what do you think about providing it in cty for the targets we know today, and not providing it for unknown targets?

@japaric
Copy link
Owner

japaric commented Feb 23, 2019

I know at least one platform where this fails: On MSP430X microcontrollers, some toolchains (notably recent versions of the gcc toolchain) use an uint20_t for size_t and an int20_t for ssize_t

From Stack Overflow. We support MSP430 at the moment but not MSP430X; IDK if the latter is supposed to be considered a different architecture when it comes to conditional compilation, but sounds like it would need type isize = i20 in any case since it has 20-bit instructions and pointers.

So what do you think about providing it in cty for the targets we know today, and not providing it for unknown targets?

Sounds good to me. How should we specify the "the targets we know today"? A mix of cfg(arch) and cfg(os)?

@gnzlbg
Copy link
Author

gnzlbg commented Feb 23, 2019

How should we specify the "the targets we know today"? A mix of cfg(arch) and cfg(os)?

Pretty much, this is how libc does it now, so we can probably adapt that. Even though it might not be technically necessary, we probably want to be conservative in the supported targets and do a minor semver version bump. If that happens to break somebody's workflow, we can then manually add support for more targets in a backwards compatible way.

EDIT: FWIW we managed to do this for libc by just being super careful and bumping the patch version a couple of months ago, so the minor version bump isn't strictly necessary. But currently cty does not have the same constraints than libc has, so I think I would prefer to do that anyways.

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

4 participants