GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-39277, pytime: Fix overflow check on double to int cast by vstinner · Pull Request #17933 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Jan 10, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Fix time.sleep() to properly detect integer overflow when converting
a floating-point number of seconds to an integer.

https://bugs.python.org/issue39277

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
bpo-39277, pytime: Fix overflow check on double to int cast
9630d9f
Fix time.sleep() to properly detect integer overflow when converting
a floating-point number of seconds to an integer.
@vstinner
vstinner requested review from abalkin and pganssle as code owners January 10, 2020 09:51
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 10, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Jan 10, 2020
@vstinner

vstinner commented Jan 11, 2020

Copy link
Copy Markdown
Member Author

cc @mdickinson

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@csabella
csabella requested a review from mdickinson May 28, 2020 23:04
mdickinson
mdickinson reviewed May 29, 2020
View reviewed changes
Comment thread Include/pymath.h
/* Check if the floating-point number v (double) would overflow when casted to
* the integral type 'type'.
*
* Test (double)type_min(type) <= v <= (double)type_max(type) where v is a

@mdickinson mdickinson May 29, 2020

Copy link
Copy Markdown
Member

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 two conditions "v would overflow when cast to 'type'" and "type_MIN <= v <= type_MAX" are not equivalent. Which one do you want to test here?

For example, assuming a 32-bit int type: type_MAX would be 2147483647. If v = 2147483647.5 (which is exactly representable in IEEE 754 binary64 floating-point), then it satisfies the first condition - it doesn't overflow when cast to int - but not the second.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mdickinson
mdickinson reviewed May 29, 2020
View reviewed changes
Comment thread Include/pymath.h
*
* In short, nextafter((double)x, 0.0) rounds the integer x towards zero. */
#define _Py_DoubleInIntegralTypeRange(type, v) \
(nextafter((double)_Py_IntegralTypeMin(type), 0.0) <= v \

@mdickinson mdickinson May 29, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, but this isn't right.

For a 32-bit signed integer type, this condition becomes:

-2147483647.9999998 <= v <= 2147483646.9999998

but what we actually want is:

-2147483649.0 < v < 2147483648.0

or if we want to use <= and express the limits in terms of doubles:

-2147483648.9999995 <= v <= 2147483647.9999998

For a 64-bit signed integer type, this condition becomes:

-9223372036854774784.0 <= v <= 9223372036854774784.0

but the actual limits we need are

-9223372036854775808.0 <= v <= 9223372036854774784.0

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mdickinson mdickinson Jul 22, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Suggested alternative approach: given a double v that we want to safely convert to an int (say), first use modf to extract the fractional part fv and the integer part iv of v (both fv and iv are still doubles at this point). We can just ignore the fractional part fv: it doesn't affect whether we can safely cast to int or not. Now for iv, check whether it satisfies (double)INT_MIN <= iv and iv < -(double)INT_MIN. Note that INT_MIN is (always in practice, even though not guaranteed by the standard) the negation of a power of two, so the conversion to double doesn't change the value and we're effectively checking whether iv lies in the half-open interval [INT_MIN, -INT_MIN). Since iv is an integer, that's equivalent to checking whether iv lies in the closed interval [INT_MIN, -INT_MIN-1]. But making all our usual assumptions about integer representation (two's complement, no trap representation, no padding bits, etc.), -INT_MIN-1 is the same as INT_MAX, so we're effectively checking that the integer part of v is representable as an int, which is exactly what we want to be checking.

Similarly for long, long long, etc.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mdickinson

mdickinson commented May 29, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

A safe way to do what you want is:

(1) Take the integral part of the double v that you want to check, for example using modf, still represented as a double. (Note that the integral part of an IEEE 754 floating-point number is always exactly representable in the same format.)
(2) Check that (double)TYPE_MIN <= int_part_of_v < (double)(TYPE_MAX + 1) (using a mixture of C casts and Python chained comparisons here, but you know what I mean)

Computing TYPE_MAX + 1 while avoiding overflow may take some type-specific effort.

The reason that step (2) is safe is that, making the fairly safe assumption of 2s complement (no trap representation, etc., etc.) for signed integers, TYPE_MIN and TYPE_MAX + 1 will always be exactly representable as a double (well, assuming that we're not dealing with 1024-bit integers, that is). But TYPE_MAX can't be assumed to be exactly representable as a double - whether it is or not will depend on the width of the type being tested.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson suggested changes May 29, 2020
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

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.

This isn't the right way to check for convertibility; see the other comments I made on the PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot removed the awaiting core review label May 29, 2020
@bedevere-bot

bedevere-bot commented May 29, 2020

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting changes label May 29, 2020
@graingert

graingert commented Aug 21, 2020

Copy link
Copy Markdown
Contributor

@vstinner any update on this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Sep 21, 2021

Copy link
Copy Markdown
Member Author

I failed finding time to finish the PR. I prefer to abandon it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner closed this Sep 21, 2021
@vstinner
vstinner deleted the pytime_round branch September 21, 2021 21:58
@vstinner vstinner mentioned this pull request Oct 4, 2022
Compiler warning for _Py_InIntegralTypeRange #97786
Closed
@vstinner vstinner mentioned this pull request Dec 5, 2022
Drop support for platforms without two's complement integer representation: require two's complement to build Python #100008
Closed
@graingert graingert mannequin mentioned this pull request Mar 4, 2023
_PyTime_FromDouble() fails to detect an integer overflow when converting a C double to a C int64_t #83458
Closed
@mdickinson mdickinson mentioned this pull request Mar 4, 2023
Remove unused internal macros #102415
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abalkin abalkin Awaiting requested review from abalkin
@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner
1 more reviewer
@mdickinson mdickinson mdickinson requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@vstinner @mdickinson @bedevere-bot @graingert @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.