gh-127271: Replace use of PyCell_GET/SET

Doc/howto/free-threading-extensions.rst

@@ -167,6 +167,8 @@ that return :term:`strong references <strong reference>`.
 +-----------------------------------+-----------------------------------+
 | :c:func:`PyImport_AddModule`      | :c:func:`PyImport_AddModuleRef`   |
 +-----------------------------------+-----------------------------------+
+| :c:func:`PyCell_GET`              | :c:func:`PyCell_Get`              |
++-----------------------------------+-----------------------------------+
 
 Not all APIs that return borrowed references are problematic.  For
 example, :c:func:`PyTuple_GetItem` is safe because tuples are immutable.

Include/Python.h

@@ -69,6 +69,7 @@
 #include "pystats.h"
 #include "pyatomic.h"
 #include "lock.h"
+#include "critical_section.h"
 #include "object.h"
 #include "refcount.h"
 #include "objimpl.h"
@@ -130,7 +131,6 @@
 #include "import.h"
 #include "abstract.h"
 #include "bltinmodule.h"
-#include "critical_section.h"
 #include "cpython/pyctype.h"
 #include "pystrtod.h"
 #include "pystrcmp.h"

Include/cpython/cellobject.h

@@ -22,18 +22,24 @@ PyAPI_FUNC(PyObject *) PyCell_Get(PyObject *);
 PyAPI_FUNC(int) PyCell_Set(PyObject *, PyObject *);
 
 static inline PyObject* PyCell_GET(PyObject *op) {
+    PyObject *res;
     PyCellObject *cell;
     assert(PyCell_Check(op));
     cell = _Py_CAST(PyCellObject*, op);
-    return cell->ob_ref;
+    Py_BEGIN_CRITICAL_SECTION(cell);
+    res = cell->ob_ref;
+    Py_END_CRITICAL_SECTION();
+    return res;
 }
 #define PyCell_GET(op) PyCell_GET(_PyObject_CAST(op))
 
 static inline void PyCell_SET(PyObject *op, PyObject *value) {
     PyCellObject *cell;
     assert(PyCell_Check(op));
     cell = _Py_CAST(PyCellObject*, op);
+    Py_BEGIN_CRITICAL_SECTION(cell);
     cell->ob_ref = value;
+    Py_END_CRITICAL_SECTION();
 }
 #define PyCell_SET(op, value) PyCell_SET(_PyObject_CAST(op), (value))
 

Lib/test/test_free_threading/test_races.py

@@ -0,0 +1,131 @@
+# It's most useful to run these tests with ThreadSanitizer enabled.
+import sys
+import functools
+import threading
+import time
+import unittest
+
+from test.support import threading_helper
+
+
+class TestBase(unittest.TestCase):
+    pass
+
+
+def do_race(func1, func2):
+    """Run func1() and func2() repeatedly in separate threads."""
+    n = 1000
+
+    def repeat(func):
+        for _i in range(n):
+            func()
+
+    threads = [
+        threading.Thread(target=functools.partial(repeat, func1)),
+        threading.Thread(target=functools.partial(repeat, func2)),
+    ]
+    for thread in threads:
+        thread.start()
+    for thread in threads:
+        thread.join()
+
+
+@threading_helper.requires_working_threading()
+class TestRaces(TestBase):
+    def test_racing_cell_set(self):
+        """Test cell object gettr/settr properties."""
+
+        def nested_func():
+            x = 0
+
+            def inner():
+                nonlocal x
+                x += 1
+
+        # This doesn't race because LOAD_DEREF and STORE_DEREF on the
+        # cell object use critical sections.
+        do_race(nested_func, nested_func)
+
+        def nested_func2():
+            x = 0
+
+            def inner():
+                y = x
+                frame = sys._getframe(1)
+                frame.f_locals["x"] = 2
+
+            return inner
+
+        def mutate_func2():
+            inner = nested_func2()
+            cell = inner.__closure__[0]
+            old_value = cell.cell_contents
+            cell.cell_contents = 1000
+            time.sleep(0)
+            cell.cell_contents = old_value
+            time.sleep(0)
+
+        # This revealed a race with cell_set_contents() since it was missing
+        # the critical section.
+        do_race(nested_func2, mutate_func2)
+
+    def test_racing_cell_cmp_repr(self):
+        """Test cell object compare and repr methods."""
+
+        def nested_func():
+            x = 0
+            y = 0
+
+            def inner():
+                return x + y
+
+            return inner.__closure__
+
+        cell_a, cell_b = nested_func()
+
+        def mutate():
+            cell_a.cell_contents += 1
+
+        def access():
+            cell_a == cell_b
+            s = repr(cell_a)
+
+        # cell_richcompare() and cell_repr used to have data races
+        do_race(mutate, access)
+
+    def test_racing_load_super_attr(self):
+        """Test (un)specialization of LOAD_SUPER_ATTR opcode."""
+
+        class C:
+            def __init__(self):
+                try:
+                    super().__init__
+                    super().__init__()
+                except RuntimeError:
+                    pass  #  happens if __class__ is replaced with non-type
+
+        def access():
+            C()
+
+        def mutate():
+            # Swap out the super() global with a different one
+            real_super = super
+            globals()["super"] = lambda s=1: s
+            time.sleep(0)
+            globals()["super"] = real_super
+            time.sleep(0)
+            # Swap out the __class__ closure value with a non-type
+            cell = C.__init__.__closure__[0]
+            real_class = cell.cell_contents
+            cell.cell_contents = 99
+            time.sleep(0)
+            cell.cell_contents = real_class
+
+        # The initial PR adding specialized opcodes for LOAD_SUPER_ATTR
+        # had some races (one with the super() global changing and one
+        # with the cell binding being changed).
+        do_race(access, mutate)
+
+
+if __name__ == "__main__":
+    unittest.main()

Objects/cellobject.c

@@ -82,6 +82,17 @@ cell_dealloc(PyObject *self)
     PyObject_GC_Del(op);
 }
 
+static PyObject *
+cell_compare_impl(PyObject *a, PyObject *b, int op)
+{
+    if (a != NULL && b != NULL) {
+        return PyObject_RichCompare(a, b, op);
+    }
+    else {
+        Py_RETURN_RICHCOMPARE(b == NULL, a == NULL, op);
+    }
+}
+
 static PyObject *
 cell_richcompare(PyObject *a, PyObject *b, int op)
 {
@@ -92,27 +103,28 @@ cell_richcompare(PyObject *a, PyObject *b, int op)
     if (!PyCell_Check(a) || !PyCell_Check(b)) {
         Py_RETURN_NOTIMPLEMENTED;
     }
+    PyObject *a_ref = PyCell_GetRef((PyCellObject *)a);
+    PyObject *b_ref = PyCell_GetRef((PyCellObject *)b);
 
     /* compare cells by contents; empty cells come before anything else */
-    a = ((PyCellObject *)a)->ob_ref;
-    b = ((PyCellObject *)b)->ob_ref;
-    if (a != NULL && b != NULL)
-        return PyObject_RichCompare(a, b, op);
+    PyObject *res = cell_compare_impl(a_ref, b_ref, op);
 
-    Py_RETURN_RICHCOMPARE(b == NULL, a == NULL, op);
+    Py_XDECREF(a_ref);
+    Py_XDECREF(b_ref);
+    return res;
 }
 
 static PyObject *
 cell_repr(PyObject *self)
 {
-    PyCellObject *op = _PyCell_CAST(self);
-    if (op->ob_ref == NULL) {
-        return PyUnicode_FromFormat("<cell at %p: empty>", op);
+    PyObject *ref = PyCell_GetRef((PyCellObject *)self);
+    if (ref == NULL) {
+        return PyUnicode_FromFormat("<cell at %p: empty>", self);
     }
-
-    return PyUnicode_FromFormat("<cell at %p: %.80s object at %p>",
-                               op, Py_TYPE(op->ob_ref)->tp_name,
-                               op->ob_ref);
+    PyObject *res = PyUnicode_FromFormat("<cell at %p: %.80s object at %p>",
+                                         self, Py_TYPE(ref)->tp_name, ref);
+    Py_DECREF(ref);
+    return res;
 }
 
 static int
@@ -135,18 +147,20 @@ static PyObject *
 cell_get_contents(PyObject *self, void *closure)
 {
     PyCellObject *op = _PyCell_CAST(self);
-    if (op->ob_ref == NULL) {
+    PyObject *res = PyCell_GetRef(op);
+    if (res == NULL) {
         PyErr_SetString(PyExc_ValueError, "Cell is empty");
         return NULL;
     }
-    return Py_NewRef(op->ob_ref);
+    return res;
 }
 
 static int
 cell_set_contents(PyObject *self, PyObject *obj, void *Py_UNUSED(ignored))
 {
-    PyCellObject *op = _PyCell_CAST(self);
-    Py_XSETREF(op->ob_ref, Py_XNewRef(obj));
+    PyCellObject *cell = _PyCell_CAST(self);
+    Py_XINCREF(obj);
+    PyCell_SetTakeRef((PyCellObject *)cell, obj);
     return 0;
 }
 

Objects/frameobject.c

@@ -8,6 +8,7 @@
 #include "pycore_moduleobject.h"  // _PyModule_GetDict()
 #include "pycore_modsupport.h"    // _PyArg_CheckPositional()
 #include "pycore_object.h"        // _PyObject_GC_UNTRACK()
+#include "pycore_cell.h"          // PyCell_GetRef() PyCell_SetTakeRef()
 #include "pycore_opcode_metadata.h" // _PyOpcode_Deopt, _PyOpcode_Caches
 
 
@@ -187,11 +188,8 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value)
             }
         }
         if (cell != NULL) {
-            PyObject *oldvalue_o = PyCell_GET(cell);
-            if (value != oldvalue_o) {
-                PyCell_SET(cell, Py_XNewRef(value));
-                Py_XDECREF(oldvalue_o);
-            }
+            Py_XINCREF(value);
+            PyCell_SetTakeRef((PyCellObject *)cell, value);
         } else if (value != PyStackRef_AsPyObjectBorrow(oldvalue)) {
             PyStackRef_XCLOSE(fast[i]);
             fast[i] = PyStackRef_FromPyObjectNew(value);
@@ -1987,19 +1985,25 @@ frame_get_var(_PyInterpreterFrame *frame, PyCodeObject *co, int i,
         if (kind & CO_FAST_FREE) {
             // The cell was set by COPY_FREE_VARS.
             assert(value != NULL && PyCell_Check(value));
-            value = PyCell_GET(value);
+            value = PyCell_GetRef((PyCellObject *)value);
         }
         else if (kind & CO_FAST_CELL) {
             if (value != NULL) {
                 if (PyCell_Check(value)) {
                     assert(!_PyFrame_IsIncomplete(frame));
-                    value = PyCell_GET(value);
+                    value = PyCell_GetRef((PyCellObject *)value);
+                }
+                else {
+                    // (likely) Otherwise it is an arg (kind & CO_FAST_LOCAL),
+                    // with the initial value set when the frame was created...
+                    // (unlikely) ...or it was set via the f_locals proxy.
+                    Py_INCREF(value);
                 }
-                // (likely) Otherwise it is an arg (kind & CO_FAST_LOCAL),
-                // with the initial value set when the frame was created...
-                // (unlikely) ...or it was set via the f_locals proxy.
             }
         }
+        else {
+            Py_XINCREF(value);
+        }
     }
     *pvalue = value;
     return 1;
@@ -2076,14 +2080,14 @@ PyFrame_GetVar(PyFrameObject *frame_obj, PyObject *name)
             continue;
         }
 
-        PyObject *value;  // borrowed reference
+        PyObject *value;
         if (!frame_get_var(frame, co, i, &value)) {
             break;
         }
         if (value == NULL) {
             break;
         }
-        return Py_NewRef(value);
+        return value;
     }
 
     PyErr_Format(PyExc_NameError, "variable %R does not exist", name);