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

Replace std::os::raw::c_ssize_t with std::os::raw::c_ptrdiff_t #90421

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Oct 30, 2021

The discussions in #88345 brought up that ssize_t is not actually the signed index type defined in stddef.h, but instead it's ptrdiff_t. It seems pretty clear that the use of ssize_t here was a mistake on my part, and that if we're going to bother having a isize-alike for FFI in std::os::raw, it should be ptrdiff_t and not ssize_t.

Anyway, both this and c_size_t are dubious in the face of the discussion in https://internals.rust-lang.org/t/pre-rfc-usize-is-not-size-t/15369, and any RFC/project-group/etc that handles those issues there should contend with these types in some manner, but that doesn't mean we shouldn't fix something wrong like this, even if it is unstable.

All that said, size_t is vastly more common in function signatures than either ssize_t or ptrdiff_t, so I'm going to update the tracking issue's list of unresolved questions to note that perhaps we only want c_size_t — I mostly added the signed version for symmetry, rather than to meet a need. (Given this, I'm also fine with modifying this patch to instead remove c_ssize_t without a replacement)

CC @magicant (who brought the issue up)
CC @chorman0773 (who has a significantly firmer grasp on the minutae of the C standard than I do)

r? @joshtriplett (original reviewer, active in the discussions around this)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2021
@thomcc
Copy link
Member Author

thomcc commented Oct 30, 2021

Actually, the numbers in #88345 (comment) has me leaning towards "only expose size_t because it significantly more important and other use cases can reach for something like libc or winapi".

Another argument in favor of this: bindgen had need for a --size_t-is-usize flag, but didn't need an equivalent for isize.

@joshtriplett
Copy link
Member

I don't think it makes sense to change this and not c_size_t; that seems inconsistent.

I'd suggest that we just leave these unstable until we settle the question of whether we support sizeof(ptrdiff_t) != sizeof(size_t).

@joshtriplett joshtriplett added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2021
@joshtriplett
Copy link
Member

(Also, props for the branch name.)

@thomcc
Copy link
Member Author

thomcc commented Oct 30, 2021

whether we support sizeof(ptrdiff_t) != sizeof(size_t).

I think you misunderstood this PR (sorry if I'm mistaken) — it's not intended to be a semantic change, beyond exposing a type that's more portable. In particular, ptrdiff_t isn't pointer-sized, it's only required to hold the result of subtracting two pointers, which in C is only valid if both pointers involved point to the same object.

The C standard even includes "The result of subtracting two pointers is not representable in an object of type ptrdiff_t" in the list behaviors left explicitly undefined, which seems to explicitly be a carve-out to allow targets sizeof(ptrdiff_t) < sizeof(void*).

While I can't find a stddef.h for a CHERI system, this document https://www.cl.cam.ac.uk/techreports/UCAM-CL-TR-947.pdf indicates that on CHERI sizeof(ptrdiff_t) is less than `sizeof(intptr_t):

The observable, integer range of a uintptr_t is the same as that of a vaddr_t (or ptrdiff_t for intptr_t), despite the increased alignment and storage requirements.

It also notes:

ptrdiff_t This integer type describes the difference of indices between two pointers to elements of the same array, and should not be used for any other purpose. It can be added to a pointer to obtain a new pointer, but the result will be dereferenceable only if the address lies within the bounds of the pointer from which it was derived. Less standards-compliant code sometimes uses ptrdiff_t when the programmer more likely meant intptr_t or (less commonly) size_t. When porting code, it is worth-while to audit use of ptrdiff_t.

which calls out not confusing it with intptr_t (or size_t, interestingly).

@chorman0773 indicated to me that this is also true for the proposed W65 ABI.


That is to say, I don't know that this would come with any equivalent change to c_size_t. This is just a change to a more portable signed size_t equivalent.

... That said, the more I think about it, the more I'm inclined to think that size_t probably justifies its own inclusion in std::os::raw, but none of the signed equivalents do.

@joshtriplett
Copy link
Member

joshtriplett commented Oct 31, 2021

Thank you for clarifying; I did indeed mix up ptrdiff_t with intptr_t. (I also wonder if it would ever not be the same as ssize_t.)

That said, it's still not at all clear to me why isize should be exposed as c_ptrdiff_t and not as c_ssize_t. It sounds like on unix platforms it's called ssize_t, and on Windows it's called SSIZE_T, with the only difference being capitalization. I think it's reasonable to expose both types as c_ssize_t.

I think we should also expose c_ptrdiff_t, though. I would be happy to r+ a PR adding that type (under the same feature).

That way, whatever integer type people encounter in a C API, they can use the corresponding FFI type rather than a less-self-documenting type they think may be equivalent.

@thomcc
Copy link
Member Author

thomcc commented Oct 31, 2021

I think we should also expose c_ptrdiff_t, though. I would be happy to r+ a PR adding that type (under the same feature).

I've updated the PR with this.

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 2, 2021

📌 Commit 8d19819 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 2, 2021
@bors
Copy link
Contributor

bors commented Nov 3, 2021

⌛ Testing commit 8d19819 with merge 87ec568...

@bors
Copy link
Contributor

bors commented Nov 3, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 87ec568 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 3, 2021
@bors bors merged commit 87ec568 into rust-lang:master Nov 3, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 3, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (87ec568): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants