gh-143050: correct PyLong_FromString() to use _PyLong_Negate() by skirpichev · Pull Request #145901 · python/cpython

skirpichev · 2026-03-13T07:53:50Z

The long_from_string_base() might return a small integer, when the _pylong.py is used to do conversion. Hence, we must be careful here to not smash it "small int" bit by using the _PyLong_FlipSign().

Co-authored-by: Sam Gross colesbury@gmail.com

Issue: Remove redundant decref from _PyLong_Negate #143050

The long_from_string_base() might return a small integer, when the _pylong.py is used to do conversion. Hence, we must be careful here to not smash it "small int" bit by using the _PyLong_FlipSign().

skirpichev · 2026-03-13T07:55:06Z

Shouldn't affect users, so - no news.

CC @colesbury, @eendebakpt

skirpichev · 2026-03-13T10:11:02Z

_long_is_small_int() logic used also in Modules/_testcapi/immortal.c. What about moving this inlined function to the Include/internal/pycore_long.h as _PyLong_IsImmortal()?

skirpichev · 2026-03-13T10:19:57Z

Lib/test/test_capi/test_long.py

+        with support.adjust_int_max_str_digits(0):
+            a = int('-' + '0' * 7000, 10)
+            del a
+            _testcapi.test_immortal_small_ints()


I'm slightly worry that this pass will non-debug builds.

@corona10 (author of #103962), are you sure there is no other way to implement this helper? Return a different value, raise an error?

That's okay as long as it catches the regression in a debug build.

eendebakpt · 2026-03-13T13:33:29Z

Objects/longobject.c


    /* Set sign and normalize */
-    if (sign < 0) {
-        _PyLong_FlipSign(z);


Calling _PyLong_FlipSign this on the 0 small int caused the problems. Can we add an assert in _PyLong_FlipSign:

static inline void _PyLong_FlipSign(PyLongObject *op) { assert(!__PyLong_IsSmallInt((PyObject *)op)); unsigned int flipped_sign = 2 - (op->long_value.lv_tag & SIGN_MASK); op->long_value.lv_tag &= NON_SIZE_MASK; op->long_value.lv_tag |= flipped_sign; }

The __PyLong_IsSmallInt does not exist yet, I used the following in testing:

static inline int /// Return 1 if the object is one of the immortal small ints _PyLong_IsSmallInt(PyObject *op) { // slow, but safe version, for use in assertions and debugging. for(int i = 0; i < _PY_NSMALLPOSINTS + _PY_NSMALLNEGINTS; i++) { if (op == (PyObject *)&_PyLong_SMALL_INTS[i]) { return 1; } } return 0; }

In a similar way we could add an assert to _PyLong_SetSignAndDigitCount.

Calling _PyLong_FlipSign this on the 0 small int caused the problems.

Yes, but any small int will trigger this. We can add test case for -1, for example.

Can we add an assert in _PyLong_FlipSign

My suggestion would be using assert(!_PyLong_IsImmortal((PyObject *)op));: as the "small int" bit will be cleared by the _PyLong_FlipSign(). (_PyLong_IsImmortal() is current _long_is_small_int().)

In a similar way we could add an assert to _PyLong_SetSignAndDigitCount.

This will affect all "setters" in Include/internal/pycore_long.h (i.e. also _PyLong_SetDigitCount).

I've added _PyLong_IsImmortal() helper.

In a similar way we could add an assert to _PyLong_SetSignAndDigitCount.

I tried to add assert here and in _PyLong_SetDigitCount(), but got:

$ make -s _freeze_module: ./Include/internal/pycore_long.h:291: _PyLong_SetSignAndDigitCount: Assertion `!_PyLong_IsImmortal(op)' failed. Aborted make: *** [Makefile:1952: Python/frozen_modules/getpath.h] Error 134

Lib/test/test_capi/test_long.py

eendebakpt

Minor comment on the naming. But the PR fixes a bug, adds a test and cleans up along the way, so good enough for me!

eendebakpt · 2026-03-17T12:11:35Z

Include/internal/pycore_long.h

 }

+static inline bool
+_PyLong_IsImmortal(const PyLongObject *op)


Two minor things:

This checks for immortality in a different way than _Py_IsImmortal. That should be reflected in the name of the method or in a comment. A user could use PyUnstable_SetImmortal to make a long object immortal, but these would still return false.

Should we add: assert(_Py_IsImmortal(op)) here?

This checks for immortality in a different way than _Py_IsImmortal. That should be reflected in the name of the method or in a comment.

Yes, I'm also not happy with the name. _PyLong_IsSmallInt, sounds better?

This will clash with the macro IS_SMALL_INT, using a different meaning of what is "small". Though, this could be solved by a comment. BTW, there is also a different macro _PY_IS_SMALL_INT, added in 0664c1a, used just in one place.

Should we add: assert(_Py_IsImmortal(op)) here?

Does make sense for me.

I'd use something like _PyLong_HasImmortalTag to make it more clear that this is a check on lv_tag.

colesbury

Looks good to me. Small comment about the function name below.

colesbury · 2026-03-17T20:02:24Z

Lib/test/test_capi/test_long.py

+        with support.adjust_int_max_str_digits(0):
+            a = int('-' + '0' * 7000, 10)
+            del a
+            _testcapi.test_immortal_small_ints()


That's okay as long as it catches the regression in a debug build.

colesbury · 2026-03-17T20:03:27Z

Include/internal/pycore_long.h

 }

+static inline bool
+_PyLong_IsImmortal(const PyLongObject *op)


I'd use something like _PyLong_HasImmortalTag to make it more clear that this is a check on lv_tag.

colesbury

Let me know if you change the function name or keep it as is.

skirpichev · 2026-03-17T23:32:42Z

@colesbury, I did renaming.

~~I've tried also to add an assert(_Py_IsImmortal(op)), but this fails build like check in the _PyLong_SetSignAndDigitCount (see #145901 (comment)):~~

make: *** [Makefile:1957: Python/frozen_modules/importlib._bootstrap.h] Error 134
./Programs/_freeze_module importlib._bootstrap_external ../cpython-ro-srcdir/Lib/importlib/_bootstrap_external.py Python/frozen_modules/importlib._bootstrap_external.h
_freeze_module: ../cpython-ro-srcdir/Include/internal/pycore_long.h:238: _PyLong_IsImmortal: Assertion `_Py_IsImmortal(op)' failed.
Aborted (core dumped)
make: *** [Makefile:1960: Python/frozen_modules/importlib._bootstrap_external.h] Error 134
Error: Process completed with exit code 2.

~~I believe it's a bug, but lets fix this separately.~~

colesbury · 2026-03-18T02:18:58Z

Include/internal/pycore_long.h

    assert(PyLong_Check(op));
    bool is_small_int = (op->long_value.lv_tag & IMMORTALITY_BIT_MASK) != 0;
    assert(PyLong_CheckExact(op) || (!is_small_int));
+    assert((is_small_int && _Py_IsImmortal(op)) || (!is_small_int));


This is equivalent to:

assert(_Py_IsImmortal(op) || (!is_small_int));

I was thinking this may be less readable. Perhaps, not.

pythongh-143050: correct PyLong_FromString() to use _PyLong_Negate()

fbcec5a

The long_from_string_base() might return a small integer, when the _pylong.py is used to do conversion. Hence, we must be careful here to not smash it "small int" bit by using the _PyLong_FlipSign().

skirpichev requested a review from a team as a code owner March 13, 2026 07:53

skirpichev added the skip news label Mar 13, 2026

skirpichev requested review from AA-Turner, emmatyping and erlend-aasland as code owners March 13, 2026 07:53

bedevere-app bot added the awaiting review label Mar 13, 2026

bedevere-app bot mentioned this pull request Mar 13, 2026

Remove redundant decref from _PyLong_Negate #143050

Open

cleanup, use _testcapi.test_immortal_small_ints()

b8333ba

skirpichev commented Mar 13, 2026

View reviewed changes

eendebakpt reviewed Mar 13, 2026

View reviewed changes

Lib/test/test_capi/test_long.py Outdated Show resolved Hide resolved

skirpichev added 2 commits March 14, 2026 03:02

+ add also test with nonzero small int

20e79c6

_long_is_small_int() -> _PyLong_IsImmortal() helper

413e56d

skirpichev requested review from colesbury and eendebakpt March 15, 2026 04:13

eendebakpt approved these changes Mar 17, 2026

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Mar 17, 2026

colesbury reviewed Mar 17, 2026

View reviewed changes

colesbury approved these changes Mar 17, 2026

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Mar 17, 2026

skirpichev added 2 commits March 18, 2026 02:12

Merge branch 'master' into fix-PyLong_FromString/143050

b892216

address review: rename _PyLong_IsImmortal -> _PyLong_HasImmortalTag

78c5b4b

skirpichev force-pushed the fix-PyLong_FromString/143050 branch from e28e94c to 78c5b4b Compare March 17, 2026 23:27

add assert

69d35bd

colesbury reviewed Mar 18, 2026

View reviewed changes

address review: simplify assert

2f927e0

Uh oh!

Conversation

skirpichev commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev commented Mar 13, 2026

Uh oh!

skirpichev commented Mar 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eendebakpt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

skirpichev commented Mar 13, 2026 •

edited

Loading

skirpichev commented Mar 17, 2026 •

edited

Loading