From 8a88fd3173edd69db3cb4c4c68b0e9aa2cb4dcc4 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 17 Sep 2021 12:40:25 +0100 Subject: [PATCH 01/13] Remove the usage of the cstack in Python to Python calls Ths commit inlines calls to Python functions in the eval loop and steals all the arguments in the call from the caller for performance. --- Include/cpython/pystate.h | 1 + Include/internal/pycore_frame.h | 1 + Include/internal/pycore_tuple.h | 1 + Objects/tupleobject.c | 20 ++++ Python/ceval.c | 160 ++++++++++++++++++++++++++------ Tools/gdb/libpython.py | 53 +++++++---- 6 files changed, 190 insertions(+), 46 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index ab4bf8bf8483c7..e45719b9341b97 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -47,6 +47,7 @@ typedef struct _cframe { */ int use_tracing; struct _cframe *previous; + int depth; } CFrame; typedef struct _err_stackitem { diff --git a/Include/internal/pycore_frame.h b/Include/internal/pycore_frame.h index 6afb95c3ad62e0..4c6b421ec2e91f 100644 --- a/Include/internal/pycore_frame.h +++ b/Include/internal/pycore_frame.h @@ -31,6 +31,7 @@ typedef struct _interpreter_frame { int f_lasti; /* Last instruction if called */ int stacktop; /* Offset of TOS from localsplus */ PyFrameState f_state; /* What state the frame is in */ + int depth; /* Depth of the frame in a ceval loop */ PyObject *localsplus[1]; } InterpreterFrame; diff --git a/Include/internal/pycore_tuple.h b/Include/internal/pycore_tuple.h index d1d0d2a92e49fb..79c827fe8800a7 100644 --- a/Include/internal/pycore_tuple.h +++ b/Include/internal/pycore_tuple.h @@ -13,6 +13,7 @@ extern "C" { #define _PyTuple_ITEMS(op) (_PyTuple_CAST(op)->ob_item) extern PyObject *_PyTuple_FromArray(PyObject *const *, Py_ssize_t); +extern PyObject *_PyTuple_FromArraySteal(PyObject *const *, Py_ssize_t); #ifdef __cplusplus } diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c index 3e3aea47cc2ab3..9127d13d208f3e 100644 --- a/Objects/tupleobject.c +++ b/Objects/tupleobject.c @@ -472,6 +472,26 @@ _PyTuple_FromArray(PyObject *const *src, Py_ssize_t n) return (PyObject *)tuple; } +PyObject * +_PyTuple_FromArraySteal(PyObject *const *src, Py_ssize_t n) +{ + if (n == 0) { + return tuple_get_empty(); + } + + PyTupleObject *tuple = tuple_alloc(n); + if (tuple == NULL) { + return NULL; + } + PyObject **dst = tuple->ob_item; + for (Py_ssize_t i = 0; i < n; i++) { + PyObject *item = src[i]; + dst[i] = item; + } + _PyObject_GC_TRACK(tuple); + return (PyObject *)tuple; +} + static PyObject * tupleslice(PyTupleObject *a, Py_ssize_t ilow, Py_ssize_t ihigh) diff --git a/Python/ceval.c b/Python/ceval.c index ab692fd8ded157..fe61cf08a504f8 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -98,6 +98,12 @@ static int check_args_iterable(PyThreadState *, PyObject *func, PyObject *vararg static void format_kwargs_error(PyThreadState *, PyObject *func, PyObject *kwargs); static void format_awaitable_error(PyThreadState *, PyTypeObject *, int, int); static int get_exception_handler(PyCodeObject *, int, int*, int*, int*); +static InterpreterFrame * +_PyEvalFramePushAndInit(PyThreadState *tstate, PyFrameConstructor *con, + PyObject *locals, PyObject* const* args, + size_t argcount, PyObject *kwnames, int steal_args); +static int +_PyEvalFrameClearAndPop(PyThreadState *tstate, InterpreterFrame * frame); #define NAME_ERROR_MSG \ "name '%.200s' is not defined" @@ -1534,10 +1540,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr PyObject *retval = NULL; /* Return value */ _Py_atomic_int * const eval_breaker = &tstate->interp->ceval.eval_breaker; - if (_Py_EnterRecursiveCall(tstate, "")) { - return NULL; - } - CFrame cframe; /* WARNING: Because the CFrame lives on the C stack, @@ -1547,11 +1549,20 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr CFrame *prev_cframe = tstate->cframe; cframe.use_tracing = prev_cframe->use_tracing; cframe.previous = prev_cframe; + cframe.depth = 0; tstate->cframe = &cframe; /* push frame */ tstate->frame = frame; +start_frame: + if (_Py_EnterRecursiveCall(tstate, "")) { + tstate->recursion_depth++; + goto exit_eval_frame; + } + + assert(frame == tstate->frame); + if (cframe.use_tracing) { if (trace_function_entry(tstate, frame)) { goto exit_eval_frame; @@ -1573,7 +1584,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr } } - +resume_frame: + frame->depth = cframe.depth; + co = frame->f_code; PyObject *names = co->co_names; PyObject *consts = co->co_consts; _Py_CODEUNIT *first_instr = co->co_firstinstr; @@ -1616,6 +1629,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr #endif if (throwflag) { /* support for generator.throw() */ + throwflag = 0; goto error; } @@ -4603,16 +4617,75 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr TARGET(CALL_FUNCTION): { PREDICTED(CALL_FUNCTION); - PyObject **sp, *res; - sp = stack_pointer; - res = call_function(tstate, &sp, oparg, NULL, cframe.use_tracing); - stack_pointer = sp; - PUSH(res); - if (res == NULL) { + PyObject *res; + + // ------------ Check if call cal be optimized -------------- // + PyObject *function = *(stack_pointer - oparg - 1); + + int optimize_call = 0; + if (Py_TYPE(function) == &PyFunction_Type) { + PyCodeObject *co = (PyCodeObject*)((PyFunctionObject*)(function))->func_code; + int is_coro = co->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR); + optimize_call = (is_coro || cframe.use_tracing) ? 0 : 1; + } + // ----------------------------------------------------------- // + + if (!optimize_call) { + PyObject **sp = stack_pointer; + res = call_function(tstate, &sp, oparg, NULL, cframe.use_tracing); + stack_pointer = sp; + PUSH(res); + if (res == NULL) { + goto error; + } + CHECK_EVAL_BREAKER(); + DISPATCH(); + } + + assert(PyFunction_Check(function)); + size_t nargsf = oparg | PY_VECTORCALL_ARGUMENTS_OFFSET; + PyObject *const *args = stack_pointer - oparg; + assert(args != NULL || PyVectorcall_NARGS(nargsf) == 0); + assert(PyVectorcall_Function(function) == _PyFunction_Vectorcall); + PyFrameConstructor *con = PyFunction_AS_FRAME_CONSTRUCTOR(function); + Py_ssize_t nargs = PyVectorcall_NARGS(nargsf); + assert(nargs >= 0); + assert(nargs == 0 || args != NULL); + PyObject *locals = (((PyCodeObject *)con->fc_code)->co_flags & CO_OPTIMIZED) ? NULL : con->fc_globals; + PyCodeObject *code = (PyCodeObject *)con->fc_code; + + assert(!(code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR)) || cframe.use_tracing); + + InterpreterFrame *new_frame = _PyEvalFramePushAndInit(tstate, con, locals, args, nargs, NULL, 1); + if (new_frame == NULL) { + // When we exit here, we own all variables in the stack (the frame creation has not stolen + // any variable) so we need to clean the whole stack (done in the "error" label). goto error; } - CHECK_EVAL_BREAKER(); - DISPATCH(); + assert(tstate->interp->eval_frame != NULL); + + Py_DECREF(function); + STACK_SHRINK(oparg + 1); + // TODO: Transform the following into a goto + _PyFrame_SetStackPointer(frame, stack_pointer); + tstate->frame = frame = new_frame; + cframe.depth++; + goto start_frame; +// res = _PyEval_EvalFrame(tstate, new_frame, 0); +// assert(_PyFrame_GetStackPointer(new_frame) == _PyFrame_Stackbase(new_frame)); +// if (_PyEvalFrameClearAndPop(tstate, new_frame)) { +// Py_XDECREF(res); +// goto error; +// } +// +// assert((res != NULL) ^ (_PyErr_Occurred(tstate) != NULL)); +// +// PUSH(res); +// if (res == NULL) { +// goto error; +// } +// CHECK_EVAL_BREAKER(); +// DISPATCH(); } TARGET(CALL_FUNCTION_KW): { @@ -4991,14 +5064,29 @@ MISS_WITH_OPARG_COUNTER(BINARY_ADD) /* pop frame */ exit_eval_frame: - /* Restore previous cframe */ - tstate->cframe = cframe.previous; - tstate->cframe->use_tracing = cframe.use_tracing; - if (PyDTrace_FUNCTION_RETURN_ENABLED()) dtrace_function_return(frame); _Py_LeaveRecursiveCall(tstate); + + if (cframe.depth) { + _PyFrame_StackPush(frame->previous, retval); + if (_PyEvalFrameClearAndPop(tstate, frame)) { + retval = NULL; + } + frame = tstate->frame; + cframe.depth--; + if (retval == NULL) { + assert(_PyErr_Occurred(tstate)); + throwflag = 1; + } + retval = NULL; + goto resume_frame; + } tstate->frame = frame->previous; + + /* Restore previous cframe */ + tstate->cframe = cframe.previous; + tstate->cframe->use_tracing = cframe.use_tracing; return _Py_CheckFunctionResult(tstate, NULL, retval, __func__); } @@ -5309,7 +5397,7 @@ get_exception_handler(PyCodeObject *code, int index, int *level, int *handler, i static int initialize_locals(PyThreadState *tstate, PyFrameConstructor *con, PyObject **localsplus, PyObject *const *args, - Py_ssize_t argcount, PyObject *kwnames) + Py_ssize_t argcount, PyObject *kwnames, int steal_args) { PyCodeObject *co = (PyCodeObject*)con->fc_code; const Py_ssize_t total_args = co->co_argcount + co->co_kwonlyargcount; @@ -5319,8 +5407,9 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con, Py_ssize_t i; if (co->co_flags & CO_VARKEYWORDS) { kwdict = PyDict_New(); - if (kwdict == NULL) + if (kwdict == NULL) { goto fail; + } i = total_args; if (co->co_flags & CO_VARARGS) { i++; @@ -5342,14 +5431,21 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con, } for (j = 0; j < n; j++) { PyObject *x = args[j]; - Py_INCREF(x); + if (!steal_args) { + Py_INCREF(x); + } assert(localsplus[j] == NULL); localsplus[j] = x; } /* Pack other positional arguments into the *args argument */ if (co->co_flags & CO_VARARGS) { - PyObject *u = _PyTuple_FromArray(args + n, argcount - n); + PyObject *u = NULL; + if (steal_args) { + u = _PyTuple_FromArraySteal(args + n, argcount - n); + } else { + u = _PyTuple_FromArray(args + n, argcount - n); + } if (u == NULL) { goto fail; } @@ -5415,6 +5511,9 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con, if (PyDict_SetItem(kwdict, keyword, value) == -1) { goto fail; } + if (steal_args) { + Py_DECREF(value); + } continue; kw_found: @@ -5424,7 +5523,9 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con, con->fc_qualname, keyword); goto fail; } - Py_INCREF(value); + if (!steal_args) { + Py_INCREF(value); + } localsplus[j] = value; } } @@ -5528,7 +5629,7 @@ make_coro_frame(PyThreadState *tstate, } _PyFrame_InitializeSpecials(frame, con, locals, code->co_nlocalsplus); assert(frame->frame_obj == NULL); - if (initialize_locals(tstate, con, frame->localsplus, args, argcount, kwnames)) { + if (initialize_locals(tstate, con, frame->localsplus, args, argcount, kwnames, 0)) { _PyFrame_Clear(frame, 1); return NULL; } @@ -5557,14 +5658,19 @@ make_coro(PyThreadState *tstate, PyFrameConstructor *con, static InterpreterFrame * _PyEvalFramePushAndInit(PyThreadState *tstate, PyFrameConstructor *con, PyObject *locals, PyObject* const* args, - size_t argcount, PyObject *kwnames) + size_t argcount, PyObject *kwnames, int steal_args) { InterpreterFrame * frame = _PyThreadState_PushFrame(tstate, con, locals); if (frame == NULL) { return NULL; } PyObject **localsarray = _PyFrame_GetLocalsArray(frame); - if (initialize_locals(tstate, con, localsarray, args, argcount, kwnames)) { + if (initialize_locals(tstate, con, localsarray, args, argcount, kwnames, steal_args)) { + if (steal_args) { + for (int i = 0; i < frame->stacktop; i++) { + Py_XINCREF(frame->localsplus[i]); + } + } _PyFrame_Clear(frame, 0); return NULL; } @@ -5579,9 +5685,9 @@ _PyEvalFrameClearAndPop(PyThreadState *tstate, InterpreterFrame * frame) ++tstate->recursion_depth; assert(frame->frame_obj == NULL || frame->frame_obj->f_own_locals_memory == 0); if (_PyFrame_Clear(frame, 0)) { + --tstate->recursion_depth; return -1; } - assert(frame->frame_obj == NULL); --tstate->recursion_depth; tstate->frame = frame->previous; _PyThreadState_PopFrame(tstate, frame); @@ -5601,7 +5707,7 @@ _PyEval_Vector(PyThreadState *tstate, PyFrameConstructor *con, return make_coro(tstate, con, locals, args, argcount, kwnames); } InterpreterFrame *frame = _PyEvalFramePushAndInit( - tstate, con, locals, args, argcount, kwnames); + tstate, con, locals, args, argcount, kwnames, 0); if (frame == NULL) { return NULL; } diff --git a/Tools/gdb/libpython.py b/Tools/gdb/libpython.py index c11b23e74b9bed..a3894850dc1950 100755 --- a/Tools/gdb/libpython.py +++ b/Tools/gdb/libpython.py @@ -45,6 +45,8 @@ # compatible (2.6+ and 3.0+). See #19308. from __future__ import print_function + +from numpy import interp import gdb import os import locale @@ -988,6 +990,11 @@ def _f_nlocalsplus(self): def _f_lasti(self): return self._f_special("f_lasti", int_from_int) + def depth(self): + return self._f_special("depth", int_from_int) + + def previous(self): + return self._f_special("previous", PyFramePtr) def iter_globals(self): ''' @@ -1794,16 +1801,20 @@ def get_selected_bytecode_frame(cls): def print_summary(self): if self.is_evalframe(): - pyop = self.get_pyop() - if pyop: - line = pyop.get_truncated_repr(MAX_OUTPUT_LEN) - write_unicode(sys.stdout, '#%i %s\n' % (self.get_index(), line)) - if not pyop.is_optimized_out(): - line = pyop.current_line() - if line is not None: - sys.stdout.write(' %s\n' % line.strip()) - else: - sys.stdout.write('#%i (unable to read python frame information)\n' % self.get_index()) + interp_frame = self.get_pyop() + while True: + if interp_frame: + line = interp_frame.get_truncated_repr(MAX_OUTPUT_LEN) + write_unicode(sys.stdout, '#%i %s\n' % (self.get_index(), line)) + if not interp_frame.is_optimized_out(): + line = interp_frame.current_line() + if line is not None: + sys.stdout.write(' %s\n' % line.strip()) + if interp_frame.depth() == 0: + break + else: + sys.stdout.write('#%i (unable to read python frame information)\n' % self.get_index()) + interp_frame = interp_frame.previous() else: info = self.is_other_python_frame() if info: @@ -1813,15 +1824,19 @@ def print_summary(self): def print_traceback(self): if self.is_evalframe(): - pyop = self.get_pyop() - if pyop: - pyop.print_traceback() - if not pyop.is_optimized_out(): - line = pyop.current_line() - if line is not None: - sys.stdout.write(' %s\n' % line.strip()) - else: - sys.stdout.write(' (unable to read python frame information)\n') + interp_frame = self.get_pyop() + while True: + if interp_frame: + interp_frame.print_traceback() + if not interp_frame.is_optimized_out(): + line = interp_frame.current_line() + if line is not None: + sys.stdout.write(' %s\n' % line.strip()) + if interp_frame.depth() == 0: + break + else: + sys.stdout.write(' (unable to read python frame information)\n') + interp_frame = interp_frame.previous() else: info = self.is_other_python_frame() if info: From 799e3668da60bd5f438d338158801dceb8ca6bb5 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 17 Sep 2021 20:49:27 +0100 Subject: [PATCH 02/13] Fix gdb --- Lib/test/gdb_sample.py | 2 +- Lib/test/test_gdb.py | 26 +++++++++++++++++++++++--- Tools/gdb/libpython.py | 33 +++++++++++++++++++++++---------- 3 files changed, 47 insertions(+), 14 deletions(-) diff --git a/Lib/test/gdb_sample.py b/Lib/test/gdb_sample.py index cab13fb4da594c..4188f50136fb97 100644 --- a/Lib/test/gdb_sample.py +++ b/Lib/test/gdb_sample.py @@ -1,7 +1,7 @@ # Sample script for use by test_gdb.py def foo(a, b, c): - bar(a, b, c) + bar(a=a, b=b, c=c) def bar(a, b, c): baz(a, b, c) diff --git a/Lib/test/test_gdb.py b/Lib/test/test_gdb.py index 22a8cf3c25fc0f..fb0f1295574b9b 100644 --- a/Lib/test/test_gdb.py +++ b/Lib/test/test_gdb.py @@ -734,8 +734,14 @@ def test_pyup_command(self): cmds_after_breakpoint=['py-up', 'py-up']) self.assertMultilineMatches(bt, r'''^.* +#[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 10, in baz \(args=\(1, 2, 3\)\) + id\(42\) #[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 7, in bar \(a=1, b=2, c=3\) baz\(a, b, c\) +#[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 4, in foo \(a=1, b=2, c=3\) + bar\(a=a, b=b, c=c\) +#[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 12, in \(\) + foo\(1, 2, 3\) $''') @unittest.skipUnless(HAS_PYUP_PYDOWN, "test requires py-up/py-down commands") @@ -763,10 +769,18 @@ def test_up_then_down(self): cmds_after_breakpoint=['py-up', 'py-up', 'py-down']) self.assertMultilineMatches(bt, r'''^.* +#[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 10, in baz \(args=\(1, 2, 3\)\) + id\(42\) #[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 7, in bar \(a=1, b=2, c=3\) baz\(a, b, c\) +#[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 4, in foo \(a=1, b=2, c=3\) + bar\(a=a, b=b, c=c\) +#[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 12, in \(\) + foo\(1, 2, 3\) #[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 10, in baz \(args=\(1, 2, 3\)\) id\(42\) +#[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 7, in bar \(a=1, b=2, c=3\) + baz\(a, b, c\) $''') class PyBtTests(DebuggerTests): @@ -785,7 +799,7 @@ def test_bt(self): File ".*gdb_sample.py", line 7, in bar baz\(a, b, c\) File ".*gdb_sample.py", line 4, in foo - bar\(a, b, c\) + bar\(a=a, b=b, c=c\) File ".*gdb_sample.py", line 12, in foo\(1, 2, 3\) ''') @@ -801,7 +815,7 @@ def test_bt_full(self): #[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 7, in bar \(a=1, b=2, c=3\) baz\(a, b, c\) #[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 4, in foo \(a=1, b=2, c=3\) - bar\(a, b, c\) + bar\(a=a, b=b, c=c\) #[0-9]+ Frame 0x-?[0-9a-f]+, for file .*gdb_sample.py, line 12, in \(\) foo\(1, 2, 3\) ''') @@ -1008,7 +1022,13 @@ def test_locals_after_up(self): bt = self.get_stack_trace(script=self.get_sample_script(), cmds_after_breakpoint=['py-up', 'py-up', 'py-locals']) self.assertMultilineMatches(bt, - r".*\na = 1\nb = 2\nc = 3\n.*") + r'''^.* +Locals for foo +a = 1 +b = 2 +c = 3 +Locals for +.*$''') def setUpModule(): diff --git a/Tools/gdb/libpython.py b/Tools/gdb/libpython.py index a3894850dc1950..cf24aa9ddda9dd 100755 --- a/Tools/gdb/libpython.py +++ b/Tools/gdb/libpython.py @@ -1926,11 +1926,16 @@ def invoke(self, args, from_tty): def move_in_stack(move_up): '''Move up or down the stack (for the py-up/py-down command)''' + # Important: + # The ammount of frames that are printed out depends on how many frames are inlined + # in the same evaluation loop. As this command links directly the C stack with the + # Python stack, the results are sensitive to the number of inlined frames and this + # is likely to change between versions and optimizations. frame = Frame.get_selected_python_frame() if not frame: print('Unable to locate python frame') return - + while frame: if move_up: iter_frame = frame.older() @@ -1952,9 +1957,10 @@ def move_in_stack(move_up): print('Unable to find an older python frame') else: print('Unable to find a newer python frame') + class PyUp(gdb.Command): - 'Select and print the python stack frame that called this one (if any)' + 'Select and print all python stack frame in the same eval loop starting from the one that called this one (if any)' def __init__(self): gdb.Command.__init__ (self, "py-up", @@ -1966,7 +1972,7 @@ def invoke(self, args, from_tty): move_in_stack(move_up=True) class PyDown(gdb.Command): - 'Select and print the python stack frame called by this one (if any)' + 'Select and print all python stack frame in the same eval loop starting from the one called this one (if any)' def __init__(self): gdb.Command.__init__ (self, "py-down", @@ -2079,13 +2085,20 @@ def invoke(self, args, from_tty): return pyop_frame = frame.get_pyop() - if not pyop_frame: - print(UNABLE_READ_INFO_PYTHON_FRAME) - return + while True: + if not pyop_frame: + print(UNABLE_READ_INFO_PYTHON_FRAME) + + sys.stdout.write('Locals for %s\n' % (pyop_frame.co_name.proxyval(set()))) + + for pyop_name, pyop_value in pyop_frame.iter_locals(): + print('%s = %s' + % (pyop_name.proxyval(set()), + pyop_value.get_truncated_repr(MAX_OUTPUT_LEN))) + + if pyop_frame.depth() == 0: + break - for pyop_name, pyop_value in pyop_frame.iter_locals(): - print('%s = %s' - % (pyop_name.proxyval(set()), - pyop_value.get_truncated_repr(MAX_OUTPUT_LEN))) + pyop_frame = pyop_frame.previous() PyLocals() From 306e29f1bafe5023c913aebaa3f7b2f520e93091 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 17 Sep 2021 21:07:14 +0100 Subject: [PATCH 03/13] Move depth to frame structure for easier retrieval in debugging tools --- Include/cpython/pystate.h | 1 - Include/internal/pycore_frame.h | 1 + Python/ceval.c | 8 +++----- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index e45719b9341b97..ab4bf8bf8483c7 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -47,7 +47,6 @@ typedef struct _cframe { */ int use_tracing; struct _cframe *previous; - int depth; } CFrame; typedef struct _err_stackitem { diff --git a/Include/internal/pycore_frame.h b/Include/internal/pycore_frame.h index 4c6b421ec2e91f..0a92c687a39f2c 100644 --- a/Include/internal/pycore_frame.h +++ b/Include/internal/pycore_frame.h @@ -86,6 +86,7 @@ _PyFrame_InitializeSpecials( frame->generator = NULL; frame->f_lasti = -1; frame->f_state = FRAME_CREATED; + frame->depth = 0; } /* Gets the pointer to the locals array diff --git a/Python/ceval.c b/Python/ceval.c index fe61cf08a504f8..337ef5c09495d2 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -1549,9 +1549,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr CFrame *prev_cframe = tstate->cframe; cframe.use_tracing = prev_cframe->use_tracing; cframe.previous = prev_cframe; - cframe.depth = 0; tstate->cframe = &cframe; + assert(frame->depth == 0); /* push frame */ tstate->frame = frame; @@ -1585,7 +1585,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr } resume_frame: - frame->depth = cframe.depth; co = frame->f_code; PyObject *names = co->co_names; PyObject *consts = co->co_consts; @@ -4668,8 +4667,8 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr STACK_SHRINK(oparg + 1); // TODO: Transform the following into a goto _PyFrame_SetStackPointer(frame, stack_pointer); + new_frame->depth = frame->depth + 1; tstate->frame = frame = new_frame; - cframe.depth++; goto start_frame; // res = _PyEval_EvalFrame(tstate, new_frame, 0); // assert(_PyFrame_GetStackPointer(new_frame) == _PyFrame_Stackbase(new_frame)); @@ -5068,13 +5067,12 @@ MISS_WITH_OPARG_COUNTER(BINARY_ADD) dtrace_function_return(frame); _Py_LeaveRecursiveCall(tstate); - if (cframe.depth) { + if (frame->depth) { _PyFrame_StackPush(frame->previous, retval); if (_PyEvalFrameClearAndPop(tstate, frame)) { retval = NULL; } frame = tstate->frame; - cframe.depth--; if (retval == NULL) { assert(_PyErr_Occurred(tstate)); throwflag = 1; From fc36be17e510619fe82a98efae5889be388f52a1 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 17 Sep 2021 22:34:22 +0100 Subject: [PATCH 04/13] Refactor and simplifications --- Python/ceval.c | 72 ++++++++++++++++++------------------------ Tools/gdb/libpython.py | 1 - 2 files changed, 31 insertions(+), 42 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index 337ef5c09495d2..f90cf704661aaa 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -1522,6 +1522,24 @@ trace_function_entry(PyThreadState *tstate, InterpreterFrame *frame) return 0; } +/* Create a frame structure from a python function stealing ownership from an array of arguments. + * In case of error, it returns NULL and the caller still owns the references to all arguments */ +static InterpreterFrame* +_PyEval_FrameFromPyFunctionAndArgs(PyThreadState *tstate, PyObject* const *args, int nargs, PyObject *function) { + assert(PyFunction_Check(function)); + size_t nargsf = nargs | PY_VECTORCALL_ARGUMENTS_OFFSET; + assert(args != NULL || PyVectorcall_NARGS(nargsf) == 0); + assert(PyVectorcall_Function(function) == _PyFunction_Vectorcall); + PyFrameConstructor *con = PyFunction_AS_FRAME_CONSTRUCTOR(function); + Py_ssize_t vector_nargs = PyVectorcall_NARGS(nargsf); + assert(vector_nargs >= 0); + assert(vector_nargs == 0 || args != NULL); + PyCodeObject *code = (PyCodeObject *)con->fc_code; + PyObject *locals = (code->co_flags & CO_OPTIMIZED) ? NULL : con->fc_globals; + assert(!(code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR))); + return _PyEvalFramePushAndInit(tstate, con, locals, args, vector_nargs, NULL, 1); +} + PyObject* _Py_HOT_FUNCTION _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int throwflag) { @@ -4618,18 +4636,16 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr PREDICTED(CALL_FUNCTION); PyObject *res; - // ------------ Check if call cal be optimized -------------- // + // Check if the call can be inlined or not PyObject *function = *(stack_pointer - oparg - 1); - - int optimize_call = 0; + int inline_call = 0; if (Py_TYPE(function) == &PyFunction_Type) { - PyCodeObject *co = (PyCodeObject*)((PyFunctionObject*)(function))->func_code; - int is_coro = co->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR); - optimize_call = (is_coro || cframe.use_tracing) ? 0 : 1; + PyCodeObject *code = (PyCodeObject*)((PyFunctionObject*)(function))->func_code; + int is_coro = code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR); + inline_call = (is_coro || cframe.use_tracing) ? 0 : 1; } - // ----------------------------------------------------------- // - if (!optimize_call) { + if (!inline_call) { PyObject **sp = stack_pointer; res = call_function(tstate, &sp, oparg, NULL, cframe.use_tracing); stack_pointer = sp; @@ -4641,50 +4657,21 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr DISPATCH(); } - assert(PyFunction_Check(function)); - size_t nargsf = oparg | PY_VECTORCALL_ARGUMENTS_OFFSET; - PyObject *const *args = stack_pointer - oparg; - assert(args != NULL || PyVectorcall_NARGS(nargsf) == 0); - assert(PyVectorcall_Function(function) == _PyFunction_Vectorcall); - PyFrameConstructor *con = PyFunction_AS_FRAME_CONSTRUCTOR(function); - Py_ssize_t nargs = PyVectorcall_NARGS(nargsf); - assert(nargs >= 0); - assert(nargs == 0 || args != NULL); - PyObject *locals = (((PyCodeObject *)con->fc_code)->co_flags & CO_OPTIMIZED) ? NULL : con->fc_globals; - PyCodeObject *code = (PyCodeObject *)con->fc_code; - - assert(!(code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR)) || cframe.use_tracing); - - InterpreterFrame *new_frame = _PyEvalFramePushAndInit(tstate, con, locals, args, nargs, NULL, 1); + assert(!cframe.use_tracing); + InterpreterFrame *new_frame = _PyEval_FrameFromPyFunctionAndArgs(tstate, stack_pointer-oparg, oparg, function); if (new_frame == NULL) { // When we exit here, we own all variables in the stack (the frame creation has not stolen // any variable) so we need to clean the whole stack (done in the "error" label). goto error; } assert(tstate->interp->eval_frame != NULL); - - Py_DECREF(function); + // The frame has stolen all the arguments from the stack, so there is no need to clean up them STACK_SHRINK(oparg + 1); - // TODO: Transform the following into a goto + Py_DECREF(function); _PyFrame_SetStackPointer(frame, stack_pointer); new_frame->depth = frame->depth + 1; tstate->frame = frame = new_frame; goto start_frame; -// res = _PyEval_EvalFrame(tstate, new_frame, 0); -// assert(_PyFrame_GetStackPointer(new_frame) == _PyFrame_Stackbase(new_frame)); -// if (_PyEvalFrameClearAndPop(tstate, new_frame)) { -// Py_XDECREF(res); -// goto error; -// } -// -// assert((res != NULL) ^ (_PyErr_Occurred(tstate) != NULL)); -// -// PUSH(res); -// if (res == NULL) { -// goto error; -// } -// CHECK_EVAL_BREAKER(); -// DISPATCH(); } TARGET(CALL_FUNCTION_KW): { @@ -5653,6 +5640,9 @@ make_coro(PyThreadState *tstate, PyFrameConstructor *con, return gen; } +// If *steal_args* is set, the function will steal the references to all the arguments. +// In case of error, the function returns null and if *steal_args* is set, the caller +// will still own all the arguments. static InterpreterFrame * _PyEvalFramePushAndInit(PyThreadState *tstate, PyFrameConstructor *con, PyObject *locals, PyObject* const* args, diff --git a/Tools/gdb/libpython.py b/Tools/gdb/libpython.py index cf24aa9ddda9dd..6d19ee5c740494 100755 --- a/Tools/gdb/libpython.py +++ b/Tools/gdb/libpython.py @@ -46,7 +46,6 @@ from __future__ import print_function -from numpy import interp import gdb import os import locale From e1091e0e747a531364545c931b16bc6c3b28d0fc Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 21 Sep 2021 14:23:21 +0100 Subject: [PATCH 05/13] Fix existing bug in the PREDICT() macro for the codepath without computed gotos --- Python/ceval.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index f90cf704661aaa..40f0c35dd28562 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -1372,6 +1372,7 @@ eval_frame_handle_pending(PyThreadState *tstate) opcode = _Py_OPCODE(word); \ if (opcode == op) { \ oparg = _Py_OPARG(word); \ + frame->f_lasti = INSTR_OFFSET(); \ next_instr++; \ goto PREDICT_ID(op); \ } \ @@ -1615,12 +1616,10 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr multiple values. When the PREDICT() macros are enabled, some opcode pairs follow in - direct succession without updating frame->f_lasti. A successful - prediction effectively links the two codes together as if they - were a single new opcode; accordingly,frame->f_lasti will point to - the first code in the pair (for instance, GET_ITER followed by - FOR_ITER is effectively a single opcode and frame->f_lasti will point - to the beginning of the combined pair.) + direct succession. A successful prediction effectively links the two + codes together as if they were a single new opcode, but the value + of frame->f_lasti is correctly updated so potential inlined calls + or lookups of frame->f_lasti are aways correct when the macros are used. */ assert(frame->f_lasti >= -1); _Py_CODEUNIT *next_instr = first_instr + frame->f_lasti + 1; From d7a99c1f6ccb4f19acbf6332a5b04388424c160d Mon Sep 17 00:00:00 2001 From: Pablo Galindo Salgado Date: Tue, 21 Sep 2021 17:54:06 +0100 Subject: [PATCH 06/13] Apply suggestions from code review Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com> --- Python/ceval.c | 4 ++-- Tools/gdb/libpython.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index 40f0c35dd28562..e2fc2744849642 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4636,10 +4636,10 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr PyObject *res; // Check if the call can be inlined or not - PyObject *function = *(stack_pointer - oparg - 1); + PyObject *function = PEEK(oparg + 1); int inline_call = 0; if (Py_TYPE(function) == &PyFunction_Type) { - PyCodeObject *code = (PyCodeObject*)((PyFunctionObject*)(function))->func_code; + PyCodeObject *code = (PyCodeObject*)PyFunction_GET_CODE(function); int is_coro = code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR); inline_call = (is_coro || cframe.use_tracing) ? 0 : 1; } diff --git a/Tools/gdb/libpython.py b/Tools/gdb/libpython.py index 6d19ee5c740494..7a97e2bf84fe6c 100755 --- a/Tools/gdb/libpython.py +++ b/Tools/gdb/libpython.py @@ -1926,7 +1926,7 @@ def invoke(self, args, from_tty): def move_in_stack(move_up): '''Move up or down the stack (for the py-up/py-down command)''' # Important: - # The ammount of frames that are printed out depends on how many frames are inlined + # The amount of frames that are printed out depends on how many frames are inlined # in the same evaluation loop. As this command links directly the C stack with the # Python stack, the results are sensitive to the number of inlined frames and this # is likely to change between versions and optimizations. @@ -1934,7 +1934,6 @@ def move_in_stack(move_up): if not frame: print('Unable to locate python frame') return - while frame: if move_up: iter_frame = frame.older() From 0721cea88f11173a4a3bd38a1d2507064892b9cc Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Thu, 23 Sep 2021 11:02:23 +0100 Subject: [PATCH 07/13] Tracing of Python functions is handled at entry not call. --- Python/ceval.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index e2fc2744849642..e5b14ed469eebb 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4641,7 +4641,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (Py_TYPE(function) == &PyFunction_Type) { PyCodeObject *code = (PyCodeObject*)PyFunction_GET_CODE(function); int is_coro = code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR); - inline_call = (is_coro || cframe.use_tracing) ? 0 : 1; + inline_call = !is_coro; } if (!inline_call) { @@ -4656,7 +4656,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr DISPATCH(); } - assert(!cframe.use_tracing); InterpreterFrame *new_frame = _PyEval_FrameFromPyFunctionAndArgs(tstate, stack_pointer-oparg, oparg, function); if (new_frame == NULL) { // When we exit here, we own all variables in the stack (the frame creation has not stolen From a2c09948dd1fb3acdfeb0390088ea0cc867b216c Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Thu, 23 Sep 2021 11:38:57 +0100 Subject: [PATCH 08/13] Inline creation of coroutines. --- Python/ceval.c | 59 +++++++++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index e5b14ed469eebb..81c01c36b280f5 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -1541,6 +1541,12 @@ _PyEval_FrameFromPyFunctionAndArgs(PyThreadState *tstate, PyObject* const *args, return _PyEvalFramePushAndInit(tstate, con, locals, args, vector_nargs, NULL, 1); } +static PyObject * +make_coro(PyThreadState *tstate, PyFrameConstructor *con, + PyObject *locals, + PyObject* const* args, size_t argcount, + PyObject *kwnames); + PyObject* _Py_HOT_FUNCTION _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int throwflag) { @@ -4637,14 +4643,38 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr // Check if the call can be inlined or not PyObject *function = PEEK(oparg + 1); - int inline_call = 0; if (Py_TYPE(function) == &PyFunction_Type) { PyCodeObject *code = (PyCodeObject*)PyFunction_GET_CODE(function); - int is_coro = code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR); - inline_call = !is_coro; + STACK_SHRINK(oparg + 1); + if (code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR)) { + PyObject *locals = code->co_flags & CO_OPTIMIZED ? NULL : PyFunction_GET_GLOBALS(function); + res = make_coro( + tstate, PyFunction_AS_FRAME_CONSTRUCTOR(function), locals, stack_pointer+1, oparg, NULL); + for (int i = 0; i < oparg+1; i++) { + Py_DECREF(stack_pointer[i]); + } + PUSH(res); + if (res == NULL) { + goto error; + } + } + else { + InterpreterFrame *new_frame = _PyEval_FrameFromPyFunctionAndArgs(tstate, stack_pointer+1, oparg, function); + if (new_frame == NULL) { + // When we exit here, we own all variables in the stack (the frame creation has not stolen + // any variable) so we need to clean the whole stack (done in the "error" label). + goto error; + } + assert(tstate->interp->eval_frame != NULL); + // The frame has stolen all the arguments from the stack, so there is no need to clean up them + Py_DECREF(function); + _PyFrame_SetStackPointer(frame, stack_pointer); + new_frame->depth = frame->depth + 1; + tstate->frame = frame = new_frame; + goto start_frame; + } } - - if (!inline_call) { + else { PyObject **sp = stack_pointer; res = call_function(tstate, &sp, oparg, NULL, cframe.use_tracing); stack_pointer = sp; @@ -4652,24 +4682,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (res == NULL) { goto error; } - CHECK_EVAL_BREAKER(); - DISPATCH(); } - - InterpreterFrame *new_frame = _PyEval_FrameFromPyFunctionAndArgs(tstate, stack_pointer-oparg, oparg, function); - if (new_frame == NULL) { - // When we exit here, we own all variables in the stack (the frame creation has not stolen - // any variable) so we need to clean the whole stack (done in the "error" label). - goto error; - } - assert(tstate->interp->eval_frame != NULL); - // The frame has stolen all the arguments from the stack, so there is no need to clean up them - STACK_SHRINK(oparg + 1); - Py_DECREF(function); - _PyFrame_SetStackPointer(frame, stack_pointer); - new_frame->depth = frame->depth + 1; - tstate->frame = frame = new_frame; - goto start_frame; + CHECK_EVAL_BREAKER(); + DISPATCH(); } TARGET(CALL_FUNCTION_KW): { From 9a63ee11a0e2fb74d81446aabbf00e337cc96126 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Thu, 23 Sep 2021 13:30:30 +0100 Subject: [PATCH 09/13] Reorder flow in CALL_FUNCTION to make it a bit clearer. --- Python/ceval.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index 81c01c36b280f5..d7d22ec062d0d4 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4646,19 +4646,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (Py_TYPE(function) == &PyFunction_Type) { PyCodeObject *code = (PyCodeObject*)PyFunction_GET_CODE(function); STACK_SHRINK(oparg + 1); - if (code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR)) { - PyObject *locals = code->co_flags & CO_OPTIMIZED ? NULL : PyFunction_GET_GLOBALS(function); - res = make_coro( - tstate, PyFunction_AS_FRAME_CONSTRUCTOR(function), locals, stack_pointer+1, oparg, NULL); - for (int i = 0; i < oparg+1; i++) { - Py_DECREF(stack_pointer[i]); - } - PUSH(res); - if (res == NULL) { - goto error; - } - } - else { + if ((code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR)) == 0) { InterpreterFrame *new_frame = _PyEval_FrameFromPyFunctionAndArgs(tstate, stack_pointer+1, oparg, function); if (new_frame == NULL) { // When we exit here, we own all variables in the stack (the frame creation has not stolen @@ -4673,15 +4661,23 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr tstate->frame = frame = new_frame; goto start_frame; } + else { + /* Callable is a generator or coroutine function: create coroutine or generator. */ + PyObject *locals = code->co_flags & CO_OPTIMIZED ? NULL : PyFunction_GET_GLOBALS(function); + res = make_coro(tstate, PyFunction_AS_FRAME_CONSTRUCTOR(function), locals, stack_pointer+1, oparg, NULL); + for (int i = 0; i < oparg+1; i++) { + Py_DECREF(stack_pointer[i]); + } + } } else { PyObject **sp = stack_pointer; res = call_function(tstate, &sp, oparg, NULL, cframe.use_tracing); stack_pointer = sp; - PUSH(res); - if (res == NULL) { - goto error; - } + } + PUSH(res); + if (res == NULL) { + goto error; } CHECK_EVAL_BREAKER(); DISPATCH(); From 9210541d03334b0b597213305396bef4034bbc20 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Salgado Date: Thu, 23 Sep 2021 21:36:39 +0100 Subject: [PATCH 10/13] Update Python/ceval.c Co-authored-by: Brett Cannon --- Python/ceval.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/ceval.c b/Python/ceval.c index d7d22ec062d0d4..87af79d29e0b6a 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4654,7 +4654,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr goto error; } assert(tstate->interp->eval_frame != NULL); - // The frame has stolen all the arguments from the stack, so there is no need to clean up them + // The frame has stolen all the arguments from the stack, so there is no need to clean them up.``` Py_DECREF(function); _PyFrame_SetStackPointer(frame, stack_pointer); new_frame->depth = frame->depth + 1; From 9ce9e6314e06b704d933159dcc37eaca7f77ff49 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Fri, 24 Sep 2021 09:47:25 +0100 Subject: [PATCH 11/13] Fix reference counting when creating a new interpreter frame fails. --- Python/ceval.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index 87af79d29e0b6a..b3efd390548dd0 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4645,14 +4645,14 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr PyObject *function = PEEK(oparg + 1); if (Py_TYPE(function) == &PyFunction_Type) { PyCodeObject *code = (PyCodeObject*)PyFunction_GET_CODE(function); - STACK_SHRINK(oparg + 1); if ((code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR)) == 0) { - InterpreterFrame *new_frame = _PyEval_FrameFromPyFunctionAndArgs(tstate, stack_pointer+1, oparg, function); + InterpreterFrame *new_frame = _PyEval_FrameFromPyFunctionAndArgs(tstate, stack_pointer-oparg, oparg, function); if (new_frame == NULL) { // When we exit here, we own all variables in the stack (the frame creation has not stolen // any variable) so we need to clean the whole stack (done in the "error" label). goto error; } + STACK_SHRINK(oparg + 1); assert(tstate->interp->eval_frame != NULL); // The frame has stolen all the arguments from the stack, so there is no need to clean them up.``` Py_DECREF(function); @@ -4664,8 +4664,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr else { /* Callable is a generator or coroutine function: create coroutine or generator. */ PyObject *locals = code->co_flags & CO_OPTIMIZED ? NULL : PyFunction_GET_GLOBALS(function); - res = make_coro(tstate, PyFunction_AS_FRAME_CONSTRUCTOR(function), locals, stack_pointer+1, oparg, NULL); - for (int i = 0; i < oparg+1; i++) { + res = make_coro(tstate, PyFunction_AS_FRAME_CONSTRUCTOR(function), locals, stack_pointer-oparg, oparg, NULL); + STACK_SHRINK(oparg + 1); + for (int i = 0; i < oparg + 1; i++) { Py_DECREF(stack_pointer[i]); } } From 9a0ceabdcad4546fb97d66f254556a8684e6073f Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Fri, 24 Sep 2021 09:48:50 +0100 Subject: [PATCH 12/13] Remove _PyEval_FrameFromPyFunctionAndArgs forwarding function. --- Python/ceval.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index b3efd390548dd0..71752eda509224 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -1523,24 +1523,6 @@ trace_function_entry(PyThreadState *tstate, InterpreterFrame *frame) return 0; } -/* Create a frame structure from a python function stealing ownership from an array of arguments. - * In case of error, it returns NULL and the caller still owns the references to all arguments */ -static InterpreterFrame* -_PyEval_FrameFromPyFunctionAndArgs(PyThreadState *tstate, PyObject* const *args, int nargs, PyObject *function) { - assert(PyFunction_Check(function)); - size_t nargsf = nargs | PY_VECTORCALL_ARGUMENTS_OFFSET; - assert(args != NULL || PyVectorcall_NARGS(nargsf) == 0); - assert(PyVectorcall_Function(function) == _PyFunction_Vectorcall); - PyFrameConstructor *con = PyFunction_AS_FRAME_CONSTRUCTOR(function); - Py_ssize_t vector_nargs = PyVectorcall_NARGS(nargsf); - assert(vector_nargs >= 0); - assert(vector_nargs == 0 || args != NULL); - PyCodeObject *code = (PyCodeObject *)con->fc_code; - PyObject *locals = (code->co_flags & CO_OPTIMIZED) ? NULL : con->fc_globals; - assert(!(code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR))); - return _PyEvalFramePushAndInit(tstate, con, locals, args, vector_nargs, NULL, 1); -} - static PyObject * make_coro(PyThreadState *tstate, PyFrameConstructor *con, PyObject *locals, @@ -4645,8 +4627,10 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr PyObject *function = PEEK(oparg + 1); if (Py_TYPE(function) == &PyFunction_Type) { PyCodeObject *code = (PyCodeObject*)PyFunction_GET_CODE(function); + PyObject *locals = code->co_flags & CO_OPTIMIZED ? NULL : PyFunction_GET_GLOBALS(function); if ((code->co_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR)) == 0) { - InterpreterFrame *new_frame = _PyEval_FrameFromPyFunctionAndArgs(tstate, stack_pointer-oparg, oparg, function); + InterpreterFrame *new_frame = _PyEvalFramePushAndInit( + tstate, PyFunction_AS_FRAME_CONSTRUCTOR(function), locals, stack_pointer-oparg, oparg, NULL, 1); if (new_frame == NULL) { // When we exit here, we own all variables in the stack (the frame creation has not stolen // any variable) so we need to clean the whole stack (done in the "error" label). @@ -4663,7 +4647,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr } else { /* Callable is a generator or coroutine function: create coroutine or generator. */ - PyObject *locals = code->co_flags & CO_OPTIMIZED ? NULL : PyFunction_GET_GLOBALS(function); res = make_coro(tstate, PyFunction_AS_FRAME_CONSTRUCTOR(function), locals, stack_pointer-oparg, oparg, NULL); STACK_SHRINK(oparg + 1); for (int i = 0; i < oparg + 1; i++) { From 1de88f6f9e96a9145eee4720c0c266c8054b8a28 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Wed, 29 Sep 2021 01:03:22 +0100 Subject: [PATCH 13/13] Fix reference leaks (needs cleanup) --- Python/ceval.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Python/ceval.c b/Python/ceval.c index 71752eda509224..1dd6785d27bef2 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -5648,7 +5648,12 @@ _PyEvalFramePushAndInit(PyThreadState *tstate, PyFrameConstructor *con, PyObject **localsarray = _PyFrame_GetLocalsArray(frame); if (initialize_locals(tstate, con, localsarray, args, argcount, kwnames, steal_args)) { if (steal_args) { - for (int i = 0; i < frame->stacktop; i++) { + // If we failed to initialize locals, make sure the caller still own all the + // arguments. Notice that we only need to increase the reference count of the + // *valid* arguments (i.e. the ones that fit into the frame). + PyCodeObject *co = (PyCodeObject*)con->fc_code; + const Py_ssize_t total_args = co->co_argcount + co->co_kwonlyargcount; + for (Py_ssize_t i = 0; i < Py_MIN(argcount, total_args); i++) { Py_XINCREF(frame->localsplus[i]); } }