Address code review comments.

python · markshannon · Nov 10, 2022 · Aug 24, 2022 · Aug 25, 2022 · Aug 26, 2022
commit b62f5f3506b86c4ccb555ad76def3823d79feb3c
diff --git a/Include/internal/pycore_code.h b/Include/internal/pycore_code.h
@@ -456,7 +456,7 @@ _PyCode_LineNumberFromArray(PyCodeObject *co, int index)
 }
 
 extern PyCodeObject *
-_Py_MakeTrampoline(const char *code, int codelen, int stacksize, const char *cname);
+_Py_MakeTrampoline(const uint8_t *code, int codelen, int stacksize, const char *cname);
 
 
 #ifdef __cplusplus

diff --git a/Include/internal/pycore_frame.h b/Include/internal/pycore_frame.h
@@ -48,12 +48,12 @@ enum _frameowner {
 
 typedef struct _PyInterpreterFrame {
     /* "Specials" section */
-    PyObject *f_funcobj; /* Strong reference. Only valid for Python frames */
-    PyObject *f_globals; /* Borrowed reference. Only valid for Python frames */
-    PyObject *f_builtins; /* Borrowed reference. Only valid for Python frames */
-    PyObject *f_locals; /* Strong reference, may be NULL. Only valid for Python frames */
+    PyObject *f_funcobj; /* Strong reference. Only valid if not on C stack */
+    PyObject *f_globals; /* Borrowed reference. Only valid if not on C stack */
+    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 */
     PyCodeObject *f_code; /* Strong reference */
-    PyFrameObject *frame_obj; /* Strong reference, may be NULL. Only valid for Python frames */
+    PyFrameObject *frame_obj; /* Strong reference, may be NULL. Only valid if not on C stack */
     /* Linkage section */
     struct _PyInterpreterFrame *previous;
     // NOTE: This is not necessarily the last instruction started in the given
@@ -62,8 +62,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  */
-    int yield_offset: 28;
-    int owner: 4;
+    char owner;
     /* Locals and stack */
     PyObject *localsplus[1];
 } _PyInterpreterFrame;

diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-09-09-10-37-34.gh-issue-96421.cyg33z.rst b/Misc/NEWS.d/next/Core and Builtins/2022-09-09-10-37-34.gh-issue-96421.cyg33z.rst
diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-10-19-15-59-08.gh-issue-96421.e22y3r.rst b/Misc/NEWS.d/next/Core and Builtins/2022-10-19-15-59-08.gh-issue-96421.e22y3r.rst
@@ -1,4 +1,14 @@
-On entry to ``PyEval_EvalFrameDefault()`` an additional, internal, shim
-frame is pushed. This simplifies the return and yield bytecode instructions.
-Only debuggers and profiles that inspect the stack of frames should be
-impacted.
+When calling into Python code from C code, though ``PyEval_EvalFrame`` or
+related C-API function, a shim frame in inserted into the call stack.
+This occurs in the ``_PyEval_EvalFrameDefault()`` function.
+The extra frame should be invisible to all Python and most C extensions,
+but out-of-process debuggers, profilers and debuggers need to be aware of
+it.
+These shim frames can be detected by checking
+``frame->owner == FRAME_OWNED_BY_CSTACK``.
+
+Extensions implementing their own interpreters using PEP 523 need to be
+aware of this shim frame and the changes to the semantics of
+``RETURN_VALUE``, ``YIELD_VALUE``, and ``RETURN_GENERATOR``,
+which now clear the frame.
+
@@ -2270,7 +2270,7 @@ _PyStaticCode_InternStrings(PyCodeObject *co)
 }
 
 PyCodeObject *
-_Py_MakeTrampoline(const char *code, int codelen, int stacksize, const char *cname)
+_Py_MakeTrampoline(const uint8_t *code, int codelen, int stacksize, const char *cname)
 {
     PyObject *name = NULL;
     PyObject *co_code = NULL;

@@ -1066,7 +1066,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
     pyframe.prev_instr = code;
     pyframe.stacktop = 0;
     pyframe.owner = FRAME_OWNED_BY_CSTACK;
-    pyframe.yield_offset = 0;
     /* Push frame */
     pyframe.previous = prev_cframe->current_frame;
     frame->previous = &pyframe;
@@ -2095,7 +2094,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
             _PyInterpreterFrame *gen_frame = frame;
             frame = cframe.current_frame = gen_frame->previous;
             gen_frame->previous = NULL;
-            frame->prev_instr += frame->yield_offset;
             _PyFrame_StackPush(frame, retval);
             goto resume_frame;
         }

diff --git a/Python/frame.c b/Python/frame.c
@@ -127,7 +127,6 @@ _PyFrame_Clear(_PyInterpreterFrame *frame)
      * to have cleared the enclosing generator, if any. */
     assert(frame->owner != FRAME_OWNED_BY_GENERATOR ||
         _PyFrame_GetGenerator(frame)->gi_frame_state == FRAME_CLEARED);
-    assert(frame->owner != FRAME_CLEARED);
     if (frame->frame_obj) {
         PyFrameObject *f = frame->frame_obj;
         frame->frame_obj = NULL;
@@ -142,12 +141,10 @@ _PyFrame_Clear(_PyInterpreterFrame *frame)
     for (int i = 0; i < frame->stacktop; i++) {
         Py_XDECREF(frame->localsplus[i]);
     }
-    frame->stacktop = 0;
     Py_XDECREF(frame->frame_obj);
     Py_XDECREF(frame->f_locals);
     Py_DECREF(frame->f_funcobj);
     Py_DECREF(frame->f_code);
-    frame->owner = FRAME_CLEARED;
 }
 
 int

@@ -752,9 +752,12 @@ pycore_init_types(PyInterpreterState *interp)
     return _PyStatus_OK();
 }
 
-static const char INTERPRETER_TRAMPOLINE_CODE[] = {
-    0, 0,
+static const uint8_t INTERPRETER_TRAMPOLINE_CODE[] = {
+    /* Put a NOP at the start, so that the IP points into
+     * the code, rather than before it */
+    NOP, 0,
     INTERPRETER_EXIT, 0,
+    /* RESUME at end makes sure that the frame appears incomplete */
     RESUME, 0
 };
 
@@ -792,7 +795,9 @@ pycore_init_builtins(PyThreadState *tstate)
     assert(object__getattribute__);
     interp->callable_cache.object__getattribute__ = object__getattribute__;
     interp->interpreter_trampoline = _Py_MakeTrampoline(
-        INTERPRETER_TRAMPOLINE_CODE, sizeof(INTERPRETER_TRAMPOLINE_CODE), 1, "<interpreter trampoline>");
+        INTERPRETER_TRAMPOLINE_CODE,
+        sizeof(INTERPRETER_TRAMPOLINE_CODE),
+        1, "<interpreter trampoline>");
     if (interp->interpreter_trampoline == NULL) {
         return _PyStatus_ERR("failed to create interpreter trampoline.");
     }

diff --git a/Python/traceback.c b/Python/traceback.c
@@ -1236,6 +1236,8 @@ dump_traceback(int fd, PyThreadState *tstate, int write_header)
         if (frame == NULL) {
             break;
         }
+        /* Can't have more than one shim frame in a row */
+        assert(frame->owner != FRAME_OWNED_BY_CSTACK);
         depth++;
     }
 }

diff --git a/Tools/gdb/libpython.py b/Tools/gdb/libpython.py
@@ -82,6 +82,8 @@ def _sizeof_void_p():
 Py_TPFLAGS_BASE_EXC_SUBCLASS = (1 << 30)
 Py_TPFLAGS_TYPE_SUBCLASS     = (1 << 31)
 
+#From pycore_frame.h
+FRAME_OWNED_BY_CSTACK = 3
 
 MAX_OUTPUT_LEN=1024
 
@@ -1077,8 +1079,8 @@ def _f_lasti(self):
         first_instr = self._f_code().field("co_code_adaptive").cast(codeunit_p)
         return int(prev_instr - first_instr)
 
-    def is_entry(self):
-        return self._f_special("owner", int) == 3
+    def is_shim(self):
+        return self._f_special("owner", int) == FRAME_OWNED_BY_CSTACK
 
     def previous(self):
         return self._f_special("previous", PyFramePtr)
@@ -1821,7 +1823,7 @@ def print_summary(self):
             interp_frame = self.get_pyop()
             while True:
                 if interp_frame:
-                    if interp_frame.is_entry():
+                    if interp_frame.is_shim():
                         break
                     line = interp_frame.get_truncated_repr(MAX_OUTPUT_LEN)
                     sys.stdout.write('#%i %s\n' % (self.get_index(), line))
@@ -1845,7 +1847,7 @@ def print_traceback(self):
             interp_frame = self.get_pyop()
             while True:
                 if interp_frame:
-                    if interp_frame.is_entry():
+                    if interp_frame.is_shim():
                         break
                     interp_frame.print_traceback()
                     if not interp_frame.is_optimized_out():
@@ -2106,7 +2108,7 @@ def invoke(self, args, from_tty):
         while True:
             if not pyop_frame:
                 print(UNABLE_READ_INFO_PYTHON_FRAME)
-            if pyop_frame.is_entry():
+            if pyop_frame.is_shim():
                 break
 
             sys.stdout.write('Locals for %s\n' % (pyop_frame.co_name.proxyval(set())))