Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-97786: Fix compiler warnings in pytime.c#101826

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@mdickinson
Copy link
Member

@mdickinsonmdickinson commentedFeb 11, 2023•
edited
Loading

This PR fixes some compiler warnings inpytime.c, and at the same time fixes our out-of-range double-to-integer checks to properly avoid undefined behaviour.

It's hard to writestrictly portable code for this kind of thing, but the new check should work under a set of (very) mild assumptions, that are highly unlikely to be violated on any actual platform that we care about:

  • Thattime_t is a two's complement signed integer type with no trap representation. (The weakest part of this is the assumption thattime_t is a signed integer typeat all, but we're already making that assumption.)
  • That_PyTime_t is also a two's complement signed integer type with no trap representation. (This is already true with the currenttypedef int64_t _PyTime_t; declaration.)
  • ThatPY_TIME_T_MIN and_PyTime_MIN are within the range of a C double. These days we're assuming IEEE 754 binary64 doubles, so we're safe so long as the integer types_PyTime_t andtime_t don't have a width ofmore than 1024.

Here's the underlying logic for the changes, for the record:

  • We want to check that a C double valuex is within the range of a (potentially unknown) signed integer typeI with min and max valuesIMIN andIMAX. More specifically, we want to be sure that a conversion fromx to typeI will not invoke undefined behaviour; this means that theinteger part ofx (discarding the fractional part, if any) must be in[IMIN, IMAX].
  • Assuming thatI uses two's complement with no trap representation,IMIN must be the negation of a power of two, andIMAX == -1 - IMIN.
  • SoIMIN is exactly representable as adouble (assuming that it's not larger than2**1023), and(double)IMIN does not change the value.
  • So the bottom-end check,(double)IMIN <= x, does exactly what we want it to.
  • For the top-end check,x <= IMAX is problematic since the implicit conversion ofIMAX to typedouble may change the value. But assuming thatx is an integer (which it is for all cases in this PR),x <= IMAX is mathematically equivalent tox < (IMAX + 1), which with our two's complement assumption is equivalent tox < -IMIN, and we can express this asx < -(double)IMIN.

@mdickinsonmdickinson added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelFeb 11, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@mdickinson for commit32aee25 🤖

If you want to schedule another build, you need to add the:hammer: test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelFeb 11, 2023
@mdickinson
Copy link
MemberAuthor

mdickinson commentedFeb 12, 2023•
edited
Loading

I've removed thefmod asserts from the code. They were somewhat obscure, and it's not necessary for correctness that the target value is an integer.

The only possible issue with a non-integerintpart is one of false positives: ifintpart is a large and negative double in the open interval(PY_TIME_T_MIN - 1, PY_TIME_T_MIN) then:

  • it's safe to castintpart totime_t, since it's in range after discarding the fractional part, but
  • the conditionif (!((double)PY_TIME_T_MIN <= intpart && intpart < -(double)PY_TIME_T_MIN)) will be true, so an overflow would be reported

So the failure mode is benign. But in any case,intpart must be an integer (or possibly an infinity or nan), so the above can't happen.

@mdickinson
Copy link
MemberAuthor

I think this is ready to merge.@gpshead Do you have bandwidth for a quick re-review?

gpshead reacted with thumbs up emoji

@miss-islington
Copy link
Contributor

Thanks@mdickinson for the PR, and@gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry,@mdickinson and@gpshead, I could not cleanly backport this to3.10 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker b1b375e2670a58fc37cb4c2629ed73b045159918 3.10

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestFeb 20, 2023
Fixes compiler warnings in pytime.c.(cherry picked from commitb1b375e)Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
@bedevere-bot
Copy link

GH-102062 is a backport of this pull request to the3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11only security fixes labelFeb 20, 2023
@gpsheadgpshead removed the needs backport to 3.10only security fixes labelFeb 20, 2023
jaraco pushed a commit to jaraco/cpython that referenced this pull requestFeb 20, 2023
carljm added a commit to carljm/cpython that referenced this pull requestFeb 20, 2023
* main: (60 commits)pythongh-102056: Fix a few bugs in error handling of exception printing code (python#102078)pythongh-102011: use sys.exception() instead of sys.exc_info() in docs where possible (python#102012)pythongh-101566: Sync with zipp 3.14. (pythonGH-102018)pythonGH-99818: improve the documentation for zipfile.Path and Traversable (pythonGH-101589)pythongh-88233: zipfile: handle extras after a zip64 extra (pythonGH-96161)pythongh-101981: Apply HOMEBREW related environment variables (pythongh-102074)pythongh-101907: Stop using `_Py_OPCODE` and `_Py_OPARG` macros (pythonGH-101912)pythongh-101819: Adapt _io types to heap types, batch 1 (pythonGH-101949)pythongh-101981: Build macOS as recommended by the devguide (pythonGH-102070)pythongh-97786: Fix compiler warnings in pytime.c (python#101826)pythongh-101578: Amend PyErr_{Set,Get}RaisedException docs (python#101962)  Misc improvements to the float tutorial (pythonGH-102052)pythongh-85417: Clarify behaviour on branch cuts in cmath module (python#102046)pythongh-100425: Update tutorial docs related to sum() accuracy (FH-101854)  Add missing 'is' to `cmath.log()` docstring (python#102049)pythongh-100210: Correct the comment link for unescaping HTML (python#100212)pythongh-97930: Also include subdirectory in makefile. (python#102030)pythongh-99735: Use required=True in argparse subparsers example (python#100927)  Fix incorrectly documented attribute in csv docs (python#101250)pythonGH-84783: Make the slice object hashable (pythonGH-101264)  ...
mdickinson added a commit that referenced this pull requestFeb 22, 2023
gh-97786: Fix compiler warnings in pytime.c (GH-101826)Fixes compiler warnings in pytime.c.(cherry picked from commitb1b375e)Co-authored-by: Mark Dickinson <dickinsm@gmail.com>Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@bedevere-bot
Copy link

GH-102150 is a backport of this pull request to the3.10 branch.

carljm added a commit to carljm/cpython that referenced this pull requestFeb 22, 2023
* main: (225 commits)pythongh-102056: Fix a few bugs in error handling of exception printing code (python#102078)pythongh-102011: use sys.exception() instead of sys.exc_info() in docs where possible (python#102012)pythongh-101566: Sync with zipp 3.14. (pythonGH-102018)pythonGH-99818: improve the documentation for zipfile.Path and Traversable (pythonGH-101589)pythongh-88233: zipfile: handle extras after a zip64 extra (pythonGH-96161)pythongh-101981: Apply HOMEBREW related environment variables (pythongh-102074)pythongh-101907: Stop using `_Py_OPCODE` and `_Py_OPARG` macros (pythonGH-101912)pythongh-101819: Adapt _io types to heap types, batch 1 (pythonGH-101949)pythongh-101981: Build macOS as recommended by the devguide (pythonGH-102070)pythongh-97786: Fix compiler warnings in pytime.c (python#101826)pythongh-101578: Amend PyErr_{Set,Get}RaisedException docs (python#101962)  Misc improvements to the float tutorial (pythonGH-102052)pythongh-85417: Clarify behaviour on branch cuts in cmath module (python#102046)pythongh-100425: Update tutorial docs related to sum() accuracy (FH-101854)  Add missing 'is' to `cmath.log()` docstring (python#102049)pythongh-100210: Correct the comment link for unescaping HTML (python#100212)pythongh-97930: Also include subdirectory in makefile. (python#102030)pythongh-99735: Use required=True in argparse subparsers example (python#100927)  Fix incorrectly documented attribute in csv docs (python#101250)pythonGH-84783: Make the slice object hashable (pythonGH-101264)  ...
mdickinson added a commit that referenced this pull requestFeb 26, 2023
* [3.10]gh-97786: Fix compiler warnings in pytime.c (GH-101826)Fixes compiler warnings in pytime.c..(cherry picked from commitb1b375e)Co-authored-by: Mark Dickinson <dickinsm@gmail.com>* Add comment about the casts---------Co-authored-by: Gregory P. Smith <greg@krypto.org>
mdickinson added a commit that referenced this pull requestMar 4, 2023
Since#101826 was merged, the internal macro `_Py_InIntegralTypeRange` is unused, as are its supporting macros `_Py_IntegralTypeMax` and `_Py_IntegralTypeMin`. This PR removes them.Note that `_Py_InIntegralTypeRange` doesn't actually work as advertised - it's not a safe way to avoid undefined behaviour in an integer to double conversion.
hugovk pushed a commit to hugovk/cpython that referenced this pull requestMar 6, 2023
Sincepython#101826 was merged, the internal macro `_Py_InIntegralTypeRange` is unused, as are its supporting macros `_Py_IntegralTypeMax` and `_Py_IntegralTypeMin`. This PR removes them.Note that `_Py_InIntegralTypeRange` doesn't actually work as advertised - it's not a safe way to avoid undefined behaviour in an integer to double conversion.
@vstinner
Copy link
Member

Thanks for fixing this old annoying compiler bug,@mdickinson!

@vstinner
Copy link
Member

I added math.nextafter() and math.ulp() to Python 3.9 to help me to understand this issue :-) But then I failed to come up with a fix for this warning.

@mdickinson
Copy link
MemberAuthor

math.nextafter andmath.ulp are very good to have, regardless! So thank you for that.

vstinner reacted with thumbs up emoji

@mdickinsonmdickinson deleted the fix-pytime-c-compiler-warnings branchApril 6, 2023 15:25
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@gpsheadgpsheadgpshead approved these changes

@brettcannonbrettcannonAwaiting requested review from brettcannon

@pgansslepganssleAwaiting requested review from pgansslepganssle is a code owner

@abalkinabalkinAwaiting requested review from abalkinabalkin is a code owner

Assignees

@gpsheadgpshead

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mdickinson@bedevere-bot@miss-islington@vstinner@gpshead
[8]ページ先頭
©2009-2025 Movatter.jp