musl: 64-bit time support

[xbjfk]

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 nest s! {} in cfg_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 involving time_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, keeping musl_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

• Relevant tests in libc-test/semver have been updated
• No placeholder or unstable values like *LAST or *MAX are
  included (see rust-lang/libc#3131)
• Tested locally (cd libc-test && cargo test --target mytarget);
  especially relevant for platforms that may not be checked in CI

@rustbot label +stable-nominated

[xbjfk]

Hmm, seems like the style checker isn't happy with mixing s! {} and cfg_if! {}, and putting cfg_if! {} inside an s! {} block does not seem to work either. I'm not sure what the best course of action is regarding this.

[xbjfk]

Since #4433 seems close to merging, I can rebase on top of it once merged.

[xbjfk]

Rebased on top of the GNU changes :)

[tgross35]

Thanks! Sorry it's taken me a while, I'll try to look at this very soon

[tgross35]

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...)

[xbjfk]

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 :).

[martinetd]

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
  )

[martinetd]

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)

[xbjfk]

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.

[martinetd]

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...

[xbjfk]

I'm not certain, but wouldn't PhantomData be fine if it is a member? I know ZST for FFI is generally not allowed - but the C code does not directly access the zero sized type? I just tested it and can see that rust does not report improper_ctypes if a struct contains PhantomData is used as an extern "C" argument - it does report it however if the only member of the struct is PhantomData.

We should probably also start some migration documentation of 1.0 changes, and how to fix them.

[xbjfk]

I thought about this - I think I will open another PR that will not be stable-nominated specifically for this.

[martinetd]

My first reaction was that it makes sense to split the thorny part out for more discussion/refining, but thinking back it's probably wrong to use time64 variant functions with a mismatched type (as in will appear to work but fail subtly because ns are not where the callee thinks they are), so I think this has to stay together?
Even if the cost is that this never makes it to stable, I don't see how we could make a "partial time64 support" work properly.

I'll try to play some with struct shaping next week to see if we can do better than relying on default; I've never heard of anything that we could use to shape the struct without affecting the API but it can't hurt to spend a bit more time on it...

[xbjfk]

Actually, I just realized - we don't need to use PhantomData hacks at all - we can just mark it as #[non_exhaustive] for all other architectures - this will have essentially the same effect, forcing people to use ..Default::default() and such. Also, I think the idea is to fully enable time64 support and remove 32 bit time support, at least on musl.

You are right regarding the types - I meant to split making timespec non_exhaustive globally out.

[xbjfk]

I created #4508 for visibility - let me know if you find a better alternative!

[tgross35]

In 1.0 we'll be making everything non_exhaustive, we more or less want everyone to go via MaybeUninit::zeroed and write individual fields like you have to do with C. Don't worry about it here while the flags are unstable.

See #4080 for a bit more.

[xbjfk]

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 cfg! change broke test compilation - there is a panic in garando_syntax.

[xbjfk]

Okay, I have made a bit more progress (not yet pushed). I ended up adding a whitelist for the "positive cfg" error - and whitelisted musl32_time64 as well as target_endian.

The duplicate s! macro seems to be erroneously counting top-level/unconditional s! macros as the the same as s! macros within cfg_if. I might try find a way around this some other way if I can't figure out how to resolve the issue.

I have investigated the style crash when using cfg! and found that returning Ok(None) in resolve_invoc in ctest here causes the issue. Always returning Err(Determinacy::Determined) seems to not crash. Looking at garando, it seems to assume - resolve_invoc returns either Err, Ok(...) or be InvocationKind::Attr.

@alexcrichton Sorry for the ping but potentially you could help here? I know it's been a while since you wrote the ctest code referenced.

If we don't manage to resolve this, I think I will just revert back to separate statements, or maybe using cfg_if and adding a fixme.

[tgross35]

Thanks for the updates!

@alexcrichton Sorry for the ping but potentially you could help here? I know it's been a while since you wrote the ctest code referenced.

I don't even think Alex could make ctest work at this point 😆 it uses a very old parser that sometimes rejects newer syntax like ? in macros. It's borderline nondeterministic (at least, the only pattern I've found is that it hates FreeBSD), touching entirely unrelated code will cause something totally unrelated to consistently fail.

We have a rewrite that we should be able to switch to in a couple of weeks, if you value your sanity enough to wait for that. It will take me a bit of time to review everything here anyway.

[alexcrichton]

Heh yes sorry at this point I'm no more an expert in ctest than anyone else!

[xbjfk]

Pushed changes. Happy to wait for the ctest rewrite while we review :). I might need some help with the duplicate s! macro style checker, and how it marks else branches as duplicate with s! macros not in cfg_if.