Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-121153: Fix some errors with use of _PyLong_CompactValue() #121154

Merged
merged 6 commits into from
Jul 13, 2024
Merged

gh-121153: Fix some errors with use of _PyLong_CompactValue() #121154

merged 6 commits into from
Jul 13, 2024

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jun 29, 2024 •
edited
Loading

  • The result has type Py_ssize_t, not intptr_t.
  • Type cast between unsigned and signed integer types should be explicit.
  • Downcasting should be explicit.
  • Fix integer overflow check in sum().

@serhiy-storchaka serhiy-storchaka added skip news needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jun 29, 2024
@bedevere-app bedevere-app bot mentioned this pull request Jun 29, 2024
Closed
@serhiy-storchaka
pythongh-121153: Fix some errors with use of _PyLong_CompactValue()
5a6872f 
* The result has type Py_ssize_t, not intptr_t.
* Type cast between unsigned and signdet integer types should be explicit.
* Downcasting should be explicit.
vstinner
vstinner reviewed Jul 5, 2024
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can _PyLong_CompactValue() really be outside [LONG_MIN; LONG_MAX] range? It should be a single digit in the range [-PyLong_BASE; PyLong_BASE] and should even fit into a C int.

Why not changing _PyLong_CompactValue() return type to long? It would make this code way simpler.

@vstinner
Copy link
Member

vstinner commented Jul 5, 2024

(I'm not sure, correct me if I'm wrong.)

@serhiy-storchaka
Copy link
Member Author

I do not know. But currently it is defined as returning Py_ssize_t. If we change its return value, we can change the code that uses it.

It perhaps does not make difference for most or all currently supported platforms, but it is easier to write the code that handles all theoretically possible cases than hope that the simpler code which works on your platform will work the same way on different platforms with different compilers.

@vstinner
Copy link
Member

vstinner commented Jul 8, 2024

@markshannon: Is there a reason for _PyLong_CompactValue() return type to be Py_ssize_t rather long or even int?

@serhiy-storchaka
Copy link
Member Author

Returning int from _PyLong_CompactValue() would simplify the code a lot.

@vstinner
Copy link
Member

vstinner commented Jul 9, 2024

Returning int from _PyLong_CompactValue() would simplify the code a lot.

I created PR gh-121536 to only do that (don't try to simply code using _PyLong_CompactValue()).

@serhiy-storchaka serhiy-storchaka mentioned this pull request Jul 10, 2024
Closed
@markshannon
Copy link
Member

@markshannon: Is there a reason for _PyLong_CompactValue() return type to be Py_ssize_t rather long or even int?

Yes.
The representation of PyLongObject is internal. It is an implementation detail that it happens to return a value in the range -1<<30 <= val <= 1<<30. A different representation could well return values in the range -1<<62 <= val < 1<<62 (for 64 bit machines).

And please don't use long for new APIs, use a stdint.h type like intptr_t.

@serhiy-storchaka
Copy link
Member Author

Changing the returning type to intptr_t needs more changes. So I'm going to merge this PR first, and if anybody want to change the returning type, it will be a separate PR.

@serhiy-storchaka serhiy-storchaka merged commit 1801545 into python:main Jul 13, 2024
34 checks passed
@serhiy-storchaka serhiy-storchaka deleted the _PyLong_CompactValue-type-cast branch July 13, 2024 10:40
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jul 17, 2024
@serhiy-storchaka
[3.13] pythongh-121153: Fix some errors with use of _PyLong_CompactVa…
d2ca2ba 
…lue() (pythonGH-121154)

* The result has type Py_ssize_t, not intptr_t.
* Type cast between unsigned and signdet integer types should be explicit.
* Downcasting should be explicit.
* Fix integer overflow check in sum().
(cherry picked from commit 1801545)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jul 17, 2024
@serhiy-storchaka
[3.12] pythongh-121153: Fix some errors with use of _PyLong_CompactVa…
2eaa86e 
…lue() (pythonGH-121154)

* The result has type Py_ssize_t, not intptr_t.
* Type cast between unsigned and signdet integer types should be explicit.
* Downcasting should be explicit.
* Fix integer overflow check in sum().
(cherry picked from commit 1801545)

Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jul 17, 2024

GH-121900 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 17, 2024
@serhiy-storchaka serhiy-storchaka mentioned this pull request Jul 17, 2024
Merged
@bedevere-app
Copy link

bedevere-app bot commented Jul 17, 2024

GH-121901 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jul 17, 2024
serhiy-storchaka added a commit that referenced this pull request Jul 17, 2024
@serhiy-storchaka
[3.12] gh-121153: Fix some errors with use of _PyLong_CompactValue() (G…
874eed6 
…H-121154)

* The result has type Py_ssize_t, not intptr_t.
* Type cast between unsigned and signed integer types should be explicit.
* Downcasting should be explicit.
* Fix integer overflow check in sum().
(cherry picked from commit 1801545)
serhiy-storchaka added a commit that referenced this pull request Jul 17, 2024
@serhiy-storchaka
[3.13] gh-121153: Fix some errors with use of _PyLong_CompactValue() (G…
09ff4ec 
…H-121154) (GH-121900)

* The result has type Py_ssize_t, not intptr_t.
* Type cast between unsigned and signed integer types should be explicit.
* Downcasting should be explicit.
* Fix integer overflow check in sum().
(cherry picked from commit 1801545)
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@serhiy-storchaka @estyxx
pythongh-121153: Fix some errors with use of _PyLong_CompactValue() (p…
e1366ca 
…ythonGH-121154)

* The result has type Py_ssize_t, not intptr_t.
* Type cast between unsigned and signdet integer types should be explicit.
* Downcasting should be explicit.
* Fix integer overflow check in sum().
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

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@serhiy-storchaka @vstinner @markshannon