Switch to 64 bit file and time APIs on GNU libc for 32bit systems

[snogge]

Use the 64 bit types and APIs included in GNU libc also for 32-bit systems. These are the types and APIs used when you compile your C code against GNU libc headers with the preprocessor options -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64.

This is required for 2038 compatible software built for 32-bit systems.

This PR is not done, I need some guidance with finishing it.

1. Does it need some sort of configuration options to turn this on and off?
2. There is a conflict for some of the stat functions where two signatures link against the same symbol. For C this is not a problem, but we probably need to make struct stat and struct stat64 be different names for the same struct. I don't know how to do that.
3. It's possible that some types should be moved/split/joined in the file hierarchy. I'd appreciate some feedback on that, but I'm waiting on the answer to question 1 before moving forward.

I've done one commit for each modified type so it is easier to review the changes in isolation.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[snogge]

Oops, left some debug/test commits in there. Removed now.

[snogge]

Hi @JohnTitor , do you have any feedback on this change? Is there anything I can do to facilitate the review? I'd appreciate any hints on solving question 2:

2. There is a conflict for some of the stat functions where two signatures link against the same symbol. For C this is not a problem, but we probably need to make struct stat and struct stat64 be different names for the same struct. I don't know how to do that.

I'm not a Rust developer and not sure how I can create that type alias.

[bors]

:umbrella: The latest upstream changes (presumably #3218) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnTitor]

Thanks for the PR! But I think this is a huge breaking change and we have to discuss it before reviewing the changes (or, even submitting a PR). Could you open an issue instead?

[snogge]

Sure, I'll do that.

[snogge]

I'm working on updating this PR to handle problem 2 mentioned above and to pass tests on more platforms. Expect an update later this week.

[snogge]

Sorry for the many pushes, that was meant only for my fork. Anyway, now the PR should handle the duplicate functions mentioned in problem 2, and also pass tests on more platforms.

[bors]

:umbrella: The latest upstream changes (presumably #3237) made this pull request unmergeable. Please resolve the merge conflicts.

[snogge]

Just rebased on main. Passes all tests in main.yml and bors.yml workflows. Still not very clean though, feedback on code style would be welcome.

I considered adding a cargo feature to control this, but as that would be braking the ABI I guess I shouldn't?

[bors]

:umbrella: The latest upstream changes (presumably #3437) made this pull request unmergeable. Please resolve the merge conflicts.

[snogge]

@rustbot review

[snogge]

I just removed the CI-changes I pushed by mistake....again.

[plugwash]

Hi, I applied a patch series based on this PR to the rust-libc package in Debian. Following Debian's descisions on which architectures to apply the time64 transition to I added a condition to disable the "gnu_time64_abi" setting on i386.

However, it seems that this patch series causes API breaks, even if the "gnu_time64_abi" config is not set. For example https://ci.debian.net/packages/r/rust-fs-at/testing/i386/45768200/#S6

[snogge]

I think the missing f_flags is actually a long-standing bug, the statfs struct in gnu/b32/x86/mod.rs does not have the f_flags member, but rather an extra item in the f_spare array. This was exposed by this patch when the statfs64 struct became an alias to the statfs struct. I'll just check the other archs as well and then update the PR.

[plugwash]

Yeah, it looks like the missing f_flags issue affects at least arm too.

[snogge]

I've filed a separate PR #3663 to fix the missing f_flags. There are more problems with the statfs64 struct that has to be fixed in this PR, I'm working on fixing those.

[snogge]

I've reconstructed the PR to support an environment variable RUST_LIBC_TIME_BITS which can be set to 32 to force 32-bit time on 32-bit platforms. The default, based on the cargo environment variables is 64-bit time for those platforms. I've also modified the CI workflows to test these variants.

[plugwash]

After patching the filetime crate to build against the patched libc crate, i'm seeing a test failure. I guess there is some sort of problem with the file time APIs.

git clone https://github.com/plugwash/filetime cd filetime cargo test

---- tests::set_file_times_test stdout ----
thread 'tests::set_file_times_test' panicked at src/lib.rs:449:9:
assertion `left == right` failed: modification time should be updated
  left: FileTime { seconds: 1073741822, nanos: 0 }
 right: FileTime { seconds: 20000, nanos: 0 }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[snogge]

@plugwash , you need to use the utimensat_time64 syscall in https://github.com/plugwash/filetime/blob/main/src/unix/linux.rs#L44 I haven't (yet) figured out why the call to set_file_times on https://github.com/plugwash/filetime/blob/main/src/lib.rs#L438 works, but something about the use of only the mtime argument causes the mtime to be set to the value of UTIME_OMIT.

Also, you might want to change to the futimens syscall. It uses only the file descriptor.

[plugwash]

Thanks a lot for the advice on the filetime issue.

I'm putting together a list of known fallout from the time64 update for rust in Debian along with the patches we are carrying to deal with it, it's possible I've missed some stuff. Some of these patches also make debian-specific assumptions, they should be regarded as pointers to stuff that needs fixing and potential fixes, not nessacerally patches that should be adopted as-is.

timespec now contains padding:

• alsa - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/coreutils/debian/patches/use-mem-zeroed.diff
• coreutils - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/coreutils/debian/patches/use-mem-zeroed.diff
• datetime - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/datetime/debian/patches/use-mem-zeroed.patch
• pipewire - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/pipewire/debian/patches/use-mem-zeroed.patch
• pleaser - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/pleaser/debian/patches/use-mem-zeroed.patch
• serial-unix - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/serial-unix/debian/patches/use-mem-zeroed.patch
• vmm-sys-util - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/vmm-sys-util/debian/patches/use-mem-zeroed.patch (note: upstream don't support 32-bit and we may well drop 32-bit support for this package in Debian too soon, but for now we are shipping this patch).

glibc has inconsitent definitions of suseconds_t and struct timeval:

• filetime - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/filetime/debian/patches/suseconds_t-workaround.patch
• laurel - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/laurel/debian/patches/use-timespec-new.patch
• libpulse-binding - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/libpulse-binding/debian/patches/avoid-suseconds_t.patch
• net2 - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/net2/debian/patches/suseconds_t-workaround.patch
• nix - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/nix/debian/patches/time_t_workarounds.diff
• socket2 - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/socket2/debian/patches/suseconds_t-workaround.patch
• unix-socket - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/unix-socket/debian/patches/suseconds_t-workaround.patch
• vsock - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/vsock/debian/patches/suseconds_t-workaround.patch

input_event defition change:

• evdev-rs - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/evdev-rs/debian/patches/time64.patch

explicit syscalls:

• filetime - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/filetime/debian/patches/use-correct-syscall-on-time64.patch to be continued...........

bindgen needs to pass time64 defines libclang:

• bindgen - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/bindgen/debian/patches/pass-time64-flags-to-libclang.patch

issues related to bindgen:

• selinux-sys - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/selinux-sys/debian/patches/fix-bindgen.diff

[snogge]

I'm putting together a list of known fallout from the time64 update for rust in Debian along with the patches we are carrying to deal with it, it's possible I've missed some stuff. Some of these patches also make debian-specific assumptions, they should be regarded as pointers to stuff that needs fixing and potential fixes, not nessacerally patches that should be adopted as-is.

This is great!

timespec now contains padding:

I don't see what we can do about this.

glibc has inconsitent definitions of suseconds_t and struct timeval:

I have no idea why glibc did not choose to redefine suseconds_t to a 64 bit type instead of using __suseconds64_t => int64_t.

* filetime - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/filetime/debian/patches/suseconds_t-workaround.patch

* laurel - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/laurel/debian/patches/use-timespec-new.patch

Not going via libc::timespec just looks better to me.

* libpulse-binding - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/libpulse-binding/debian/patches/avoid-suseconds_t.patch

https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/libpulse-binding/debian/patches/avoid-suseconds_t.patch#L34 Using time_t here is easy but maybe a bit misleading. time_t is never involved in defining this member type in glibc.

* net2 - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/net2/debian/patches/suseconds_t-workaround.patch

* nix - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/nix/debian/patches/time_t_workarounds.diff

https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/nix/debian/patches/time_t_workarounds.diff#L66 Shouldn't that be i64? suseconds_t is a signed integer type.

* socket2 - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/socket2/debian/patches/suseconds_t-workaround.patch

* unix-socket - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/unix-socket/debian/patches/suseconds_t-workaround.patch

* vsock - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/vsock/debian/patches/suseconds_t-workaround.patch

input_event defition change:

* evdev-rs - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/evdev-rs/debian/patches/time64.patch

I've pushed the new input_event struct that looks like (except for conditionals and stuff)

    pub struct input_event {
        pub input_event_sec: ::time_t,
        pub input_event_sec: ::c_ulong,
    };

I think it is correct for all archs, I cannot figure out any where it shouldn't be.

explicit syscalls:

* filetime - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/filetime/debian/patches/use-correct-syscall-on-time64.patch
  to be continued...........

Just use the libc::utimensat function instead? It will call the correct glibc wrapper around the correct syscall. I think filetime calls a some other libc functions anyway.

bindgen needs to pass time64 defines libclang:

* bindgen - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/bindgen/debian/patches/pass-time64-flags-to-libclang.patch

issues related to bindgen:

* selinux-sys - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/selinux-sys/debian/patches/fix-bindgen.diff

[snogge]

input_event defition change:

* evdev-rs - https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/evdev-rs/debian/patches/time64.patch

I've pushed the new input_event struct that looks like (except for conditionals and stuff)

    pub struct input_event {
        pub input_event_sec: ::time_t,
        pub input_event_sec: ::c_ulong,
    };

I think it is correct for all archs, I cannot figure out any where it shouldn't be.

@plugwash, I reread your comment about this, and I now I can figure out where it wouldn't work. :)

[snogge]

@JohnTitor @tgross35 @joshtriplett I apologize for taking this shotgun-like approach for attention. This PR is now ~1.5 years old, and I'm still not sure whether there is any plan to include it in the 1.0 release and whether my approach is acceptable. I realize it is a large and controversial change, but I'd really like some feedback on the PRs viability and the choices made.

[tgross35]

@snogge thank you for the change and for being willing to keep up with it. We need this change and I think something like this may be about the only reasonable approach.

I looked at this and have some small feedback, but I need to double check with the libs team that this is the path we want/need to go. I'm going to try to get a meeting scheduled for this which will probably take about a month, after that we will know what is needed to get this merged. Hold off on any rebases until then just in case, but I think we will still want most of this even if something like the config has to change.