gh-109094: replace frame->prev_instr by frame->instr_ptr (#109095)

Author: iritkatriel

Include/internal/pycore_frame.h

@@ -58,26 +58,16 @@ typedef struct _PyInterpreterFrame {
     PyObject *f_builtins; /* Borrowed reference. Only valid if not on C stack */
     PyObject *f_locals; /* Strong reference, may be NULL. Only valid if not on C stack */
     PyFrameObject *frame_obj; /* Strong reference, may be NULL. Only valid if not on C stack */
-    // NOTE: This is not necessarily the last instruction started in the given
-    // frame. Rather, it is the code unit *prior to* the *next* instruction. For
-    // example, it may be an inline CACHE entry, an instruction we just jumped
-    // over, or (in the case of a newly-created frame) a totally invalid value:
-    _Py_CODEUNIT *prev_instr;
+    _Py_CODEUNIT *instr_ptr; /* Instruction currently executing (or about to begin) */
     int stacktop;  /* Offset of TOS from localsplus  */
-    /* The return_offset determines where a `RETURN` should go in the caller,
-     * relative to `prev_instr`.
-     * It is only meaningful to the callee,
-     * so it needs to be set in any CALL (to a Python function)
-     * or SEND (to a coroutine or generator).
-     * If there is no callee, then it is meaningless. */
-    uint16_t return_offset;
+    uint16_t return_offset;  /* Only relevant during a function call */
     char owner;
     /* Locals and stack */
     PyObject *localsplus[1];
 } _PyInterpreterFrame;
 
 #define _PyInterpreterFrame_LASTI(IF) \
-    ((int)((IF)->prev_instr - _PyCode_CODE(_PyFrame_GetCode(IF))))
+    ((int)((IF)->instr_ptr - _PyCode_CODE(_PyFrame_GetCode(IF))))
 
 static inline PyCodeObject *_PyFrame_GetCode(_PyInterpreterFrame *f) {
     assert(PyCode_Check(f->f_executable));
@@ -134,7 +124,7 @@ _PyFrame_Initialize(
     frame->f_locals = locals;
     frame->stacktop = code->co_nlocalsplus;
     frame->frame_obj = NULL;
-    frame->prev_instr = _PyCode_CODE(code) - 1;
+    frame->instr_ptr = _PyCode_CODE(code);
     frame->return_offset = 0;
     frame->owner = FRAME_OWNED_BY_THREAD;
 
@@ -185,7 +175,7 @@ _PyFrame_IsIncomplete(_PyInterpreterFrame *frame)
         return true;
     }
     return frame->owner != FRAME_OWNED_BY_GENERATOR &&
-    frame->prev_instr < _PyCode_CODE(_PyFrame_GetCode(frame)) + _PyFrame_GetCode(frame)->_co_firsttraceable;
+        frame->instr_ptr < _PyCode_CODE(_PyFrame_GetCode(frame)) + _PyFrame_GetCode(frame)->_co_firsttraceable;
 }
 
 static inline _PyInterpreterFrame *
@@ -297,7 +287,7 @@ _PyFrame_PushTrampolineUnchecked(PyThreadState *tstate, PyCodeObject *code, int
     frame->f_locals = NULL;
     frame->stacktop = code->co_nlocalsplus + stackdepth;
     frame->frame_obj = NULL;
-    frame->prev_instr = _PyCode_CODE(code);
+    frame->instr_ptr = _PyCode_CODE(code);
     frame->owner = FRAME_OWNED_BY_THREAD;
     frame->return_offset = 0;
     return frame;

Include/internal/pycore_opcode_metadata.h

@@ -87,7 +87,7 @@ typedef struct _Py_DebugOffsets {
     struct _interpreter_frame {
         off_t previous;
         off_t executable;
-        off_t prev_instr;
+        off_t instr_ptr;
         off_t localsplus;
         off_t owner;
     } interpreter_frame;

Include/internal/pycore_runtime.h

@@ -58,7 +58,7 @@ extern PyTypeObject _PyExc_MemoryError;
             .interpreter_frame = { \
                 .previous = offsetof(_PyInterpreterFrame, previous), \
                 .executable = offsetof(_PyInterpreterFrame, f_executable), \
-                .prev_instr = offsetof(_PyInterpreterFrame, prev_instr), \
+                .instr_ptr = offsetof(_PyInterpreterFrame, instr_ptr), \
                 .localsplus = offsetof(_PyInterpreterFrame, localsplus), \
                 .owner = offsetof(_PyInterpreterFrame, owner), \
             }, \

Include/internal/pycore_runtime_init.h

@@ -0,0 +1,3 @@
+Replace ``prev_instr`` on the interpreter frame by ``instr_ptr`` which
+points to the beginning of the instruction that is currently executing (or
+will execute once the frame resumes).

Misc/NEWS.d/next/Core and Builtins/2023-10-13-16-55-55.gh-issue-109094.ziL4cJ.rst

@@ -130,3 +130,28 @@ returns. This extra frame is inserted so that `RETURN_VALUE`, `YIELD_VALUE`, and
 `RETURN_GENERATOR` do not need to check whether the current frame is the entry frame.
 The shim frame points to a special code object containing the `INTERPRETER_EXIT`
 instruction which cleans up the shim frame and returns.
+
+
+### The Instruction Pointer
+
+`_PyInterpreterFrame` has two fields which are used to maintain the instruction
+pointer: `instr_ptr` and `return_offset`.
+
+When a frame is executing, `instr_ptr` points to the instruction currently being
+executed. In a suspended frame, it points to the instruction that would execute
+if the frame were to resume. After `frame.f_lineno` is set, `instr_ptr` points to
+the next instruction to be executed. During a call to a python function,
+`instr_ptr` points to the call instruction, because this is what we would expect
+to see in an exception traceback.
+
+The `return_offset` field determines where a `RETURN` should go in the caller,
+relative to `instr_ptr`.  It is only meaningful to the callee, so it needs to
+be set in any instruction that implements a call (to a Python function),
+including CALL, SEND and BINARY_SUBSCR_GETITEM, among others. If there is no
+callee, then return_offset is meaningless.  It is necessary to have a separate
+field for the return offset because (1) if we apply this offset to `instr_ptr`
+while executing the `RETURN`, this is too early and would lose us information
+about the previous instruction which we could need for introspecting and
+debugging. (2) `SEND` needs to pass two offsets to the generator: one for
+`RETURN` and one for `YIELD`. It uses the `oparg` for one, and the
+`return_offset` for the other.

Objects/frame_layout.md

@@ -821,7 +821,7 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore
     }
     /* Finally set the new lasti and return OK. */
     f->f_lineno = 0;
-    f->f_frame->prev_instr = _PyCode_CODE(code) + best_addr;
+    f->f_frame->instr_ptr = _PyCode_CODE(code) + best_addr;
     return 0;
 }
 
@@ -1079,7 +1079,7 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code,
     f->f_frame = (_PyInterpreterFrame *)f->_f_frame_data;
     f->f_frame->owner = FRAME_OWNED_BY_FRAME_OBJECT;
     // This frame needs to be "complete", so pretend that the first RESUME ran:
-    f->f_frame->prev_instr = _PyCode_CODE(code) + code->_co_firsttraceable;
+    f->f_frame->instr_ptr = _PyCode_CODE(code) + code->_co_firsttraceable + 1;
     assert(!_PyFrame_IsIncomplete(f->f_frame));
     Py_DECREF(func);
     _PyObject_GC_TRACK(f);
@@ -1093,7 +1093,7 @@ _PyFrame_OpAlreadyRan(_PyInterpreterFrame *frame, int opcode, int oparg)
     assert(_PyOpcode_Deopt[opcode] == opcode);
     int check_oparg = 0;
     for (_Py_CODEUNIT *instruction = _PyCode_CODE(_PyFrame_GetCode(frame));
-         instruction < frame->prev_instr; instruction++)
+         instruction < frame->instr_ptr; instruction++)
     {
         int check_opcode = _PyOpcode_Deopt[instruction->op.code];
         check_oparg |= instruction->op.arg;
@@ -1135,7 +1135,7 @@ frame_init_get_vars(_PyInterpreterFrame *frame)
         frame->localsplus[offset + i] = Py_NewRef(o);
     }
     // COPY_FREE_VARS doesn't have inline CACHEs, either:
-    frame->prev_instr = _PyCode_CODE(_PyFrame_GetCode(frame));
+    frame->instr_ptr = _PyCode_CODE(_PyFrame_GetCode(frame));
 }
 
 

Objects/frameobject.c

@@ -9,6 +9,7 @@
 #include "pycore_genobject.h"     // struct _Py_async_gen_state
 #include "pycore_modsupport.h"    // _PyArg_CheckPositional()
 #include "pycore_object.h"        // _PyObject_GC_UNTRACK()
+#include "pycore_opcode_metadata.h" // _PyOpcode_Caches
 #include "pycore_pyerrors.h"      // _PyErr_ClearExcState()
 #include "pycore_pystate.h"       // _PyThreadState_GET()
 
@@ -362,8 +363,7 @@ _PyGen_yf(PyGenObject *gen)
             assert(_PyCode_CODE(_PyGen_GetCode(gen))[0].op.code != SEND);
             return NULL;
         }
-        _Py_CODEUNIT next = frame->prev_instr[1];
-        if (!is_resume(&next) || next.op.arg < 2)
+        if (!is_resume(frame->instr_ptr) || frame->instr_ptr->op.arg < 2)
         {
             /* Not in a yield from */
             return NULL;
@@ -398,9 +398,12 @@ gen_close(PyGenObject *gen, PyObject *args)
     _PyInterpreterFrame *frame = (_PyInterpreterFrame *)gen->gi_iframe;
     /* It is possible for the previous instruction to not be a
      * YIELD_VALUE if the debugger has changed the lineno. */
-    if (err == 0 && is_yield(frame->prev_instr)) {
-        assert(is_resume(frame->prev_instr + 1));
-        int exception_handler_depth = frame->prev_instr[0].op.arg;
+    assert(_PyOpcode_Caches[YIELD_VALUE] == 0);
+    assert(_PyOpcode_Caches[INSTRUMENTED_YIELD_VALUE] == 0);
+    if (err == 0 && is_yield(frame->instr_ptr - 1)) {
+        _Py_CODEUNIT *yield_instr = frame->instr_ptr - 1;
+        assert(is_resume(frame->instr_ptr));
+        int exception_handler_depth = yield_instr->op.arg;
         assert(exception_handler_depth > 0);
         /* We can safely ignore the outermost try block
          * as it automatically generated to handle