Fixes overflow trap when creating DispatchTime objects with large upt… #12387
Conversation
|
@swift-ci please clean smoke test |
| + UInt64(DispatchTime.timebaseInfo.numer - 1)) / UInt64(DispatchTime.timebaseInfo.numer) | ||
|
|
||
| // UInt64.max means distantFuture. Do not try to scale it. | ||
| if rawValue != UInt64.max && DispatchTime.timebaseInfo.numer != DispatchTime.timebaseInfo.denom { |
There was a problem hiding this comment.
It seems like the scaling could always trigger an overflow, right? Is this sufficient?
There was a problem hiding this comment.
I protected the operations with overflow checks, apart from the division which can't overflow, so I don't think anything else can trap.
There was a problem hiding this comment.
So what does this if do? Is it simply trying to optimize some values on some platforms?
There was a problem hiding this comment.
The first part lets UInt64.max get through unchanged, the second part avoids the arithmetic when there is no timebase scaling. So yes, the second part is an optimization (that was already there).
test/stdlib/Dispatch.swift
Outdated
| time = DispatchTime(uptimeNanoseconds: UInt64.max) | ||
| expectEqual(time.uptimeNanoseconds, UInt64.max) | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think any of these exercise the overflow paths. You should add values like UInt64.max-1.
There was a problem hiding this comment.
That's a good point and when I tried some of the larger values, it turns out that the forced ceiling of UInt64.max for the multiplication produces some odd results. I am going to revisit that.
|
@swift-ci please clean smoke test |
1 similar comment
|
@swift-ci please clean smoke test |
…imeNanoseconds values and re-enables the tests that failed. Adds some additional tests.
|
Squashed commits. |
|
@swift-ci please smoke test |
…imeNanoseconds values and re-enables the tests that failed.
Adds some additional tests.
(rdar://problem/34751238)
The change is in three parts: