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.

diff --git a/Include/internal/pycore_cell.h b/Include/internal/pycore_cell.h
new file mode 100644 (file)
index 0000000..27f67d5
--- /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 5b89d6ba1acf710cc60d19a62c8e1c4ce5467cc3..f5c2af0696ac339aa9ebdf36c9b12b44fbf9619c 100644 (file)
--- 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 f1a43be38b2b58511cb598cd7f2cd768f53e749a..b1154e4ca4ace64da790b20d0beef4d837730bab 100644 (file)
--- 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 c944bbafdba7e595ca814ff4725a207b42d4d5d8..7a2a98df6511a1651fff91df98818659e2662967 100644 (file)
--- a/PCbuild/pythoncore.vcxproj
+++ b/PCbuild/pythoncore.vcxproj
@@ -210,6 +210,7 @@
     <ClInclude Include="..\Include\internal\pycore_bytesobject.h" />
     <ClInclude Include="..\Include\internal\pycore_call.h" />
     <ClInclude Include="..\Include\internal\pycore_capsule.h" />
+    <ClInclude Include="..\Include\internal\pycore_cell.h" />
     <ClInclude Include="..\Include\internal\pycore_ceval.h" />
     <ClInclude Include="..\Include\internal\pycore_ceval_state.h" />
     <ClInclude Include="..\Include\internal\pycore_cfg.h" />

diff --git a/PCbuild/pythoncore.vcxproj.filters b/PCbuild/pythoncore.vcxproj.filters
index 0afad125ce1e97781f2f67a8801ae55636eb1744..89b56ec1267104fa8b2534fdb73b431885bc58b2 100644 (file)
--- a/PCbuild/pythoncore.vcxproj.filters
+++ b/PCbuild/pythoncore.vcxproj.filters
@@ -555,6 +555,9 @@
     <ClInclude Include="..\Include\internal\pycore_capsule.h">
       <Filter>Include\internal</Filter>
     </ClInclude>
+    <ClInclude Include="..\Include\internal\pycore_cell.h">
+      <Filter>Include\internal</Filter>
+    </ClInclude>
     <ClInclude Include="..\Include\internal\pycore_ceval.h">
       <Filter>Include\internal</Filter>
     </ClInclude>

diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index 5cd9db97c71e375375aa4f104c58475a78c8f8d7..bfb378c4a41500e3818d6e378fef729eb7071b73 100644 (file)
--- 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 d34db61eecbae21263f8171ab8d270cc063d677c..1b13eb1702355f9f06c72bdd48da971f70db2890 100644 (file)
--- 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 224b600b8f6a4ac8c6c2b5fa5152183df9c761c3..ce0dc235c54fcf14f34c7650d82116cb725070a9 100644 (file)
--- 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 c66eb678d3847505d3f147f9cb792e788dfd5760..e8e2397b11cd489dff2a288d2c95558cddcd80c1 100644 (file)
--- 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 2329205ad31d09d01244a2813f3ef9396c380c00..ddafcf99ca1e373f10aee93e52a56b882d28dd9f 100644 (file)
--- 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 f8be4d7f78facd3373c261853be77e1d4d8fda4d..54160084cda46062026756c85f26027bdac9dd00 100644 (file)
--- 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"