-
Notifications
You must be signed in to change notification settings - Fork 1.1k
musl: 64-bit time support #4463
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
base: main
Are you sure you want to change the base?
Conversation
Hmm, seems like the style checker isn't happy with mixing |
Since #4433 seems close to merging, I can rebase on top of it once merged. |
8b32bb3
to
e91f182
Compare
Rebased on top of the GNU changes :) |
Thanks! Sorry it's taken me a while, I'll try to look at this very soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Left a few comments here.
For the style check, are the changes it is suggesting possible? If not, that will probably have to be updated unfortunately (hopefully we will be able to reorganize things at some point so it's less annoying...)
Thanks for the review! I'll get around to implementing the changes hopefully in the next few days. I'll try figure out how to pass the style lints - but it might need a new file. After this merges and when 1.0 is closer to release feel free to ping me on GitHub and I can help fully complete the transition for these changes in the 1.0 branch on the musl side :). |
#[cfg(all(target_arch = "x86_64", target_pointer_width = "32"))] | ||
pub tv_nsec: i64, | ||
#[cfg(not(all(target_arch = "x86_64", target_pointer_width = "32")))] | ||
pub tv_nsec: c_long, | ||
#[cfg(all(musl_time64, target_endian = "little"))] | ||
__pad0: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playing with this PR, this makes constructing timespec structs directly as was done in std fail:
error: cannot construct `timespec` with struct literal syntax due to private fields
--> library/std/src/sys/pal/unix/thread.rs:251:30
|
251 | let mut ts = libc::timespec {
| ^^^^^^^^^^^^^^
|
= note: ...and other private field `__pad0` that was not provided
I'm not quite sure what to suggest here -- making these pub doesn't really make sense, and even if we did I believe we'd need something like ..Default::default()
to make it work so users of libc::timespec (including the compiler) will need to be fixed?... :/
At this build step there are only 4 (as of rust-lang/rust@255aa22) so this is probably not too bad, but here goes my hopes to see this backported in 0.2 :p
(for reference:
- library/std/src/sys/pal/unix/thread.rs:251:30
- library/std/src/sys/pal/unix/time.rs:10:5
- library/std/src/sys/pal/unix/time.rs:199:14
- library/std/src/sys/fs/unix.rs:1507:24
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, just reporting it's working as expected here by working around this issue by making the fields pub
here and patching std as follow:
rust-lang/rust diff
diff --git a/Cargo.lock b/Cargo.lock
index 1a619096d34c..6f91779999af 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2046,8 +2046,6 @@ checksum = "9fa0e2a1fcbe2f6be6c42e342259976206b383122fc152e872795338b5a3f3a7"
[[package]]
name = "libc"
version = "0.2.174"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1171693293099992e19cddea4e8b849964e9846f4acee11b3948bcc337be8776"
[[package]]
name = "libdbus-sys"
diff --git a/Cargo.toml b/Cargo.toml
index c4d2a06f4cb1..12757101281e 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -89,3 +89,5 @@ codegen-units = 1
# FIXME: LTO cannot be enabled for binaries in a workspace
# <https://github.com/rust-lang/cargo/issues/9330>
# lto = true
+[patch.crates-io.libc]
+path = "/mnt/rust/libc"
diff --git a/library/Cargo.lock b/library/Cargo.lock
index 34012d6943b7..b112d7ab9ac6 100644
--- a/library/Cargo.lock
+++ b/library/Cargo.lock
@@ -142,8 +142,6 @@ dependencies = [
[[package]]
name = "libc"
version = "0.2.174"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1171693293099992e19cddea4e8b849964e9846f4acee11b3948bcc337be8776"
dependencies = [
"rustc-std-workspace-core",
]
diff --git a/library/Cargo.toml b/library/Cargo.toml
index 35480b9319d7..955ae17cca35 100644
--- a/library/Cargo.toml
+++ b/library/Cargo.toml
@@ -51,3 +51,5 @@ rustc-std-workspace-core = { path = 'rustc-std-workspace-core' }
rustc-std-workspace-alloc = { path = 'rustc-std-workspace-alloc' }
rustc-std-workspace-std = { path = 'rustc-std-workspace-std' }
compiler_builtins = { path = "compiler-builtins/compiler-builtins" }
+[patch.crates-io.libc]
+path = "/mnt/rust/libc"
diff --git a/library/std/src/sys/fs/unix.rs b/library/std/src/sys/fs/unix.rs
index dc278274f00f..db1fc8a527f0 100644
--- a/library/std/src/sys/fs/unix.rs
+++ b/library/std/src/sys/fs/unix.rs
@@ -1504,7 +1504,12 @@ pub fn set_times(&self, times: FileTimes) -> io::Result<()> {
io::ErrorKind::InvalidInput,
"timestamp is too small to set as a file time",
)),
- None => Ok(libc::timespec { tv_sec: 0, tv_nsec: libc::UTIME_OMIT as _ }),
+ None => Ok(libc::timespec {
+ tv_sec: 0,
+ tv_nsec: libc::UTIME_OMIT as _,
+ #[cfg(target_pointer_width = "32")]
+ __pad0: 0,
+ }),
};
cfg_if::cfg_if! {
if #[cfg(any(target_os = "redox", target_os = "espidf", target_os = "horizon", target_os = "nuttx"))] {
diff --git a/library/std/src/sys/pal/unix/thread.rs b/library/std/src/sys/pal/unix/thread.rs
index d8b189413f4a..20002a4caf16 100644
--- a/library/std/src/sys/pal/unix/thread.rs
+++ b/library/std/src/sys/pal/unix/thread.rs
@@ -251,6 +251,8 @@ pub fn sleep(dur: Duration) {
let mut ts = libc::timespec {
tv_sec: cmp::min(libc::time_t::MAX as u64, secs) as libc::time_t,
tv_nsec: nsecs,
+ #[cfg(target_pointer_width = "32")]
+ __pad0: 0,
};
secs -= ts.tv_sec as u64;
let ts_ptr = &raw mut ts;
diff --git a/library/std/src/sys/pal/unix/time.rs b/library/std/src/sys/pal/unix/time.rs
index 0074d7674741..262f2d70fb18 100644
--- a/library/std/src/sys/pal/unix/time.rs
+++ b/library/std/src/sys/pal/unix/time.rs
@@ -6,8 +6,12 @@
const NSEC_PER_SEC: u64 = 1_000_000_000;
pub const UNIX_EPOCH: SystemTime = SystemTime { t: Timespec::zero() };
#[allow(dead_code)] // Used for pthread condvar timeouts
-pub const TIMESPEC_MAX: libc::timespec =
- libc::timespec { tv_sec: <libc::time_t>::MAX, tv_nsec: 1_000_000_000 - 1 };
+pub const TIMESPEC_MAX: libc::timespec = libc::timespec {
+ tv_sec: <libc::time_t>::MAX,
+ tv_nsec: 1_000_000_000 - 1,
+ #[cfg(target_pointer_width = "32")]
+ __pad0: 0,
+};
// This additional constant is only used when calling
// `libc::pthread_cond_timedwait`.
@@ -199,6 +203,8 @@ pub fn to_timespec(&self) -> Option<libc::timespec> {
Some(libc::timespec {
tv_sec: self.tv_sec.try_into().ok()?,
tv_nsec: self.tv_nsec.as_inner().try_into().ok()?,
+ #[cfg(target_pointer_width = "32")]
+ __pad0: 0,
})
}
made our axum-based server run properly after changing the date past 2038.
(I'd be curious if there's a better way to test this though, perhaps with -Zbuild-std
?, but this is good enough for high level testing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah - I'm not sure what the best path forward for this is. I believe the GNU changes will also result in a similar issue. I guess I will make the migration process to 1.0 more painful. I think I can make another PR to rust
in that case. I think consumers will need to use ..Default::default()
. Another problem I can think of is people naively not including that when compiling for 64bit locally breaking things for time64 users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another problem I can think of is people naively not including that when compiling for 64bit locally breaking things for time64 users.
Yeah, if we go the default way I think we'll want to make ..Default::default()
mandatory -- I 've just tried and it's possible with a zero-length field like PhantomData (...except I guess we can't use that in libc, so we'll need another way to express it)
For the transition we could add the default trait so someone could add ..Default::default()
now even if not required (... but that triggers a clippy warning...)
(Oh, and there's also deconstruction by pattern matching that'll need a , ..
...)
At this point if we're breaking API though there might be a better way, implicit cast through a well defined struct with the current fields? ... or maybe there's even a way to specify the physical representation of the fields without adding explicit padding? (but if there is I sure don't know 😁 )
Anyway, yeah, this will probably make the transition to 1.0 a bit painful so it's probably worth spending a bit of time now to figure out the path of least resistance...
9b5ca40
to
f669296
Compare
This feature is enabled with independently from musl_v1_2_3 to support time64. Defining this feature makes this roughly equivalent to upstream commit bminor/musl@f12bd8e.
This is equivalent to upstream commit bminor/musl@9b2921b.
This corresponds to upstream commit bminor/musl@1febd21 (most symbols) and bminor/musl@22daaea (only dlsym)
A bunch of properties were removed upstream and set to reserved. This matches upstream commit bminor/musl@827aa8f and bminor/musl@2d69fcf
Change time_t type to i64 Change struct stat, msqid_ds and shmid_ds to reflect This commit follows upstream bminor/musl@3814333 and bminor/musl@d6dcfe4 It also implements a fix from bminor/musl@0fbd7d6
This is primarily based on a small part of bminor/musl@3814333. This also integrates bminor/musl@3c02bac, which update MSG_STAT, SEM_STAT, SEM_STAT_ANY. These are based on the value of IPC_STAT, however we can just use `cfg` as it is effectively the same.
This fixes test failures on musl.
This was incorrectly named in upstream musl and fixed in bminor/musl@cabc369 This commit adds SIGEMT unconditionally as adding a constant should not cause any backwards-incompatible changes.
This is because the struct is different depending on whether time64 is enabled.
Thanks for waiting - I've fixed most of the points, however I haven't done the style check just yet. It also looks like the |
Description
This change includes time64 support for applicable architectures (x86, arm, mips and powerpc). This is based on the previous PRs to this repo as well as the musl changelog from 1.1.24 -> 1.2. It can be enabled with the environment variable
RUST_LIBC_UNSTABLE_MUSL_TIME64
only when musl_v1_2_3 is enabled and the architecture is supported.A lot of structures, especially ones with mixed endian became excessively complicated, so I used
cfg_if
to separate them. It looks like you can only nests! {}
incfg_if! {}
, but not vice versa.As a note, I'm considering removing
musl_not_time64
, and just keeping the old logic of allowing deprecated for function definitions involvingtime_t
as it introduces necessary complexity.When libc 1.0 is released, I believe the best path would be to remove the
musl_v1_2_3
feature, making it unconditionally enabled, keepingmusl_time64
which will expand to(musl && time64_arch)
.Tested through QEMU for all architectures.
Sources
Sources are located on each commit, in the form of upstream commits
Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see rust-lang/libc#3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI
@rustbot label +stable-nominated