From 9216e637731fed0920f16787296079c7e70d06ca Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 16 Sep 2024 12:48:36 -0500 Subject: [PATCH 1/8] Clean up initialization/creation of greenlets. --- CHANGES.rst | 3 +- src/greenlet/PyGreenlet.cpp | 16 ---------- src/greenlet/PyGreenlet.hpp | 1 - src/greenlet/TGreenlet.cpp | 18 +++++------ src/greenlet/TGreenlet.hpp | 11 ++++--- src/greenlet/TMainGreenlet.cpp | 6 ---- src/greenlet/TUserGreenlet.cpp | 12 ++------ src/greenlet/greenlet_internal.hpp | 3 +- src/greenlet/greenlet_thread_state.hpp | 42 ++++++++++++++++++++------ 9 files changed, 54 insertions(+), 58 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 99c45c61..8c24a4e1 100644 --- a/CHANGES.rst +++ b/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 `_. - Remove unnecessary logging sometimes during interpreter shutdown. diff --git a/src/greenlet/PyGreenlet.cpp b/src/greenlet/PyGreenlet.cpp index b80bdc65..147a2312 100644 --- a/src/greenlet/PyGreenlet.cpp +++ b/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* diff --git a/src/greenlet/PyGreenlet.hpp b/src/greenlet/PyGreenlet.hpp index 67169532..d56f339f 100644 --- a/src/greenlet/PyGreenlet.hpp +++ b/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)); diff --git a/src/greenlet/TGreenlet.cpp b/src/greenlet/TGreenlet.cpp index ee7ee868..a9e56b6e 100644 --- a/src/greenlet/TGreenlet.cpp +++ b/src/greenlet/TGreenlet.cpp @@ -21,8 +21,15 @@ 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() @@ -30,14 +37,7 @@ 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 diff --git a/src/greenlet/TGreenlet.hpp b/src/greenlet/TGreenlet.hpp index fbfdfbfc..1250d0e1 100644 --- a/src/greenlet/TGreenlet.hpp +++ b/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 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); }; diff --git a/src/greenlet/TMainGreenlet.cpp b/src/greenlet/TMainGreenlet.cpp index cbc7c30e..25f33ddc 100644 --- a/src/greenlet/TMainGreenlet.cpp +++ b/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 diff --git a/src/greenlet/TUserGreenlet.cpp b/src/greenlet/TUserGreenlet.cpp index ef141b57..02bf7b7f 100644 --- a/src/greenlet/TUserGreenlet.cpp +++ b/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(); diff --git a/src/greenlet/greenlet_internal.hpp b/src/greenlet/greenlet_internal.hpp index 8c96eb2a..c6897db3 100644 --- a/src/greenlet/greenlet_internal.hpp +++ b/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(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); diff --git a/src/greenlet/greenlet_thread_state.hpp b/src/greenlet/greenlet_thread_state.hpp index 045371f8..9e94f267 100644 --- a/src/greenlet/greenlet_thread_state.hpp +++ b/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(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() From 85b39df2b5e15732e6b3c5265776fecbf817476b Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 16 Sep 2024 13:24:51 -0500 Subject: [PATCH 2/8] Remove src/greenlet/greenlet_thread_state_dict_cleanup.hpp. --- src/greenlet/greenlet_thread_state.hpp | 18 ++- .../greenlet_thread_state_dict_cleanup.hpp | 118 ------------------ 2 files changed, 15 insertions(+), 121 deletions(-) delete mode 100644 src/greenlet/greenlet_thread_state_dict_cleanup.hpp diff --git a/src/greenlet/greenlet_thread_state.hpp b/src/greenlet/greenlet_thread_state.hpp index 9e94f267..1d3765cb 100644 --- a/src/greenlet/greenlet_thread_state.hpp +++ b/src/greenlet/greenlet_thread_state.hpp @@ -496,9 +496,21 @@ class ThreadStateCreator // Set to 0 on destruction. ThreadState* _state; G_NO_COPIES_OF_CLS(ThreadStateCreator); + + inline bool has_initialized_state() const + { + return this->_state != (ThreadState*)1; + } + + inline bool has_state() const + { + return this->has_initialized_state() && this->_state != nullptr; + } + public: - // Only one of these, auto created per thread + // Only one of these, auto created per thread. + // Constructing the state constructs the MainGreenlet. ThreadStateCreator() : _state((ThreadState*)1) { @@ -522,7 +534,7 @@ class ThreadStateCreator // access the pointer from the main greenlet. Deleting the // thread, and hence the thread-local storage, will delete the // state pointer in the main greenlet. - if (this->_state == (ThreadState*)1) { + if (!this->has_initialized_state()) { // XXX: Assuming allocation never fails this->_state = new ThreadState; // For non-standard threading, we need to store an object @@ -548,7 +560,7 @@ class ThreadStateCreator inline int tp_traverse(visitproc visit, void* arg) { - if (this->_state) { + if (this->has_state()) { return this->_state->tp_traverse(visit, arg); } return 0; diff --git a/src/greenlet/greenlet_thread_state_dict_cleanup.hpp b/src/greenlet/greenlet_thread_state_dict_cleanup.hpp deleted file mode 100644 index acf39c8f..00000000 --- a/src/greenlet/greenlet_thread_state_dict_cleanup.hpp +++ /dev/null @@ -1,118 +0,0 @@ -#ifndef GREENLET_THREAD_STATE_DICT_CLEANUP_HPP -#define GREENLET_THREAD_STATE_DICT_CLEANUP_HPP - -#include "greenlet_internal.hpp" -#include "greenlet_thread_state.hpp" - -#ifdef __clang__ -# pragma clang diagnostic push -# pragma clang diagnostic ignored "-Wmissing-field-initializers" -#endif - -#ifndef G_THREAD_STATE_DICT_CLEANUP_TYPE -// shut the compiler up if it looks at this file in isolation -#define ThreadStateCreator int -#endif - -// Define a Python object that goes in the Python thread state dict -// when the greenlet thread state is created, and which owns the -// reference to the greenlet thread local state. -// When the thread state dict is cleaned up, so too is the thread -// state. This works best if we make sure there are no circular -// references to the thread state. -typedef struct _PyGreenletCleanup { - PyObject_HEAD - ThreadStateCreator* thread_state_creator; -} PyGreenletCleanup; - -static void -cleanup_do_dealloc(PyGreenletCleanup* self) -{ - ThreadStateCreator* tmp = self->thread_state_creator; - self->thread_state_creator = nullptr; - if (tmp) { - delete tmp; - } -} - -static void -cleanup_dealloc(PyGreenletCleanup* self) -{ - PyObject_GC_UnTrack(self); - cleanup_do_dealloc(self); -} - -static int -cleanup_clear(PyGreenletCleanup* self) -{ - // This method is never called by our test cases. - cleanup_do_dealloc(self); - return 0; -} - -static int -cleanup_traverse(PyGreenletCleanup* self, visitproc visit, void* arg) -{ - if (self->thread_state_creator) { - return self->thread_state_creator->tp_traverse(visit, arg); - } - return 0; -} - -static int -cleanup_is_gc(PyGreenlet* UNUSED(self)) -{ - return 1; -} - -static PyTypeObject PyGreenletCleanup_Type = { - PyVarObject_HEAD_INIT(NULL, 0) - "greenlet._greenlet.ThreadStateCleanup", - sizeof(struct _PyGreenletCleanup), - 0, /* tp_itemsize */ - /* methods */ - (destructor)cleanup_dealloc, /* tp_dealloc */ - 0, /* tp_print */ - 0, /* tp_getattr */ - 0, /* tp_setattr */ - 0, /* tp_compare */ - 0, /* tp_repr */ - 0, /* tp_as _number*/ - 0, /* tp_as _sequence*/ - 0, /* tp_as _mapping*/ - 0, /* tp_hash */ - 0, /* tp_call */ - 0, /* tp_str */ - 0, /* tp_getattro */ - 0, /* tp_setattro */ - 0, /* tp_as_buffer*/ - G_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */ - "Internal use only", /* tp_doc */ - (traverseproc)cleanup_traverse, /* tp_traverse */ - (inquiry)cleanup_clear, /* tp_clear */ - 0, /* tp_richcompare */ - // XXX: Don't our flags promise a weakref? - 0, /* tp_weaklistoffset */ - 0, /* tp_iter */ - 0, /* tp_iternext */ - 0, /* tp_methods */ - 0, /* tp_members */ - 0, /* tp_getset */ - 0, /* tp_base */ - 0, /* tp_dict */ - 0, /* tp_descr_get */ - 0, /* tp_descr_set */ - 0, /* tp_dictoffset */ - 0, /* tp_init */ - PyType_GenericAlloc, /* tp_alloc */ - PyType_GenericNew, /* tp_new */ - PyObject_GC_Del, /* tp_free */ - (inquiry)cleanup_is_gc, /* tp_is_gc */ -}; - -#ifdef __clang__ -# pragma clang diagnostic pop -#endif - - -#endif From e6ad75932caf34a35e03ab38332ccafa17c7d389 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Wed, 18 Sep 2024 09:58:50 -0500 Subject: [PATCH 3/8] Fix warnings on GCC; simplify some more things. --- .github/workflows/tests.yml | 6 ++-- make-manylinux | 2 +- setup.py | 2 +- src/greenlet/TGreenlet.hpp | 2 +- src/greenlet/TUserGreenlet.cpp | 2 +- src/greenlet/greenlet_compiler_compat.hpp | 8 ----- src/greenlet/greenlet_internal.hpp | 4 +-- src/greenlet/greenlet_refs.hpp | 41 ++++++++++++++--------- src/greenlet/greenlet_thread_state.hpp | 4 +-- 9 files changed, 37 insertions(+), 34 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 06025880..db238e5b 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -66,7 +66,8 @@ jobs: # thus only use O3 (because Ofast enables fast-math, which has # process-wide effects), we test with Ofast here, because we # expect that some people will compile it themselves with that setting. - CPPFLAGS: "-Ofast -UNDEBUG" + CPPFLAGS: "-Ofast -UNDEBUG -Wall" + CFLAGS: "-Ofast -UNDEBUG -Wall" - name: Install greenlet (Mac) if: startsWith(runner.os, 'Mac') run: | @@ -84,7 +85,8 @@ jobs: env: # Unlike the above, we are actually distributing these # wheels, so they need to be built for production use. - CPPFLAGS: "-O3" + CPPFLAGS: "-O3 -flto -ffunction-sections" + CFLAGS: "-O3 -flto -ffunction-sections" # Build for both architectures ARCHFLAGS: "-arch x86_64 -arch arm64" diff --git a/make-manylinux b/make-manylinux index 20b74637..787a98b1 100755 --- a/make-manylinux +++ b/make-manylinux @@ -22,7 +22,7 @@ if [ -d /greenlet -a -d /opt/python ]; then # Note: -Ofast includes -ffast-math which affects process-wide floating-point flags (e.g. can affect numpy). # It may also violate standards compliance in a few ways. Rather than opt-out with -fno-fast-math, # we use O3, which has neither of those problems. - export CFLAGS="-O3 -DNDEBUG" + export CFLAGS="-O3 -DNDEBUG -Wall" # Build in an isolated directory mkdir /tmp/build cd /tmp/build diff --git a/setup.py b/setup.py index 4fc0cce5..f3d51f00 100755 --- a/setup.py +++ b/setup.py @@ -64,7 +64,7 @@ if sys.platform == 'darwin' or 'clang' in plat_compiler: # The clang compiler doesn't use --std=c++11 by default - cpp_compile_args.append("--std=gnu++11") + cpp_compile_args.append("--std=c++11") elif is_win and "MSC" in plat_compiler: # Older versions of MSVC (Python 2.7) don't handle C++ exceptions # correctly by default. While newer versions do handle exceptions diff --git a/src/greenlet/TGreenlet.hpp b/src/greenlet/TGreenlet.hpp index 1250d0e1..512f7fb3 100644 --- a/src/greenlet/TGreenlet.hpp +++ b/src/greenlet/TGreenlet.hpp @@ -152,7 +152,7 @@ namespace greenlet void set_new_cframe(_PyCFrame& frame) noexcept; #endif - inline void may_switch_away() noexcept; + void may_switch_away() noexcept; inline void will_switch_from(PyThreadState *const origin_tstate) noexcept; void did_finish(PyThreadState* tstate) noexcept; }; diff --git a/src/greenlet/TUserGreenlet.cpp b/src/greenlet/TUserGreenlet.cpp index 02bf7b7f..83db972f 100644 --- a/src/greenlet/TUserGreenlet.cpp +++ b/src/greenlet/TUserGreenlet.cpp @@ -388,7 +388,7 @@ UserGreenlet::inner_bootstrap(PyGreenlet* origin_greenlet, PyObject* run) //PyObject* run = _run.relinquish_ownership(); /* in the new greenlet */ - assert(this->thread_state()->borrow_current() == this->_self); + assert(this->thread_state()->borrow_current() == BorrowedGreenlet(this->_self)); // C++ exceptions cannot propagate to the parent greenlet from // here. (TODO: Do we need a catch(...) clause, perhaps on the // function itself? ALl we could do is terminate the program.) diff --git a/src/greenlet/greenlet_compiler_compat.hpp b/src/greenlet/greenlet_compiler_compat.hpp index ee5bbdd2..6fe892f4 100644 --- a/src/greenlet/greenlet_compiler_compat.hpp +++ b/src/greenlet/greenlet_compiler_compat.hpp @@ -38,14 +38,6 @@ #include -# if defined(__clang__) -# define G_FP_TMPL_STATIC static -# else -// GCC has no problem allowing static function pointers, but emits -// tons of warnings about "whose type uses the anonymous namespace [-Wsubobject-linkage]" -# define G_FP_TMPL_STATIC -# endif - # define G_NO_COPIES_OF_CLS(Cls) private: \ Cls(const Cls& other) = delete; \ Cls& operator=(const Cls& other) = delete diff --git a/src/greenlet/greenlet_internal.hpp b/src/greenlet/greenlet_internal.hpp index c6897db3..6d79cc2a 100644 --- a/src/greenlet/greenlet_internal.hpp +++ b/src/greenlet/greenlet_internal.hpp @@ -36,7 +36,7 @@ namespace greenlet { #include "greenlet.h" -G_FP_TMPL_STATIC inline void +void greenlet::refs::MainGreenletExactChecker(void *p) { if (!p) { @@ -89,7 +89,7 @@ extern PyTypeObject PyGreenlet_Type; * Forward declarations needed in multiple files. */ static PyObject* green_switch(PyGreenlet* self, PyObject* args, PyObject* kwargs); -static int green_is_gc(BorrowedGreenlet self); + #ifdef __clang__ # pragma clang diagnostic pop diff --git a/src/greenlet/greenlet_refs.hpp b/src/greenlet/greenlet_refs.hpp index e0850c4b..b7e5e3f2 100644 --- a/src/greenlet/greenlet_refs.hpp +++ b/src/greenlet/greenlet_refs.hpp @@ -3,6 +3,9 @@ #define PY_SSIZE_T_CLEAN #include + +#include + //#include "greenlet_internal.hpp" #include "greenlet_compiler_compat.hpp" #include "greenlet_cpython_compat.hpp" @@ -34,13 +37,13 @@ namespace greenlet // implemented as macros.) typedef void (*TypeChecker)(void*); - G_FP_TMPL_STATIC inline void + void NoOpChecker(void*) { return; } - G_FP_TMPL_STATIC inline void + void GreenletChecker(void *p) { if (!p) { @@ -63,7 +66,7 @@ namespace greenlet } } - G_FP_TMPL_STATIC inline void + void MainGreenletExactChecker(void *p); template @@ -93,7 +96,7 @@ namespace greenlet typedef _BorrowedGreenlet BorrowedGreenlet; - G_FP_TMPL_STATIC inline void + void ContextExactChecker(void *p) { if (!p) { @@ -171,7 +174,7 @@ namespace greenlet { protected: T* p; public: - explicit PyObjectPointer(T* it=nullptr) : p(it) + PyObjectPointer(T* it=nullptr) : p(it) { TC(p); } @@ -186,7 +189,7 @@ namespace greenlet { // TODO: This should probably not exist here, but be moved // down to relevant sub-types. - inline T* borrow() const noexcept + T* borrow() const noexcept { return this->p; } @@ -196,7 +199,7 @@ namespace greenlet { return reinterpret_cast(this->p); } - inline T* operator->() const noexcept + T* operator->() const noexcept { return this->p; } @@ -206,7 +209,7 @@ namespace greenlet { return this->p == Py_None; } - inline PyObject* acquire_or_None() const noexcept + PyObject* acquire_or_None() const noexcept { PyObject* result = this->p ? reinterpret_cast(this->p) : Py_None; Py_INCREF(result); @@ -215,15 +218,20 @@ namespace greenlet { explicit operator bool() const noexcept { - return p != nullptr; + return this->p != nullptr; + } + + bool operator!() const noexcept + { + return this->p == nullptr; } - inline Py_ssize_t REFCNT() const noexcept + Py_ssize_t REFCNT() const noexcept { return p ? Py_REFCNT(p) : -42; } - inline PyTypeObject* TYPE() const noexcept + PyTypeObject* TYPE() const noexcept { return p ? Py_TYPE(p) : nullptr; } @@ -270,9 +278,9 @@ namespace greenlet { #endif template - inline bool operator==(const PyObjectPointer& lhs, const void* const rhs) noexcept + inline bool operator==(const PyObjectPointer& lhs, const PyObject* const rhs) noexcept { - return lhs.borrow_o() == rhs; + return static_cast(lhs.borrow_o()) == static_cast(rhs); } template @@ -422,6 +430,7 @@ namespace greenlet { target = o.relinquish_ownership(); } + class NewReference : public OwnedObject { private: @@ -572,8 +581,8 @@ namespace greenlet { { return reinterpret_cast(this->p); } - inline Greenlet* operator->() const noexcept; - inline operator Greenlet*() const noexcept; + Greenlet* operator->() const noexcept; + operator Greenlet*() const noexcept; }; typedef _BorrowedGreenlet BorrowedGreenlet; @@ -789,7 +798,7 @@ namespace greenlet { return OwnedObject::consuming(PyObject_Call(this->p, args.borrow(), kwargs.borrow())); } - G_FP_TMPL_STATIC inline void + inline void ListChecker(void * p) { if (!p) { diff --git a/src/greenlet/greenlet_thread_state.hpp b/src/greenlet/greenlet_thread_state.hpp index 1d3765cb..39cc0d00 100644 --- a/src/greenlet/greenlet_thread_state.hpp +++ b/src/greenlet/greenlet_thread_state.hpp @@ -192,9 +192,9 @@ class ThreadState { #endif } - inline bool has_main_greenlet() + inline bool has_main_greenlet() const noexcept { - return !!this->main_greenlet; + return bool(this->main_greenlet); } // Called from the ThreadStateCreator when we're in non-standard From 5144f7070cd7882f643f6b16a3dbc9138dbbf483 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Wed, 18 Sep 2024 10:12:20 -0500 Subject: [PATCH 4/8] Sigh. Pip hides compiler output which is, you know, important, and the only way to get it back is to ask for verbose output, which provides a bunch of junk we don't care about. But what's the point of warnings if we can't see them? So ask for all the junk. --- .github/workflows/tests.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index db238e5b..37cbbf93 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -58,7 +58,13 @@ jobs: - name: Install greenlet (non-Mac) if: ${{ ! startsWith(runner.os, 'Mac') }} run: | - python -m pip wheel --wheel-dir ./dist . + # Stupid setuptools doesn't want you running 'python setup.py' anymore, + # but stupid pip hides all the intersting compiler output by default, and the + # only way to get anything useful out is to ask *everything* to be verbose, + # which is much more junk than we need to wade through, making it hard to + # see what we want. What's the point of having warnings at all if we can't + # see them, though? + python -m pip wheel -v --wheel-dir ./dist . python -m pip install -U -e ".[test,docs]" env: # Ensure we test with assertions enabled. @@ -71,7 +77,7 @@ jobs: - name: Install greenlet (Mac) if: startsWith(runner.os, 'Mac') run: | - python -m pip wheel --wheel-dir ./dist . + python -m pip wheel -v --wheel-dir ./dist . python -m pip install -U -e ".[test,docs]" ls -l dist # Something in the build system isn't detecting that we're building for both, From 358a2e8f20816a68e65703b5488dd0337722a9a9 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Wed, 18 Sep 2024 12:36:55 -0500 Subject: [PATCH 5/8] Keep greenlet_thread_state.hpp --- ... => greenlet_thread_state.hpp-move-to-TThreadStateCreator.hpp} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/greenlet/{greenlet_thread_state.hpp => greenlet_thread_state.hpp-move-to-TThreadStateCreator.hpp} (100%) diff --git a/src/greenlet/greenlet_thread_state.hpp b/src/greenlet/greenlet_thread_state.hpp-move-to-TThreadStateCreator.hpp similarity index 100% rename from src/greenlet/greenlet_thread_state.hpp rename to src/greenlet/greenlet_thread_state.hpp-move-to-TThreadStateCreator.hpp From 64e0b4f6ceaa5cc2debafe551caea1276aa84aa9 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Wed, 18 Sep 2024 12:36:55 -0500 Subject: [PATCH 6/8] Copy greenlet_thread_state.hpp into TThreadStateCreator.hpp --- .../{greenlet_thread_state.hpp => TThreadStateCreator.hpp} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/greenlet/{greenlet_thread_state.hpp => TThreadStateCreator.hpp} (100%) diff --git a/src/greenlet/greenlet_thread_state.hpp b/src/greenlet/TThreadStateCreator.hpp similarity index 100% rename from src/greenlet/greenlet_thread_state.hpp rename to src/greenlet/TThreadStateCreator.hpp From 9e8a90b1a47e1254df65057af444671e22ca61e0 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Wed, 18 Sep 2024 12:36:55 -0500 Subject: [PATCH 7/8] Set back greenlet_thread_state.hpp file --- ...-move-to-TThreadStateCreator.hpp => greenlet_thread_state.hpp} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/greenlet/{greenlet_thread_state.hpp-move-to-TThreadStateCreator.hpp => greenlet_thread_state.hpp} (100%) diff --git a/src/greenlet/greenlet_thread_state.hpp-move-to-TThreadStateCreator.hpp b/src/greenlet/greenlet_thread_state.hpp similarity index 100% rename from src/greenlet/greenlet_thread_state.hpp-move-to-TThreadStateCreator.hpp rename to src/greenlet/greenlet_thread_state.hpp From dbf311a021d80aecb38033a9f44fcf13be9e7a6f Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Wed, 18 Sep 2024 13:52:19 -0500 Subject: [PATCH 8/8] Greater safety and fewer assumptions doing cross-thread cleanup. --- make-manylinux | 2 +- src/greenlet/CObjects.cpp | 2 +- src/greenlet/PyGreenlet.cpp | 2 +- src/greenlet/PyGreenlet.hpp | 2 +- src/greenlet/PyGreenletUnswitchable.cpp | 2 +- src/greenlet/PyModule.cpp | 2 +- src/greenlet/TGreenlet.cpp | 2 +- src/greenlet/TGreenletGlobals.cpp | 2 +- src/greenlet/TMainGreenlet.cpp | 2 +- ...nlet_thread_state.hpp => TThreadState.hpp} | 86 +-- src/greenlet/TThreadStateCreator.hpp | 499 +----------------- src/greenlet/TThreadStateDestroy.cpp | 212 +++++--- src/greenlet/TUserGreenlet.cpp | 2 +- src/greenlet/greenlet.cpp | 8 +- src/greenlet/greenlet_internal.hpp | 4 + 15 files changed, 158 insertions(+), 671 deletions(-) rename src/greenlet/{greenlet_thread_state.hpp => TThreadState.hpp} (87%) diff --git a/make-manylinux b/make-manylinux index 787a98b1..677e4898 100755 --- a/make-manylinux +++ b/make-manylinux @@ -45,7 +45,7 @@ if [ -d /greenlet -a -d /opt/python ]; then python -mpip install -U pip python -mpip install -U setuptools wheel - python -mpip wheel --wheel-dir ./dist . + python -mpip wheel -v --wheel-dir ./dist . python -mpip install -U .[test] python -m unittest discover -v greenlet.tests PATH="$OPATH" auditwheel repair dist/greenlet*.whl diff --git a/src/greenlet/CObjects.cpp b/src/greenlet/CObjects.cpp index a2bce441..c135995b 100644 --- a/src/greenlet/CObjects.cpp +++ b/src/greenlet/CObjects.cpp @@ -15,7 +15,7 @@ #include "greenlet_internal.hpp" #include "greenlet_refs.hpp" -#include "greenlet_thread_state.hpp" + #include "TThreadStateDestroy.cpp" #include "PyGreenlet.hpp" diff --git a/src/greenlet/PyGreenlet.cpp b/src/greenlet/PyGreenlet.cpp index 147a2312..29c0bba0 100644 --- a/src/greenlet/PyGreenlet.cpp +++ b/src/greenlet/PyGreenlet.cpp @@ -20,7 +20,7 @@ The Python slot functions for TGreenlet. #include "greenlet_refs.hpp" #include "greenlet_slp_switch.hpp" -#include "greenlet_thread_state.hpp" + #include "greenlet_thread_support.hpp" #include "TGreenlet.hpp" diff --git a/src/greenlet/PyGreenlet.hpp b/src/greenlet/PyGreenlet.hpp index d56f339f..df6cd805 100644 --- a/src/greenlet/PyGreenlet.hpp +++ b/src/greenlet/PyGreenlet.hpp @@ -5,7 +5,7 @@ #include "greenlet.h" #include "greenlet_compiler_compat.hpp" #include "greenlet_refs.hpp" -#include "greenlet_thread_state.hpp" + using greenlet::refs::OwnedGreenlet; using greenlet::refs::BorrowedGreenlet; diff --git a/src/greenlet/PyGreenletUnswitchable.cpp b/src/greenlet/PyGreenletUnswitchable.cpp index cf14b02a..1b768ee3 100644 --- a/src/greenlet/PyGreenletUnswitchable.cpp +++ b/src/greenlet/PyGreenletUnswitchable.cpp @@ -17,7 +17,7 @@ // as well. #include "greenlet_refs.hpp" #include "greenlet_slp_switch.hpp" -#include "greenlet_thread_state.hpp" + #include "greenlet_thread_support.hpp" #include "TGreenlet.hpp" diff --git a/src/greenlet/PyModule.cpp b/src/greenlet/PyModule.cpp index 6a236dfe..6adcb5c3 100644 --- a/src/greenlet/PyModule.cpp +++ b/src/greenlet/PyModule.cpp @@ -3,7 +3,7 @@ #include "greenlet_internal.hpp" -#include "greenlet_thread_state.hpp" + #include "TGreenletGlobals.cpp" #include "TMainGreenlet.cpp" #include "TThreadStateDestroy.cpp" diff --git a/src/greenlet/TGreenlet.cpp b/src/greenlet/TGreenlet.cpp index a9e56b6e..4698a178 100644 --- a/src/greenlet/TGreenlet.cpp +++ b/src/greenlet/TGreenlet.cpp @@ -13,7 +13,7 @@ #define TGREENLET_CPP #include "greenlet_internal.hpp" #include "TGreenlet.hpp" -#include "greenlet_thread_state.hpp" + #include "TGreenletGlobals.cpp" #include "TThreadStateDestroy.cpp" diff --git a/src/greenlet/TGreenletGlobals.cpp b/src/greenlet/TGreenletGlobals.cpp index c71c963b..0087d2ff 100644 --- a/src/greenlet/TGreenletGlobals.cpp +++ b/src/greenlet/TGreenletGlobals.cpp @@ -15,7 +15,7 @@ #include "greenlet_refs.hpp" #include "greenlet_exceptions.hpp" #include "greenlet_thread_support.hpp" -#include "greenlet_thread_state.hpp" +#include "greenlet_internal.hpp" namespace greenlet { diff --git a/src/greenlet/TMainGreenlet.cpp b/src/greenlet/TMainGreenlet.cpp index 25f33ddc..a2a9cfe4 100644 --- a/src/greenlet/TMainGreenlet.cpp +++ b/src/greenlet/TMainGreenlet.cpp @@ -13,7 +13,7 @@ #define T_MAIN_GREENLET_CPP #include "TGreenlet.hpp" -#include "greenlet_thread_state.hpp" + // Protected by the GIL. Incremented when we create a main greenlet, diff --git a/src/greenlet/greenlet_thread_state.hpp b/src/greenlet/TThreadState.hpp similarity index 87% rename from src/greenlet/greenlet_thread_state.hpp rename to src/greenlet/TThreadState.hpp index 39cc0d00..e4e6f6cb 100644 --- a/src/greenlet/greenlet_thread_state.hpp +++ b/src/greenlet/TThreadState.hpp @@ -214,14 +214,14 @@ class ThreadState { return 0; } - inline BorrowedMainGreenlet borrow_main_greenlet() const + inline BorrowedMainGreenlet borrow_main_greenlet() const noexcept { assert(this->main_greenlet); assert(this->main_greenlet.REFCNT() >= 2); return this->main_greenlet; }; - inline OwnedMainGreenlet get_main_greenlet() + inline OwnedMainGreenlet get_main_greenlet() const noexcept { return this->main_greenlet; } @@ -488,91 +488,9 @@ ImmortalString ThreadState::get_referrers_name(nullptr); PythonAllocator ThreadState::allocator; std::clock_t ThreadState::_clocks_used_doing_gc(0); -template -class ThreadStateCreator -{ -private: - // Initialized to 1, and, if still 1, created on access. - // Set to 0 on destruction. - ThreadState* _state; - G_NO_COPIES_OF_CLS(ThreadStateCreator); - - inline bool has_initialized_state() const - { - return this->_state != (ThreadState*)1; - } - - inline bool has_state() const - { - return this->has_initialized_state() && this->_state != nullptr; - } - -public: - // Only one of these, auto created per thread. - // Constructing the state constructs the MainGreenlet. - ThreadStateCreator() : - _state((ThreadState*)1) - { - } - - ~ThreadStateCreator() - { - ThreadState* tmp = this->_state; - this->_state = nullptr; - if (tmp && tmp != (ThreadState*)1) { - Destructor x(tmp); - } - } - - inline ThreadState& state() - { - // The main greenlet will own this pointer when it is created, - // which will be right after this. The plan is to give every - // greenlet a pointer to the main greenlet for the thread it - // runs in; if we are doing something cross-thread, we need to - // access the pointer from the main greenlet. Deleting the - // thread, and hence the thread-local storage, will delete the - // state pointer in the main greenlet. - if (!this->has_initialized_state()) { - // XXX: Assuming allocation never fails - this->_state = new ThreadState; - // For non-standard threading, we need to store an object - // in the Python thread state dictionary so that it can be - // DECREF'd when the thread ends (ideally; the dict could - // last longer) and clean this object up. - } - if (!this->_state) { - throw std::runtime_error("Accessing state after destruction."); - } - return *this->_state; - } - - operator ThreadState&() - { - return this->state(); - } - - operator ThreadState*() - { - return &this->state(); - } - - inline int tp_traverse(visitproc visit, void* arg) - { - if (this->has_state()) { - return this->_state->tp_traverse(visit, arg); - } - return 0; - } - -}; -// We can't use the PythonAllocator for this, because we push to it -// from the thread state destructor, which doesn't have the GIL, -// and Python's allocators can only be called with the GIL. -typedef std::vector cleanup_queue_t; }; // namespace greenlet diff --git a/src/greenlet/TThreadStateCreator.hpp b/src/greenlet/TThreadStateCreator.hpp index 39cc0d00..2ec7ab55 100644 --- a/src/greenlet/TThreadStateCreator.hpp +++ b/src/greenlet/TThreadStateCreator.hpp @@ -1,5 +1,5 @@ -#ifndef GREENLET_THREAD_STATE_HPP -#define GREENLET_THREAD_STATE_HPP +#ifndef GREENLET_THREAD_STATE_CREATOR_HPP +#define GREENLET_THREAD_STATE_CREATOR_HPP #include #include @@ -8,487 +8,14 @@ #include "greenlet_refs.hpp" #include "greenlet_thread_support.hpp" -using greenlet::refs::BorrowedObject; -using greenlet::refs::BorrowedGreenlet; -using greenlet::refs::BorrowedMainGreenlet; -using greenlet::refs::OwnedMainGreenlet; -using greenlet::refs::OwnedObject; -using greenlet::refs::OwnedGreenlet; -using greenlet::refs::OwnedList; -using greenlet::refs::PyErrFetchParam; -using greenlet::refs::PyArgParseParam; -using greenlet::refs::ImmortalString; -using greenlet::refs::CreatedModule; -using greenlet::refs::PyErrPieces; -using greenlet::refs::NewReference; +#include "TThreadState.hpp" namespace greenlet { -/** - * Thread-local state of greenlets. - * - * Each native thread will get exactly one of these objects, - * automatically accessed through the best available thread-local - * mechanism the compiler supports (``thread_local`` for C++11 - * compilers or ``__thread``/``declspec(thread)`` for older GCC/clang - * or MSVC, respectively.) - * - * Previously, we kept thread-local state mostly in a bunch of - * ``static volatile`` variables in the main greenlet file.. This had - * the problem of requiring extra checks, loops, and great care - * accessing these variables if we potentially invoked any Python code - * that could release the GIL, because the state could change out from - * under us. Making the variables thread-local solves this problem. - * - * When we detected that a greenlet API accessing the current greenlet - * was invoked from a different thread than the greenlet belonged to, - * we stored a reference to the greenlet in the Python thread - * dictionary for the thread the greenlet belonged to. This could lead - * to memory leaks if the thread then exited (because of a reference - * cycle, as greenlets referred to the thread dictionary, and deleting - * non-current greenlets leaked their frame plus perhaps arguments on - * the C stack). If a thread exited while still having running - * greenlet objects (perhaps that had just switched back to the main - * greenlet), and did not invoke one of the greenlet APIs *in that - * thread, immediately before it exited, without some other thread - * then being invoked*, such a leak was guaranteed. - * - * This can be partly solved by using compiler thread-local variables - * instead of the Python thread dictionary, thus avoiding a cycle. - * - * To fully solve this problem, we need a reliable way to know that a - * thread is done and we should clean up the main greenlet. On POSIX, - * we can use the destructor function of ``pthread_key_create``, but - * there's nothing similar on Windows; a C++11 thread local object - * reliably invokes its destructor when the thread it belongs to exits - * (non-C++11 compilers offer ``__thread`` or ``declspec(thread)`` to - * create thread-local variables, but they can't hold C++ objects that - * invoke destructors; the C++11 version is the most portable solution - * I found). When the thread exits, we can drop references and - * otherwise manipulate greenlets and frames that we know can no - * longer be switched to. For compilers that don't support C++11 - * thread locals, we have a solution that uses the python thread - * dictionary, though it may not collect everything as promptly as - * other compilers do, if some other library is using the thread - * dictionary and has a cycle or extra reference. - * - * There are two small wrinkles. The first is that when the thread - * exits, it is too late to actually invoke Python APIs: the Python - * thread state is gone, and the GIL is released. To solve *this* - * problem, our destructor uses ``Py_AddPendingCall`` to transfer the - * destruction work to the main thread. (This is not an issue for the - * dictionary solution.) - * - * The second is that once the thread exits, the thread local object - * is invalid and we can't even access a pointer to it, so we can't - * pass it to ``Py_AddPendingCall``. This is handled by actually using - * a second object that's thread local (ThreadStateCreator) and having - * it dynamically allocate this object so it can live until the - * pending call runs. - */ +typedef void (*ThreadStateDestructor)(ThreadState* const); -class ThreadState { -private: - // As of commit 08ad1dd7012b101db953f492e0021fb08634afad - // this class needed 56 bytes in o Py_DEBUG build - // on 64-bit macOS 11. - // Adding the vector takes us up to 80 bytes () - - /* Strong reference to the main greenlet */ - OwnedMainGreenlet main_greenlet; - - /* Strong reference to the current greenlet. */ - OwnedGreenlet current_greenlet; - - /* Strong reference to the trace function, if any. */ - OwnedObject tracefunc; - - typedef std::vector > deleteme_t; - /* A vector of raw PyGreenlet pointers representing things that need - deleted when this thread is running. The vector owns the - references, but you need to manually INCREF/DECREF as you use - them. We don't use a vector because we - make copy of this vector, and that would become O(n) as all the - refcounts are incremented in the copy. - */ - deleteme_t deleteme; - -#ifdef GREENLET_NEEDS_EXCEPTION_STATE_SAVED - void* exception_state; -#endif - - static std::clock_t _clocks_used_doing_gc; - static ImmortalString get_referrers_name; - static PythonAllocator allocator; - - 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(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)) - { - return ThreadState::allocator.allocate(1); - } - - static void operator delete(void* ptr) - { - return ThreadState::allocator.deallocate(static_cast(ptr), - 1); - } - - static void init() - { - ThreadState::get_referrers_name = "get_referrers"; - ThreadState::_clocks_used_doing_gc = 0; - } - - ThreadState() - { - -#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() - { -#ifdef GREENLET_NEEDS_EXCEPTION_STATE_SAVED - // It's probably important this be inlined and only call C - // functions to avoid adding an SEH frame. - slp_set_exception_state(this->exception_state); -#endif - } - - inline bool has_main_greenlet() const noexcept - { - return bool(this->main_greenlet); - } - - // Called from the ThreadStateCreator when we're in non-standard - // threading mode. In that case, there is an object in the Python - // thread state dictionary that points to us. The main greenlet - // also traverses into us, in which case it's crucial not to - // traverse back into the main greenlet. - int tp_traverse(visitproc visit, void* arg, bool traverse_main=true) - { - if (traverse_main) { - Py_VISIT(main_greenlet.borrow_o()); - } - if (traverse_main || current_greenlet != main_greenlet) { - Py_VISIT(current_greenlet.borrow_o()); - } - Py_VISIT(tracefunc.borrow()); - return 0; - } - - inline BorrowedMainGreenlet borrow_main_greenlet() const - { - assert(this->main_greenlet); - assert(this->main_greenlet.REFCNT() >= 2); - return this->main_greenlet; - }; - - inline OwnedMainGreenlet get_main_greenlet() - { - return this->main_greenlet; - } - - /** - * In addition to returning a new reference to the currunt - * greenlet, this performs any maintenance needed. - */ - inline OwnedGreenlet get_current() - { - /* green_dealloc() cannot delete greenlets from other threads, so - it stores them in the thread dict; delete them now. */ - this->clear_deleteme_list(); - //assert(this->current_greenlet->main_greenlet == this->main_greenlet); - //assert(this->main_greenlet->main_greenlet == this->main_greenlet); - return this->current_greenlet; - } - - /** - * As for non-const get_current(); - */ - inline BorrowedGreenlet borrow_current() - { - this->clear_deleteme_list(); - return this->current_greenlet; - } - - /** - * Does no maintenance. - */ - inline OwnedGreenlet get_current() const - { - return this->current_greenlet; - } - - template - inline bool is_current(const refs::PyObjectPointer& obj) const - { - return this->current_greenlet.borrow_o() == obj.borrow_o(); - } - - inline void set_current(const OwnedGreenlet& target) - { - this->current_greenlet = target; - } - -private: - /** - * Deref and remove the greenlets from the deleteme list. Must be - * holding the GIL. - * - * If *murder* is true, then we must be called from a different - * thread than the one that these greenlets were running in. - * In that case, if the greenlet was actually running, we destroy - * the frame reference and otherwise make it appear dead before - * proceeding; otherwise, we would try (and fail) to raise an - * exception in it and wind up right back in this list. - */ - inline void clear_deleteme_list(const bool murder=false) - { - if (!this->deleteme.empty()) { - // It's possible we could add items to this list while - // running Python code if there's a thread switch, so we - // need to defensively copy it before that can happen. - deleteme_t copy = this->deleteme; - this->deleteme.clear(); // in case things come back on the list - for(deleteme_t::iterator it = copy.begin(), end = copy.end(); - it != end; - ++it ) { - PyGreenlet* to_del = *it; - if (murder) { - // Force each greenlet to appear dead; we can't raise an - // exception into it anymore anyway. - to_del->pimpl->murder_in_place(); - } - - // The only reference to these greenlets should be in - // this list, decreffing them should let them be - // deleted again, triggering calls to green_dealloc() - // in the correct thread (if we're not murdering). - // This may run arbitrary Python code and switch - // threads or greenlets! - Py_DECREF(to_del); - if (PyErr_Occurred()) { - PyErr_WriteUnraisable(nullptr); - PyErr_Clear(); - } - } - } - } - -public: - - /** - * Returns a new reference, or a false object. - */ - inline OwnedObject get_tracefunc() const - { - return tracefunc; - }; - - - inline void set_tracefunc(BorrowedObject tracefunc) - { - assert(tracefunc); - if (tracefunc == BorrowedObject(Py_None)) { - this->tracefunc.CLEAR(); - } - else { - this->tracefunc = tracefunc; - } - } - - /** - * Given a reference to a greenlet that some other thread - * attempted to delete (has a refcount of 0) store it for later - * deletion when the thread this state belongs to is current. - */ - inline void delete_when_thread_running(PyGreenlet* to_del) - { - Py_INCREF(to_del); - this->deleteme.push_back(to_del); - } - - /** - * Set to std::clock_t(-1) to disable. - */ - inline static std::clock_t& clocks_used_doing_gc() - { - return ThreadState::_clocks_used_doing_gc; - } - - ~ThreadState() - { - if (!PyInterpreterState_Head()) { - // We shouldn't get here (our callers protect us) - // but if we do, all we can do is bail early. - return; - } - - // We should not have an "origin" greenlet; that only exists - // for the temporary time during a switch, which should not - // be in progress as the thread dies. - //assert(!this->switching_state.origin); - - this->tracefunc.CLEAR(); - - // Forcibly GC as much as we can. - this->clear_deleteme_list(true); - - // The pending call did this. - assert(this->main_greenlet->thread_state() == nullptr); - - // If the main greenlet is the current greenlet, - // then we "fell off the end" and the thread died. - // It's possible that there is some other greenlet that - // switched to us, leaving a reference to the main greenlet - // on the stack, somewhere uncollectible. Try to detect that. - if (this->current_greenlet == this->main_greenlet && this->current_greenlet) { - assert(this->current_greenlet->is_currently_running_in_some_thread()); - // Drop one reference we hold. - this->current_greenlet.CLEAR(); - assert(!this->current_greenlet); - // Only our reference to the main greenlet should be left, - // But hold onto the pointer in case we need to do extra cleanup. - PyGreenlet* old_main_greenlet = this->main_greenlet.borrow(); - Py_ssize_t cnt = this->main_greenlet.REFCNT(); - this->main_greenlet.CLEAR(); - if (ThreadState::_clocks_used_doing_gc != std::clock_t(-1) - && cnt == 2 && Py_REFCNT(old_main_greenlet) == 1) { - // Highly likely that the reference is somewhere on - // the stack, not reachable by GC. Verify. - // XXX: This is O(n) in the total number of objects. - // TODO: Add a way to disable this at runtime, and - // another way to report on it. - std::clock_t begin = std::clock(); - NewReference gc(PyImport_ImportModule("gc")); - if (gc) { - OwnedObject get_referrers = gc.PyRequireAttr(ThreadState::get_referrers_name); - OwnedList refs(get_referrers.PyCall(old_main_greenlet)); - if (refs && refs.empty()) { - assert(refs.REFCNT() == 1); - // We found nothing! So we left a dangling - // reference: Probably the last thing some - // other greenlet did was call - // 'getcurrent().parent.switch()' to switch - // back to us. Clean it up. This will be the - // case on CPython 3.7 and newer, as they use - // an internal calling conversion that avoids - // creating method objects and storing them on - // the stack. - Py_DECREF(old_main_greenlet); - } - else if (refs - && refs.size() == 1 - && PyCFunction_Check(refs.at(0)) - && Py_REFCNT(refs.at(0)) == 2) { - assert(refs.REFCNT() == 1); - // Ok, we found a C method that refers to the - // main greenlet, and its only referenced - // twice, once in the list we just created, - // once from...somewhere else. If we can't - // find where else, then this is a leak. - // This happens in older versions of CPython - // that create a bound method object somewhere - // on the stack that we'll never get back to. - if (PyCFunction_GetFunction(refs.at(0).borrow()) == (PyCFunction)green_switch) { - BorrowedObject function_w = refs.at(0); - refs.clear(); // destroy the reference - // from the list. - // back to one reference. Can *it* be - // found? - assert(function_w.REFCNT() == 1); - refs = get_referrers.PyCall(function_w); - if (refs && refs.empty()) { - // Nope, it can't be found so it won't - // ever be GC'd. Drop it. - Py_CLEAR(function_w); - } - } - } - std::clock_t end = std::clock(); - ThreadState::_clocks_used_doing_gc += (end - begin); - } - } - } - - // We need to make sure this greenlet appears to be dead, - // because otherwise deallocing it would fail to raise an - // exception in it (the thread is dead) and put it back in our - // deleteme list. - if (this->current_greenlet) { - this->current_greenlet->murder_in_place(); - this->current_greenlet.CLEAR(); - } - - if (this->main_greenlet) { - // Couldn't have been the main greenlet that was running - // when the thread exited (because we already cleared this - // pointer if it was). This shouldn't be possible? - - // If the main greenlet was current when the thread died (it - // should be, right?) then we cleared its self pointer above - // when we cleared the current greenlet's main greenlet pointer. - // assert(this->main_greenlet->main_greenlet == this->main_greenlet - // || !this->main_greenlet->main_greenlet); - // // self reference, probably gone - // this->main_greenlet->main_greenlet.CLEAR(); - - // This will actually go away when the ivar is destructed. - this->main_greenlet.CLEAR(); - } - - if (PyErr_Occurred()) { - PyErr_WriteUnraisable(NULL); - PyErr_Clear(); - } - - } - -}; - -ImmortalString ThreadState::get_referrers_name(nullptr); -PythonAllocator ThreadState::allocator; -std::clock_t ThreadState::_clocks_used_doing_gc(0); - -template +template class ThreadStateCreator { private: @@ -497,12 +24,12 @@ class ThreadStateCreator ThreadState* _state; G_NO_COPIES_OF_CLS(ThreadStateCreator); - inline bool has_initialized_state() const + inline bool has_initialized_state() const noexcept { return this->_state != (ThreadState*)1; } - inline bool has_state() const + inline bool has_state() const noexcept { return this->has_initialized_state() && this->_state != nullptr; } @@ -518,11 +45,11 @@ class ThreadStateCreator ~ThreadStateCreator() { - ThreadState* tmp = this->_state; - this->_state = nullptr; - if (tmp && tmp != (ThreadState*)1) { - Destructor x(tmp); + if (this->has_state()) { + Destructor(this->_state); } + + this->_state = nullptr; } inline ThreadState& state() @@ -569,10 +96,6 @@ class ThreadStateCreator }; -// We can't use the PythonAllocator for this, because we push to it -// from the thread state destructor, which doesn't have the GIL, -// and Python's allocators can only be called with the GIL. -typedef std::vector cleanup_queue_t; }; // namespace greenlet diff --git a/src/greenlet/TThreadStateDestroy.cpp b/src/greenlet/TThreadStateDestroy.cpp index e5a9661a..9c4f2e03 100644 --- a/src/greenlet/TThreadStateDestroy.cpp +++ b/src/greenlet/TThreadStateDestroy.cpp @@ -13,24 +13,141 @@ #define T_THREADSTATE_DESTROY #include "TGreenlet.hpp" -#include "greenlet_thread_state.hpp" + #include "greenlet_thread_support.hpp" #include "greenlet_cpython_add_pending.hpp" #include "TGreenletGlobals.cpp" +#include "TThreadState.hpp" +#include "TThreadStateCreator.hpp" namespace greenlet { -struct ThreadState_DestroyWithGIL +extern "C" { + +struct ThreadState_DestroyNoGIL { - ThreadState_DestroyWithGIL(ThreadState* state) + /** + This function uses the same lock that the PendingCallback does + */ + static void + MarkGreenletDeadAndQueueCleanup(ThreadState* const state) + { + // We are *NOT* holding the GIL. Our thread is in the middle + // of its death throes and the Python thread state is already + // gone so we can't use most Python APIs. One that is safe is + // ``Py_AddPendingCall``, unless the interpreter itself has + // been torn down. There is a limited number of calls that can + // be queued: 32 (NPENDINGCALLS) in CPython 3.10, so we + // coalesce these calls using our own queue. + + if (!MarkGreenletDeadIfNeeded(state)) { + // No state, or no greenlet + return; + } + + // XXX: Because we don't have the GIL, this is a race condition. + if (!PyInterpreterState_Head()) { + // We have to leak the thread state, if the + // interpreter has shut down when we're getting + // deallocated, we can't run the cleanup code that + // deleting it would imply. + return; + } + + AddToCleanupQueue(state); + + } + +private: + + // If the state has an allocated main greenlet: + // - mark the greenlet as dead by disassociating it from the state; + // - return 1 + // Otherwise, return 0. + static bool + MarkGreenletDeadIfNeeded(ThreadState* const state) { if (state && state->has_main_greenlet()) { - DestroyWithGIL(state); + // mark the thread as dead ASAP. + // this is racy! If we try to throw or switch to a + // greenlet from this thread from some other thread before + // we clear the state pointer, it won't realize the state + // is dead which can crash the process. + PyGreenlet* p(state->borrow_main_greenlet().borrow()); + assert(p->pimpl->thread_state() == state || p->pimpl->thread_state() == nullptr); + dynamic_cast(p->pimpl)->thread_state(nullptr); + return true; + } + return false; + } + + static void + AddToCleanupQueue(ThreadState* const state) + { + assert(state && state->has_main_greenlet()); + + // NOTE: Because we're not holding the GIL here, some other + // Python thread could run and call ``os.fork()``, which would + // be bad if that happened while we are holding the cleanup + // lock (it wouldn't function in the child process). + // Make a best effort to try to keep the duration we hold the + // lock short. + // TODO: On platforms that support it, use ``pthread_atfork`` to + // drop this lock. + LockGuard cleanup_lock(*mod_globs->thread_states_to_destroy_lock); + + mod_globs->queue_to_destroy(state); + if (mod_globs->thread_states_to_destroy.size() == 1) { + // We added the first item to the queue. We need to schedule + // the cleanup. + + // A size greater than 1 means that we have already added the pending call, + // and in fact, it may be executing now. + // If it is executing, our lock makes sure that it will see the item we just added + // to the queue on its next iteration (after we release the lock) + // + // A size of 1 means there is no pending call, OR the pending call is + // currently executing, has dropped the lock, and is deleting the last item + // from the queue; its next iteration will go ahead and delete the item we just added. + // And the pending call we schedule here will have no work to do. + int result = AddPendingCall( + PendingCallback_DestroyQueueWithGIL, + nullptr); + if (result < 0) { + // Hmm, what can we do here? + fprintf(stderr, + "greenlet: WARNING: failed in call to Py_AddPendingCall; " + "expect a memory leak.\n"); + } } } static int - DestroyWithGIL(ThreadState* state) + PendingCallback_DestroyQueueWithGIL(void* UNUSED(arg)) + { + // We're holding the GIL here, so no Python code should be able to + // run to call ``os.fork()``. + while (1) { + ThreadState* to_destroy; + { + LockGuard cleanup_lock(*mod_globs->thread_states_to_destroy_lock); + if (mod_globs->thread_states_to_destroy.empty()) { + break; + } + to_destroy = mod_globs->take_next_to_destroy(); + } + assert(to_destroy); + assert(to_destroy->has_main_greenlet()); + // Drop the lock while we do the actual deletion. + // This allows other calls to MarkGreenletDeadAndQueueCleanup + // to enter and add to our queue. + DestroyOneWithGIL(to_destroy); + } + return 0; + } + + static void + DestroyOneWithGIL(const ThreadState* const state) { // Holding the GIL. // Passed a non-shared pointer to the actual thread state. @@ -42,17 +159,11 @@ struct ThreadState_DestroyWithGIL // We do this here, rather than in a Python dealloc function // for the greenlet, in case there's still a reference out // there. - static_cast(main->pimpl)->thread_state(nullptr); + dynamic_cast(main->pimpl)->thread_state(nullptr); delete state; // Deleting this runs the destructor, DECREFs the main greenlet. - return 0; } -}; - - -struct ThreadState_DestroyNoGIL -{ // ensure this is actually defined. static_assert(GREENLET_BROKEN_PY_ADD_PENDING == 1 || GREENLET_BROKEN_PY_ADD_PENDING == 0, "GREENLET_BROKEN_PY_ADD_PENDING not defined correctly."); @@ -121,83 +232,10 @@ struct ThreadState_DestroyNoGIL } #endif - ThreadState_DestroyNoGIL(ThreadState* state) - { - // We are *NOT* holding the GIL. Our thread is in the middle - // of its death throes and the Python thread state is already - // gone so we can't use most Python APIs. One that is safe is - // ``Py_AddPendingCall``, unless the interpreter itself has - // been torn down. There is a limited number of calls that can - // be queued: 32 (NPENDINGCALLS) in CPython 3.10, so we - // coalesce these calls using our own queue. - if (state && state->has_main_greenlet()) { - // mark the thread as dead ASAP. - // this is racy! If we try to throw or switch to a - // greenlet from this thread from some other thread before - // we clear the state pointer, it won't realize the state - // is dead which can crash the process. - PyGreenlet* p = state->borrow_main_greenlet(); - assert(p->pimpl->thread_state() == state || p->pimpl->thread_state() == nullptr); - static_cast(p->pimpl)->thread_state(nullptr); - } - - // NOTE: Because we're not holding the GIL here, some other - // Python thread could run and call ``os.fork()``, which would - // be bad if that happened while we are holding the cleanup - // lock (it wouldn't function in the child process). - // Make a best effort to try to keep the duration we hold the - // lock short. - // TODO: On platforms that support it, use ``pthread_atfork`` to - // drop this lock. - LockGuard cleanup_lock(*mod_globs->thread_states_to_destroy_lock); - - if (state && state->has_main_greenlet()) { - // Because we don't have the GIL, this is a race condition. - if (!PyInterpreterState_Head()) { - // We have to leak the thread state, if the - // interpreter has shut down when we're getting - // deallocated, we can't run the cleanup code that - // deleting it would imply. - return; - } - mod_globs->queue_to_destroy(state); - if (mod_globs->thread_states_to_destroy.size() == 1) { - // We added the first item to the queue. We need to schedule - // the cleanup. - int result = ThreadState_DestroyNoGIL::AddPendingCall( - ThreadState_DestroyNoGIL::DestroyQueueWithGIL, - NULL); - if (result < 0) { - // Hmm, what can we do here? - fprintf(stderr, - "greenlet: WARNING: failed in call to Py_AddPendingCall; " - "expect a memory leak.\n"); - } - } - } - } - static int - DestroyQueueWithGIL(void* UNUSED(arg)) - { - // We're holding the GIL here, so no Python code should be able to - // run to call ``os.fork()``. - while (1) { - ThreadState* to_destroy; - { - LockGuard cleanup_lock(*mod_globs->thread_states_to_destroy_lock); - if (mod_globs->thread_states_to_destroy.empty()) { - break; - } - to_destroy = mod_globs->take_next_to_destroy(); - } - // Drop the lock while we do the actual deletion. - ThreadState_DestroyWithGIL::DestroyWithGIL(to_destroy); - } - return 0; - } +}; }; }; // namespace greenlet @@ -209,7 +247,7 @@ struct ThreadState_DestroyNoGIL // initial function call in each function that uses a thread local); // in contrast, static volatile variables are at some pre-computed // offset. -typedef greenlet::ThreadStateCreator ThreadStateCreator; +typedef greenlet::ThreadStateCreator ThreadStateCreator; static thread_local ThreadStateCreator g_thread_state_global; #define GET_THREAD_STATE() g_thread_state_global diff --git a/src/greenlet/TUserGreenlet.cpp b/src/greenlet/TUserGreenlet.cpp index 83db972f..73a81330 100644 --- a/src/greenlet/TUserGreenlet.cpp +++ b/src/greenlet/TUserGreenlet.cpp @@ -14,7 +14,7 @@ #include "greenlet_internal.hpp" #include "TGreenlet.hpp" -#include "greenlet_thread_state.hpp" + #include "TThreadStateDestroy.cpp" diff --git a/src/greenlet/greenlet.cpp b/src/greenlet/greenlet.cpp index 7a7b5095..e8d92a00 100644 --- a/src/greenlet/greenlet.cpp +++ b/src/greenlet/greenlet.cpp @@ -22,12 +22,12 @@ // as well. #include "greenlet_refs.hpp" #include "greenlet_slp_switch.hpp" -#include "greenlet_thread_state.hpp" + #include "greenlet_thread_support.hpp" #include "TGreenlet.hpp" #include "TGreenletGlobals.cpp" -#include "TThreadStateDestroy.cpp" + #include "TGreenlet.cpp" #include "TMainGreenlet.cpp" #include "TUserGreenlet.cpp" @@ -36,6 +36,10 @@ #include "TPythonState.cpp" #include "TStackState.cpp" +#include "TThreadState.hpp" +#include "TThreadStateCreator.hpp" +#include "TThreadStateDestroy.cpp" + #include "PyGreenlet.cpp" #include "PyGreenletUnswitchable.cpp" #include "CObjects.cpp" diff --git a/src/greenlet/greenlet_internal.hpp b/src/greenlet/greenlet_internal.hpp index 6d79cc2a..f2b15d5f 100644 --- a/src/greenlet/greenlet_internal.hpp +++ b/src/greenlet/greenlet_internal.hpp @@ -27,6 +27,10 @@ typedef struct _greenlet PyGreenlet; namespace greenlet { class ThreadState; + // We can't use the PythonAllocator for this, because we push to it + // from the thread state destructor, which doesn't have the GIL, + // and Python's allocators can only be called with the GIL. + typedef std::vector cleanup_queue_t; };