bpo-34423: Fix check for overflow when casting from a double to integral types. - #8802
bpo-34423: Fix check for overflow when casting from a double to integral types.#8802enedil wants to merge 5 commits into
Conversation
Sorry, something went wrong.
…ral types The added comment explains the issue. This fix disallows overflow: if a double `d` passes the test, it can mean that `d == 2**63`. When casted to `int64_t`, the variable overflows to -2**63.
|
this appears linked to the wrong issue -- is this the right one? https://bugs.python.org/issue34423 |
Sorry, something went wrong.
|
@asottile Yes, you're right. My branch is wrongly named too. Should I create new pull request? |
Sorry, something went wrong.
|
It might be enough to edit the commit message and title? I don't actually know |
Sorry, something went wrong.
vstinner
left a comment
There was a problem hiding this comment.
A test should be added to test_time.
Sorry, something went wrong.
| # _PyTime_t is a 64-bit signed integer | ||
| OVERFLOW_SECONDS = math.ceil((2**63 + 1) / SEC_TO_NS) | ||
| OVERFLOW_SECONDS = math.ceil(( | ||
| + 1) / SEC_TO_NS) |
There was a problem hiding this comment.
unintentional change?
Sorry, something went wrong.
jdemeyer
left a comment
There was a problem hiding this comment.
I don't think that the condition sizeof(*v*) == sizeof(*type*) is really the right condition. Besides, you are applying the fix (changing <= to <) unconditionally, also when the maximal value can be represented exactly.
The problem is that the maximal value 2^N - 1 is hard to work with. So I suggest to replace
v < _Py_IntegralTypeMax(type)
by something like
v < ldexp(_Py_IntegralTypeSigned(type) ? 0.5 : 1.0, CHAR_BIT * sizeof(type))
Sorry, something went wrong.
|
I'm not sure that we will be able to find a solution using a "simple" macro. Maybe we need new functions which takes a double as input and return an integer as output and report overflow. I'm not sure that it's possible to write a generic function. Right now, _Py_InIntegralTypeRange() is only used to convert a double to _PyTime_t (int64_t) and time_t in pytime.c. Maybe 2 functions would be enough? About generic code, we can maybe use the preprocessor as a template engine to generate these functions. |
Sorry, something went wrong.
It would certainly be easier to support specific types only instead of being generic. We could for example assume that the number of bits in a double (53 assuming IEEE-754) is less than the number of bits in a int64_t (63). |
Sorry, something went wrong.
iritkatriel
left a comment
There was a problem hiding this comment.
This has merge conflicts, as well as several unresolved issues raised by reviewers.
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. And if you don't make the requested changes, you will be put in the comfy chair! |
Sorry, something went wrong.
|
I'm fine with leaving this as unresolved. |
Sorry, something went wrong.
Sure, thanks. |
Sorry, something went wrong.
The added comment explains the issue. This fix disallows overflow: if a double d passes the test, it can mean that d == 2**63. When casted to int64_t, the variable overflows to -2**63.
https://bugs.python.org/issue34423