Skip to content

Replace check_shim with check_shim_abi in unix/foreign_items shims #4263

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

Merged
merged 1 commit into from
Apr 17, 2025

Conversation

geetanshjuneja
Copy link
Contributor

Links to #3842.

@geetanshjuneja geetanshjuneja changed the title Replace check_shim with check_shim_abi Replace check_shim with check_shim_abi in unix/foreign_items shims Apr 10, 2025
@geetanshjuneja
Copy link
Contributor Author

Would you prefer I update all the remaining shims in this file as part of this PR, or should I submit a separate PR for that? Updating all check_shim calls here would result in a large change, which might make the review process more difficult.

@RalfJung
Copy link
Member

No please don't make this PR any bigger. :)

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! You uncovered a few cases where we are not properly consistent with our types.

We generally assume that long is isize and unsigned long is usize, so please also keep doing that here. Same for size_t and ssize_t, those are usize and isize, respectively.

For types like off_t, off64_t, clockid_t, we should be using their proper types, but some functions do not. So please do one of the following:

  • change the logic inside these functions to use the proper type. Typically this means using e.g. to_int(this.libc_ty_layout("off_t").size) instead of to_i32().
  • or keep the logic the same, put i32 into the check_shim_abi invocation, and add a comment saying "FIXME: should be off_t".

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Apr 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 10, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@geetanshjuneja
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Apr 11, 2025
@RalfJung
Copy link
Member

This looks great, thanks! Please squash the commits, then we can land this. Please use the --keep-base flag when squashing so that the force-push diff is easier to review; do not rebase on the latest master branch. Then write @rustbot ready after you pushed the squashed PR.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 16, 2025
…me related shims

Making type consistent in shims

pread return type fix

make clock_gettime shim type consistent
@geetanshjuneja
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Apr 16, 2025
@RalfJung RalfJung added this pull request to the merge queue Apr 17, 2025
Merged via the queue into rust-lang:master with commit 527e2a0 Apr 17, 2025
7 checks passed
@geetanshjuneja geetanshjuneja deleted the check_shim_abi branch April 17, 2025 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants