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

Consider making std::time::SystemTime platform-independent #44394

Open
kennytm opened this issue Sep 7, 2017 · 16 comments
Open

Consider making std::time::SystemTime platform-independent #44394

kennytm opened this issue Sep 7, 2017 · 16 comments
Labels
A-time Area: Time C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@kennytm
Copy link
Member

kennytm commented Sep 7, 2017

It was found in #44220 (comment) that certain systems in Tier-2 support still uses a 32-bit time_t. SystemTime on them will suffer from the Year-2038 problem, and also cannot perform arithmetic for a duration >68 years. This introduces a portability hazard, in which on some systems the time arithmetic works normally, and in some more legacy system it suddenly panics.

The SystemTime struct is a wrapper around timespec on Unix and Redox, and FILETIME on Windows. Nevertheless, the public API involving SystemTime never exposes this detail:

  • You cannot construct a SystemTime from a timespec/FILETIME, nor the other way round

  • The only place where SystemTime interacts with the OS are being returned

    1. SystemTime::now, returning a SystemTime
    2. Metadata::{modified, accessed, created}, returning an io::Result<SystemTime>
  • There are no OS-specific APIs reading a SystemTime.

This means expanding the precision and range of SystemTime is safe. In fact, we could even make the SystemTime structure itself platform-agnostic, just like Duration:

struct SystemTime {
    secs: i64,
    nanos: u32,
}
@retep998
Copy link
Member

retep998 commented Sep 8, 2017

What would the epoch be for this? Would we still use the platform specific epoch or would we have a platform agnostic epoch?

@kennytm
Copy link
Member Author

kennytm commented Sep 8, 2017

I'd suggest everyone uses 1970 Jan 1st (Unix epoch) for consistency.

Since the public APIs won't see the raw numbers at all, special-casing Windows to use 1601 Jan 1st as epoch is also possible. However converting a FILETIME to timespec-like structure already involves the complex arithmetic of divide-and-modulus-by-107, adding a further epoch shift will not be a comparatively bigger performance issue.

The range of FILETIME is ±263 × 100ns ≈ 29227 years, while the range of 64-bit timespec is ±263 s ≈ 3 × 1011 years, so moving the epoch by 369 years is not going to cause overflow.

Another choice is keep Windows to use FILETIME, and just change all Unix to always use (i64, u32).

@retep998
Copy link
Member

retep998 commented Sep 8, 2017

Using the Unix Epoch for consistency would completely break any times on Windows from before the the unix epoch. If someone manually changes a file to be created in the 1600s, Rust better damn well support that.

@sfackler
Copy link
Member

sfackler commented Sep 8, 2017

@retep998 How exactly does that break times before the epoch? secs is signed.

@retep998
Copy link
Member

retep998 commented Sep 8, 2017

@sfackler Oh, right. Anyway, this would still cause an increase in the size of SystemTime on Windows. It would also allow you to create a SystemTime which cannot be represented in the platform specific type, resulting in errors when you call functions to set file times. Of course this might be desirable so that calculations don't overflow when doing weird arithmetic with time.

@kennytm
Copy link
Member Author

kennytm commented Sep 8, 2017

@retep998 Sure, but there are no functions that set the file time using SystemTime in libstd.

There is the filetime crate for setting file times, but it uses its own FileTime structure, not affected by this proposed change (btw it uses the 1970 epoch on Unix and 1601 on Windows).

@retep998
Copy link
Member

retep998 commented Sep 8, 2017

@kennytm Maybe one day std will have such functions. Maybe in the future it would be possible to actually work with std's SystemTime instead of having to duplicate it with your own version.

@kennytm
Copy link
Member Author

kennytm commented Sep 8, 2017

@retep998 those functions would return io::Error.

@joshlf
Copy link
Contributor

joshlf commented Sep 8, 2017

@retep998

It would also allow you to create a SystemTime which cannot be represented in the platform specific type, resulting in errors when you call functions to set file times.

Since Windows represents time as a 64-bit value representing 100-ns intervals, this would only happen if you tried to set a file time that was:

  • more than 2^63/10,000,000 seconds = ~30K years before the Windows epoch
  • more than approximately the same amount of time after the Windows epoch

Wanting to use these kinds of times to set file timestamps and other system operations (as in, other than just doing computations in pure-Rust land) seems very very unlikely in practice.

@retep998
Copy link
Member

retep998 commented Sep 8, 2017

@joshlf Trying to set a file time to any time before the Windows epoch would result in an error, not only 30K years before.

@joshlf
Copy link
Contributor

joshlf commented Sep 8, 2017

Oh so file times can't be negative? Because the function that I linked to returns a signed integer. Do you know if the same limitation exists on Unix?

@retep998
Copy link
Member

retep998 commented Sep 8, 2017

@joshlf FILETIME is unsigned which is what GetSystemTimeAsFileTime returns which is what you should be looking at, not the internal NT function NtQuerySystemTime. Some functions, such as SetFileTime, also use 0xFFFFFFFF as a sentinel value. I tested SetFileTime and it errors with ERROR_INVALID_PARAMETER on any value that would be negative if interpreted as a signed integer.

@joshlf
Copy link
Contributor

joshlf commented Sep 8, 2017

Gotcha, thanks!

@TimNN TimNN added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 17, 2017
@steveklabnik
Copy link
Member

Triage: it's not clear to me what the conclusion of this discussion is, or if this is still desired or not.

@dkg
Copy link

dkg commented Feb 9, 2021

fwiw, if there is no reason to bind rust's std::time::SystemTime to the underlying platform's time_t, it would be great to abstract it to a something platform-agnostic, like Duration.

This is causing problems for sequoia on platforms like i386 and armhf (where time_t is 32-bit).

@teythoon
Copy link

This is still a problem for Sequoia:

Internally, we store timestamps as our own type, which comes down to u32. However, on the API boundary we convert to SystemTime: getters return SystemTime, and setters are polymorphic over Into<SystemTime>. This allows convenient use with chrono's types, but loses precision on platforms that have a signed 32-bit time_t.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-time Area: Time C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants