Skip to content

Commit f6fab21

Browse files
markshannonmarkshannon
authored
GH-118095: Make invalidating and clearing executors memory safe (GH-118459)
1 parent 21c09d9 commit f6fab21

File tree

5 files changed

+103
-42
lines changed

5 files changed

+103
-42
lines changed

‎Include/cpython/optimizer.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ typedef struct {
2424
uint8_t opcode;
2525
uint8_t oparg;
2626
uint8_t valid;
27+
uint8_t linked;
2728
int index; // Index of ENTER_EXECUTOR (if code isn't NULL, below).
2829
_PyBloomFilter bloom;
2930
_PyExecutorLinkListNode links;
@@ -135,7 +136,7 @@ PyAPI_FUNC(_PyOptimizerObject *) PyUnstable_GetOptimizer(void);
135136
PyAPI_FUNC(_PyExecutorObject *) PyUnstable_GetExecutor(PyCodeObject *code, int offset);
136137

137138
void _Py_ExecutorInit(_PyExecutorObject *, const _PyBloomFilter *);
138-
void _Py_ExecutorClear(_PyExecutorObject *);
139+
void _Py_ExecutorDetach(_PyExecutorObject *);
139140
void _Py_BloomFilter_Init(_PyBloomFilter *);
140141
void _Py_BloomFilter_Add(_PyBloomFilter *bloom, void *obj);
141142
PyAPI_FUNC(void) _Py_Executor_DependsOn(_PyExecutorObject *executor, void *obj);

‎Objects/codeobject.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1504,7 +1504,8 @@ clear_executors(PyCodeObject *co)
15041504
assert(co->co_executors);
15051505
for (int i = 0; i < co->co_executors->size; i++) {
15061506
if (co->co_executors->executors[i]) {
1507-
_Py_ExecutorClear(co->co_executors->executors[i]);
1507+
_Py_ExecutorDetach(co->co_executors->executors[i]);
1508+
assert(co->co_executors->executors[i] == NULL);
15081509
}
15091510
}
15101511
PyMem_Free(co->co_executors);

‎Python/bytecodes.c‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,9 +163,15 @@ dummy_func(
163163
if ((oparg & RESUME_OPARG_LOCATION_MASK) < RESUME_AFTER_YIELD_FROM) {
164164
CHECK_EVAL_BREAKER();
165165
}
166-
#if ENABLE_SPECIALIZATION
167-
FT_ATOMIC_STORE_UINT8_RELAXED(this_instr->op.code, RESUME_CHECK);
168-
#endif /* ENABLE_SPECIALIZATION */
166+
assert(this_instr->op.code == RESUME ||
167+
this_instr->op.code == RESUME_CHECK ||
168+
this_instr->op.code == INSTRUMENTED_RESUME ||
169+
this_instr->op.code == ENTER_EXECUTOR);
170+
if (this_instr->op.code == RESUME) {
171+
#if ENABLE_SPECIALIZATION
172+
FT_ATOMIC_STORE_UINT8_RELAXED(this_instr->op.code, RESUME_CHECK);
173+
#endif /* ENABLE_SPECIALIZATION */
174+
}
169175
}
170176
}
171177

‎Python/generated_cases.c.h‎

Lines changed: 9 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Python/optimizer.c‎

Lines changed: 81 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ insert_executor(PyCodeObject *code, _Py_CODEUNIT *instr, int index, _PyExecutorO
7575
Py_INCREF(executor);
7676
if (instr->op.code == ENTER_EXECUTOR) {
7777
assert(index == instr->op.arg);
78-
_Py_ExecutorClear(code->co_executors->executors[index]);
78+
_Py_ExecutorDetach(code->co_executors->executors[index]);
7979
}
8080
else {
8181
assert(code->co_executors->size == index);
@@ -270,10 +270,14 @@ static PyMethodDef executor_methods[] = {
270270

271271
///////////////////// Experimental UOp Optimizer /////////////////////
272272

273+
static int executor_clear(_PyExecutorObject *executor);
274+
static void unlink_executor(_PyExecutorObject *executor);
275+
273276
static void
274277
uop_dealloc(_PyExecutorObject *self) {
275278
_PyObject_GC_UNTRACK(self);
276-
_Py_ExecutorClear(self);
279+
assert(self->vm_data.code == NULL);
280+
unlink_executor(self);
277281
#ifdef _Py_JIT
278282
_PyJIT_Free(self);
279283
#endif
@@ -379,13 +383,6 @@ PySequenceMethods uop_as_sequence = {
379383
.sq_item = (ssizeargfunc)uop_item,
380384
};
381385

382-
static int
383-
executor_clear(PyObject *o)
384-
{
385-
_Py_ExecutorClear((_PyExecutorObject *)o);
386-
return 0;
387-
}
388-
389386
static int
390387
executor_traverse(PyObject *o, visitproc visit, void *arg)
391388
{
@@ -412,7 +409,7 @@ PyTypeObject _PyUOpExecutor_Type = {
412409
.tp_as_sequence = &uop_as_sequence,
413410
.tp_methods = executor_methods,
414411
.tp_traverse = executor_traverse,
415-
.tp_clear = executor_clear,
412+
.tp_clear = (inquiry)executor_clear,
416413
.tp_is_gc = executor_is_gc,
417414
};
418415

@@ -1190,6 +1187,7 @@ init_cold_exit_executor(_PyExecutorObject *executor, int oparg)
11901187
inst->opcode = _COLD_EXIT;
11911188
inst->oparg = oparg;
11921189
executor->vm_data.valid = true;
1190+
executor->vm_data.linked = false;
11931191
for (int i = 0; i < BLOOM_FILTER_WORDS; i++) {
11941192
assert(executor->vm_data.bloom.bits[i] == 0);
11951193
}
@@ -1328,7 +1326,7 @@ PyTypeObject _PyCounterExecutor_Type = {
13281326
.tp_dealloc = (destructor)counter_dealloc,
13291327
.tp_methods = executor_methods,
13301328
.tp_traverse = executor_traverse,
1331-
.tp_clear = executor_clear,
1329+
.tp_clear = (inquiry)executor_clear,
13321330
};
13331331

13341332
static int
@@ -1503,23 +1501,25 @@ link_executor(_PyExecutorObject *executor)
15031501
links->next = NULL;
15041502
}
15051503
else {
1506-
_PyExecutorObject *next = head->vm_data.links.next;
1507-
links->previous = head;
1508-
links->next = next;
1509-
if (next != NULL) {
1510-
next->vm_data.links.previous = executor;
1511-
}
1512-
head->vm_data.links.next = executor;
1504+
assert(head->vm_data.links.previous == NULL);
1505+
links->previous = NULL;
1506+
links->next = head;
1507+
head->vm_data.links.previous = executor;
1508+
interp->executor_list_head = executor;
15131509
}
1514-
executor->vm_data.valid = true;
1510+
executor->vm_data.linked = true;
15151511
/* executor_list_head must be first in list */
15161512
assert(interp->executor_list_head->vm_data.links.previous == NULL);
15171513
}
15181514

15191515
static void
15201516
unlink_executor(_PyExecutorObject *executor)
15211517
{
1518+
if (!executor->vm_data.linked) {
1519+
return;
1520+
}
15221521
_PyExecutorLinkListNode *links = &executor->vm_data.links;
1522+
assert(executor->vm_data.valid);
15231523
_PyExecutorObject *next = links->next;
15241524
_PyExecutorObject *prev = links->previous;
15251525
if (next != NULL) {
@@ -1534,7 +1534,7 @@ unlink_executor(_PyExecutorObject *executor)
15341534
assert(interp->executor_list_head == executor);
15351535
interp->executor_list_head = next;
15361536
}
1537-
executor->vm_data.valid = false;
1537+
executor->vm_data.linked = false;
15381538
}
15391539

15401540
/* This must be called by optimizers before using the executor */
@@ -1548,31 +1548,52 @@ _Py_ExecutorInit(_PyExecutorObject *executor, const _PyBloomFilter *dependency_s
15481548
link_executor(executor);
15491549
}
15501550

1551-
/* This must be called by executors during dealloc */
1551+
/* Detaches the executor from the code object (if any) that
1552+
* holds a reference to it */
15521553
void
1553-
_Py_ExecutorClear(_PyExecutorObject *executor)
1554+
_Py_ExecutorDetach(_PyExecutorObject *executor)
15541555
{
1555-
if (!executor->vm_data.valid) {
1556-
return;
1557-
}
1558-
unlink_executor(executor);
15591556
PyCodeObject *code = executor->vm_data.code;
15601557
if (code == NULL) {
15611558
return;
15621559
}
1563-
for (uint32_t i = 0; i < executor->exit_count; i++) {
1564-
Py_DECREF(executor->exits[i].executor);
1565-
executor->exits[i].executor = &COLD_EXITS[i];
1566-
executor->exits[i].temperature = initial_unreachable_backoff_counter();
1567-
}
15681560
_Py_CODEUNIT *instruction = &_PyCode_CODE(code)[executor->vm_data.index];
15691561
assert(instruction->op.code == ENTER_EXECUTOR);
15701562
int index = instruction->op.arg;
15711563
assert(code->co_executors->executors[index] == executor);
15721564
instruction->op.code = executor->vm_data.opcode;
15731565
instruction->op.arg = executor->vm_data.oparg;
15741566
executor->vm_data.code = NULL;
1575-
Py_CLEAR(code->co_executors->executors[index]);
1567+
code->co_executors->executors[index] = NULL;
1568+
Py_DECREF(executor);
1569+
}
1570+
1571+
static int
1572+
executor_clear(_PyExecutorObject *executor)
1573+
{
1574+
if (!executor->vm_data.valid) {
1575+
return 0;
1576+
}
1577+
assert(executor->vm_data.valid == 1);
1578+
unlink_executor(executor);
1579+
executor->vm_data.valid = 0;
1580+
/* It is possible for an executor to form a reference
1581+
* cycle with itself, so decref'ing a side exit could
1582+
* free the executor unless we hold a strong reference to it
1583+
*/
1584+
Py_INCREF(executor);
1585+
for (uint32_t i = 0; i < executor->exit_count; i++) {
1586+
const _PyExecutorObject *cold = &COLD_EXITS[i];
1587+
const _PyExecutorObject *side = executor->exits[i].executor;
1588+
executor->exits[i].temperature = initial_unreachable_backoff_counter();
1589+
if (side != cold) {
1590+
executor->exits[i].executor = cold;
1591+
Py_DECREF(side);
1592+
}
1593+
}
1594+
_Py_ExecutorDetach(executor);
1595+
Py_DECREF(executor);
1596+
return 0;
15761597
}
15771598

15781599
void
@@ -1593,17 +1614,42 @@ _Py_Executors_InvalidateDependency(PyInterpreterState *interp, void *obj, int is
15931614
_Py_BloomFilter_Add(&obj_filter, obj);
15941615
/* Walk the list of executors */
15951616
/* TO DO -- Use a tree to avoid traversing as many objects */
1617+
bool no_memory = false;
1618+
PyObject *invalidate = PyList_New(0);
1619+
if (invalidate == NULL) {
1620+
PyErr_Clear();
1621+
no_memory = true;
1622+
}
1623+
/* Clearing an executor can deallocate others, so we need to make a list of
1624+
* executors to invalidate first */
15961625
for (_PyExecutorObject *exec = interp->executor_list_head; exec != NULL;) {
15971626
assert(exec->vm_data.valid);
15981627
_PyExecutorObject *next = exec->vm_data.links.next;
15991628
if (bloom_filter_may_contain(&exec->vm_data.bloom, &obj_filter)) {
1600-
_Py_ExecutorClear(exec);
1629+
unlink_executor(exec);
1630+
if (no_memory) {
1631+
exec->vm_data.valid = 0;
1632+
} else {
1633+
if (PyList_Append(invalidate, (PyObject *)exec) < 0) {
1634+
PyErr_Clear();
1635+
no_memory = true;
1636+
exec->vm_data.valid = 0;
1637+
}
1638+
}
16011639
if (is_invalidation) {
16021640
OPT_STAT_INC(executors_invalidated);
16031641
}
16041642
}
16051643
exec = next;
16061644
}
1645+
if (invalidate != NULL) {
1646+
for (Py_ssize_t i = 0; i < PyList_GET_SIZE(invalidate); i++) {
1647+
_PyExecutorObject *exec = (_PyExecutorObject *)PyList_GET_ITEM(invalidate, i);
1648+
executor_clear(exec);
1649+
}
1650+
Py_DECREF(invalidate);
1651+
}
1652+
return;
16071653
}
16081654

16091655
/* Invalidate all executors */
@@ -1612,12 +1658,13 @@ _Py_Executors_InvalidateAll(PyInterpreterState *interp, int is_invalidation)
16121658
{
16131659
while (interp->executor_list_head) {
16141660
_PyExecutorObject *executor = interp->executor_list_head;
1661+
assert(executor->vm_data.valid == 1 && executor->vm_data.linked == 1);
16151662
if (executor->vm_data.code) {
16161663
// Clear the entire code object so its co_executors array be freed:
16171664
_PyCode_Clear_Executors(executor->vm_data.code);
16181665
}
16191666
else {
1620-
_Py_ExecutorClear(executor);
1667+
executor_clear(executor);
16211668
}
16221669
if (is_invalidation) {
16231670
OPT_STAT_INC(executors_invalidated);

0 commit comments

Comments
 (0)