https://github.com/python/cpython/commit/19c1dd60c5b53fb0533610ad139ef591294f26e8
commit: 19c1dd60c5b53fb0533610ad139ef591294f26e8
branch: main
author: Sam Gross <[email protected]>
committer: colesbury <[email protected]>
date: 2024-03-29T13:35:43-04:00
summary:
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.
files:
A Include/internal/pycore_cell.h
M Makefile.pre.in
M Objects/cellobject.c
M PCbuild/pythoncore.vcxproj
M PCbuild/pythoncore.vcxproj.filters
M Python/bytecodes.c
M Python/ceval.c
M Python/executor_cases.c.h
M Python/generated_cases.c.h
M Tools/cases_generator/analyzer.py
M Tools/jit/template.c
diff --git a/Include/internal/pycore_cell.h b/Include/internal/pycore_cell.h
new file mode 100644
index 00000000000000..27f67d57b2fb79
--- /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 5b89d6ba1acf71..f5c2af0696ac33 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 f1a43be38b2b58..b1154e4ca4ace6 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 c944bbafdba7e5..7a2a98df6511a1 100644
--- 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 0afad125ce1e97..89b56ec1267104 100644
--- 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 5cd9db97c71e37..bfb378c4a41500 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 d34db61eecbae2..1b13eb1702355f 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 224b600b8f6a4a..ce0dc235c54fcf 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 c66eb678d38475..e8e2397b11cd48 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 2329205ad31d09..ddafcf99ca1e37 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 f8be4d7f78facd..54160084cda460 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"
_______________________________________________
Python-checkins mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://mail.python.org/mailman3/lists/python-checkins.python.org/
Member address: [email protected]
