8000 gh-127271: Replace use of PyCell_GET/SET (gh-127272) · python/cpython@fc5a0dc · GitHub
[go: up one dir, main page]
Skip to content

Commit fc5a0dc

Browse files
naschemenascheme
authored
gh-127271: Replace use of PyCell_GET/SET (gh-127272)
* Replace uses of `PyCell_GET` and `PyCell_SET`. These macros are not safe to use in the free-threaded build. Use `PyCell_GetRef()` and `PyCell_SetTakeRef()` instead. * Since `PyCell_GetRef()` returns a strong rather than borrowed ref, some code restructuring was required, e.g. `frame_get_var()` returns a strong ref now. * Add critical sections to `PyCell_GET` and `PyCell_SET`. * Move critical_section.h earlier in the Python.h file. * Add `PyCell_GET` to the free-threading howto table of APIs that return borrowed refs. * Add additional unit tests for free-threading.
1 parent 276cd66 commit fc5a0dc

File tree

8 files changed

+231
-48
lines changed

8 files changed

+231
-48
lines changed

‎Doc/howto/free-threading-extensions.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,8 @@ that return :term:`strong references <strong reference>`.
167167
+-----------------------------------+-----------------------------------+
168168
| :c:func:`PyImport_AddModule` | :c:func:`PyImport_AddModuleRef` |
169169
+-----------------------------------+-----------------------------------+
170+
| :c:func:`PyCell_GET` | :c:func:`PyCell_Get` |
171+
+-----------------------------------+-----------------------------------+
170172

171173
Not all APIs that return borrowed references are problematic. For
172174
example, :c:func:`PyTuple_GetItem` is safe because tuples are immutable.

‎Include/Python.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@
6969
#include "pystats.h"
7070
#include "pyatomic.h"
7171
#include "lock.h"
72+
#include "critical_section.h"
7273
#include "object.h"
7374
#include "refcount.h"
7475
#include "objimpl.h"
@@ -130,7 +131,6 @@
130131
#include "import.h"
131132
#include "abstract.h"
132133
#include "bltinmodule.h"
133-
#include "critical_section.h"
134134
#include "cpython/pyctype.h"
135135
#include "pystrtod.h"
136136
#include "pystrcmp.h"

‎Include/cpython/cellobject.h

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,18 +22,24 @@ PyAPI_FUNC(PyObject *) PyCell_Get(PyObject *);
2222
PyAPI_FUNC(int) PyCell_Set(PyObject *, PyObject *);
2323

2424
static inline PyObject* PyCell_GET(PyObject *op) {
25+
PyObject *res;
2526
PyCellObject *cell;
2627
assert(PyCell_Check(op));
2728
cell = _Py_CAST(PyCellObject*, op);
28-
return cell->ob_ref;
29+
Py_BEGIN_CRITICAL_SECTION(cell);
30+
res = cell->ob_ref;
31+
Py_END_CRITICAL_SECTION();
32+
return res;
2933
}
3034
#define PyCell_GET(op) PyCell_GET(_PyObject_CAST(op))
3135

3236
static inline void PyCell_SET(PyObject *op, PyObject *value) {
3337
PyCellObject *cell;
3438
assert(PyCell_Check(op));
3539
cell = _Py_CAST(PyCellObject*, op);
40+
Py_BEGIN_CRITICAL_SECTION(cell);
3641
cell->ob_ref = value;
42+
Py_END_CRITICAL_SECTION();
3743
}
3844
#define PyCell_SET(op, value) PyCell_SET(_PyObject_CAST(op), (value))
3945

‎Lib/test/test_free_threading/test_races.py

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
# It's most useful to run these tests with ThreadSanitizer enabled.
2+
import sys
3+
import functools
4+
import threading
5+
import time
6+
import unittest
7+
8+
from test.support import threading_helper
9+
10+
11+
class TestBase(unittest.TestCase):
12+
pass
13+
14+
15+
def do_race(func1, func2):
16+
"""Run func1() and func2() repeatedly in separate threads."""
17+
n = 1000
18+
19+
barrier = threading.Barrier(2)
20+
21+
def repeat(func):
22+
barrier.wait()
23+
for _i in range(n):
24+
func()
25+
26+
threads = [
27+
threading.Thread(target=functools.partial(repeat, func1)),
28+
threading.Thread(target=functools.partial(repeat, func2)),
29+
]
30+
for thread in threads:
31+
thread.start()
32+
for thread in threads:
33+
thread.join()
34+
35+
36+
@threading_helper.requires_working_threading()
37+
class TestRaces(TestBase):
38+
def test_racing_cell_set(self):
39+
"""Test cell object gettr/settr properties."""
40+
41+
def nested_func():
42+
x = 0
43+
44+
def inner():
45+
nonlocal x
46+
x += 1
47+
48+
# This doesn't race because LOAD_DEREF and STORE_DEREF on the
49+
# cell object use critical sections.
50+
do_race(nested_func, nested_func)
51+
52+
def nested_func2():
53+
x = 0
54+
55+
def inner():
56+
y = x
57+
frame = sys._getframe(1)
58+
frame.f_locals["x"] = 2
59+
60+
return inner
61+
62+
def mutate_func2():
63+
inner = nested_func2()
64+
cell = inner.__closure__[0]
65+
old_value = cell.cell_contents
66+
cell.cell_contents = 1000
67+
time.sleep(0)
68+
cell.cell_contents = old_value
69+
time.sleep(0)
70+
71+
# This revealed a race with cell_set_contents() since it was missing
72+
# the critical section.
73+
do_race(nested_func2, mutate_func2)
74+
75+
def test_racing_cell_cmp_repr(self):
76+
"""Test cell object compare and repr methods."""
77+
78+
def nested_func():
79+
x = 0
80+
y = 0
81+
82+
def inner():
83+
return x + y
84+
85+
return inner.__closure__
86+
87+
cell_a, cell_b = nested_func()
88+
89+
def mutate():
90+
cell_a.cell_contents += 1
91+
92+
def access():
93+
cell_a == cell_b
94+
s = repr(cell_a)
95+
96+
# cell_richcompare() and cell_repr used to have data races
97+
do_race(mutate, access)
98+
99+
def test_racing_load_super_attr(self):
100+
"""Test (un)specialization of LOAD_SUPER_ATTR opcode."""
101+
102+
class C:
103+
def __init__(self):
104+
try:
105+
super().__init__
106+
super().__init__()
107+
except RuntimeError:
108+
pass # happens if __class__ is replaced with non-type
109+
110+
def access():
111+
C()
112+
113+
def mutate():
114+
# Swap out the super() global with a different one
115+
real_super = super
116+
globals()["super"] = lambda s=1: s
117+
time.sleep(0)
118+
globals()["super"] = real_super
119+
time.sleep(0)
120+
# Swap out the __class__ closure value with a non-type
121+
cell = C.__init__.__closure__[0]
122+
real_class = cell.cell_contents
123+
cell.cell_contents = 99
124+
time.sleep(0)
125+
cell.cell_contents = real_class
126+
127+
# The initial PR adding specialized opcodes for LOAD_SUPER_ATTR
128+
# had some races (one with the super() global changing and one
129+
# with the cell binding being changed).
130+
do_race(access, mutate)
131+
132+
133+
if __name__ == "__main__":
134+
unittest.main()

‎Objects/cellobject.c

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,17 @@ cell_dealloc(PyObject *self)
8282
PyObject_GC_Del(op);
8383
}
8484

85+
static PyObject *
86+
cell_compare_impl(PyObject *a, PyObject *b, int op)
87+
{
88+
if (a != NULL && b != NULL) {
89+
return PyObject_RichCompare(a, b, op);
90+
}
91+
else {
92+
Py_RETURN_RICHCOMPARE(b == NULL, a == NULL, op);
93+
}
94+
}
95+
8596
static PyObject *
8697
cell_richcompare(PyObject *a, PyObject *b, int op)
8798
{
@@ -92,27 +103,28 @@ cell_richcompare(PyObject *a, PyObject *b, int op)
92103
if (!PyCell_Check(a) || !PyCell_Check(b)) {
93104
Py_RETURN_NOTIMPLEMENTED;
94105
}
106+
PyObject *a_ref = PyCell_GetRef((PyCellObject *)a);
107+
PyObject *b_ref = PyCell_GetRef((PyCellObject *)b);
95108

96109
/* compare cells by contents; empty cells come before anything else */
97-
a = ((PyCellObject *)a)->ob_ref;
98-
b = ((PyCellObject *)b)->ob_ref;
99-
if (a != NULL && b != NULL)
100-
return PyObject_RichCompare(a, b, op);
110+
PyObject *res = cell_compare_impl(a_ref, b_ref, op);
101111

102-
Py_RETURN_RICHCOMPARE(b == NULL, a == NULL, op);
112+
Py_XDECREF(a_ref);
113+
Py_XDECREF(b_ref);
114+
return res;
103115
}
104116

105117
static PyObject *
106118
cell_repr(PyObject *self)
107119
{
108-
PyCellObject *op = _PyCell_CAST(self);
109-
if (op->ob_ref == NULL) {
110-
return PyUnicode_FromFormat("<cell at %p: empty>", op);
120+
PyObject *ref = PyCell_GetRef((PyCellObject *)self);
121+
if (ref == NULL) {
122+
return PyUnicode_FromFormat("<cell at %p: empty>", self);
111123
}
112-
113-
return PyUnicode_FromFormat("<cell at %p: %.80s object at %p>",
114-
op, Py_TYPE(op->ob_ref)->tp_name,
115-
op->ob_ref);
124+
PyObject *res = PyUnicode_FromFormat("<cell at %p: %.80s object at %p>",
125+
self, Py_TYPE(ref)->tp_name, ref);
126+
Py_DECREF(ref);
127+
return res;
116128
}
117129

118130
static int
@@ -135,11 +147,12 @@ static PyObject *
135147
cell_get_contents(PyObject *self, void *closure)
136148
{
137149
PyCellObject *op = _PyCell_CAST(self);
138-
if (op->ob_ref == NULL) {
150+
PyObject *res = PyCell_GetRef(op);
151+
if (res == NULL) {
139152
PyErr_SetString(PyExc_ValueError, "Cell is empty");
140153
return NULL;
141154
}
142-
return Py_NewRef(op->ob_ref);
155+
return res;
143156
}
144157

145158
static int

‎Objects/frameobject.c

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "pycore_moduleobject.h" // _PyModule_GetDict()
99
#include "pycore_modsupport.h" // _PyArg_CheckPositional()
1010
#include "pycore_object.h" // _PyObject_GC_UNTRACK()
11+
#include "pycore_cell.h" // PyCell_GetRef() PyCell_SetTakeRef()
1112
#include "pycore_opcode_metadata.h" // _PyOpcode_Deopt, _PyOpcode_Caches
1213

1314

@@ -187,11 +188,8 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value)
187188
}
188189
}
189190
if (cell != NULL) {
190-
PyObject *oldvalue_o = PyCell_GET(cell);
191-
if (value != oldvalue_o) {
192-
PyCell_SET(cell, Py_XNewRef(value));
193-
Py_XDECREF(oldvalue_o);
194-
}
191+
Py_XINCREF(value);
192+
PyCell_SetTakeRef((PyCellObject *)cell, value);
195193
} else if (value != PyStackRef_AsPyObjectBorrow(oldvalue)) {
196194
PyStackRef_XCLOSE(fast[i]);
197195
fast[i] = PyStackRef_FromPyObjectNew(value);
@@ -1987,19 +1985,25 @@ frame_get_var(_PyInterpreterFrame *frame, PyCodeObject *co, int i,
19871985
if (kind & CO_FAST_FREE) {
19881986
// The cell was set by COPY_FREE_VARS.
19891987
assert(value != NULL && PyCell_Check(value));
1990-
value = PyCell_GET(value);
1988+
value = PyCell_GetRef((PyCellObject *)value);
19911989
}
19921990
else if (kind & CO_FAST_CELL) {
19931991
if (value != NULL) {
19941992
if (PyCell_Check(value)) {
19951993
assert(!_PyFrame_IsIncomplete(frame));
1996-
value = PyCell_GET(value);
1994+
value = PyCell_GetRef((PyCellObject *)value);
1995+
}
1996+
else {
1997+
// (likely) Otherwise it is an arg (kind & CO_FAST_LOCAL),
1998+
// with the initial value set when the frame was created...
1999+
// (unlikely) ...or it was set via the f_locals proxy.
2000+
Py_INCREF(value);
19972001
}
1998-
// (likely) Otherwise it is an arg (kind & CO_FAST_LOCAL),
1999-
// with the initial value set when the frame was created...
2000-
// (unlikely) ...or it was set via the f_locals proxy.
20012002
}
20022003
}
2004+
else {
2005+
Py_XINCREF(value);
2006+
}
20032007
}
20042008
*pvalue = value;
20052009
return 1;
@@ -2076,14 +2080,14 @@ PyFrame_GetVar(PyFrameObject *frame_obj, PyObject *name)
20762080
continue;
20772081
}
20782082

2079-
PyObject *value; // borrowed reference
2083+
PyObject *value;
20802084
if (!frame_get_var(frame, co, i, &value)) {
20812085
break;
20822086
}
20832087
if (value == NULL) {
20842088
break;
20852089
}
2086-
return Py_NewRef(value);
2090+
return value;
20872091
}
20882092

20892093
PyErr_Format(PyExc_NameError, "variable %R does not exist", name);

0 commit comments

Comments
 (0)
0