Skip to content

Conversation

@mbyx
Copy link
Contributor

@mbyx mbyx commented Aug 2, 2025

Description

Changes the types of fields with name __pad, _pad, __padding, _padding, __rsvd, __unused to use a new type wrapper around MaybeUninit as per #1453

Only modifies private fields so that the changes can be backported.

Sources

Checklist

  • Relevant tests in libc-test/semver have been updated
  • 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

src/types.rs Outdated
@@ -0,0 +1,12 @@
use core::mem::MaybeUninit;

/// A transparent wrapper over `MaybeUninit<T>` to represent uninitialized padding.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// A transparent wrapper over `MaybeUninit<T>` to represent uninitialized padding.
/// A transparent wrapper over `MaybeUninit<T>` to represent uninitialized padding
/// while providing `Default`.

To clarify why we don't just use Default

@tgross35
Copy link
Contributor

tgross35 commented Aug 9, 2025

@rustbot blocked
on ctest2

@tgross35
Copy link
Contributor

@mbyx could you put up a separate PR that only adds the Padding type? I'd like to start directing new PRs to use this.

@mbyx mbyx force-pushed the libc-wrap-padding branch 4 times, most recently from b1e6991 to e2a5397 Compare August 14, 2025 08:08
@tgross35 tgross35 changed the title libc: wrap padding fields with Padding newtype. Wrap private padding fields with Padding newtype Sep 23, 2025
@tgross35
Copy link
Contributor

Would you mind rebasing this? CI will still fail because of the eq/ord traits, but with #4711 the Debug messages should be resolved.

@mbyx mbyx force-pushed the libc-wrap-padding branch from e2a5397 to 271d31b Compare September 25, 2025 17:39
@tgross35
Copy link
Contributor

tgross35 commented Nov 2, 2025

I think most of the errors should be resolved with 141aea1. If you would be able to rebase again, I think this might be able to merge with a few smaller deltas.

@rustbot author

@rustbot
Copy link
Collaborator

rustbot commented Nov 2, 2025

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

@mbyx mbyx force-pushed the libc-wrap-padding branch 4 times, most recently from 77679b6 to 3f1c30f Compare November 4, 2025 14:18
@mbyx
Copy link
Contributor Author

mbyx commented Nov 4, 2025

Rebased. Seems to error because Padding doesn't implement Hash, Eq, and PartialEq, because the s macro tries to implement it for the struct. Would implementing those for Padding be well defined?

Also it seems that the style check fails at the first formatting error? My editor seems to have a different style for imports than libc so I may have caused breakage in more files than just the one that the style check shows.

@tgross35
Copy link
Contributor

tgross35 commented Nov 4, 2025

Rebased. Seems to error because Padding doesn't implement Hash, Eq, and PartialEq, because the s macro tries to implement it for the struct. Would implementing those for Padding be well defined?

That is what I was thinking, would you be able to add a commit that adds these implementations if extra_traits is enabled? Hash should do nothing and PartialEq should return true, so they have no effect on the results.

Also it seems that the style check fails at the first formatting error? My editor seems to have a different style for imports than libc so I may have caused breakage in more files than just the one that the style check shows.

That config file changed somewhat recently, if you rebased without re-saving the files in your editor or running locally then it might not have picked that up.

@mbyx mbyx force-pushed the libc-wrap-padding branch 4 times, most recently from f7e75a4 to 33b5c86 Compare November 5, 2025 16:27
@mbyx
Copy link
Contributor Author

mbyx commented Nov 5, 2025

That seems to fix most errors, except now we have a bit of an issue. One of the constants in unix/bsd/netbsdlike/netbsd/mod.rs, PTHREAD_MUTEX_INITIALIZER needs to initialize the padding field, but Default::default is not a const function. We can't make a seperate const function either because MaybeUninit::zeroed was made a const function in 1.75 and MSRV is 1.63.

For now I'll try to just not make that type using padding and see if I can get something mergeable.

EDIT: Seems to be many more constants like this. Even if those are ignored for now a good number should still be wrapped.

@mbyx mbyx force-pushed the libc-wrap-padding branch 3 times, most recently from a66f7c2 to 6afc113 Compare November 5, 2025 16:43
@mbyx mbyx marked this pull request as ready for review November 5, 2025 16:56
@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2025

Some changes occurred in OpenBSD module

cc @semarie

Some changes occurred in the Android module

cc @maurer

Some changes occurred in solarish module

cc @jclulow, @pfmooney

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.

All the switches to Padding LGTM, just a few small things about the traits

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you split the changes to this file to a separate commit? It's distinct from the rest of the mechanical changes in this PR.

Comment on lines 35 to 47
#[cfg(feature = "extra_traits")]
impl<T: Copy> Hash for Padding<T> {
fn hash<H: hash::Hasher>(&self, _state: &mut H) {}
}

#[cfg(feature = "extra_traits")]
impl<T: Copy> PartialEq for Padding<T> {
fn eq(&self, _other: &Self) -> bool {
true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These are user-facing, so add a doc comment to each of these impls saying their behavior (i.e. that they give the results they do to act like the fields don't exist)

@tgross35
Copy link
Contributor

tgross35 commented Nov 5, 2025

You should rebase as well, musl downloads have been flaky so I just switched us to a mirror

@mbyx mbyx force-pushed the libc-wrap-padding branch from 6afc113 to 607e39d Compare November 6, 2025 13:30
@rustbot
Copy link
Collaborator

rustbot commented Nov 6, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@mbyx mbyx force-pushed the libc-wrap-padding branch from 607e39d to 38d07d4 Compare November 6, 2025 13:32
@mbyx mbyx requested a review from tgross35 November 6, 2025 13:33
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