Skip to content

Commit

Permalink
gh-117323: Make cell thread-safe in free-threaded builds (#117330)
Browse files Browse the repository at this point in the history
Use critical sections to lock around accesses to cell contents. The critical sections are no-ops in the default (with GIL) build.
  • Loading branch information
colesbury authored Mar 29, 2024
1 parent 397d88d commit 19c1dd6
Show file tree
Hide file tree
Showing 11 changed files with 83 additions and 43 deletions.
48 changes: 48 additions & 0 deletions Include/internal/pycore_cell.h
Original file line number Diff line number Diff line change
@@ -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 */
1 change: 1 addition & 0 deletions Makefile.pre.in
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
8 changes: 3 additions & 5 deletions Objects/cellobject.c
Original file line number Diff line number Diff line change
@@ -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"

Expand Down Expand Up @@ -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
Expand All @@ -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;
}

Expand Down
1 change: 1 addition & 0 deletions PCbuild/pythoncore.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand Down
3 changes: 3 additions & 0 deletions PCbuild/pythoncore.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
20 changes: 8 additions & 12 deletions Python/bytecodes.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
}

Expand All @@ -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, (--)) {
Expand Down
1 change: 1 addition & 0 deletions Python/ceval.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 7 additions & 12 deletions Python/executor_cases.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 7 additions & 12 deletions Python/generated_cases.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions Tools/cases_generator/analyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
1 change: 1 addition & 0 deletions Tools/jit/template.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 19c1dd6

Please sign in to comment.