From 30bade1f92463d67f2864cda453cf2d13a7073f5 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Tue, 12 Dec 2023 04:46:23 +0000 Subject: [PATCH 1/3] Change return value of executor function to _Py_CODEUNIT and guarantee forward progress. --- Include/cpython/optimizer.h | 2 +- Include/internal/pycore_opcode_metadata.h | 2 +- Include/internal/pycore_uops.h | 2 +- Python/bytecodes.c | 10 +++------- Python/generated_cases.c.h | 11 ++++------- Python/optimizer.c | 8 ++++---- 6 files changed, 14 insertions(+), 21 deletions(-) diff --git a/Include/cpython/optimizer.h b/Include/cpython/optimizer.h index adc2c1fc442280..d521eac79d1b97 100644 --- a/Include/cpython/optimizer.h +++ b/Include/cpython/optimizer.h @@ -32,7 +32,7 @@ typedef struct { typedef struct _PyExecutorObject { PyObject_VAR_HEAD /* WARNING: execute consumes a reference to self. This is necessary to allow executors to tail call into each other. */ - struct _PyInterpreterFrame *(*execute)(struct _PyExecutorObject *self, struct _PyInterpreterFrame *frame, PyObject **stack_pointer); + _Py_CODEUNIT *(*execute)(struct _PyExecutorObject *self, struct _PyInterpreterFrame *frame, PyObject **stack_pointer); _PyVMData vm_data; /* Used by the VM, but opaque to the optimizer */ /* Data needed by the executor goes here, but is opaque to the VM */ } _PyExecutorObject; diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index 1f460640a1e398..2c512d97c421c9 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -1568,7 +1568,7 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[OPCODE_METADATA_SIZE] = { [JUMP_BACKWARD] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_JUMP_FLAG | HAS_EVAL_BREAK_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [JUMP] = { true, 0, HAS_ARG_FLAG | HAS_JUMP_FLAG | HAS_EVAL_BREAK_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [JUMP_NO_INTERRUPT] = { true, 0, HAS_ARG_FLAG | HAS_JUMP_FLAG }, - [ENTER_EXECUTOR] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_JUMP_FLAG | HAS_EVAL_BREAK_FLAG | HAS_ERROR_FLAG }, + [ENTER_EXECUTOR] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_EVAL_BREAK_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [_POP_JUMP_IF_FALSE] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_JUMP_FLAG }, [_POP_JUMP_IF_TRUE] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_JUMP_FLAG }, [_IS_NONE] = { true, INSTR_FMT_IX, 0 }, diff --git a/Include/internal/pycore_uops.h b/Include/internal/pycore_uops.h index ea8f90bf8c1d8f..153884f4bd2902 100644 --- a/Include/internal/pycore_uops.h +++ b/Include/internal/pycore_uops.h @@ -24,7 +24,7 @@ typedef struct { _PyUOpInstruction trace[1]; } _PyUOpExecutorObject; -_PyInterpreterFrame *_PyUOpExecute( +_Py_CODEUNIT *_PyUOpExecute( _PyExecutorObject *executor, _PyInterpreterFrame *frame, PyObject **stack_pointer); diff --git a/Python/bytecodes.c b/Python/bytecodes.c index bcad8dcf0e7dab..23e11006b80e69 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2352,20 +2352,16 @@ dummy_func( PyCodeObject *code = _PyFrame_GetCode(frame); _PyExecutorObject *executor = (_PyExecutorObject *)code->co_executors->executors[oparg&255]; - int original_oparg = executor->vm_data.oparg | (oparg & 0xfffff00); - JUMPBY(1-original_oparg); - frame->instr_ptr = next_instr; Py_INCREF(executor); if (executor->execute == _PyUOpExecute) { current_executor = (_PyUOpExecutorObject *)executor; GOTO_TIER_TWO(); } - frame = executor->execute(executor, frame, stack_pointer); - if (frame == NULL) { - frame = tstate->current_frame; + next_instr = executor->execute(executor, frame, stack_pointer); + if (next_instr == NULL) { goto resume_with_error; } - goto enter_tier_one; + stack_pointer = _PyFrame_GetStackPointer(frame); } replaced op(_POP_JUMP_IF_FALSE, (unused/1, cond -- )) { diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 24243ecfb5b8df..2e3011e0a04edf 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -2327,20 +2327,17 @@ CHECK_EVAL_BREAKER(); PyCodeObject *code = _PyFrame_GetCode(frame); _PyExecutorObject *executor = (_PyExecutorObject *)code->co_executors->executors[oparg&255]; - int original_oparg = executor->vm_data.oparg | (oparg & 0xfffff00); - JUMPBY(1-original_oparg); - frame->instr_ptr = next_instr; Py_INCREF(executor); if (executor->execute == _PyUOpExecute) { current_executor = (_PyUOpExecutorObject *)executor; GOTO_TIER_TWO(); } - frame = executor->execute(executor, frame, stack_pointer); - if (frame == NULL) { - frame = tstate->current_frame; + next_instr = executor->execute(executor, frame, stack_pointer); + if (next_instr == NULL) { goto resume_with_error; } - goto enter_tier_one; + stack_pointer = _PyFrame_GetStackPointer(frame); + DISPATCH(); } TARGET(EXIT_INIT_CHECK) { diff --git a/Python/optimizer.c b/Python/optimizer.c index dd24fbebbfd2a9..7c46bd69157170 100644 --- a/Python/optimizer.c +++ b/Python/optimizer.c @@ -167,6 +167,7 @@ _PyOptimizer_BackEdge(_PyInterpreterFrame *frame, _Py_CODEUNIT *src, _Py_CODEUNI } _PyOptimizerObject *opt = interp->optimizer; _PyExecutorObject *executor = NULL; + /* Start optimizing at the destination to guarantee forward progress */ int err = opt->optimize(opt, code, dest, &executor, (int)(stack_pointer - _PyFrame_Stackbase(frame))); if (err <= 0) { assert(executor == NULL); @@ -247,14 +248,13 @@ PyTypeObject _PyCounterExecutor_Type = { .tp_methods = executor_methods, }; -static _PyInterpreterFrame * +static _Py_CODEUNIT * counter_execute(_PyExecutorObject *self, _PyInterpreterFrame *frame, PyObject **stack_pointer) { ((_PyCounterExecutorObject *)self)->optimizer->count++; _PyFrame_SetStackPointer(frame, stack_pointer); - frame->instr_ptr = ((_PyCounterExecutorObject *)self)->next_instr; Py_DECREF(self); - return frame; + return ((_PyCounterExecutorObject *)self)->next_instr; } static int @@ -891,7 +891,7 @@ uop_optimize( /* Dummy execute() function for UOp Executor. * The actual implementation is inlined in ceval.c, * in _PyEval_EvalFrameDefault(). */ -_PyInterpreterFrame * +_Py_CODEUNIT * _PyUOpExecute(_PyExecutorObject *executor, _PyInterpreterFrame *frame, PyObject **stack_pointer) { Py_FatalError("Tier 2 is now inlined into Tier 1"); From fd58cce873b6945ee348d640542f20f544a88c46 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Tue, 12 Dec 2023 04:53:37 +0000 Subject: [PATCH 2/3] Add news --- .../2023-12-12-04-53-19.gh-issue-108866.xbJ-9a.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-12-12-04-53-19.gh-issue-108866.xbJ-9a.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-12-12-04-53-19.gh-issue-108866.xbJ-9a.rst b/Misc/NEWS.d/next/Core and Builtins/2023-12-12-04-53-19.gh-issue-108866.xbJ-9a.rst new file mode 100644 index 00000000000000..96606924d4a3ec --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-12-12-04-53-19.gh-issue-108866.xbJ-9a.rst @@ -0,0 +1,3 @@ +Change the API and contract of ``_PyExecutorObject`` to return the +next_instr pointer, instead of the frame, and to always execute at least one +instruction. From 6cbaa9bfd04b68e7835e4df215e47316e95a3661 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Tue, 12 Dec 2023 10:47:02 +0000 Subject: [PATCH 3/3] Update frame when returning from custom executor --- Python/bytecodes.c | 1 + Python/generated_cases.c.h | 1 + 2 files changed, 2 insertions(+) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 23e11006b80e69..53f422de3828fa 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2358,6 +2358,7 @@ dummy_func( GOTO_TIER_TWO(); } next_instr = executor->execute(executor, frame, stack_pointer); + frame = tstate->current_frame; if (next_instr == NULL) { goto resume_with_error; } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 2e3011e0a04edf..e9953d6e1ee86e 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -2333,6 +2333,7 @@ GOTO_TIER_TWO(); } next_instr = executor->execute(executor, frame, stack_pointer); + frame = tstate->current_frame; if (next_instr == NULL) { goto resume_with_error; }