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-34423: Fix check for overflow when casting from a double to integral types. by enedil · Pull Request #8802 · python/cpython

Conversation

@enedil

@enedil enedil commented Aug 17, 2018 •
edited by serhiy-storchaka
Loading

Uh oh!

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

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@enedil
bpo-32367: Fix check for overflow when casting from a double to integ…
c05d58d 
…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.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 17, 2018
@bedevere-bot bedevere-bot added the awaiting review label Aug 17, 2018
@enedil
ACKS and blurb update
58b69f0
@asottile

asottile commented Aug 19, 2018

Copy link
Copy Markdown
Contributor

this appears linked to the wrong issue -- is this the right one? https://bugs.python.org/issue34423
All reactions

Sorry, something went wrong.

Uh oh!

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

enedil commented Aug 19, 2018

Copy link
Copy Markdown
Author

@asottile Yes, you're right. My branch is wrongly named too. Should I create new pull request?
All reactions

Sorry, something went wrong.

Uh oh!

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

asottile commented Aug 19, 2018

Copy link
Copy Markdown
Contributor

It might be enough to edit the commit message and title? I don't actually know
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@enedil enedil changed the title bpo-32367: Fix check for overflow when casting from a double to integ… bpo-34423: Fix check for overflow when casting from a double to integ… Aug 19, 2018
@serhiy-storchaka serhiy-storchaka changed the title bpo-34423: Fix check for overflow when casting from a double to integ… bpo-34423: Fix check for overflow when casting from a double to integral types. Aug 20, 2018
vstinner
vstinner reviewed Aug 20, 2018
View reviewed changes

@vstinner vstinner 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.

A test should be added to test_time.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
enedil added 2 commits August 21, 2018 14:02
@enedil
Bound checks for time.sleep
f5a7b94
@enedil
Test update
cc0bcda
asottile
asottile reviewed Aug 21, 2018
View reviewed changes
Comment thread Lib/test/test_time.py Outdated
# _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)

@asottile asottile Aug 21, 2018

Copy link
Copy Markdown
Contributor

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.

unintentional change?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@enedil
Revert typo
4e2c8b8
jdemeyer
jdemeyer requested changes Feb 4, 2019
View reviewed changes

@jdemeyer jdemeyer left a comment

Copy link
Copy Markdown
Contributor

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 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.

Uh oh!

There was an error while loading. Please reload this page.
torokati44 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Feb 4, 2019
@vstinner

vstinner commented Feb 13, 2019

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

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

jdemeyer commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

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?

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
iritkatriel
iritkatriel requested changes Dec 7, 2022
View reviewed changes

@iritkatriel iritkatriel 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 has merge conflicts, as well as several unresolved issues raised by reviewers.

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 Dec 7, 2022
@bedevere-bot

bedevere-bot commented Dec 7, 2022

Copy link
Copy Markdown

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!
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 Dec 7, 2022
@enedil

enedil commented Dec 7, 2022

Copy link
Copy Markdown
Author

I'm fine with leaving this as unresolved.
All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel commented Dec 8, 2022

Copy link
Copy Markdown
Member

I'm fine with leaving this as unresolved.

Sure, thanks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel iritkatriel closed this Dec 8, 2022
@enedil enedil mannequin mentioned this pull request Dec 8, 2022
Overflow when casting from double to time_t, and_PyTime_t. #78604
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@iritkatriel iritkatriel iritkatriel requested changes
@jdemeyer jdemeyer jdemeyer requested changes
+1 more reviewer
@asottile asottile asottile left review comments
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.

8 participants

@enedil @asottile @vstinner @jdemeyer @bedevere-bot @iritkatriel @the-knights-who-say-ni @ezio-melotti
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.