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

Copy definitions from core::ffi and centralize them #4256

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

pheki
Copy link
Contributor

@pheki pheki commented Feb 3, 2025

Description

This PR removes definitions of FFI types that already exist in core and re-export them instead. The objective is to reduce duplication, to:

  • Avoid making target maintainers having to change / add types in both places, potentially forgetting to update one of them.
  • Avoid clashing definitions between std definitions and libc. For example, recently c_char changed from i8 to u8 for various targets, if you have a dependency that gets a *const libc::c_char and passes it to CStr::from_ptr, an older Cargo.lock will fail to compile in Rust 1.84.1 as e.g. CStr::from_ptr expects a *const i8 but libc 0.2.149 returns *const u8. This will not happen for future versions of libc if it re-exports these types from core.

Edit: after thinking more about it, do we even need to re-export these on 1.0? Can't we just remove them and let our users use core::ffi directly? (for 0.2 we still need them for backwards compatibility)

MSRV

Unfortunately, these definitions were only stabilized in core on Rust 1.84 1.64, so this would required a bump in the MSRV...

There's still no policy for MSRV, but the current value (1.83 1.63) was chosen as a conservative option due to being the one available in Debian Stable: #4040. The first freeze for the new Debian release (trixie) is scheduled to start on March and the Hard Freeze on May. Looking at the release history, it's probable that we will get the new version later this year.

So if this PR is to be approved, I'm guessing we could either merge it now or wait until Debian 13 "trixie" arrives.

Checklist

  • Relevant tests in libc-test/semver have been updated: no API change
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI: This impacts all targets, I only tested it for my target.

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2025

Some changes occurred in solarish module

cc @jclulow, @pfmooney

Some changes occurred in OpenBSD module

cc @semarie

Some changes occurred in the Android module

cc @maurer

@tgross35
Copy link
Contributor

tgross35 commented Feb 3, 2025

Thank you for putting this together! I think that you are right that we should probably drop these entirely for 1.0, but don't do this now - that will probably have to be done not long before 1.0 is actually released (I am trying not to let the branches get too different so I can still feasibly diff between them).

Since this requires an MSRV change, we can't change 0.2 now as you pointed out. MSRV will be revisited after Trixie but I don't know what exactly the new minimum will be.

One concern is that we just did the big update to correct c_char; if anybody on pre-1.85 rustc has updated anything to correct for this, changing to reexport core would break them again.

@rustbot blocked

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

I opened #4257 to track removing the reexports.

Based on the concerns above, I don't want to jump to reexporting core::ffi. I think we could still use the rest for cleanup though. Could you drop the changes to src/lib.rs and the MSRV bump? Instead, rename fixed_width_ints.rs to primitives.rs and copy the definitions from https://github.com/rust-lang/rust/blob/613bdd49978298648ed05ace086bd1ecad54b44a/library/core/src/ffi/mod.rs (without the type_alias macro).

This can then work for both branches.

@pheki
Copy link
Contributor Author

pheki commented Feb 4, 2025

Thank you for putting this together!

No prob! :)

Could you drop the changes to src/lib.rs and the MSRV bump? Instead, rename fixed_width_ints.rs to primitives.rs and copy the definitions from https://github.com/rust-lang/rust/blob/613bdd49978298648ed05ace086bd1ecad54b44a/library/core/src/ffi/mod.rs (without the type_alias macro).

Done! I did not copy the c_char docs as they would probably fall out of sync. I updated the module docs a bit but it may still need some improvements.

Just one clarification, I previously wrote 1.83 and 1.84, but I actually meant 1.63 and 1.64.

@pheki pheki force-pushed the use-core-ffi-types branch from 91ce791 to 6840e30 Compare February 4, 2025 23:58
@pheki
Copy link
Contributor Author

pheki commented Feb 5, 2025

@rustbot ready

@pheki pheki changed the title Re-export FFI definitions from core instead of re-defining them Copy definitions from core::ffi and centralize them Feb 5, 2025
Comment on lines 4 to 5
//! The platform-specific types definitions were taken from rust-lang/rust in
//! library/core/src/ffi/mod.rs and should be kept in sync while the MSRV is below 1.64.
Copy link
Contributor

Choose a reason for hiding this comment

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

//! The platform-specific types definitions were taken from rust-lang/rust in
//! library/core/src/ffi/primitives.rs

The file moved, and probably no need to mention MSRV here because of the other issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

One nit then lgtm. We should clean up intptr_t and others in a similar way, but that doesn't have to happen here.

@pheki pheki force-pushed the use-core-ffi-types branch from 6840e30 to 95446f4 Compare February 6, 2025 03:07
@pheki pheki force-pushed the use-core-ffi-types branch from bc79689 to b54607f Compare February 6, 2025 04:15
@pheki
Copy link
Contributor Author

pheki commented Feb 6, 2025

Updated the docs, rebased and squashed. Tests failed for 3 targets due to files not using the prelude (the definitions were in the same file), so I fixed them in a new commit to make it easier to review.

We should clean up intptr_t and others in a similar way

Yeah, that makes sense. Specifically for intptr_t and uintptr_t I also wonder if they could be deprecated, as just like e.g. int8_t is the same as i8, they are just aliases for isize and usize.

but that doesn't have to happen here

👍

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the cleanup!

@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Feb 7, 2025
@tgross35 tgross35 added this pull request to the merge queue Feb 7, 2025
Merged via the queue into rust-lang:main with commit f6d00b4 Feb 7, 2025
44 checks passed
@pheki pheki deleted the use-core-ffi-types branch February 7, 2025 22:27
@pheki
Copy link
Contributor Author

pheki commented Feb 7, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants