Fix SystemTime::duration_since error for extreme value

[eval-exec]

This PR intends to fix #146228

[rustbot]

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[clarfonthey]

Note that the same issue is present in several implementations, so, you're going to want to change all of them.

But also, you could just use saturating_sub instead of extra branches to avoid changing too much here.

[eval-exec]

Note that the same issue is present in several implementations, so, you're going to want to change all of them.

yes, there also a same issue in library/std/src/sys/pal/hermit/time.rs

But also, you could just use saturating_sub instead of extra branches to avoid changing too much here.

Thank you.

[eval-exec]

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Why it failed, I execute ./x test library/std --test-args system_time_extreme_values_regression on my local env, the test is successful.

[stepancheg]

It is supposed to panic, no?

You changed

let mut cmp = self.tv_sec - other.tv_sec

to

let mut cmp = self.tv_sec.saturating_sub(other.tv_sec);

to avoid overflow, but code is panicking on exactly the same subtraction:

self.tv_sec - other.tv_sec

I think wrapping_sub followed by cast may be used.

[stepancheg]

Also the test you added, it is Linux/mac specific, won't work on Window, because FILETIME has different limits than Timespec.

[clarfonthey]

I think wrapping_sub followed by cast may be used.

Or, wrapping_sub_unsigned also works to do both.

[stepancheg]

What is the easiest way to check all these platforms? At least check they compile?

[clarfonthey]

I included a list of the targets I tested in the PR, but the answer is it's a bit of a mess. You need a few different cross-compilers to get it working. The merge job for Bors will run all them regardless, though, so worst case you can do a try job before adding to the queue to verify.

You might get lucky with only needing the x86_64 ones, though, in which case it'll be much easier. And, since you're just modifying places where the code was copied, it's less likely to break than figuring out what const markers are needed.

[stepancheg]

ttv_sec

Typo here.

[workingjubilee]

This has had a number of changes since it was first opened. Can you explain why the current approach? I know an issue was opened but it is not guaranteed that we should simply fix it, or at least it is not guaranteed how exactly we should, especially given the "that is allowed to break, actually" quality of some of the operations.

View changes since this review

[workingjubilee]

Offset of the SystemTime by a Duration is not guaranteed to give a sensible result and I do not believe cfg(unix) is sufficient for this.

[stepancheg]

In current std implementation, this operation supposed to succeed, because, SystemTime is defined using Timespec that has i64 seconds part:

rust/library/std/src/sys/pal/mod.rs

Lines 27 to 31 in 7ad23f4

	cfg_select! {
	unix => {
	mod unix;
	pub use self::unix::*;
	}

rust/library/std/src/sys/pal/unix/time.rs

Lines 20 to 29 in 7ad23f4

	#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
	pub struct SystemTime {
	pub(crate) t: Timespec,
	}
	#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
	pub(crate) struct Timespec {
	tv_sec: i64,
	tv_nsec: Nanoseconds,
	}

Rust std may want to reduce the range of SystemTime or Timespec one day, but I don't think this is likely, and if that happens, the test can be changed (and likely other tests too).

[workingjubilee]

Hmm. My concern was that I am not entirely sure everything that uses cfg(unix) uses that file, but I can allow such problems to be handled separately, so I suppose I will.

[stepancheg]

it is not guaranteed that we should simply fix it, or at least it is not guaranteed how exactly we should, especially given the "that is allowed to break, actually" quality of some of the operations

I don't this this statement is true in regards to SystemTime::duration_since, because:

• Duration defined in a way it covers the whole range of SystemTime on any platform, at least at the moment and this is unlikely to change in foreseeable future
• SystemTime::duration_since is only allowed to return error if the argument is earlier than self (and more precisely "error contains how far from self the time is"); it does not allow panicking either; nightly breakage is not compatible with that statement in the API documentation. SystemTime::duration_since is quite precise and does not allow an interpretation of such breakage.

[stepancheg]

Fix SystemTime::duration_since panic for extreme value

It is panic only if std is compiled with debug assertions; for regular user of std, nightly behavior is incorrectly returned error, but not panic: play. Previous title may have been more correct.

[workingjubilee]

I see. Thank you for elaborating. I wish we converted to some internal representation of a datetime that made sense relative to Duration and then created the Duration from that...

[workingjubilee]

@eval-exec Can you take what stepancheg just said and paraphrase it as the PR description? Or provide your own words. Just make it a little clearer on reading the description what is being fixed and why it must be fixed, then I can r+ this.

View changes since this review

[workingjubilee]

Hmm. My concern was that I am not entirely sure everything that uses cfg(unix) uses that file, but I can allow such problems to be handled separately, so I suppose I will.

[workingjubilee]

Noting to myself:

• SOLID's changes were purely organizational
• SGX's changed to explicit match because or_else isn't constifiable
• "unsupported" removed or_else
• WASI removed or_else and try_into
• Windows removed try_into

So they had no notable logic changes.

[workingjubilee]

Ah, and since this is waiting for that PR description update:

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[RalfJung]

IMO the better fix is to revert the code changes from #144519, and wait until the original code can be constified unchanged. That PR went too far with introducing const-hacks for not enough gain. We're in no rush to make SystemTime available in const fn, I think.

Also see Zulip discussion.

[clarfonthey]

I think that it's reasonable to just revert the change at least until PartialOrd is made unstably const (this is the only thing that's const-hacked, at the moment.)

I also have a particular… issue, with the way the time system code is defined. A lot of it is copy-pasted which makes issues harder to test outside of extremely niche targets, when the code itself is target-independent.

[RalfJung]

Revert is up at #146473.

[bors]

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

[RalfJung]

Superseded by #146473.