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

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

snogge
Copy link
Contributor

@snogge snogge commented Mar 29, 2023

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
Copy link
Collaborator

rustbot commented Mar 29, 2023

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
Copy link
Contributor Author

snogge commented Mar 29, 2023

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

@snogge
Copy link
Contributor Author

snogge commented Apr 11, 2023

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:

  1. 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
Copy link
Contributor

bors commented Apr 23, 2023

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

@snogge snogge force-pushed the time_bits=64 branch 7 times, most recently from 5dfc8ed to 7d2c859 Compare April 24, 2023 13:36
@JohnTitor
Copy link
Member

JohnTitor commented Apr 24, 2023

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
Copy link
Contributor Author

snogge commented Apr 25, 2023

Sure, I'll do that.

@snogge
Copy link
Contributor Author

snogge commented Apr 25, 2023

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 snogge force-pushed the time_bits=64 branch 2 times, most recently from 5314484 to eb997cb Compare April 25, 2023 12:02
@snogge
Copy link
Contributor Author

snogge commented Apr 25, 2023

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
Copy link
Contributor

bors commented May 6, 2023

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

@snogge
Copy link
Contributor Author

snogge commented Sep 7, 2023

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
Copy link
Contributor

bors commented Nov 12, 2023

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

Set the basic types correctly for gnu_time64_abi (_TIME_BITS=64 and
_FILE_OFFSET_BITS=64).
Set the link names of relevant symbols to use be the same as when a C
program is built against GNU libc with -D_TIME_BITS=64 and
-D_FILE_OFFSET_BITS=64.
The actual values may be different on 32bit archs and glibc with
64-bit time.
Also add the F_[SG]ET*64 constants.
For 64 bit time on 32 bit linux glibc timeval.tv_usec is actually
__suseconds64_t (64 bits) while suseconds_t is still 32 bits.
Big-endian platforms wants 32 bits of padding before tv_nsec,
little-endian after.

GNU libc always uses long for tv_nsec.
Do not use gnu/b32/time*.rs for riscv32 and sparc.  Add timex to
gnu/b32/(riscv32|sparc)/mod.rs instead.
Move the stat struct from gnu/b32/mod.rs to gnu/b32/time*.rs

For arm, mips, powerpc, riscv32, and x86, move stat64 to
time32.rs and add an stat64 = stat alias to time64.rs.

For sparc, do the same for statfs64 and statvfs64.
In Linux Tier1, run the tests for i686-unknown-linux-gnu with
RUST_LIBC_TIME_BITS set to 32, 64, and default.

In Linux Tier2, run the tests for arm-unknown-linux-gnueabihf and
powerpc-unknown-linux-gnu with RUST_LIBC_TIME_BITS set to 32, 64,
and default.  Use RUST_LIBC_TIME_BITS=defaults for the other
platforms.

In Build Channels Linux, build the relevant platforms with
RUST_LIBC_TIME_BITS unset and set to 32 and 64.
@snogge
Copy link
Contributor Author

snogge commented Aug 15, 2024

@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
Copy link
Contributor

tgross35 commented Aug 29, 2024

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

@snogge
Copy link
Contributor Author

snogge commented Oct 21, 2024

@tgross35 Any updates? It's been almost two months now.

@snogge
Copy link
Contributor Author

snogge commented Nov 19, 2024

Ping @tgross35 , it's been another month and rebasing will just be getting more and more difficult.

@tgross35
Copy link
Contributor

Timing, I was actually just looking at this :) I think we just need to start merging small parts of this to unblock things. Would it be possible to split the following changes to a new PR?

  • The changes to src/unix/linux_like/linux/mod.rs
  • Adding gnu_time64_abi to ALLOWED_CFGS in build.rs

If we can get that merged as a basis then the rest (testing, platform support, defaults) can fall into place as we figure it out.

@tgross35
Copy link
Contributor

Actually, a more general request than that. Could uapi-related changes be split to a PR that we can merge first?

I mentioned this in the musl PR at #3791 (comment) but we should have a config linux_time_bits64 that corresponds to __USE_TIME_BITS64 used by uapi (example in struct input_event, my suggested rust field implementation). This should be used for anything that isn't glibc-specific.

Doing this first makes the rest of the changes here a lot smaller and unblocks the musl PR as well.

@tgross35
Copy link
Contributor

Per the above, I think getting the Linux cfg in place is a reasonable first separate first since it affects all libraries and is a smaller portion of this change. After that is in place it will be easier to merge the rest.

Let me know if you need any help here.

@rustbot author

@snogge
Copy link
Contributor Author

snogge commented Nov 25, 2024

I'm working on a new PR, but it's been a while so I need to double check some things.

@snogge
Copy link
Contributor Author

snogge commented Nov 25, 2024

@tgross35 new PR for linux_time_bits64: #4148

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.

6 participants