From bbee57fa8c318cb26d6c8651254927a1972c9738 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 21 Mar 2024 09:56:12 -0600 Subject: [PATCH] gh-76785: Clean Up Interpreter ID Conversions (gh-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(). --- Include/cpython/interpreteridobject.h | 5 +- Include/internal/pycore_interp.h | 3 + Lib/test/test_capi/test_misc.py | 4 +- Modules/_testcapimodule.c | 26 ------- Modules/_testinternalcapi.c | 32 +++++++- Modules/_xxsubinterpretersmodule.c | 82 +------------------- Objects/interpreteridobject.c | 66 ++++++----------- Python/pystate.c | 103 ++++++++++++++++++++------ 8 files changed, 143 insertions(+), 178 deletions(-) diff --git a/Include/cpython/interpreteridobject.h b/Include/cpython/interpreteridobject.h index 4ab9ad5d315..d425c909806 100644 --- 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 index 942f47340b3..b28e8a3ff45 100644 --- 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 index 5b4f67e7f5f..fe5e19d46d8 100644 --- 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 index b73085bb8f6..e68d083955d 100644 --- 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 index 1c10dd02138..f73a29e5afe 100644 --- 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 index 28c2f9c08bc..606b2a36481 100644 --- 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)); +#define look_up_interp _PyInterpreterState_LookUpIDObject - 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); -} - - -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 index 16e27b64c0c..4844d6a9bf7 100644 --- 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); -} diff --git a/Python/pystate.c b/Python/pystate.c index 5a334e8721e..5332b8a827d 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2,6 +2,8 @@ /* Thread and interpreter state structures and their interfaces */ #include "Python.h" +#include "interpreteridobject.h" // PyInterpreterID_Type +#include "pycore_abstract.h" // _PyIndex_Check() #include "pycore_ceval.h" #include "pycore_code.h" // stats #include "pycore_critical_section.h" // _PyCriticalSection_Resume() @@ -1064,6 +1066,73 @@ _PyInterpreterState_FailIfRunningMain(PyInterpreterState *interp) // accessors //---------- +PyObject * +PyUnstable_InterpreterState_GetMainModule(PyInterpreterState *interp) +{ + PyObject *modules = _PyImport_GetModules(interp); + if (modules == NULL) { + PyErr_SetString(PyExc_RuntimeError, "interpreter not initialized"); + return NULL; + } + return PyMapping_GetItemString(modules, "__main__"); +} + +PyObject * +PyInterpreterState_GetDict(PyInterpreterState *interp) +{ + if (interp->dict == NULL) { + interp->dict = PyDict_New(); + if (interp->dict == NULL) { + PyErr_Clear(); + } + } + /* Returning NULL means no per-interpreter dict is available. */ + return interp->dict; +} + + +//---------- +// interp ID +//---------- + +int64_t +_PyInterpreterState_ObjectToID(PyObject *idobj) +{ + if (PyObject_TypeCheck(idobj, &PyInterpreterID_Type)) { + return _PyInterpreterID_GetID(idobj); + } + + if (!_PyIndex_Check(idobj)) { + PyErr_Format(PyExc_TypeError, + "interpreter ID must be an int, got %.100s", + Py_TYPE(idobj)->tp_name); + return -1; + } + + // This may raise OverflowError. + // For now, we don't worry about if LLONG_MAX < INT64_MAX. + long long id = PyLong_AsLongLong(idobj); + if (id == -1 && PyErr_Occurred()) { + return -1; + } + + if (id < 0) { + PyErr_Format(PyExc_ValueError, + "interpreter ID must be a non-negative int, got %R", + idobj); + return -1; + } +#if LLONG_MAX > INT64_MAX + else if (id > INT64_MAX) { + PyErr_SetString(PyExc_OverflowError, "int too big to convert"); + return -1; + } +#endif + else { + return (int64_t)id; + } +} + int64_t PyInterpreterState_GetID(PyInterpreterState *interp) { @@ -1142,30 +1211,6 @@ _PyInterpreterState_RequireIDRef(PyInterpreterState *interp, int required) interp->requires_idref = required ? 1 : 0; } -PyObject * -PyUnstable_InterpreterState_GetMainModule(PyInterpreterState *interp) -{ - PyObject *modules = _PyImport_GetModules(interp); - if (modules == NULL) { - PyErr_SetString(PyExc_RuntimeError, "interpreter not initialized"); - return NULL; - } - return PyMapping_GetItemString(modules, "__main__"); -} - -PyObject * -PyInterpreterState_GetDict(PyInterpreterState *interp) -{ - if (interp->dict == NULL) { - interp->dict = PyDict_New(); - if (interp->dict == NULL) { - PyErr_Clear(); - } - } - /* Returning NULL means no per-interpreter dict is available. */ - return interp->dict; -} - //----------------------------- // look up an interpreter state @@ -1227,6 +1272,16 @@ _PyInterpreterState_LookUpID(int64_t requested_id) return interp; } +PyInterpreterState * +_PyInterpreterState_LookUpIDObject(PyObject *requested_id) +{ + int64_t id = _PyInterpreterState_ObjectToID(requested_id); + if (id < 0) { + return NULL; + } + return _PyInterpreterState_LookUpID(id); +} + /********************************/ /* the per-thread runtime state */