Test failure on Python 3.12 beta

[brenns10]

While this was encountered on a separate branch from main, and it reproduces on main as well. The manylinux Docker image now has 3.12 beta in its list of interpreters, and I got a test failure building wheels:

test_number (tests.test_language_c.TestLexer.test_number) ... ok
test_symbols (tests.test_language_c.TestLexer.test_symbols) ... ok
test_bool (tests.test_language_c.TestLiteral.test_bool) ... ok
test_float (tests.test_language_c.TestLiteral.test_float) ... ok
test_int (tests.test_language_c.TestLiteral.test_int) ... python: /opt/_internal/cpython-3.12.0b1/include/python3.12/object.h:215: Py_SIZE: Assertion `ob->ob_type != &PyLong_Type' failed.
/io/scripts/build_manylinux_in_docker.sh: line 96: 220046 Aborted                 (core dumped) 

Git blame for the assertion shows it was added in python/cpython@7559f5f, related to python/cpython#101291.

I found the easiest way to reproduce (if you don't already have Python3.12 beta installed) was on Arch:

# install AUR python312 package which builds from source
yay -S python312

# it doesn't come with setuptools or pip...
mkdir tmp
cd tmp
cp -r /usr/lib/python3.11/site-packages/{pip,setuptools}/ .
python3.12 -m pip install --user pip setuptools
cd ..
rm -r tmp

python setup.py test

The coredump generated by the assertion shows the failure in DrgnObject_literal.

[brenns10]

As far as I can tell this is simply trying to detect whether the Python object is a negative number, and the commit which added the assertion added the following:

static inline bool
_PyLong_IsNegative(const PyLongObject *op)
{
    return (op->long_value.lv_tag & SIGN_MASK) == SIGN_NEGATIVE;
}

But of course this is private, which makes me wonder how we're intended to detect this?

[osandov]

Oof, DrgnObject_literal() is relying on an implementation detail here. We could stop relying on implementation details and do something like:

PyLong *zero = PyLong_FromLong(0);
if (zero) handle error;
int is_negative = PyObject_RichCompareBool(literal, zero, Py_LT);
if (is_negative < 0) handle error;
...

I don't know if this would have any performance impact. Most importantly, DrgnObject_literal() is used for any binary operation involving a drgn Object and a Python integer. We could probably mitigate a large chunk of that potential overhead by caching PyLong *zero: make it global and initialize it from PyInit__drgn.

If it is significantly slower, it looks like the internal headers are still installed publicly, so we might be able to use _PyLong_IsNegative() if we really need it...

[brenns10]

We could alternatively rely on the idea that a negative number would be an overflow error for an unsigned integer... And use the "better to ask forgiveness than permission" approach:

diff --git a/libdrgn/python/object.c b/libdrgn/python/object.c
index 1e866dc..0da05a5 100644
--- a/libdrgn/python/object.c
+++ b/libdrgn/python/object.c
@@ -18,15 +18,20 @@ static int DrgnObject_literal(struct drgn_object *res, PyObject *literal)
 	if (PyBool_Check(literal)) {
 		err = drgn_object_bool_literal(res, literal == Py_True);
 	} else if (PyLong_Check(literal)) {
-		bool is_negative = Py_SIZE(literal) < 0;
-		if (is_negative) {
+		bool is_negative = false;
+		uint64_t uvalue = PyLong_AsUint64(literal);
+
+		/* If overflow, it may be negative, assume so and retry */
+		if (uvalue == (uint64_t)-1 && PyErr_Occurred() &&
+		    PyErr_ExceptionMatches(PyExc_OverflowError)) {
+			is_negative = true;
+			PyErr_Clear();
 			literal = PyNumber_Negative(literal);
 			if (!literal)
 				return -1;
-		}
-		uint64_t uvalue = PyLong_AsUint64(literal);
-		if (is_negative)
+			uvalue = PyLong_AsUint64(literal);
 			Py_DECREF(literal);
+		}
 		if (uvalue == (uint64_t)-1 && PyErr_Occurred())
 			return -1;
 		err = drgn_object_integer_literal(res, uvalue);

This passes tests on 3.11 and 3.12

[brenns10]

Maybe it's easier to read the function rather than the diff :)

bool is_negative = false;
uint64_t uvalue = PyLong_AsUint64(literal);

/* If overflow, it may be negative, assume so and retry */
if (uvalue == (uint64_t)-1 && PyErr_Occurred() &&
	PyErr_ExceptionMatches(PyExc_OverflowError)) {
	is_negative = true;
	PyErr_Clear();
	literal = PyNumber_Negative(literal);
	if (!literal)
		return -1;
	uvalue = PyLong_AsUint64(literal);
	Py_DECREF(literal);
}
if (uvalue == (uint64_t)-1 && PyErr_Occurred())
	return -1;
err = drgn_object_integer_literal(res, uvalue);
if (!err && is_negative)
	err = drgn_object_neg(res, res);

[osandov]

I like it! I was worried about stuff which actually overflows (>= 2^64 or <= -2^64), but I think the second PyLong_AsUint64() call will catch that. Feel free to open a PR.