-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Fix duration_since panic on unix when std is built with integer overflow checks #146556
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
base: master
Are you sure you want to change the base?
Conversation
87b71c0
to
f335c20
Compare
This comment has been minimized.
This comment has been minimized.
Seems like the bug was there before constify of |
If anyone is around, not do I compile std with debug assertions including integer overflow? I'm running
and such assertions are not triggered. |
put this in your bootstrap.toml rust.debug-assertions-std = true from Lines 603 to 604 in 52618eb
|
|
Found it.
|
f335c20
to
9cc8046
Compare
Ah, yes, down a few lines then. |
r? libs |
9cc8046
to
162ee46
Compare
This comment has been minimized.
This comment has been minimized.
Ping @tgross35. |
} else { | ||
( | ||
(self.tv_sec - other.tv_sec - 1) as u64, | ||
(self.tv_sec - 1).wrapping_sub(other.tv_sec) as u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to think for a few seconds before I realised why the - 1
couldn't panic (since other.tv_nsec
is larger than self.tv_nsec
, but other
smaller than self
, other.tv_sec
must be smaller than self.tv_sec
, meaning that there is a value smaller than self.tv_sec
, making the subtraction work). Could you perhaps add a comment to explain it? Or use wrapping_sub
for the decrement as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few assertions, so that sequence of assertion will explain why it does not underflow.
Or use wrapping_sub for the decrement as well?
That would not be right, because if there is a bug here, instead of debug assertion, we may quietly get wrong result.
library/std/src/sys/pal/unix/time.rs
Outdated
// is outside of the `if`-`else`, not duplicated in both branches | ||
// | ||
// Ideally this code could be rearranged such that it more | ||
// directly expresses the lower-cost behavior we want from it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above here needs to be updated, considering it specifically calls out self.tv_sec - 1 - other.tv_sec
vs. self.tv_sec - other.tv_sec - 1
. I don't think it's all relevant anymore, given https://rust.godbolt.org/z/1Ex913qr5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the comment because:
- you asked to update it
- I don't understand it
- this does not seem like performance critical code to do
Hey, this was open for only three days :) I don't think this is high priority? |
162ee46
to
8d641ba
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
The bugfix is not high priority, but I wanted to add a couple PRs on top, and if each one takes weeks to review, I will lose context and/or switch to some other work (my primary work is far from opensource). Sorry for the ping. Is there expected time to a comment from the reviewer for a PR in rust repo so next time I'll avoid pinging the maintainers too early? |
The comment that the bot posts for new contributors says
|
Add a test for regression #146228, and turns out this test fails detects error when std is compiled with integer overflow checks.
Original regression was reverted in #146473.
First attempt to fix was in #146247; test and some code is copied from there, thanks @eval-exec
r? @RalfJung