pythongh-76785: Clean Up Interpreter ID Conversions (pythongh-117048)

Mostly we unify the two different implementations of the conversion code (from PyObject * to int64_t. We also drop the PyArg_ParseTuple()-style converter function, as well as rename and move PyInterpreterID_LookUp().
diegorusso · Apr 17, 2024 · fc1e7bf · fc1e7bf
1 parent 26fa43f
commit fc1e7bf
Show file tree

Hide file tree

Showing 8 changed files with 143 additions and 178 deletions.
diff --git a/Include/cpython/interpreteridobject.h b/Include/cpython/interpreteridobject.h
@@ -8,4 +8,7 @@ PyAPI_DATA(PyTypeObject) PyInterpreterID_Type;
 
 PyAPI_FUNC(PyObject *) PyInterpreterID_New(int64_t);
 PyAPI_FUNC(PyObject *) PyInterpreterState_GetIDObject(PyInterpreterState *);
-PyAPI_FUNC(PyInterpreterState *) PyInterpreterID_LookUp(PyObject *);
+
+#ifdef Py_BUILD_CORE
+extern int64_t _PyInterpreterID_GetID(PyObject *);
+#endif
diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h
@@ -295,8 +295,11 @@ _PyInterpreterState_SetFinalizing(PyInterpreterState *interp, PyThreadState *tst
 }
 
 
+extern int64_t _PyInterpreterState_ObjectToID(PyObject *);
+
 // Export for the _xxinterpchannels module.
 PyAPI_FUNC(PyInterpreterState *) _PyInterpreterState_LookUpID(int64_t);
+PyAPI_FUNC(PyInterpreterState *) _PyInterpreterState_LookUpIDObject(PyObject *);
 
 PyAPI_FUNC(int) _PyInterpreterState_IDInitref(PyInterpreterState *);
 PyAPI_FUNC(int) _PyInterpreterState_IDIncref(PyInterpreterState *);

diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py
@@ -2303,7 +2303,7 @@ def test_equality(self):
 
     def test_linked_lifecycle(self):
         id1 = _interpreters.create()
-        _testcapi.unlink_interpreter_refcount(id1)
+        _testinternalcapi.unlink_interpreter_refcount(id1)
         self.assertEqual(
             _testinternalcapi.get_interpreter_refcount(id1),
             0)
@@ -2319,7 +2319,7 @@ def test_linked_lifecycle(self):
             _testinternalcapi.get_interpreter_refcount(id1),
             0)
 
-        _testcapi.link_interpreter_refcount(id1)
+        _testinternalcapi.link_interpreter_refcount(id1)
         self.assertEqual(
             _testinternalcapi.get_interpreter_refcount(id1),
             0)

diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
@@ -1455,30 +1455,6 @@ get_interpreterid_type(PyObject *self, PyObject *Py_UNUSED(ignored))
     return Py_NewRef(&PyInterpreterID_Type);
 }
 
-static PyObject *
-link_interpreter_refcount(PyObject *self, PyObject *idobj)
-{
-    PyInterpreterState *interp = PyInterpreterID_LookUp(idobj);
-    if (interp == NULL) {
-        assert(PyErr_Occurred());
-        return NULL;
-    }
-    _PyInterpreterState_RequireIDRef(interp, 1);
-    Py_RETURN_NONE;
-}
-
-static PyObject *
-unlink_interpreter_refcount(PyObject *self, PyObject *idobj)
-{
-    PyInterpreterState *interp = PyInterpreterID_LookUp(idobj);
-    if (interp == NULL) {
-        assert(PyErr_Occurred());
-        return NULL;
-    }
-    _PyInterpreterState_RequireIDRef(interp, 0);
-    Py_RETURN_NONE;
-}
-
 static PyMethodDef ml;
 
 static PyObject *
@@ -3324,8 +3300,6 @@ static PyMethodDef TestMethods[] = {
     {"test_current_tstate_matches", test_current_tstate_matches, METH_NOARGS},
     {"run_in_subinterp",        run_in_subinterp,                METH_VARARGS},
     {"get_interpreterid_type",  get_interpreterid_type,          METH_NOARGS},
-    {"link_interpreter_refcount", link_interpreter_refcount,     METH_O},
-    {"unlink_interpreter_refcount", unlink_interpreter_refcount, METH_O},
     {"create_cfunction",        create_cfunction,                METH_NOARGS},
     {"call_in_temporary_c_thread", call_in_temporary_c_thread, METH_VARARGS,
      PyDoc_STR("set_error_class(error_class) -> None")},

diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c
@@ -29,8 +29,6 @@
 #include "pycore_pyerrors.h"      // _PyErr_ChainExceptions1()
 #include "pycore_pystate.h"       // _PyThreadState_GET()
 
-#include "interpreteridobject.h"  // PyInterpreterID_LookUp()
-
 #include "clinic/_testinternalcapi.c.h"
 
 // Include test definitions from _testinternalcapi/
@@ -1112,7 +1110,7 @@ pending_identify(PyObject *self, PyObject *args)
     if (!PyArg_ParseTuple(args, "O:pending_identify", &interpid)) {
         return NULL;
     }
-    PyInterpreterState *interp = PyInterpreterID_LookUp(interpid);
+    PyInterpreterState *interp = _PyInterpreterState_LookUpIDObject(interpid);
     if (interp == NULL) {
         if (!PyErr_Occurred()) {
             PyErr_SetString(PyExc_ValueError, "interpreter not found");
@@ -1480,13 +1478,37 @@ run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs)
 static PyObject *
 get_interpreter_refcount(PyObject *self, PyObject *idobj)
 {
-    PyInterpreterState *interp = PyInterpreterID_LookUp(idobj);
+    PyInterpreterState *interp = _PyInterpreterState_LookUpIDObject(idobj);
     if (interp == NULL) {
         return NULL;
     }
     return PyLong_FromLongLong(interp->id_refcount);
 }
 
+static PyObject *
+link_interpreter_refcount(PyObject *self, PyObject *idobj)
+{
+    PyInterpreterState *interp = _PyInterpreterState_LookUpIDObject(idobj);
+    if (interp == NULL) {
+        assert(PyErr_Occurred());
+        return NULL;
+    }
+    _PyInterpreterState_RequireIDRef(interp, 1);
+    Py_RETURN_NONE;
+}
+
+static PyObject *
+unlink_interpreter_refcount(PyObject *self, PyObject *idobj)
+{
+    PyInterpreterState *interp = _PyInterpreterState_LookUpIDObject(idobj);
+    if (interp == NULL) {
+        assert(PyErr_Occurred());
+        return NULL;
+    }
+    _PyInterpreterState_RequireIDRef(interp, 0);
+    Py_RETURN_NONE;
+}
+
 
 static void
 _xid_capsule_destructor(PyObject *capsule)
@@ -1728,6 +1750,8 @@ static PyMethodDef module_functions[] = {
      _PyCFunction_CAST(run_in_subinterp_with_config),
      METH_VARARGS | METH_KEYWORDS},
     {"get_interpreter_refcount", get_interpreter_refcount, METH_O},
+    {"link_interpreter_refcount", link_interpreter_refcount,     METH_O},
+    {"unlink_interpreter_refcount", unlink_interpreter_refcount, METH_O},
     {"compile_perf_trampoline_entry", compile_perf_trampoline_entry, METH_VARARGS},
     {"perf_trampoline_set_persist_after_fork", perf_trampoline_set_persist_after_fork, METH_VARARGS},
     {"get_crossinterp_data",    get_crossinterp_data,            METH_VARARGS},

diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c
@@ -35,83 +35,8 @@ _get_current_interp(void)
     return PyInterpreterState_Get();
 }
 
-static int64_t
-pylong_to_interpid(PyObject *idobj)
-{
-    assert(PyLong_CheckExact(idobj));
-
-    if (_PyLong_IsNegative((PyLongObject *)idobj)) {
-        PyErr_Format(PyExc_ValueError,
-                     "interpreter ID must be a non-negative int, got %R",
-                     idobj);
-        return -1;
-    }
-
-    int overflow;
-    long long id = PyLong_AsLongLongAndOverflow(idobj, &overflow);
-    if (id == -1) {
-        if (!overflow) {
-            assert(PyErr_Occurred());
-            return -1;
-        }
-        assert(!PyErr_Occurred());
-        // For now, we don't worry about if LLONG_MAX < INT64_MAX.
-        goto bad_id;
-    }
-#if LLONG_MAX > INT64_MAX
-    if (id > INT64_MAX) {
-        goto bad_id;
-    }
-#endif
-    return (int64_t)id;
-
-bad_id:
-    PyErr_Format(PyExc_RuntimeError,
-                 "unrecognized interpreter ID %O", idobj);
-    return -1;
-}
-
-static int64_t
-convert_interpid_obj(PyObject *arg)
-{
-    int64_t id = -1;
-    if (_PyIndex_Check(arg)) {
-        PyObject *idobj = PyNumber_Long(arg);
-        if (idobj == NULL) {
-            return -1;
-        }
-        id = pylong_to_interpid(idobj);
-        Py_DECREF(idobj);
-        if (id < 0) {
-            return -1;
-        }
-    }
-    else {
-        PyErr_Format(PyExc_TypeError,
-                     "interpreter ID must be an int, got %.100s",
-                     Py_TYPE(arg)->tp_name);
-        return -1;
-    }
-    return id;
-}
-
-static PyInterpreterState *
-look_up_interp(PyObject *arg)
-{
-    int64_t id = convert_interpid_obj(arg);
-    if (id < 0) {
-        return NULL;
-    }
-    return _PyInterpreterState_LookUpID(id);
-}
-
+#define look_up_interp _PyInterpreterState_LookUpIDObject
 
-static PyObject *
-interpid_to_pylong(int64_t id)
-{
-    assert(id < LLONG_MAX);
-    return PyLong_FromLongLong(id);
-}
 
 static PyObject *
 get_interpid_obj(PyInterpreterState *interp)
@@ -123,7 +48,8 @@ get_interpid_obj(PyInterpreterState *interp)
     if (id < 0) {
         return NULL;
     }
-    return interpid_to_pylong(id);
+    assert(id < LLONG_MAX);
+    return PyLong_FromLongLong(id);
 }
 
 static PyObject *
@@ -699,7 +625,7 @@ interp_set___main___attrs(PyObject *self, PyObject *args)
     }
 
     // Look up the interpreter.
-    PyInterpreterState *interp = PyInterpreterID_LookUp(id);
+    PyInterpreterState *interp = look_up_interp(id);
     if (interp == NULL) {
         return NULL;
     }

diff --git a/Objects/interpreteridobject.c b/Objects/interpreteridobject.c
@@ -1,8 +1,7 @@
 /* InterpreterID object */
 
 #include "Python.h"
-#include "pycore_abstract.h"   // _PyIndex_Check()
-#include "pycore_interp.h"     // _PyInterpreterState_LookUpID()
+#include "pycore_interp.h"        // _PyInterpreterState_LookUpID()
 #include "interpreteridobject.h"
 
 
@@ -11,6 +10,21 @@ typedef struct interpid {
     int64_t id;
 } interpid;
 
+int64_t
+_PyInterpreterID_GetID(PyObject *self)
+{
+    if (!PyObject_TypeCheck(self, &PyInterpreterID_Type)) {
+        PyErr_Format(PyExc_TypeError,
+                     "expected an InterpreterID, got %R",
+                     self);
+        return -1;
+
+    }
+    int64_t id = ((interpid *)self)->id;
+    assert(id >= 0);
+    return id;
+}
+
 static interpid *
 newinterpid(PyTypeObject *cls, int64_t id, int force)
 {
@@ -42,43 +56,19 @@ newinterpid(PyTypeObject *cls, int64_t id, int force)
     return self;
 }
 
-static int
-interp_id_converter(PyObject *arg, void *ptr)
-{
-    int64_t id;
-    if (PyObject_TypeCheck(arg, &PyInterpreterID_Type)) {
-        id = ((interpid *)arg)->id;
-    }
-    else if (_PyIndex_Check(arg)) {
-        id = PyLong_AsLongLong(arg);
-        if (id == -1 && PyErr_Occurred()) {
-            return 0;
-        }
-        if (id < 0) {
-            PyErr_Format(PyExc_ValueError,
-                         "interpreter ID must be a non-negative int, got %R", arg);
-            return 0;
-        }
-    }
-    else {
-        PyErr_Format(PyExc_TypeError,
-                     "interpreter ID must be an int, got %.100s",
-                     Py_TYPE(arg)->tp_name);
-        return 0;
-    }
-    *(int64_t *)ptr = id;
-    return 1;
-}
-
 static PyObject *
 interpid_new(PyTypeObject *cls, PyObject *args, PyObject *kwds)
 {
     static char *kwlist[] = {"id", "force", NULL};
-    int64_t id;
+    PyObject *idobj;
     int force = 0;
     if (!PyArg_ParseTupleAndKeywords(args, kwds,
-                                     "O&|$p:InterpreterID.__init__", kwlist,
-                                     interp_id_converter, &id, &force)) {
+                                     "O|$p:InterpreterID.__init__", kwlist,
+                                     &idobj, &force)) {
+        return NULL;
+    }
+    int64_t id = _PyInterpreterState_ObjectToID(idobj);
+    if (id < 0) {
         return NULL;
     }
 
@@ -282,13 +272,3 @@ PyInterpreterState_GetIDObject(PyInterpreterState *interp)
     }
     return (PyObject *)newinterpid(&PyInterpreterID_Type, id, 0);
 }
-
-PyInterpreterState *
-PyInterpreterID_LookUp(PyObject *requested_id)
-{
-    int64_t id;
-    if (!interp_id_converter(requested_id, &id)) {
-        return NULL;
-    }
-    return _PyInterpreterState_LookUpID(id);
-}