Clean up initialization/creation of greenlets.

CHANGES.rst

@@ -5,7 +5,8 @@
 3.1.1 (unreleased)
 ==================
 
-- Fix crashes on 32-bit PPC Linux.
+- Fix crashes on 32-bit PPC Linux. Note that there is no CI for this,
+  and support is best effort; there may be other issues lurking.
   See `issue 422
   <https://github.com/python-greenlet/greenlet/issues/422>`_.
 - Remove unnecessary logging sometimes during interpreter shutdown.

src/greenlet/PyGreenlet.cpp

@@ -48,22 +48,6 @@ using greenlet::BrokenGreenlet;
 using greenlet::ThreadState;
 using greenlet::PythonState;
 
-static PyGreenlet*
-green_create_main(ThreadState* state)
-{
-    PyGreenlet* gmain;
-
-    /* create the main greenlet for this thread */
-    gmain = (PyGreenlet*)PyType_GenericAlloc(&PyGreenlet_Type, 0);
-    if (gmain == NULL) {
-        Py_FatalError("green_create_main failed to alloc");
-        return NULL;
-    }
-    new MainGreenlet(gmain, state);
-
-    assert(Py_REFCNT(gmain) == 1);
-    return gmain;
-}
 
 
 static PyGreenlet*

src/greenlet/PyGreenlet.hpp

@@ -15,7 +15,6 @@ using greenlet::refs::PyErrPieces;
 
 
 // XXX: These doesn't really belong here, it's not a Python slot.
-static PyGreenlet* green_create_main(greenlet::ThreadState* state);
 static OwnedObject internal_green_throw(BorrowedGreenlet self, PyErrPieces& err_pieces);
 
 static PyGreenlet* green_new(PyTypeObject* type, PyObject* UNUSED(args), PyObject* UNUSED(kwds));

src/greenlet/TGreenlet.cpp

@@ -21,23 +21,23 @@
 namespace greenlet {
 
 Greenlet::Greenlet(PyGreenlet* p)
+    :  Greenlet(p, StackState())
 {
-    p ->pimpl = this;
+}
+
+Greenlet::Greenlet(PyGreenlet* p, const StackState& initial_stack)
+    :  _self(p), stack_state(initial_stack)
+{
+    assert(p->pimpl == nullptr);
+    p->pimpl = this;
 }
 
 Greenlet::~Greenlet()
 {
     // XXX: Can't do this. tp_clear is a virtual function, and by the
     // time we're here, we've sliced off our child classes.
     //this->tp_clear();
-}
-
-Greenlet::Greenlet(PyGreenlet* p, const StackState& initial_stack)
-    : stack_state(initial_stack)
-{
-    // can't use a delegating constructor because of
-    // MSVC for Python 2.7
-    p->pimpl = this;
+    this->_self->pimpl = nullptr;
 }
 
 bool

src/greenlet/TGreenlet.hpp

@@ -319,6 +319,7 @@ namespace greenlet
     {
     private:
         G_NO_COPIES_OF_CLS(Greenlet);
+        PyGreenlet* const _self;
     private:
         // XXX: Work to remove these.
         friend class ThreadState;
@@ -331,6 +332,8 @@ namespace greenlet
         PythonState python_state;
         Greenlet(PyGreenlet* p, const StackState& initial_state);
     public:
+        // This constructor takes ownership of the PyGreenlet, by
+        // setting ``p->pimpl = this;``.
         Greenlet(PyGreenlet* p);
         virtual ~Greenlet();
 
@@ -461,7 +464,10 @@ namespace greenlet
 
         // Return a borrowed greenlet that is the Python object
         // this object represents.
-        virtual BorrowedGreenlet self() const noexcept = 0;
+        inline BorrowedGreenlet self() const noexcept
+        {
+            return BorrowedGreenlet(this->_self);
+        }
 
         // For testing. If this returns true, we should pretend that
         // slp_switch() failed.
@@ -645,7 +651,6 @@ class TracingGuard
     {
     private:
         static greenlet::PythonAllocator<UserGreenlet> allocator;
-        BorrowedGreenlet _self;
         OwnedMainGreenlet _main_greenlet;
         OwnedObject _run_callable;
         OwnedGreenlet _parent;
@@ -674,7 +679,6 @@ class TracingGuard
 
         virtual const refs::BorrowedMainGreenlet main_greenlet() const;
 
-        virtual BorrowedGreenlet self() const noexcept;
         virtual void murder_in_place();
         virtual bool belongs_to_thread(const ThreadState* state) const;
         virtual int tp_traverse(visitproc visit, void* arg);
@@ -748,7 +752,6 @@ class TracingGuard
         virtual ThreadState* thread_state() const noexcept;
         void thread_state(ThreadState*) noexcept;
         virtual OwnedObject g_switch();
-        virtual BorrowedGreenlet self() const noexcept;
         virtual int tp_traverse(visitproc visit, void* arg);
     };
 

src/greenlet/TMainGreenlet.cpp

@@ -63,12 +63,6 @@ MainGreenlet::thread_state(ThreadState* t) noexcept
     this->_thread_state = t;
 }
 
-BorrowedGreenlet
-MainGreenlet::self() const noexcept
-{
-    return BorrowedGreenlet(this->_self.borrow());
-}
-
 
 const BorrowedMainGreenlet
 MainGreenlet::main_greenlet() const

src/greenlet/TUserGreenlet.cpp

@@ -38,7 +38,6 @@ void UserGreenlet::operator delete(void* ptr)
 UserGreenlet::UserGreenlet(PyGreenlet* p, BorrowedGreenlet the_parent)
     : Greenlet(p), _parent(the_parent)
 {
-    this->_self = p;
 }
 
 UserGreenlet::~UserGreenlet()
@@ -50,13 +49,6 @@ UserGreenlet::~UserGreenlet()
     this->tp_clear();
 }
 
-BorrowedGreenlet
-UserGreenlet::self() const noexcept
-{
-    return this->_self;
-}
-
-
 
 const BorrowedMainGreenlet
 UserGreenlet::main_greenlet() const
@@ -240,7 +232,7 @@ UserGreenlet::g_initialstub(void* mark)
           self.run is the object to call in the new greenlet.
           This could run arbitrary python code and switch greenlets!
         */
-        run = this->_self.PyRequireAttr(mod_globs->str_run);
+        run = this->self().PyRequireAttr(mod_globs->str_run);
         /* restore saved exception */
         saved.PyErrRestore();
 
@@ -600,7 +592,7 @@ UserGreenlet::parent(const BorrowedObject raw_new_parent)
                                                           // throw
                                                           // TypeError!
     for (BorrowedGreenlet p = new_parent; p; p = p->parent()) {
-        if (p == this->_self) {
+        if (p == this->self()) {
             throw ValueError("cyclic parent chain");
         }
         main_greenlet_of_new_parent = p->main_greenlet();

src/greenlet/greenlet_internal.hpp

@@ -52,7 +52,7 @@ greenlet::refs::MainGreenletExactChecker(void *p)
     // Greenlets from dead threads no longer respond to main() with a
     // true value; so in that case we need to perform an additional
     // check.
-    Greenlet* g = ((PyGreenlet*)p)->pimpl;
+    Greenlet* g = static_cast<PyGreenlet*>(p)->pimpl;
     if (g->main()) {
         return;
     }
@@ -88,7 +88,6 @@ extern PyTypeObject PyGreenlet_Type;
 /**
   * Forward declarations needed in multiple files.
   */
-static PyGreenlet* green_create_main(greenlet::ThreadState*);
 static PyObject* green_switch(PyGreenlet* self, PyObject* args, PyObject* kwargs);
 static int green_is_gc(BorrowedGreenlet self);
 

src/greenlet/greenlet_thread_state.hpp

@@ -124,6 +124,27 @@ class ThreadState {
 
     G_NO_COPIES_OF_CLS(ThreadState);
 
+
+    // Allocates a main greenlet for the thread state. If this fails,
+    // exits the process. Called only during constructing a ThreadState.
+    MainGreenlet* alloc_main()
+    {
+        PyGreenlet* gmain;
+
+        /* create the main greenlet for this thread */
+        gmain = reinterpret_cast<PyGreenlet*>(PyType_GenericAlloc(&PyGreenlet_Type, 0));
+        if (gmain == NULL) {
+            throw PyFatalError("alloc_main failed to alloc"); //exits the process
+        }
+
+        MainGreenlet* const main = new MainGreenlet(gmain, this);
+
+        assert(Py_REFCNT(gmain) == 1);
+        assert(gmain->pimpl == main);
+        return main;
+    }
+
+
 public:
     static void* operator new(size_t UNUSED(count))
     {
@@ -143,20 +164,23 @@ class ThreadState {
     }
 
     ThreadState()
-        : main_greenlet(OwnedMainGreenlet::consuming(green_create_main(this))),
-          current_greenlet(main_greenlet)
     {
-        if (!this->main_greenlet) {
-            // We failed to create the main greenlet. That's bad.
-            throw PyFatalError("Failed to create main greenlet");
-        }
-        // The main greenlet starts with 1 refs: The returned one. We
-        // then copied it to the current greenlet.
-        assert(this->main_greenlet.REFCNT() == 2);
 
 #ifdef GREENLET_NEEDS_EXCEPTION_STATE_SAVED
         this->exception_state = slp_get_exception_state();
 #endif
+
+        // XXX: Potentially dangerous, exposing a not fully
+        // constructed object.
+        MainGreenlet* const main = this->alloc_main();
+        this->main_greenlet = OwnedMainGreenlet::consuming(
+            main->self()
+        );
+        assert(this->main_greenlet);
+        this->current_greenlet = main->self();
+        // The main greenlet starts with 1 refs: The returned one. We
+        // then copied it to the current greenlet.
+        assert(this->main_greenlet.REFCNT() == 2);
     }
 
     inline void restore_exception_state()