From 24bbfa1bedb3f9f6f848d7b1c586f7c763821d13 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Mon, 17 Oct 2022 13:42:04 -0700 Subject: [PATCH 1/2] First pass at handling various cleanup contexts --- Include/internal/pycore_frame.h | 12 +- Objects/frameobject.c | 2 +- Python/ceval.c | 203 +++++++++++++++++++++++--------- Tools/gdb/libpython.py | 2 +- 4 files changed, 162 insertions(+), 57 deletions(-) diff --git a/Include/internal/pycore_frame.h b/Include/internal/pycore_frame.h index 5bd0a7f2f517ef..d751cef5b2784a 100644 --- a/Include/internal/pycore_frame.h +++ b/Include/internal/pycore_frame.h @@ -45,6 +45,14 @@ enum _frameowner { FRAME_OWNED_BY_FRAME_OBJECT = 2 }; +enum _frame_cleanup { + FRAME_CLEANUP_INVALID = 0, + FRAME_CLEANUP_ENTRY = 1, + FRAME_CLEANUP_INLINED_CLASS = 2, + FRAME_CLEANUP_INLINED_FUNCTION = 3, + FRAME_CLEANUP_INLINED_GENERATOR = 4, +}; + typedef struct _PyInterpreterFrame { /* "Specials" section */ PyObject *f_funcobj; /* Strong reference */ @@ -61,7 +69,7 @@ typedef struct _PyInterpreterFrame { // over, or (in the case of a newly-created frame) a totally invalid value: _Py_CODEUNIT *prev_instr; int stacktop; /* Offset of TOS from localsplus */ - bool is_entry; // Whether this is the "root" frame for the current _PyCFrame. + char cleanup; char owner; /* Locals and stack */ PyObject *localsplus[1]; @@ -109,7 +117,7 @@ _PyFrame_InitializeSpecials( frame->stacktop = code->co_nlocalsplus; frame->frame_obj = NULL; frame->prev_instr = _PyCode_CODE(code) - 1; - frame->is_entry = false; + frame->cleanup = FRAME_CLEANUP_INVALID; frame->owner = FRAME_OWNED_BY_THREAD; } diff --git a/Objects/frameobject.c b/Objects/frameobject.c index dd69207571b538..febb6dae2fd5d5 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1342,7 +1342,7 @@ int _PyFrame_IsEntryFrame(PyFrameObject *frame) { assert(frame != NULL); assert(!_PyFrame_IsIncomplete(frame->f_frame)); - return frame->f_frame->is_entry; + return frame->f_frame->cleanup = FRAME_CLEANUP_ENTRY; } diff --git a/Python/ceval.c b/Python/ceval.c index ac995d7c178dfe..4eac48faa2925a 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -864,7 +864,7 @@ GETITEM(PyObject *v, Py_ssize_t i) { #define TRACE_FUNCTION_EXIT() \ if (cframe.use_tracing) { \ if (trace_function_exit(tstate, frame, retval)) { \ - Py_DECREF(retval); \ + Py_CLEAR(retval); \ goto exit_unwind; \ } \ } @@ -1057,11 +1057,13 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int cframe.previous = prev_cframe; tstate->cframe = &cframe; - frame->is_entry = true; + frame->cleanup = FRAME_CLEANUP_ENTRY; /* Push frame */ frame->previous = prev_cframe->current_frame; cframe.current_frame = frame; + PyObject *retval = NULL; + if (_Py_EnterRecursiveCallTstate(tstate, "")) { tstate->c_recursion_remaining--; tstate->py_recursion_remaining--; @@ -1105,7 +1107,8 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int values are not visible to the cycle GC. \ We choose -1 rather than 0 to assist debugging. \ */ \ - frame->stacktop = -1; + frame->stacktop = -1; \ + retval = NULL; start_frame: @@ -1702,6 +1705,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int new_frame->previous = frame; frame = cframe.current_frame = new_frame; CALL_STAT_INC(inlined_py_calls); + frame->cleanup = FRAME_CLEANUP_INLINED_FUNCTION; goto start_frame; } @@ -1861,24 +1865,12 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int } TARGET(RETURN_VALUE) { - PyObject *retval = POP(); + retval = POP(); assert(EMPTY()); _PyFrame_SetStackPointer(frame, stack_pointer); TRACE_FUNCTION_EXIT(); DTRACE_FUNCTION_EXIT(); - _Py_LeaveRecursiveCallPy(tstate); - if (!frame->is_entry) { - frame = cframe.current_frame = pop_frame(tstate, frame); - _PyFrame_StackPush(frame, retval); - goto resume_frame; - } - _Py_LeaveRecursiveCallTstate(tstate); - /* Restore previous cframe and return. */ - tstate->cframe = cframe.previous; - tstate->cframe->use_tracing = cframe.use_tracing; - assert(tstate->cframe->current_frame == frame->previous); - assert(!_PyErr_Occurred(tstate)); - return retval; + goto cleanup; } TARGET(GET_AITER) { @@ -2012,7 +2004,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int } TARGET(SEND) { - assert(frame->is_entry); assert(STACK_LEVEL() >= 2); PyObject *v = POP(); PyObject *receiver = TOP(); @@ -2077,20 +2068,12 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int // The compiler treats any exception raised here as a failed close() // or throw() call. assert(oparg == STACK_LEVEL()); - assert(frame->is_entry); - PyObject *retval = POP(); + retval = POP(); _PyFrame_GetGenerator(frame)->gi_frame_state = FRAME_SUSPENDED; _PyFrame_SetStackPointer(frame, stack_pointer); TRACE_FUNCTION_EXIT(); DTRACE_FUNCTION_EXIT(); - _Py_LeaveRecursiveCallPy(tstate); - _Py_LeaveRecursiveCallTstate(tstate); - /* Restore previous cframe and return. */ - tstate->cframe = cframe.previous; - tstate->cframe->use_tracing = cframe.use_tracing; - assert(tstate->cframe->current_frame == frame->previous); - assert(!_PyErr_Occurred(tstate)); - return retval; + goto cleanup; } TARGET(POP_EXCEPT) { @@ -3149,6 +3132,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int new_frame->previous = frame; frame = cframe.current_frame = new_frame; CALL_STAT_INC(inlined_py_calls); + frame->cleanup = FRAME_CLEANUP_INLINED_FUNCTION; goto start_frame; } @@ -3190,6 +3174,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int new_frame->previous = frame; frame = cframe.current_frame = new_frame; CALL_STAT_INC(inlined_py_calls); + frame->cleanup = FRAME_CLEANUP_INLINED_FUNCTION; goto start_frame; } @@ -4203,6 +4188,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int new_frame->previous = frame; cframe.current_frame = frame = new_frame; CALL_STAT_INC(inlined_py_calls); + frame->cleanup = FRAME_CLEANUP_INLINED_FUNCTION; goto start_frame; } /* Callable is not a normal Python function */ @@ -4287,6 +4273,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int frame->prev_instr = next_instr - 1; new_frame->previous = frame; frame = cframe.current_frame = new_frame; + frame->cleanup = FRAME_CLEANUP_INLINED_FUNCTION; goto start_frame; } @@ -4327,6 +4314,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int frame->prev_instr = next_instr - 1; new_frame->previous = frame; frame = cframe.current_frame = new_frame; + frame->cleanup = FRAME_CLEANUP_INLINED_FUNCTION; goto start_frame; } @@ -4843,26 +4831,13 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int assert(frame->frame_obj == NULL); gen->gi_frame_state = FRAME_CREATED; gen_frame->owner = FRAME_OWNED_BY_GENERATOR; - _Py_LeaveRecursiveCallPy(tstate); - if (!frame->is_entry) { - _PyInterpreterFrame *prev = frame->previous; - _PyThreadState_PopFrame(tstate, frame); - frame = cframe.current_frame = prev; - _PyFrame_StackPush(frame, (PyObject *)gen); - goto resume_frame; - } - _Py_LeaveRecursiveCallTstate(tstate); /* Make sure that frame is in a valid state */ frame->stacktop = 0; frame->f_locals = NULL; Py_INCREF(frame->f_funcobj); Py_INCREF(frame->f_code); - /* Restore previous cframe and return. */ - tstate->cframe = cframe.previous; - tstate->cframe->use_tracing = cframe.use_tracing; - assert(tstate->cframe->current_frame == frame->previous); - assert(!_PyErr_Occurred(tstate)); - return (PyObject *)gen; + retval = (PyObject *)gen; + goto cleanup; } TARGET(BUILD_SLICE) { @@ -5221,22 +5196,144 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int } exit_unwind: - assert(_PyErr_Occurred(tstate)); - _Py_LeaveRecursiveCallPy(tstate); - if (frame->is_entry) { - /* Restore previous cframe and exit */ - tstate->cframe = cframe.previous; - tstate->cframe->use_tracing = cframe.use_tracing; - assert(tstate->cframe->current_frame == frame->previous); - _Py_LeaveRecursiveCallTstate(tstate); - return NULL; + switch (frame->cleanup) { + + case FRAME_CLEANUP_ENTRY: + tstate->cframe = cframe.previous; + tstate->cframe->use_tracing = cframe.use_tracing; + assert(tstate->cframe->current_frame == frame->previous); + _Py_LeaveRecursiveCallTstate(tstate); + _Py_LeaveRecursiveCallPy(tstate); + assert(_PyErr_Occurred(tstate)); + assert(retval == NULL); + return NULL; + + case FRAME_CLEANUP_INLINED_CLASS: + case FRAME_CLEANUP_INLINED_FUNCTION: + frame = cframe.current_frame = pop_frame(tstate, frame); + _Py_LeaveRecursiveCallPy(tstate); + assert(_PyErr_Occurred(tstate)); + assert(retval == NULL); + goto resume_with_error; + + // XXX: This is not actually used! + case FRAME_CLEANUP_INLINED_GENERATOR: + frame = cframe.current_frame = pop_frame(tstate, frame); + // XXX: Most of this is just copy-pasted from genobject.c. Refactor + // to share a single implementation. + PyGenObject *gen = _PyFrame_GetGenerator(frame); + if (gen->gi_frame_state == FRAME_EXECUTING) { + gen->gi_frame_state = FRAME_COMPLETED; + } + tstate->exc_info = gen->gi_exc_state.previous_item; + gen->gi_exc_state.previous_item = NULL; + assert(tstate->cframe->current_frame == frame->previous); + // Don't keep the reference to previous any longer than necessary. + // It may keep a chain of frames alive or create a reference cycle: + frame->previous = NULL; + if (PyErr_ExceptionMatches(PyExc_StopIteration)) { + const char *e = "generator raised StopIteration"; + if (PyCoro_CheckExact(gen)) { + e = "coroutine raised StopIteration"; + } + else if (PyAsyncGen_CheckExact(gen)) { + e = "async generator raised StopIteration"; + } + _PyErr_FormatFromCause(PyExc_RuntimeError, "%s", e); + } + else if (PyAsyncGen_CheckExact(gen) && + PyErr_ExceptionMatches(PyExc_StopAsyncIteration)) + { + const char *e = "async generator raised StopAsyncIteration"; + _PyErr_FormatFromCause(PyExc_RuntimeError, "%s", e); + } + // Generator can't be rerun, so release the frame. First, clean the + // reference cycle through the stored exception traceback: + _PyErr_ClearExcState(&gen->gi_exc_state); + gen->gi_frame_state = FRAME_CLEARED; + _PyFrame_Clear(frame); + _Py_LeaveRecursiveCallPy(tstate); + assert(_PyErr_Occurred(tstate)); + assert(retval == NULL); + goto resume_with_error; + } - frame = cframe.current_frame = pop_frame(tstate, frame); + Py_UNREACHABLE(); resume_with_error: SET_LOCALS_FROM_FRAME(); goto error; +cleanup: + switch (frame->cleanup) { + + case FRAME_CLEANUP_ENTRY: + tstate->cframe = cframe.previous; + tstate->cframe->use_tracing = cframe.use_tracing; + assert(tstate->cframe->current_frame == frame->previous); + _Py_LeaveRecursiveCallTstate(tstate); + _Py_LeaveRecursiveCallPy(tstate); + assert(!_PyErr_Occurred(tstate)); + assert(retval); + return retval; + + // XXX: This is not actually used yet! + case FRAME_CLEANUP_INLINED_CLASS: + if (!Py_IsNone(retval)) { + const char *e = "__init__() should return None, not '%.200s'"; + PyErr_Format(PyExc_TypeError, e, Py_TYPE(retval)->tp_name); + Py_CLEAR(retval); + goto exit_unwind; + } + Py_DECREF(retval); + frame = cframe.current_frame = pop_frame(tstate, frame); + // frame's stack already has the new instance pushed: + assert(frame->f_code->co_nlocalsplus < frame->stacktop); + _Py_LeaveRecursiveCallPy(tstate); + assert(!_PyErr_Occurred(tstate)); + assert(retval); + goto resume_frame; + + case FRAME_CLEANUP_INLINED_FUNCTION: + frame = cframe.current_frame = pop_frame(tstate, frame); + _PyFrame_StackPush(frame, retval); + _Py_LeaveRecursiveCallPy(tstate); + assert(!_PyErr_Occurred(tstate)); + assert(retval); + goto resume_frame; + + // XXX: This is not actually used yet! + case FRAME_CLEANUP_INLINED_GENERATOR: + frame = cframe.current_frame = pop_frame(tstate, frame); + // XXX: Most of this is just copy-pasted from genobject.c. Refactor + // to share a single implementation: + PyGenObject *gen = _PyFrame_GetGenerator(frame); + if (gen->gi_frame_state == FRAME_EXECUTING) { + gen->gi_frame_state = FRAME_COMPLETED; + } + tstate->exc_info = gen->gi_exc_state.previous_item; + gen->gi_exc_state.previous_item = NULL; + assert(tstate->cframe->current_frame == frame->previous); + // Don't keep the reference to previous any longer than necessary. + // It may keep a chain of frames alive or create a reference cycle: + frame->previous = NULL; + if (gen->gi_frame_state == FRAME_SUSPENDED) { + _PyFrame_StackPush(frame, retval); + } + else { + PyObject *iterator = _PyFrame_StackPop(frame); + assert(iterator == (PyObject *)gen); + Py_DECREF(iterator); + _PyFrame_StackPush(frame, retval); + // XXX: Jump out of whatever loop we're in! + } + _Py_LeaveRecursiveCallPy(tstate); + assert(!_PyErr_Occurred(tstate)); + assert(retval); + goto resume_frame; + + } + Py_UNREACHABLE(); } static void diff --git a/Tools/gdb/libpython.py b/Tools/gdb/libpython.py index 303409cb0077d1..e20b052b5a216d 100755 --- a/Tools/gdb/libpython.py +++ b/Tools/gdb/libpython.py @@ -1078,7 +1078,7 @@ def _f_lasti(self): return int(prev_instr - first_instr) def is_entry(self): - return self._f_special("is_entry", bool) + return self._f_special("cleanup", int) == 1 def previous(self): return self._f_special("previous", PyFramePtr) From 661ccd814a4ffaff9ffcbf0c8d705fc1f4192a0c Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Sun, 30 Oct 2022 21:11:09 -0700 Subject: [PATCH 2/2] Rip out the generator and class cleanup code --- Include/internal/pycore_frame.h | 4 +- Python/ceval.c | 89 --------------------------------- 2 files changed, 1 insertion(+), 92 deletions(-) diff --git a/Include/internal/pycore_frame.h b/Include/internal/pycore_frame.h index d751cef5b2784a..d01efe348c43ad 100644 --- a/Include/internal/pycore_frame.h +++ b/Include/internal/pycore_frame.h @@ -48,9 +48,7 @@ enum _frameowner { enum _frame_cleanup { FRAME_CLEANUP_INVALID = 0, FRAME_CLEANUP_ENTRY = 1, - FRAME_CLEANUP_INLINED_CLASS = 2, - FRAME_CLEANUP_INLINED_FUNCTION = 3, - FRAME_CLEANUP_INLINED_GENERATOR = 4, + FRAME_CLEANUP_INLINED_FUNCTION = 2, }; typedef struct _PyInterpreterFrame { diff --git a/Python/ceval.c b/Python/ceval.c index 4eac48faa2925a..f87334fc9da2b0 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -5208,7 +5208,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int assert(retval == NULL); return NULL; - case FRAME_CLEANUP_INLINED_CLASS: case FRAME_CLEANUP_INLINED_FUNCTION: frame = cframe.current_frame = pop_frame(tstate, frame); _Py_LeaveRecursiveCallPy(tstate); @@ -5216,47 +5215,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int assert(retval == NULL); goto resume_with_error; - // XXX: This is not actually used! - case FRAME_CLEANUP_INLINED_GENERATOR: - frame = cframe.current_frame = pop_frame(tstate, frame); - // XXX: Most of this is just copy-pasted from genobject.c. Refactor - // to share a single implementation. - PyGenObject *gen = _PyFrame_GetGenerator(frame); - if (gen->gi_frame_state == FRAME_EXECUTING) { - gen->gi_frame_state = FRAME_COMPLETED; - } - tstate->exc_info = gen->gi_exc_state.previous_item; - gen->gi_exc_state.previous_item = NULL; - assert(tstate->cframe->current_frame == frame->previous); - // Don't keep the reference to previous any longer than necessary. - // It may keep a chain of frames alive or create a reference cycle: - frame->previous = NULL; - if (PyErr_ExceptionMatches(PyExc_StopIteration)) { - const char *e = "generator raised StopIteration"; - if (PyCoro_CheckExact(gen)) { - e = "coroutine raised StopIteration"; - } - else if (PyAsyncGen_CheckExact(gen)) { - e = "async generator raised StopIteration"; - } - _PyErr_FormatFromCause(PyExc_RuntimeError, "%s", e); - } - else if (PyAsyncGen_CheckExact(gen) && - PyErr_ExceptionMatches(PyExc_StopAsyncIteration)) - { - const char *e = "async generator raised StopAsyncIteration"; - _PyErr_FormatFromCause(PyExc_RuntimeError, "%s", e); - } - // Generator can't be rerun, so release the frame. First, clean the - // reference cycle through the stored exception traceback: - _PyErr_ClearExcState(&gen->gi_exc_state); - gen->gi_frame_state = FRAME_CLEARED; - _PyFrame_Clear(frame); - _Py_LeaveRecursiveCallPy(tstate); - assert(_PyErr_Occurred(tstate)); - assert(retval == NULL); - goto resume_with_error; - } Py_UNREACHABLE(); @@ -5277,23 +5235,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int assert(retval); return retval; - // XXX: This is not actually used yet! - case FRAME_CLEANUP_INLINED_CLASS: - if (!Py_IsNone(retval)) { - const char *e = "__init__() should return None, not '%.200s'"; - PyErr_Format(PyExc_TypeError, e, Py_TYPE(retval)->tp_name); - Py_CLEAR(retval); - goto exit_unwind; - } - Py_DECREF(retval); - frame = cframe.current_frame = pop_frame(tstate, frame); - // frame's stack already has the new instance pushed: - assert(frame->f_code->co_nlocalsplus < frame->stacktop); - _Py_LeaveRecursiveCallPy(tstate); - assert(!_PyErr_Occurred(tstate)); - assert(retval); - goto resume_frame; - case FRAME_CLEANUP_INLINED_FUNCTION: frame = cframe.current_frame = pop_frame(tstate, frame); _PyFrame_StackPush(frame, retval); @@ -5302,36 +5243,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int assert(retval); goto resume_frame; - // XXX: This is not actually used yet! - case FRAME_CLEANUP_INLINED_GENERATOR: - frame = cframe.current_frame = pop_frame(tstate, frame); - // XXX: Most of this is just copy-pasted from genobject.c. Refactor - // to share a single implementation: - PyGenObject *gen = _PyFrame_GetGenerator(frame); - if (gen->gi_frame_state == FRAME_EXECUTING) { - gen->gi_frame_state = FRAME_COMPLETED; - } - tstate->exc_info = gen->gi_exc_state.previous_item; - gen->gi_exc_state.previous_item = NULL; - assert(tstate->cframe->current_frame == frame->previous); - // Don't keep the reference to previous any longer than necessary. - // It may keep a chain of frames alive or create a reference cycle: - frame->previous = NULL; - if (gen->gi_frame_state == FRAME_SUSPENDED) { - _PyFrame_StackPush(frame, retval); - } - else { - PyObject *iterator = _PyFrame_StackPop(frame); - assert(iterator == (PyObject *)gen); - Py_DECREF(iterator); - _PyFrame_StackPush(frame, retval); - // XXX: Jump out of whatever loop we're in! - } - _Py_LeaveRecursiveCallPy(tstate); - assert(!_PyErr_Occurred(tstate)); - assert(retval); - goto resume_frame; - } Py_UNREACHABLE(); }