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

[skirpichev]

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

[skirpichev]

Shouldn't affect users, so - no news.

CC @colesbury, @eendebakpt

[skirpichev]

_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]

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?

[colesbury]

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

[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!

[colesbury]

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

[colesbury]

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

[colesbury]

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

[skirpichev]

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

[skirpichev]

For failure in #145901 (comment).

I think we can set required asserts (operand: not a small int) for _PyLong_SetDigitCount and _PyLong_SetSignAndDigitCount. In most cases such assumption is true. Few exceptions coming from directly malloc'ed (not coming from the freelist) PyLongObject's: here we need to properly initialize the lv_tag, e.g. set it to 1 (zero). Following patch seems to be working:

diff --git a/Include/internal/pycore_long.h b/Include/internal/pycore_long.h
index 672e53a9427..f53dcbad2fc 100644
--- a/Include/internal/pycore_long.h
+++ b/Include/internal/pycore_long.h
@@ -289,6 +289,7 @@ _PyLong_SetSignAndDigitCount(PyLongObject *op, int sign, Py_ssize_t size)
     assert(size >= 0);
     assert(-1 <= sign && sign <= 1);
     assert(sign != 0 || size == 0);
+    assert(!_PyLong_HasImmortalTag(op));
     op->long_value.lv_tag = TAG_FROM_SIGN_AND_SIZE(sign, size);
 }
 
@@ -296,6 +297,7 @@ static inline void
 _PyLong_SetDigitCount(PyLongObject *op, Py_ssize_t size)
 {
     assert(size >= 0);
+    assert(!_PyLong_HasImmortalTag(op));
     op->long_value.lv_tag = (((size_t)size) << NON_SIZE_BITS) | (op->long_value.lv_tag & SIGN_MASK);
 }
 
diff --git a/Objects/longobject.c b/Objects/longobject.c
index b126efca0ef..73a051c3926 100644
--- a/Objects/longobject.c
+++ b/Objects/longobject.c
@@ -184,6 +184,7 @@ long_alloc(Py_ssize_t size)
             return NULL;
         }
         _PyObject_Init((PyObject*)result, &PyLong_Type);
+        result->long_value.lv_tag = 1;
     }
     _PyLong_SetSignAndDigitCount(result, size != 0, size);
     /* The digit has to be initialized explicitly to avoid
@@ -257,6 +258,7 @@ _PyLong_FromMedium(sdigit x)
             return NULL;
         }
         _PyObject_Init((PyObject*)v, &PyLong_Type);
+        v->long_value.lv_tag = 1;
     }
     digit abs_x = x < 0 ? -x : x;
     _PyLong_SetSignAndDigitCount(v, x<0?-1:1, 1);
@@ -336,6 +338,7 @@ medium_from_stwodigits(stwodigits x)
             return PyStackRef_NULL;
         }
         _PyObject_Init((PyObject*)v, &PyLong_Type);
+        v->long_value.lv_tag = 1;
     }
     digit abs_x = x < 0 ? (digit)(-x) : (digit)x;
     _PyLong_SetSignAndDigitCount(v, x<0?-1:1, 1);

Does it make sense here?

Maybe we need some helper for initialization of the PyLongObject struct, e.g.:

static inline void
_PyLong_Init(PyLongObject *op, Py_ssize_t size)
{
    assert(size >= 1);
    op->long_value.lv_tag = TAG_FROM_SIGN_AND_SIZE(0, size);
    op->long_value.digits[0] = 0;
}

[vstinner]

IMO the intent here is to check if an integer is a small integer, rather than checking for a tag. So I would prefer to rename the function to _PyLong_IsSmallInt().

I wanted to suggest referring to _PY_IS_SMALL_INT() but I noticed the macro is wrong. I wrote #146631 to fix the macro.

[skirpichev]

There are two notions for "small int"'s. One - by value and other is - this. If @colesbury is ok - I'm fine with renaming..

[vstinner]

_PyLong_HasImmortalTag() and _PY_IS_SMALL_INT() should be true or false for the same values.

For example, you can add the following check for _PyLong_HasImmortalTag():

+    assert((_PyLong_IsCompact(op) && _PY_IS_SMALL_INT(_PyLong_CompactValue(op)))
+           || (!is_small_int));

(You need to update your branch to retrieve my _PY_IS_SMALL_INT() change.)

For me, testing IMMORTALITY_BIT_MASK is just an implementation detail to check if an integer is "a small int".

[skirpichev]

I've added an assert. BTW, what do you think on #145901 (comment)?

I don't think we should mention _PY_IS_SMALL_INT in the description of the function, assert is readable enough.

For me, testing IMMORTALITY_BIT_MASK is just an implementation detail to check if an integer is "a small int".

For now, we use _PY_IS_SMALL_INT() to check this. But eventually this could be just testing a flag.

[vstinner]

I would prefer to rename the function to _PyLong_IsSmallInt().