From 96d7297c8e593166cf515ab1a818fcb43e080459 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 29 Mar 2024 13:35:43 -0400 Subject: [PATCH] gh-117323: Make `cell` thread-safe in free-threaded builds (#117330) Use critical sections to lock around accesses to cell contents. The critical sections are no-ops in the default (with GIL) build. --- Include/internal/pycore_cell.h | 48 ++++++++++++++++++++++++++++++ Makefile.pre.in | 1 + Objects/cellobject.c | 8 ++--- PCbuild/pythoncore.vcxproj | 1 + PCbuild/pythoncore.vcxproj.filters | 3 ++ Python/bytecodes.c | 20 +++++-------- Python/ceval.c | 1 + Python/executor_cases.c.h | 19 +++++------- Python/generated_cases.c.h | 19 +++++------- Tools/cases_generator/analyzer.py | 5 ++-- Tools/jit/template.c | 1 + 11 files changed, 83 insertions(+), 43 deletions(-) create mode 100644 Include/internal/pycore_cell.h diff --git a/Include/internal/pycore_cell.h b/Include/internal/pycore_cell.h new file mode 100644 index 000000000000000..27f67d57b2fb794 --- /dev/null +++ b/Include/internal/pycore_cell.h @@ -0,0 +1,48 @@ +#ifndef Py_INTERNAL_CELL_H +#define Py_INTERNAL_CELL_H + +#include "pycore_critical_section.h" + +#ifdef __cplusplus +extern "C" { +#endif + +#ifndef Py_BUILD_CORE +# error "this header requires Py_BUILD_CORE define" +#endif + +// Sets the cell contents to `value` and return previous contents. Steals a +// reference to `value`. +static inline PyObject * +PyCell_SwapTakeRef(PyCellObject *cell, PyObject *value) +{ + PyObject *old_value; + Py_BEGIN_CRITICAL_SECTION(cell); + old_value = cell->ob_ref; + cell->ob_ref = value; + Py_END_CRITICAL_SECTION(); + return old_value; +} + +static inline void +PyCell_SetTakeRef(PyCellObject *cell, PyObject *value) +{ + PyObject *old_value = PyCell_SwapTakeRef(cell, value); + Py_XDECREF(old_value); +} + +// Gets the cell contents. Returns a new reference. +static inline PyObject * +PyCell_GetRef(PyCellObject *cell) +{ + PyObject *res; + Py_BEGIN_CRITICAL_SECTION(cell); + res = Py_XNewRef(cell->ob_ref); + Py_END_CRITICAL_SECTION(); + return res; +} + +#ifdef __cplusplus +} +#endif +#endif /* !Py_INTERNAL_CELL_H */ diff --git a/Makefile.pre.in b/Makefile.pre.in index 5b89d6ba1acf710..f5c2af0696ac339 100644 --- a/Makefile.pre.in +++ b/Makefile.pre.in @@ -1130,6 +1130,7 @@ PYTHON_HEADERS= \ $(srcdir)/Include/internal/pycore_bytesobject.h \ $(srcdir)/Include/internal/pycore_call.h \ $(srcdir)/Include/internal/pycore_capsule.h \ + $(srcdir)/Include/internal/pycore_cell.h \ $(srcdir)/Include/internal/pycore_ceval.h \ $(srcdir)/Include/internal/pycore_ceval_state.h \ $(srcdir)/Include/internal/pycore_code.h \ diff --git a/Objects/cellobject.c b/Objects/cellobject.c index f1a43be38b2b585..b1154e4ca4ace64 100644 --- a/Objects/cellobject.c +++ b/Objects/cellobject.c @@ -1,6 +1,7 @@ /* Cell object implementation */ #include "Python.h" +#include "pycore_cell.h" // PyCell_GetRef() #include "pycore_modsupport.h" // _PyArg_NoKeywords() #include "pycore_object.h" @@ -56,8 +57,7 @@ PyCell_Get(PyObject *op) PyErr_BadInternalCall(); return NULL; } - PyObject *value = PyCell_GET(op); - return Py_XNewRef(value); + return PyCell_GetRef((PyCellObject *)op); } int @@ -67,9 +67,7 @@ PyCell_Set(PyObject *op, PyObject *value) PyErr_BadInternalCall(); return -1; } - PyObject *old_value = PyCell_GET(op); - PyCell_SET(op, Py_XNewRef(value)); - Py_XDECREF(old_value); + PyCell_SetTakeRef((PyCellObject *)op, Py_XNewRef(value)); return 0; } diff --git a/PCbuild/pythoncore.vcxproj b/PCbuild/pythoncore.vcxproj index c944bbafdba7e59..7a2a98df6511a16 100644 --- a/PCbuild/pythoncore.vcxproj +++ b/PCbuild/pythoncore.vcxproj @@ -210,6 +210,7 @@ + diff --git a/PCbuild/pythoncore.vcxproj.filters b/PCbuild/pythoncore.vcxproj.filters index 0afad125ce1e977..89b56ec1267104f 100644 --- a/PCbuild/pythoncore.vcxproj.filters +++ b/PCbuild/pythoncore.vcxproj.filters @@ -555,6 +555,9 @@ Include\internal + + Include\internal + Include\internal diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 5cd9db97c71e375..bfb378c4a41500e 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -8,6 +8,7 @@ #include "Python.h" #include "pycore_abstract.h" // _PyIndex_Check() +#include "pycore_cell.h" // PyCell_GetRef() #include "pycore_code.h" #include "pycore_emscripten_signal.h" // _Py_CHECK_EMSCRIPTEN_SIGNALS #include "pycore_function.h" @@ -1523,14 +1524,13 @@ dummy_func( inst(DELETE_DEREF, (--)) { PyObject *cell = GETLOCAL(oparg); - PyObject *oldobj = PyCell_GET(cell); // Can't use ERROR_IF here. // Fortunately we don't need its superpower. + PyObject *oldobj = PyCell_SwapTakeRef((PyCellObject *)cell, NULL); if (oldobj == NULL) { _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); ERROR_NO_POP(); } - PyCell_SET(cell, NULL); Py_DECREF(oldobj); } @@ -1543,32 +1543,28 @@ dummy_func( ERROR_NO_POP(); } if (!value) { - PyObject *cell = GETLOCAL(oparg); - value = PyCell_GET(cell); + PyCellObject *cell = (PyCellObject *)GETLOCAL(oparg); + value = PyCell_GetRef(cell); if (value == NULL) { _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); ERROR_NO_POP(); } - Py_INCREF(value); } Py_DECREF(class_dict); } inst(LOAD_DEREF, ( -- value)) { - PyObject *cell = GETLOCAL(oparg); - value = PyCell_GET(cell); + PyCellObject *cell = (PyCellObject *)GETLOCAL(oparg); + value = PyCell_GetRef(cell); if (value == NULL) { _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); ERROR_IF(true, error); } - Py_INCREF(value); } inst(STORE_DEREF, (v --)) { - PyObject *cell = GETLOCAL(oparg); - PyObject *oldobj = PyCell_GET(cell); - PyCell_SET(cell, v); - Py_XDECREF(oldobj); + PyCellObject *cell = (PyCellObject *)GETLOCAL(oparg); + PyCell_SetTakeRef(cell, v); } inst(COPY_FREE_VARS, (--)) { diff --git a/Python/ceval.c b/Python/ceval.c index d34db61eecbae21..1b13eb1702355f9 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -5,6 +5,7 @@ #include "Python.h" #include "pycore_abstract.h" // _PyIndex_Check() #include "pycore_call.h" // _PyObject_CallNoArgs() +#include "pycore_cell.h" // PyCell_GetRef() #include "pycore_ceval.h" #include "pycore_code.h" #include "pycore_emscripten_signal.h" // _Py_CHECK_EMSCRIPTEN_SIGNALS diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 224b600b8f6a4ac..ce0dc235c54fcf1 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -1362,14 +1362,13 @@ case _DELETE_DEREF: { oparg = CURRENT_OPARG(); PyObject *cell = GETLOCAL(oparg); - PyObject *oldobj = PyCell_GET(cell); // Can't use ERROR_IF here. // Fortunately we don't need its superpower. + PyObject *oldobj = PyCell_SwapTakeRef((PyCellObject *)cell, NULL); if (oldobj == NULL) { _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); JUMP_TO_ERROR(); } - PyCell_SET(cell, NULL); Py_DECREF(oldobj); break; } @@ -1387,13 +1386,12 @@ JUMP_TO_ERROR(); } if (!value) { - PyObject *cell = GETLOCAL(oparg); - value = PyCell_GET(cell); + PyCellObject *cell = (PyCellObject *)GETLOCAL(oparg); + value = PyCell_GetRef(cell); if (value == NULL) { _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); JUMP_TO_ERROR(); } - Py_INCREF(value); } Py_DECREF(class_dict); stack_pointer[-1] = value; @@ -1403,13 +1401,12 @@ case _LOAD_DEREF: { PyObject *value; oparg = CURRENT_OPARG(); - PyObject *cell = GETLOCAL(oparg); - value = PyCell_GET(cell); + PyCellObject *cell = (PyCellObject *)GETLOCAL(oparg); + value = PyCell_GetRef(cell); if (value == NULL) { _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); if (true) JUMP_TO_ERROR(); } - Py_INCREF(value); stack_pointer[0] = value; stack_pointer += 1; break; @@ -1419,10 +1416,8 @@ PyObject *v; oparg = CURRENT_OPARG(); v = stack_pointer[-1]; - PyObject *cell = GETLOCAL(oparg); - PyObject *oldobj = PyCell_GET(cell); - PyCell_SET(cell, v); - Py_XDECREF(oldobj); + PyCellObject *cell = (PyCellObject *)GETLOCAL(oparg); + PyCell_SetTakeRef(cell, v); stack_pointer += -1; break; } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index c66eb678d384750..e8e2397b11cd489 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -2320,14 +2320,13 @@ next_instr += 1; INSTRUCTION_STATS(DELETE_DEREF); PyObject *cell = GETLOCAL(oparg); - PyObject *oldobj = PyCell_GET(cell); // Can't use ERROR_IF here. // Fortunately we don't need its superpower. + PyObject *oldobj = PyCell_SwapTakeRef((PyCellObject *)cell, NULL); if (oldobj == NULL) { _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); goto error; } - PyCell_SET(cell, NULL); Py_DECREF(oldobj); DISPATCH(); } @@ -4096,13 +4095,12 @@ next_instr += 1; INSTRUCTION_STATS(LOAD_DEREF); PyObject *value; - PyObject *cell = GETLOCAL(oparg); - value = PyCell_GET(cell); + PyCellObject *cell = (PyCellObject *)GETLOCAL(oparg); + value = PyCell_GetRef(cell); if (value == NULL) { _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); if (true) goto error; } - Py_INCREF(value); stack_pointer[0] = value; stack_pointer += 1; DISPATCH(); @@ -4186,13 +4184,12 @@ goto error; } if (!value) { - PyObject *cell = GETLOCAL(oparg); - value = PyCell_GET(cell); + PyCellObject *cell = (PyCellObject *)GETLOCAL(oparg); + value = PyCell_GetRef(cell); if (value == NULL) { _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); goto error; } - Py_INCREF(value); } Py_DECREF(class_dict); stack_pointer[-1] = value; @@ -5436,10 +5433,8 @@ INSTRUCTION_STATS(STORE_DEREF); PyObject *v; v = stack_pointer[-1]; - PyObject *cell = GETLOCAL(oparg); - PyObject *oldobj = PyCell_GET(cell); - PyCell_SET(cell, v); - Py_XDECREF(oldobj); + PyCellObject *cell = (PyCellObject *)GETLOCAL(oparg); + PyCell_SetTakeRef(cell, v); stack_pointer += -1; DISPATCH(); } diff --git a/Tools/cases_generator/analyzer.py b/Tools/cases_generator/analyzer.py index 2329205ad31d09d..ddafcf99ca1e373 100644 --- a/Tools/cases_generator/analyzer.py +++ b/Tools/cases_generator/analyzer.py @@ -520,8 +520,9 @@ def effect_depends_on_oparg_1(op: parser.InstDef) -> bool: def compute_properties(op: parser.InstDef) -> Properties: has_free = ( variable_used(op, "PyCell_New") - or variable_used(op, "PyCell_GET") - or variable_used(op, "PyCell_SET") + or variable_used(op, "PyCell_GetRef") + or variable_used(op, "PyCell_SetTakeRef") + or variable_used(op, "PyCell_SwapTakeRef") ) deopts_if = variable_used(op, "DEOPT_IF") exits_if = variable_used(op, "EXIT_IF") diff --git a/Tools/jit/template.c b/Tools/jit/template.c index f8be4d7f78facd3..54160084cda4606 100644 --- a/Tools/jit/template.c +++ b/Tools/jit/template.c @@ -2,6 +2,7 @@ #include "pycore_call.h" #include "pycore_ceval.h" +#include "pycore_cell.h" #include "pycore_dict.h" #include "pycore_emscripten_signal.h" #include "pycore_intrinsics.h"