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

Update use of libc::timespec to prepare for future libc version #1886

Merged
merged 1 commit into from Nov 28, 2022

Conversation

wesleywiser
Copy link
Contributor

@wesleywiser wesleywiser commented Nov 28, 2022

In a future release of the libc crate, libc::timespec will contain private padding fields on *-linux-musl targets and so the struct will no longer be able to be created using the literal initialization syntax.

Update places where libc::timespec is created to first zero initialize the value and then update the tv_sec and tv_nsec fields manually. Many of these places are in const fns so a helper function zero_init_timespec() is introduced to help with this as std::mem::MaybeUninit::zeroed() is not a const function.

Some matches on libc::timespec are also updated to include a trailing .. pattern which works when libc::timespec has additional, private fields as well as when it does not (like for
x86_64-unknown-linux-gnu).

See also rust-lang/libc#2088

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

You should rebase on master, which already has the Clippy fixes. Otherwise, this change looks good.

src/sys/time.rs Outdated Show resolved Hide resolved
@@ -118,6 +116,7 @@ pub(crate) mod timer {
libc::timespec {
tv_sec: 0,
tv_nsec: 0,
..
Copy link
Member

Choose a reason for hiding this comment

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

What's the function of this line? Are you programming defensively, just in case freebsd, netbsd, dragonfly, or illumos grow some padding fields too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's required once Rust starts targeting musl 1.2 as i686-unknown-linux-musl will have a padding field:

$ cargo +e392e7b50913d5cb6e2c08d70cdeb72e56d289f9 build --target i686-unknown-linux-musl
   Compiling libc v0.2.137 (https://github.com/wesleywiser/libc?branch=musl-1.2#3049e59a)
   Compiling nix v0.25.0 (/tmp/nix)
error: pattern requires `..` due to inaccessible fields
   --> src/sys/time.rs:116:25
    |
116 | /                         libc::timespec {
117 | |                             tv_sec: 0,
118 | |                             tv_nsec: 0,
119 | |                         },
    | |_________________________^
    |
help: ignore the inaccessible and unused fields
    |
118 |                             tv_nsec: 0, ..,
    |                                       ++++

Copy link
Member

Choose a reason for hiding this comment

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

Oh, my bad. I saw that #[cfg()] block on line 100 and thought that it applied here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, thanks for the quick reviews! 🙂

In a future release of the `libc` crate, `libc::timespec` will contain
private padding fields on `*-linux-musl` targets and so the struct will
no longer be able to be created using the literal initialization syntax.

Update places where `libc::timespec` is created to first zero initialize
the value and then update the `tv_sec` and `tv_nsec` fields manually.
Many of these places are in `const fn`s so a helper function
`zero_init_timespec()` is introduced to help with this as
`std::mem::MaybeUninit::zeroed()` is not a `const` function.

Some matches on `libc::timespec` are also updated to include a trailing
`..` pattern which works when `libc::timespec` has additional, private
fields as well as when it does not (like for
`x86_64-unknown-linux-gnu`).
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@asomers
Copy link
Member

asomers commented Nov 29, 2022

@wesleywiser it looks like mem::transmute only became const in Rust 1.56.1. That means we can't backport this PR. Can you come up with a version that doesn't rely on mem::transmute? CC @rtzoeller .

@wesleywiser
Copy link
Contributor Author

wesleywiser commented Nov 30, 2022

Sorry for the delay! I'll work on the backports tomorrow.

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

Successfully merging this pull request may close these issues.

None yet

2 participants