Skip to content

Commit bc054db

Browse files
committed
gh-120321: Make gi_frame_state transitions atomic in FT build
This makes generator frame state transitions atomic in the free threading build, which avoids segfaults when trying to execute a generator from multiple threads concurrently. There are still a few operations that aren't thread-safe and may crash if performed concurrently on the same generator/coroutine: * Accessing gi_yieldfrom/cr_await/ag_await * Accessing gi_frame/cr_frame/ag_frame * Async generator operations
1 parent dac4589 commit bc054db

File tree

17 files changed

+1032
-851
lines changed

17 files changed

+1032
-851
lines changed

Include/cpython/pyatomic.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,9 @@ _Py_atomic_store_uintptr_release(uintptr_t *obj, uintptr_t value);
523523
static inline void
524524
_Py_atomic_store_ssize_release(Py_ssize_t *obj, Py_ssize_t value);
525525

526+
static inline void
527+
_Py_atomic_store_int8_release(int8_t *obj, int8_t value);
528+
526529
static inline void
527530
_Py_atomic_store_int_release(int *obj, int value);
528531

Include/cpython/pyatomic_gcc.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,10 @@ static inline void
572572
_Py_atomic_store_int_release(int *obj, int value)
573573
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
574574

575+
static inline void
576+
_Py_atomic_store_int8_release(int8_t *obj, int8_t value)
577+
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
578+
575579
static inline void
576580
_Py_atomic_store_ssize_release(Py_ssize_t *obj, Py_ssize_t value)
577581
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }

Include/cpython/pyatomic_msc.h

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,6 +1066,19 @@ _Py_atomic_store_int_release(int *obj, int value)
10661066
#endif
10671067
}
10681068

1069+
static inline void
1070+
_Py_atomic_store_int8_release(int8_t *obj, int8_t value)
1071+
{
1072+
#if defined(_M_X64) || defined(_M_IX86)
1073+
*(int8_t volatile *)obj = value;
1074+
#elif defined(_M_ARM64)
1075+
_Py_atomic_ASSERT_ARG_TYPE(unsigned __int8);
1076+
__stlr8((unsigned __int8 volatile *)obj, (unsigned __int8)value);
1077+
#else
1078+
# error "no implementation of _Py_atomic_store_int8_release"
1079+
#endif
1080+
}
1081+
10691082
static inline void
10701083
_Py_atomic_store_ssize_release(Py_ssize_t *obj, Py_ssize_t value)
10711084
{

Include/cpython/pyatomic_std.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1023,6 +1023,14 @@ _Py_atomic_store_int_release(int *obj, int value)
10231023
memory_order_release);
10241024
}
10251025

1026+
static inline void
1027+
_Py_atomic_store_int8_release(int8_t *obj, int8_t value)
1028+
{
1029+
_Py_USING_STD;
1030+
atomic_store_explicit((_Atomic(int8_t)*)obj, value,
1031+
memory_order_release);
1032+
}
1033+
10261034
static inline void
10271035
_Py_atomic_store_uint_release(unsigned int *obj, unsigned int value)
10281036
{

Include/internal/pycore_frame.h

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -41,20 +41,6 @@ struct _frame {
4141
extern PyFrameObject* _PyFrame_New_NoTrack(PyCodeObject *code);
4242

4343

44-
/* other API */
45-
46-
typedef enum _framestate {
47-
FRAME_CREATED = -3,
48-
FRAME_SUSPENDED = -2,
49-
FRAME_SUSPENDED_YIELD_FROM = -1,
50-
FRAME_EXECUTING = 0,
51-
FRAME_COMPLETED = 1,
52-
FRAME_CLEARED = 4
53-
} PyFrameState;
54-
55-
#define FRAME_STATE_SUSPENDED(S) ((S) == FRAME_SUSPENDED || (S) == FRAME_SUSPENDED_YIELD_FROM)
56-
#define FRAME_STATE_FINISHED(S) ((S) >= FRAME_COMPLETED)
57-
5844
#ifdef __cplusplus
5945
}
6046
#endif

Include/internal/pycore_genobject.h

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,21 @@ extern "C" {
1010

1111
#include "pycore_interpframe_structs.h" // _PyGenObject
1212

13+
#include <stdbool.h> // bool
1314
#include <stddef.h> // offsetof()
15+
#include "pycore_object.h" // _PyObject_IsUniquelyReferenced()
1416

17+
typedef enum _framestate {
18+
FRAME_CREATED = -3,
19+
FRAME_SUSPENDED = -2,
20+
FRAME_SUSPENDED_YIELD_FROM = -1,
21+
FRAME_EXECUTING = 0,
22+
FRAME_COMPLETED = 1,
23+
FRAME_CLEARED = 4
24+
} PyFrameState;
25+
26+
#define FRAME_STATE_SUSPENDED(S) ((S) == FRAME_SUSPENDED || (S) == FRAME_SUSPENDED_YIELD_FROM)
27+
#define FRAME_STATE_FINISHED(S) ((S) >= FRAME_COMPLETED)
1528

1629
static inline
1730
PyGenObject *_PyGen_GetGeneratorFromFrame(_PyInterpreterFrame *frame)
@@ -21,7 +34,30 @@ PyGenObject *_PyGen_GetGeneratorFromFrame(_PyInterpreterFrame *frame)
2134
return (PyGenObject *)(((char *)frame) - offset_in_gen);
2235
}
2336

24-
PyAPI_FUNC(PyObject *)_PyGen_yf(PyGenObject *);
37+
// Mark the generator as executing. Returns true if the state was changed,
38+
// false if it was already executing or finished.
39+
static inline bool
40+
_PyGen_SetExecuting(PyGenObject *gen)
41+
{
42+
#ifdef Py_GIL_DISABLED
43+
if (!_PyObject_IsUniquelyReferenced((PyObject *)gen)) {
44+
int8_t frame_state = _Py_atomic_load_int8_relaxed(&gen->gi_frame_state);
45+
while (frame_state < FRAME_EXECUTING) {
46+
if (_Py_atomic_compare_exchange_int8(&gen->gi_frame_state,
47+
&frame_state,
48+
FRAME_EXECUTING)) {
49+
return true;
50+
}
51+
}
52+
}
53+
#endif
54+
if (gen->gi_frame_state < FRAME_EXECUTING) {
55+
gen->gi_frame_state = FRAME_EXECUTING;
56+
return true;
57+
}
58+
return false;
59+
}
60+
2561
extern void _PyGen_Finalize(PyObject *self);
2662

2763
// Export for '_asyncio' shared extension

Include/internal/pycore_pyatomic_ft_wrappers.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ extern "C" {
3939
_Py_atomic_load_uint8(&value)
4040
#define FT_ATOMIC_STORE_UINT8(value, new_value) \
4141
_Py_atomic_store_uint8(&value, new_value)
42+
#define FT_ATOMIC_LOAD_INT8_RELAXED(value) \
43+
_Py_atomic_load_int8_relaxed(&value)
4244
#define FT_ATOMIC_LOAD_UINT8_RELAXED(value) \
4345
_Py_atomic_load_uint8_relaxed(&value)
4446
#define FT_ATOMIC_LOAD_UINT16_RELAXED(value) \
@@ -53,6 +55,10 @@ extern "C" {
5355
_Py_atomic_store_ptr_release(&value, new_value)
5456
#define FT_ATOMIC_STORE_UINTPTR_RELEASE(value, new_value) \
5557
_Py_atomic_store_uintptr_release(&value, new_value)
58+
#define FT_ATOMIC_STORE_INT8_RELAXED(value, new_value) \
59+
_Py_atomic_store_int8_relaxed(&value, new_value)
60+
#define FT_ATOMIC_STORE_INT8_RELEASE(value, new_value) \
61+
_Py_atomic_store_int8_release(&value, new_value)
5662
#define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) \
5763
_Py_atomic_store_ssize_relaxed(&value, new_value)
5864
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) \
@@ -129,13 +135,16 @@ extern "C" {
129135
#define FT_ATOMIC_LOAD_PTR_RELAXED(value) value
130136
#define FT_ATOMIC_LOAD_UINT8(value) value
131137
#define FT_ATOMIC_STORE_UINT8(value, new_value) value = new_value
138+
#define FT_ATOMIC_LOAD_INT8_RELAXED(value) value
132139
#define FT_ATOMIC_LOAD_UINT8_RELAXED(value) value
133140
#define FT_ATOMIC_LOAD_UINT16_RELAXED(value) value
134141
#define FT_ATOMIC_LOAD_UINT32_RELAXED(value) value
135142
#define FT_ATOMIC_LOAD_ULONG_RELAXED(value) value
136143
#define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) value = new_value
137144
#define FT_ATOMIC_STORE_PTR_RELEASE(value, new_value) value = new_value
138145
#define FT_ATOMIC_STORE_UINTPTR_RELEASE(value, new_value) value = new_value
146+
#define FT_ATOMIC_STORE_INT8_RELAXED(value, new_value) value = new_value
147+
#define FT_ATOMIC_STORE_INT8_RELEASE(value, new_value) value = new_value
139148
#define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) value = new_value
140149
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) value = new_value
141150
#define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value

Include/internal/pycore_tstate.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,10 @@ typedef struct _PyThreadStateImpl {
113113
// When >1, code objects do not immortalize their non-string constants.
114114
int suppress_co_const_immortalization;
115115

116+
// Last known frame state for generators/coroutines in this thread
117+
// Used by genobject.c
118+
int8_t gen_last_frame_state;
119+
116120
#ifdef Py_STATS
117121
// per-thread stats, will be merged into interp->pystats_struct
118122
PyStats *pystats_struct; // allocated by _PyStats_ThreadInit()

0 commit comments

Comments
 (0)