Restore stack introspection ability on 3.12

Author: oremanj

CHANGES.rst

@@ -2,11 +2,20 @@
  Changes
 =========
 
-3.0.3 (unreleased)
+3.1.0 (unreleased)
 ==================
 
-- Nothing changed yet.
-
+- Python 3.12: Restore the full ability to walk the stack of a suspended
+  greenlet; previously only the innermost frame was exposed.
+  For performance reasons, there are still some restrictions relative to
+  older Python versions; in particular, by default it is only valid to walk
+  a suspended greenlet's stack in between a ``gr_frame`` access and the next
+  resumption of the greenlet. A more flexible mode can be enabled by setting
+  the greenlet's new ``gr_frames_always_exposed`` attribute to True. If you
+  get a Python interpreter crash on 3.12+ when accessing the ``f_back`` of a
+  suspended greenlet frame, you're probably accessing it in a way that
+  requires you to set this attribute. See `issue 388
+  <https://github.com/python-greenlet/greenlet/issues/388>`_.
 
 3.0.2 (2023-12-08)
 ==================

docs/api.rst

@@ -32,7 +32,6 @@ Greenlets
 
    .. autoattribute:: gr_context
 
-
       The :class:`contextvars.Context` in which ``g`` will run.
       Writable; defaults to ``None``, reflecting that a greenlet
       starts execution in an empty context unless told otherwise.
@@ -56,6 +55,30 @@ Greenlets
       for suspended greenlets; it is None if the greenlet is dead, not
       yet started, or currently executing.
 
+      .. warning:: Greenlet stack introspection is fragile on CPython 3.12
+         and later. The frame objects of a suspended greenlet are not safe
+         to access as-is, but must be adjusted by the greenlet package in
+         order to make traversing ``f_back`` links not crash the interpreter,
+         and restored to their original state when resuming the greenlet.
+         This is all handled transparently as long as you obtain references
+         to greenlet frames only via the ``gr_frame`` attribute and you finish
+         accessing them before the greenlet next resumes. If you obtain
+         frames in other ways, or hold onto them across their greenlet's
+         resumption, you must set the ``gr_frames_always_exposed`` attribute
+         in order to make that safe.
+
+   .. autoattribute:: gr_frames_always_exposed
+
+      Writable boolean indicating whether this greenlet will take extra action,
+      each time it is suspended, to ensure that its frame objects are always
+      safe to access. Normally such action is only taken when an access
+      to the ``gr_frame`` attribute occurs, which means you can only safely
+      walk a greenlet's stack in between accessing ``gr_frame`` and resuming
+      the greenlet. This is relevant only on CPython 3.12 and later; earlier
+      versions still permit writing the attribute, but because their frame
+      objects are safe to access regardless, such writes have no effect and
+      the attribute always reads as true.
+
    .. autoattribute:: parent
 
       The parent greenlet. This is writable, but it is not allowed to create

src/greenlet/TGreenlet.cpp

@@ -168,6 +168,11 @@ Greenlet::g_switchstack(void)
         current->exception_state << tstate;
         this->python_state.will_switch_from(tstate);
         switching_thread_state = this;
+#if GREENLET_PY312
+        if (current->python_state.expose_frames_on_every_suspension) {
+            current->expose_frames();
+        }
+#endif
     }
     assert(this->args() || PyErr_Occurred());
     // If this is the first switch into a greenlet, this will
@@ -606,5 +611,103 @@ bool Greenlet::is_currently_running_in_some_thread() const
     return this->stack_state.active() && !this->python_state.top_frame();
 }
 
+#if GREENLET_PY312
+void GREENLET_NOINLINE(Greenlet::expose_frames)()
+{
+    if (!this->python_state.frame_exposure_needs_stack_rewrite()) {
+        return; // nothing to do
+    }
+
+    _PyInterpreterFrame* last_complete_iframe = nullptr;
+    _PyInterpreterFrame* iframe = this->python_state.top_frame()->f_frame;
+    while (iframe) {
+        // We must make a copy before looking at the iframe contents,
+        // since iframe might point to a portion of the greenlet's C stack
+        // that was spilled when switching greenlets.
+        _PyInterpreterFrame iframe_copy;
+        this->stack_state.copy_from_stack(&iframe_copy, iframe, sizeof(*iframe));
+        if (!_PyFrame_IsIncomplete(&iframe_copy)) {
+            // If the iframe were OWNED_BY_CSTACK then it would always be
+            // incomplete. Since it's not incomplete, it's not on the C stack
+            // and we can access it through the original `iframe` pointer
+            // directly.  This is important since GetFrameObject might
+            // lazily _create_ the frame object and we don't want the
+            // interpreter to lose track of it.
+            assert(iframe_copy.owner != FRAME_OWNED_BY_CSTACK);
+
+            // We really want to just write:
+            //     PyFrameObject* frame = _PyFrame_GetFrameObject(iframe);
+            // but _PyFrame_GetFrameObject calls _PyFrame_MakeAndSetFrameObject
+            // which is not a visible symbol in libpython. The easiest
+            // way to get a public function to call it is using
+            // PyFrame_GetBack, which is defined as follows:
+            //     assert(frame != NULL);
+            //     assert(!_PyFrame_IsIncomplete(frame->f_frame));
+            //     PyFrameObject *back = frame->f_back;
+            //     if (back == NULL) {
+            //         _PyInterpreterFrame *prev = frame->f_frame->previous;
+            //         prev = _PyFrame_GetFirstComplete(prev);
+            //         if (prev) {
+            //             back = _PyFrame_GetFrameObject(prev);
+            //         }
+            //     }
+            //     return (PyFrameObject*)Py_XNewRef(back);
+            if (!iframe->frame_obj) {
+                PyFrameObject dummy_frame;
+                _PyInterpreterFrame dummy_iframe;
+                dummy_frame.f_back = nullptr;
+                dummy_frame.f_frame = &dummy_iframe;
+                // force the iframe to be considered complete without
+                // needing to check its code object:
+                dummy_iframe.owner = FRAME_OWNED_BY_GENERATOR;
+                dummy_iframe.previous = iframe;
+                assert(!_PyFrame_IsIncomplete(&dummy_iframe));
+                // Drop the returned reference immediately; the iframe
+                // continues to hold a strong reference
+                Py_XDECREF(PyFrame_GetBack(&dummy_frame));
+                assert(iframe->frame_obj);
+            }
+
+            // This is a complete frame, so make the last one of those we saw
+            // point at it, bypassing any incomplete frames (which may have
+            // been on the C stack) in between the two. We're overwriting
+            // last_complete_iframe->previous and need that to be reversible,
+            // so we store the original previous ptr in the frame object
+            // (which we must have created on a previous iteration through
+            // this loop). The frame object has a bunch of storage that is
+            // only used when its iframe is OWNED_BY_FRAME_OBJECT, which only
+            // occurs when the frame object outlives the frame's execution,
+            // which can't have happened yet because the frame is currently
+            // executing as far as the interpreter is concerned. So, we can
+            // reuse it for our own purposes.
+            assert(iframe->owner == FRAME_OWNED_BY_THREAD ||
+                   iframe->owner == FRAME_OWNED_BY_GENERATOR);
+            if (last_complete_iframe) {
+                assert(last_complete_iframe->frame_obj);
+                memcpy(&last_complete_iframe->frame_obj->_f_frame_data[0],
+                       &last_complete_iframe->previous, sizeof(void *));
+                last_complete_iframe->previous = iframe;
+            }
+            last_complete_iframe = iframe;
+        }
+        // Frames that are OWNED_BY_FRAME_OBJECT are linked via the
+        // frame's f_back while all others are linked via the iframe's
+        // previous ptr. Since all the frames we traverse are running
+        // as far as the interpreter is concerned, we don't have to
+        // worry about the OWNED_BY_FRAME_OBJECT case.
+        iframe = iframe_copy.previous;
+    }
+
+    // Give the outermost complete iframe a null previous pointer to
+    // account for any potential incomplete/C-stack iframes between it
+    // and the actual top-of-stack
+    if (last_complete_iframe) {
+        assert(last_complete_iframe->frame_obj);
+        memcpy(&last_complete_iframe->frame_obj->_f_frame_data[0],
+               &last_complete_iframe->previous, sizeof(void *));
+        last_complete_iframe->previous = nullptr;
+    }
+}
+#endif
 
 }; // namespace greenlet

src/greenlet/TPythonState.cpp

@@ -26,7 +26,8 @@ PythonState::PythonState()
     ,datastack_limit(nullptr)
 #endif
 #if GREENLET_PY312
-    ,_prev_frame(nullptr)
+    ,frames_were_exposed(false)
+    ,expose_frames_on_every_suspension(false)
 #endif
 {
 #if GREENLET_USE_CFRAME
@@ -146,12 +147,6 @@ void PythonState::operator<<(const PyThreadState *const tstate) noexcept
     Py_XDECREF(frame);  // PyThreadState_GetFrame gives us a new
                         // reference.
     this->_top_frame.steal(frame);
-  #if GREENLET_PY312
-    if (frame) {
-        this->_prev_frame = frame->f_frame->previous;
-        frame->f_frame->previous = nullptr;
-    }
-  #endif
   #if GREENLET_PY312
     this->trash_delete_nesting = tstate->trash.delete_nesting;
   #else // not 312
@@ -164,6 +159,25 @@ void PythonState::operator<<(const PyThreadState *const tstate) noexcept
 #endif // GREENLET_PY311
 }
 
+#if GREENLET_PY312
+void GREENLET_NOINLINE(PythonState::unexpose_frames)()
+{
+    if (!this->frames_were_exposed) {
+        return;
+    }
+    // See GreenletState::expose_frames() and the comment on frames_were_exposed
+    // for more information about this logic.
+    for (_PyInterpreterFrame *iframe = this->_top_frame->f_frame;
+         iframe != nullptr; ) {
+        _PyInterpreterFrame *prev_exposed = iframe->previous;
+        assert(iframe->frame_obj);
+        memcpy(&iframe->previous, &iframe->frame_obj->_f_frame_data[0],
+               sizeof(void *));
+        iframe = prev_exposed;
+    }
+    this->frames_were_exposed = false;
+}
+#endif
 
 void PythonState::operator>>(PyThreadState *const tstate) noexcept
 {
@@ -187,13 +201,9 @@ void PythonState::operator>>(PyThreadState *const tstate) noexcept
   #if GREENLET_PY312
     tstate->py_recursion_remaining = tstate->py_recursion_limit - this->py_recursion_depth;
     tstate->c_recursion_remaining = C_RECURSION_LIMIT - this->c_recursion_depth;
-    // We're just going to throw this object away anyway, go ahead and
-    // do it now.
-    PyFrameObject* frame = this->_top_frame.relinquish_ownership();
-    if (frame && frame->f_frame) {
-      frame->f_frame->previous = this->_prev_frame;
+    if (this->frames_were_exposed) {
+        this->unexpose_frames();
     }
-    this->_prev_frame = nullptr;
   #else // \/ 3.11
     tstate->recursion_remaining = tstate->recursion_limit - this->recursion_depth;
   #endif // GREENLET_PY312

src/greenlet/TStackState.cpp

@@ -226,6 +226,37 @@ StackState::~StackState()
     }
 }
 
+void StackState::copy_from_stack(void* vdest, const void* vsrc, size_t n) const
+{
+    char* dest = static_cast<char*>(vdest);
+    const char* src = static_cast<const char*>(vsrc);
+    if (src + n <= _stack_start || src >= _stack_start + _stack_saved ||
+        _stack_saved == 0) {
+        // Nothing we're copying was spilled from the stack
+        memcpy(dest, src, n);
+        return;
+    }
+    if (src < _stack_start) {
+        // Copy the part before the saved stack.
+        // We know src + n > _stack_start due to the test above.
+        size_t nbefore = _stack_start - src;
+        memcpy(dest, src, nbefore);
+        dest += nbefore;
+        src += nbefore;
+        n -= nbefore;
+    }
+    // We know src >= _stack_start after the before-copy, and
+    // src < _stack_start + _stack_saved due to the first if condition
+    size_t nspilled = std::min<size_t>(n, _stack_start + _stack_saved - src);
+    memcpy(dest, stack_copy + (src - _stack_start), nspilled);
+    dest += nspilled;
+    src += nspilled;
+    n -= nspilled;
+    if (n > 0) {
+        // Copy the part after the saved stack
+        memcpy(dest, src, n);
+    }
+}
 
 }; // namespace greenlet
 

src/greenlet/__init__.py

@@ -25,7 +25,7 @@
 ###
 # Metadata
 ###
-__version__ = '3.0.3.dev0'
+__version__ = '3.1.0.dev0'
 from ._greenlet import _C_API # pylint:disable=no-name-in-module
 
 ###

src/greenlet/greenlet.cpp

@@ -758,10 +758,39 @@ green_setcontext(BorrowedGreenlet self, PyObject* nctx, void* UNUSED(context))
 static PyObject*
 green_getframe(BorrowedGreenlet self, void* UNUSED(context))
 {
+#if GREENLET_PY312
+    self->expose_frames();
+#endif
     const PythonState::OwnedFrame& top_frame = self->top_frame();
     return top_frame.acquire_or_None();
 }
 
+static PyObject*
+green_getframeexposed(BorrowedGreenlet self, void* UNUSED(context))
+{
+#if GREENLET_PY312
+    if (!self->expose_frames_on_every_suspension()) {
+        Py_RETURN_FALSE;
+    }
+#endif
+    Py_RETURN_TRUE;
+}
+
+static int
+green_setframeexposed(BorrowedGreenlet self, PyObject* val, void* UNUSED(context))
+{
+    if (val != Py_True && val != Py_False) {
+        PyErr_Format(PyExc_TypeError,
+                     "expected a bool, not '%s'",
+                     Py_TYPE(val)->tp_name);
+        return -1;
+    }
+#if GREENLET_PY312
+    self->set_expose_frames_on_every_suspension(val == Py_True);
+#endif
+    return 0;
+}
+
 static PyObject*
 green_getstate(PyGreenlet* self)
 {
@@ -979,6 +1008,10 @@ static PyGetSetDef green_getsets[] = {
     {"run", (getter)green_getrun, (setter)green_setrun, /*XXX*/ NULL},
     {"parent", (getter)green_getparent, (setter)green_setparent, /*XXX*/ NULL},
     {"gr_frame", (getter)green_getframe, NULL, /*XXX*/ NULL},
+    {"gr_frames_always_exposed",
+     (getter)green_getframeexposed,
+     (setter)green_setframeexposed,
+     /*XXX*/ NULL},
     {"gr_context",
      (getter)green_getcontext,
      (setter)green_setcontext,