From c3012a046272b854a6dac5f4406f8b770475b039 Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Tue, 10 Aug 2021 14:36:29 -0700 Subject: [PATCH 1/5] Integrate trashcan into _Py_Dealloc. This is a WIP/proof-of-concept of doing away with Py_TRASHCAN_BEGIN and Py_TRASHCAN_END and instead integrating the functionality into _Py_Dealloc. There are a few advantages: - all container objects have the risk of overflowing the C stack if a long reference chain of them is created and then deallocated. So, to be safe, the tp_dealloc methods for those objects should be protected from overflowing the stack. - the Py_TRASHCAN_BEGIN and Py_TRASHCAN_END macros are hard to understand and a bit hard to use correctly. Making the mechanism internal avoids confusion. The code can be slightly simplified as well. This proof-of-concept seems to pass tests but it will need some careful review. The exact rules related to calling GC Track/Untrack are subtle and this changes things a bit. I.e. tp_dealloc is called with GC objects already untracked. For 3rd party extensions, they are calling PyObject_GC_UnTrack() and so I believe they should still work. The fact that PyObject_CallFinalizerFromDealloc() wants GC objects to definitely be tracked is a bit of a mystery to me (there is an assert to check that). I changed the code to track objects if they are not already tracked but I'm not sure that's correct. There could be a performance hit, due to the _PyType_IS_GC() test that was added to the _Py_Dealloc() function. For non-GC objects, that's going to be a new branch and I'm worried it might hurt a bit. OTOH, maybe it's just in the noise. Profiling will need to be done. --- Include/cpython/object.h | 81 +++-------------------------------- Modules/_elementtree.c | 5 --- Modules/_io/bufferedio.c | 2 - Modules/_io/bytesio.c | 1 - Modules/_io/fileio.c | 1 - Modules/_io/iobase.c | 1 - Modules/_io/stringio.c | 1 - Modules/_io/textio.c | 1 - Modules/_io/winconsoleio.c | 1 - Objects/bytearrayobject.c | 1 - Objects/bytesobject.c | 1 - Objects/cellobject.c | 1 - Objects/classobject.c | 2 - Objects/descrobject.c | 6 --- Objects/dictobject.c | 8 ---- Objects/exceptions.c | 11 ----- Objects/frameobject.c | 6 --- Objects/funcobject.c | 3 -- Objects/genericaliasobject.c | 1 - Objects/genobject.c | 6 --- Objects/iterobject.c | 3 -- Objects/listobject.c | 4 -- Objects/memoryobject.c | 4 +- Objects/methodobject.c | 5 --- Objects/object.c | 83 ++++++++++++++++++++++++++++-------- Objects/odictobject.c | 6 --- Objects/setobject.c | 6 --- Objects/sliceobject.c | 1 - Objects/tupleobject.c | 10 +---- Objects/typeobject.c | 50 +--------------------- Objects/unicodeobject.c | 1 - Objects/unionobject.c | 2 - Python/context.c | 2 - Python/hamt.c | 12 ------ Python/traceback.c | 3 -- 35 files changed, 75 insertions(+), 257 deletions(-) diff --git a/Include/cpython/object.h b/Include/cpython/object.h index 75cd0f9002215b..ca55cdee569c5d 100644 --- a/Include/cpython/object.h +++ b/Include/cpython/object.h @@ -456,83 +456,14 @@ PyAPI_FUNC(int) _PyObject_CheckConsistency( int check_content); -/* Trashcan mechanism, thanks to Christian Tismer. - -When deallocating a container object, it's possible to trigger an unbounded -chain of deallocations, as each Py_DECREF in turn drops the refcount on "the -next" object in the chain to 0. This can easily lead to stack overflows, -especially in threads (which typically have less stack space to work with). - -A container object can avoid this by bracketing the body of its tp_dealloc -function with a pair of macros: - -static void -mytype_dealloc(mytype *p) -{ - ... declarations go here ... - - PyObject_GC_UnTrack(p); // must untrack first - Py_TRASHCAN_BEGIN(p, mytype_dealloc) - ... The body of the deallocator goes here, including all calls ... - ... to Py_DECREF on contained objects. ... - Py_TRASHCAN_END // there should be no code after this -} - -CAUTION: Never return from the middle of the body! If the body needs to -"get out early", put a label immediately before the Py_TRASHCAN_END -call, and goto it. Else the call-depth counter (see below) will stay -above 0 forever, and the trashcan will never get emptied. - -How it works: The BEGIN macro increments a call-depth counter. So long -as this counter is small, the body of the deallocator is run directly without -further ado. But if the counter gets large, it instead adds p to a list of -objects to be deallocated later, skips the body of the deallocator, and -resumes execution after the END macro. The tp_dealloc routine then returns -without deallocating anything (and so unbounded call-stack depth is avoided). - -When the call stack finishes unwinding again, code generated by the END macro -notices this, and calls another routine to deallocate all the objects that -may have been added to the list of deferred deallocations. In effect, a -chain of N deallocations is broken into (N-1)/(_PyTrash_UNWIND_LEVEL-1) pieces, -with the call stack never exceeding a depth of _PyTrash_UNWIND_LEVEL. - -Since the tp_dealloc of a subclass typically calls the tp_dealloc of the base -class, we need to ensure that the trashcan is only triggered on the tp_dealloc -of the actual class being deallocated. Otherwise we might end up with a -partially-deallocated object. To check this, the tp_dealloc function must be -passed as second argument to Py_TRASHCAN_BEGIN(). -*/ - -/* Forward declarations for PyThreadState */ -struct _ts; - -/* Python 3.9 private API, invoked by the macros below. */ -PyAPI_FUNC(int) _PyTrash_begin(struct _ts *tstate, PyObject *op); -PyAPI_FUNC(void) _PyTrash_end(struct _ts *tstate); -/* Python 3.10 private API, invoked by the Py_TRASHCAN_BEGIN(). */ -PyAPI_FUNC(int) _PyTrash_cond(PyObject *op, destructor dealloc); - -#define Py_TRASHCAN_BEGIN_CONDITION(op, cond) \ - do { \ - PyThreadState *_tstate = NULL; \ - /* If "cond" is false, then _tstate remains NULL and the deallocator \ - * is run normally without involving the trashcan */ \ - if (cond) { \ - _tstate = PyThreadState_Get(); \ - if (_PyTrash_begin(_tstate, _PyObject_CAST(op))) { \ - break; \ - } \ - } - /* The body of the deallocator is here. */ +#if 1 +/* do nothing trashcan macros, for backwards compatibility */ +#define Py_TRASHCAN_BEGIN(op, dealloc) \ + do { #define Py_TRASHCAN_END \ - if (_tstate) { \ - _PyTrash_end(_tstate); \ - } \ + assert(1); \ } while (0); - -#define Py_TRASHCAN_BEGIN(op, dealloc) \ - Py_TRASHCAN_BEGIN_CONDITION(op, \ - _PyTrash_cond(_PyObject_CAST(op), (destructor)dealloc)) +#endif /* For backwards compatibility, these macros enable the trashcan * unconditionally */ diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index b4528a90b3e095..393d9530e51f33 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -664,10 +664,6 @@ element_gc_clear(ElementObject *self) static void element_dealloc(ElementObject* self) { - /* bpo-31095: UnTrack is needed before calling any callbacks */ - PyObject_GC_UnTrack(self); - Py_TRASHCAN_BEGIN(self, element_dealloc) - if (self->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) self); @@ -677,7 +673,6 @@ element_dealloc(ElementObject* self) RELEASE(sizeof(ElementObject), "destroy element"); Py_TYPE(self)->tp_free((PyObject *)self); - Py_TRASHCAN_END } /* -------------------------------------------------------------------- */ diff --git a/Modules/_io/bufferedio.c b/Modules/_io/bufferedio.c index 5984d34cc08290..f4618a1ba7655d 100644 --- a/Modules/_io/bufferedio.c +++ b/Modules/_io/bufferedio.c @@ -383,7 +383,6 @@ buffered_dealloc(buffered *self) self->finalizing = 1; if (_PyIOBase_finalize((PyObject *) self) < 0) return; - _PyObject_GC_UNTRACK(self); self->ok = 0; if (self->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *)self); @@ -2144,7 +2143,6 @@ bufferedrwpair_clear(rwpair *self) static void bufferedrwpair_dealloc(rwpair *self) { - _PyObject_GC_UNTRACK(self); if (self->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *)self); Py_CLEAR(self->reader); diff --git a/Modules/_io/bytesio.c b/Modules/_io/bytesio.c index 2468f45f941e2e..9e226c001918f5 100644 --- a/Modules/_io/bytesio.c +++ b/Modules/_io/bytesio.c @@ -886,7 +886,6 @@ bytesio_setstate(bytesio *self, PyObject *state) static void bytesio_dealloc(bytesio *self) { - _PyObject_GC_UNTRACK(self); if (self->exports > 0) { PyErr_SetString(PyExc_SystemError, "deallocated BytesIO object has exported buffers"); diff --git a/Modules/_io/fileio.c b/Modules/_io/fileio.c index b9856b3b631657..374f718dcbdae4 100644 --- a/Modules/_io/fileio.c +++ b/Modules/_io/fileio.c @@ -527,7 +527,6 @@ fileio_dealloc(fileio *self) self->finalizing = 1; if (_PyIOBase_finalize((PyObject *) self) < 0) return; - _PyObject_GC_UNTRACK(self); if (self->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) self); Py_CLEAR(self->dict); diff --git a/Modules/_io/iobase.c b/Modules/_io/iobase.c index 5b687b78176e8c..a745348f1da95e 100644 --- a/Modules/_io/iobase.c +++ b/Modules/_io/iobase.c @@ -355,7 +355,6 @@ iobase_dealloc(iobase *self) } return; } - _PyObject_GC_UNTRACK(self); if (self->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) self); Py_CLEAR(self->dict); diff --git a/Modules/_io/stringio.c b/Modules/_io/stringio.c index e76152e617bdc5..93d24951f8c261 100644 --- a/Modules/_io/stringio.c +++ b/Modules/_io/stringio.c @@ -596,7 +596,6 @@ stringio_clear(stringio *self) static void stringio_dealloc(stringio *self) { - _PyObject_GC_UNTRACK(self); self->ok = 0; if (self->buf) { PyMem_Free(self->buf); diff --git a/Modules/_io/textio.c b/Modules/_io/textio.c index eb05ae1a16eb03..afb3d5d33b61c0 100644 --- a/Modules/_io/textio.c +++ b/Modules/_io/textio.c @@ -1431,7 +1431,6 @@ textiowrapper_dealloc(textio *self) if (_PyIOBase_finalize((PyObject *) self) < 0) return; self->ok = 0; - _PyObject_GC_UNTRACK(self); if (self->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *)self); textiowrapper_clear(self); diff --git a/Modules/_io/winconsoleio.c b/Modules/_io/winconsoleio.c index 460f2d3fa071a8..25ff1f83a914b9 100644 --- a/Modules/_io/winconsoleio.c +++ b/Modules/_io/winconsoleio.c @@ -431,7 +431,6 @@ winconsoleio_dealloc(winconsoleio *self) self->finalizing = 1; if (_PyIOBase_finalize((PyObject *) self) < 0) return; - _PyObject_GC_UNTRACK(self); if (self->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) self); Py_CLEAR(self->dict); diff --git a/Objects/bytearrayobject.c b/Objects/bytearrayobject.c index 1ab9621b1f2656..beaed7954941f0 100644 --- a/Objects/bytearrayobject.c +++ b/Objects/bytearrayobject.c @@ -2371,7 +2371,6 @@ typedef struct { static void bytearrayiter_dealloc(bytesiterobject *it) { - _PyObject_GC_UNTRACK(it); Py_XDECREF(it->it_seq); PyObject_GC_Del(it); } diff --git a/Objects/bytesobject.c b/Objects/bytesobject.c index eaedb0b5689b2a..32f57082607141 100644 --- a/Objects/bytesobject.c +++ b/Objects/bytesobject.c @@ -3095,7 +3095,6 @@ typedef struct { static void striter_dealloc(striterobject *it) { - _PyObject_GC_UNTRACK(it); Py_XDECREF(it->it_seq); PyObject_GC_Del(it); } diff --git a/Objects/cellobject.c b/Objects/cellobject.c index 86a89f02e60d3c..9fbae514ded27e 100644 --- a/Objects/cellobject.c +++ b/Objects/cellobject.c @@ -78,7 +78,6 @@ PyCell_Set(PyObject *op, PyObject *obj) static void cell_dealloc(PyCellObject *op) { - _PyObject_GC_UNTRACK(op); Py_XDECREF(op->ob_ref); PyObject_GC_Del(op); } diff --git a/Objects/classobject.c b/Objects/classobject.c index af73be3d262411..c47514ec7f94fd 100644 --- a/Objects/classobject.c +++ b/Objects/classobject.c @@ -234,7 +234,6 @@ method_new(PyTypeObject* type, PyObject* args, PyObject *kw) static void method_dealloc(PyMethodObject *im) { - _PyObject_GC_UNTRACK(im); if (im->im_weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *)im); Py_DECREF(im->im_func); @@ -448,7 +447,6 @@ instancemethod_getattro(PyObject *self, PyObject *name) static void instancemethod_dealloc(PyObject *self) { - _PyObject_GC_UNTRACK(self); Py_DECREF(PyInstanceMethod_GET_FUNCTION(self)); PyObject_GC_Del(self); } diff --git a/Objects/descrobject.c b/Objects/descrobject.c index 0565992bdb79f7..1b2998e5760d4a 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -18,7 +18,6 @@ class property "propertyobject *" "&PyProperty_Type" static void descr_dealloc(PyDescrObject *descr) { - _PyObject_GC_UNTRACK(descr); Py_XDECREF(descr->d_type); Py_XDECREF(descr->d_name); Py_XDECREF(descr->d_qualname); @@ -1154,7 +1153,6 @@ static PyMethodDef mappingproxy_methods[] = { static void mappingproxy_dealloc(mappingproxyobject *pp) { - _PyObject_GC_UNTRACK(pp); Py_DECREF(pp->mapping); PyObject_GC_Del(pp); } @@ -1265,12 +1263,9 @@ typedef struct { static void wrapper_dealloc(wrapperobject *wp) { - PyObject_GC_UnTrack(wp); - Py_TRASHCAN_BEGIN(wp, wrapper_dealloc) Py_XDECREF(wp->descr); Py_XDECREF(wp->self); PyObject_GC_Del(wp); - Py_TRASHCAN_END } static PyObject * @@ -1577,7 +1572,6 @@ property_dealloc(PyObject *self) { propertyobject *gs = (propertyobject *)self; - _PyObject_GC_UNTRACK(self); Py_XDECREF(gs->prop_get); Py_XDECREF(gs->prop_set); Py_XDECREF(gs->prop_del); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index c78cbafe583bdc..385274908282f3 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1928,9 +1928,6 @@ dict_dealloc(PyDictObject *mp) PyDictKeysObject *keys = mp->ma_keys; Py_ssize_t i, n; - /* bpo-31095: UnTrack is needed before calling any callbacks */ - PyObject_GC_UnTrack(mp); - Py_TRASHCAN_BEGIN(mp, dict_dealloc) if (values != NULL) { if (values != empty_values) { for (i = 0, n = mp->ma_keys->dk_nentries; i < n; i++) { @@ -1955,7 +1952,6 @@ dict_dealloc(PyDictObject *mp) else { Py_TYPE(mp)->tp_free((PyObject *)mp); } - Py_TRASHCAN_END } @@ -3570,8 +3566,6 @@ dictiter_new(PyDictObject *dict, PyTypeObject *itertype) static void dictiter_dealloc(dictiterobject *di) { - /* bpo-31095: UnTrack is needed before calling any callbacks */ - _PyObject_GC_UNTRACK(di); Py_XDECREF(di->di_dict); Py_XDECREF(di->di_result); PyObject_GC_Del(di); @@ -4066,8 +4060,6 @@ PyTypeObject PyDictRevIterValue_Type = { static void dictview_dealloc(_PyDictViewObject *dv) { - /* bpo-31095: UnTrack is needed before calling any callbacks */ - _PyObject_GC_UNTRACK(dv); Py_XDECREF(dv->dv_dict); PyObject_GC_Del(dv); } diff --git a/Objects/exceptions.c b/Objects/exceptions.c index c6a7aa4aeccf04..e47e1ad959279d 100644 --- a/Objects/exceptions.c +++ b/Objects/exceptions.c @@ -89,7 +89,6 @@ BaseException_clear(PyBaseExceptionObject *self) static void BaseException_dealloc(PyBaseExceptionObject *self) { - _PyObject_GC_UNTRACK(self); BaseException_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -535,7 +534,6 @@ StopIteration_clear(PyStopIterationObject *self) static void StopIteration_dealloc(PyStopIterationObject *self) { - _PyObject_GC_UNTRACK(self); StopIteration_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -602,7 +600,6 @@ SystemExit_clear(PySystemExitObject *self) static void SystemExit_dealloc(PySystemExitObject *self) { - _PyObject_GC_UNTRACK(self); SystemExit_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -684,7 +681,6 @@ ImportError_clear(PyImportErrorObject *self) static void ImportError_dealloc(PyImportErrorObject *self) { - _PyObject_GC_UNTRACK(self); ImportError_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -1087,7 +1083,6 @@ OSError_clear(PyOSErrorObject *self) static void OSError_dealloc(PyOSErrorObject *self) { - _PyObject_GC_UNTRACK(self); OSError_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -1364,7 +1359,6 @@ NameError_clear(PyNameErrorObject *self) static void NameError_dealloc(PyNameErrorObject *self) { - _PyObject_GC_UNTRACK(self); NameError_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -1443,7 +1437,6 @@ AttributeError_clear(PyAttributeErrorObject *self) static void AttributeError_dealloc(PyAttributeErrorObject *self) { - _PyObject_GC_UNTRACK(self); AttributeError_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -1538,7 +1531,6 @@ SyntaxError_clear(PySyntaxErrorObject *self) static void SyntaxError_dealloc(PySyntaxErrorObject *self) { - _PyObject_GC_UNTRACK(self); SyntaxError_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -1988,7 +1980,6 @@ UnicodeError_clear(PyUnicodeErrorObject *self) static void UnicodeError_dealloc(PyUnicodeErrorObject *self) { - _PyObject_GC_UNTRACK(self); UnicodeError_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -2429,8 +2420,6 @@ MemoryError_dealloc(PyBaseExceptionObject *self) return; } - _PyObject_GC_UNTRACK(self); - struct _Py_exc_state *state = get_exc_state(); if (state->memerrors_numfree >= MEMERRORS_SAVE) { Py_TYPE(self)->tp_free((PyObject *)self); diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 2af69597c396ee..5dce8efd893bb4 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -615,11 +615,6 @@ static PyGetSetDef frame_getsetlist[] = { static void _Py_HOT_FUNCTION frame_dealloc(PyFrameObject *f) { - if (_PyObject_GC_IS_TRACKED(f)) { - _PyObject_GC_UNTRACK(f); - } - - Py_TRASHCAN_BEGIN(f, frame_dealloc); PyCodeObject *co = NULL; /* Kill all local variables including specials, if we own them */ @@ -659,7 +654,6 @@ frame_dealloc(PyFrameObject *f) } Py_XDECREF(co); - Py_TRASHCAN_END; } static int diff --git a/Objects/funcobject.c b/Objects/funcobject.c index 5a170380cb3e9b..52dee8bfa9908c 100644 --- a/Objects/funcobject.c +++ b/Objects/funcobject.c @@ -653,7 +653,6 @@ func_clear(PyFunctionObject *op) static void func_dealloc(PyFunctionObject *op) { - _PyObject_GC_UNTRACK(op); if (op->func_weakreflist != NULL) { PyObject_ClearWeakRefs((PyObject *) op); } @@ -814,7 +813,6 @@ typedef struct { static void cm_dealloc(classmethod *cm) { - _PyObject_GC_UNTRACK((PyObject *)cm); Py_XDECREF(cm->cm_callable); Py_XDECREF(cm->cm_dict); Py_TYPE(cm)->tp_free((PyObject *)cm); @@ -1011,7 +1009,6 @@ typedef struct { static void sm_dealloc(staticmethod *sm) { - _PyObject_GC_UNTRACK((PyObject *)sm); Py_XDECREF(sm->sm_callable); Py_XDECREF(sm->sm_dict); Py_TYPE(sm)->tp_free((PyObject *)sm); diff --git a/Objects/genericaliasobject.c b/Objects/genericaliasobject.c index 38b68e410c443c..0b0137c87ac27f 100644 --- a/Objects/genericaliasobject.c +++ b/Objects/genericaliasobject.c @@ -18,7 +18,6 @@ ga_dealloc(PyObject *self) { gaobject *alias = (gaobject *)self; - _PyObject_GC_UNTRACK(self); if (alias->weakreflist != NULL) { PyObject_ClearWeakRefs((PyObject *)alias); } diff --git a/Objects/genobject.c b/Objects/genobject.c index 86cd9cf7254589..569f9f85c3527c 100644 --- a/Objects/genobject.c +++ b/Objects/genobject.c @@ -113,8 +113,6 @@ gen_dealloc(PyGenObject *gen) { PyObject *self = (PyObject *) gen; - _PyObject_GC_UNTRACK(gen); - if (gen->gi_weakreflist != NULL) PyObject_ClearWeakRefs(self); @@ -1183,7 +1181,6 @@ PyTypeObject PyCoro_Type = { static void coro_wrapper_dealloc(PyCoroWrapper *cw) { - _PyObject_GC_UNTRACK((PyObject *)cw); Py_CLEAR(cw->cw_coroutine); PyObject_GC_Del(cw); } @@ -1663,7 +1660,6 @@ async_gen_unwrap_value(PyAsyncGenObject *gen, PyObject *result) static void async_gen_asend_dealloc(PyAsyncGenASend *o) { - _PyObject_GC_UNTRACK((PyObject *)o); Py_CLEAR(o->ags_gen); Py_CLEAR(o->ags_sendval); struct _Py_async_gen_state *state = get_async_gen_state(); @@ -1864,7 +1860,6 @@ async_gen_asend_new(PyAsyncGenObject *gen, PyObject *sendval) static void async_gen_wrapped_val_dealloc(_PyAsyncGenWrappedValue *o) { - _PyObject_GC_UNTRACK((PyObject *)o); Py_CLEAR(o->agw_val); struct _Py_async_gen_state *state = get_async_gen_state(); #ifdef Py_DEBUG @@ -1970,7 +1965,6 @@ _PyAsyncGenValueWrapperNew(PyObject *val) static void async_gen_athrow_dealloc(PyAsyncGenAThrow *o) { - _PyObject_GC_UNTRACK((PyObject *)o); Py_CLEAR(o->agt_gen); Py_CLEAR(o->agt_args); PyObject_GC_Del(o); diff --git a/Objects/iterobject.c b/Objects/iterobject.c index 6961fc3b4a9497..0deb48ec2a24cd 100644 --- a/Objects/iterobject.c +++ b/Objects/iterobject.c @@ -33,7 +33,6 @@ PySeqIter_New(PyObject *seq) static void iter_dealloc(seqiterobject *it) { - _PyObject_GC_UNTRACK(it); Py_XDECREF(it->it_seq); PyObject_GC_Del(it); } @@ -194,7 +193,6 @@ PyCallIter_New(PyObject *callable, PyObject *sentinel) static void calliter_dealloc(calliterobject *it) { - _PyObject_GC_UNTRACK(it); Py_XDECREF(it->it_callable); Py_XDECREF(it->it_sentinel); PyObject_GC_Del(it); @@ -299,7 +297,6 @@ typedef struct { static void anextawaitable_dealloc(anextawaitableobject *obj) { - _PyObject_GC_UNTRACK(obj); Py_XDECREF(obj->wrapped); Py_XDECREF(obj->default_value); PyObject_GC_Del(obj); diff --git a/Objects/listobject.c b/Objects/listobject.c index 898cbc20c5f81c..9cdc953e4fee2a 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -331,8 +331,6 @@ static void list_dealloc(PyListObject *op) { Py_ssize_t i; - PyObject_GC_UnTrack(op); - Py_TRASHCAN_BEGIN(op, list_dealloc) if (op->ob_item != NULL) { /* Do it backwards, for Christian Tismer. There's a simple test case where somehow this reduces @@ -355,7 +353,6 @@ list_dealloc(PyListObject *op) else { Py_TYPE(op)->tp_free((PyObject *)op); } - Py_TRASHCAN_END } static PyObject * @@ -3166,7 +3163,6 @@ list_iter(PyObject *seq) static void listiter_dealloc(listiterobject *it) { - _PyObject_GC_UNTRACK(it); Py_XDECREF(it->it_seq); PyObject_GC_Del(it); } diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c index 913d358062219b..7d06489d2516dc 100644 --- a/Objects/memoryobject.c +++ b/Objects/memoryobject.c @@ -121,7 +121,7 @@ mbuf_release(_PyManagedBufferObject *self) self->flags |= _Py_MANAGED_BUFFER_RELEASED; /* PyBuffer_Release() decrements master->obj and sets it to NULL. */ - _PyObject_GC_UNTRACK(self); + PyObject_GC_UnTrack(self); PyBuffer_Release(&self->master); } @@ -1077,7 +1077,6 @@ static void memory_dealloc(PyMemoryViewObject *self) { assert(self->exports == 0); - _PyObject_GC_UNTRACK(self); (void)_memory_release(self); Py_CLEAR(self->mbuf); if (self->weakreflist != NULL) @@ -3177,7 +3176,6 @@ typedef struct { static void memoryiter_dealloc(memoryiterobject *it) { - _PyObject_GC_UNTRACK(it); Py_XDECREF(it->it_seq); PyObject_GC_Del(it); } diff --git a/Objects/methodobject.c b/Objects/methodobject.c index 2df63cfdf6a818..30437957fb70f0 100644 --- a/Objects/methodobject.c +++ b/Objects/methodobject.c @@ -160,10 +160,6 @@ PyCMethod_GetClass(PyObject *op) static void meth_dealloc(PyCFunctionObject *m) { - // The Py_TRASHCAN mechanism requires that we be able to - // call PyObject_GC_UnTrack twice on an object. - PyObject_GC_UnTrack(m); - Py_TRASHCAN_BEGIN(m, meth_dealloc); if (m->m_weakreflist != NULL) { PyObject_ClearWeakRefs((PyObject*) m); } @@ -173,7 +169,6 @@ meth_dealloc(PyCFunctionObject *m) Py_XDECREF(m->m_self); Py_XDECREF(m->m_module); PyObject_GC_Del(m); - Py_TRASHCAN_END; } static PyObject * diff --git a/Objects/object.c b/Objects/object.c index 446c974f8e614b..8abde96371aa21 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -249,6 +249,10 @@ PyObject_CallFinalizerFromDealloc(PyObject *self) _Py_NewReference(self); Py_SET_REFCNT(self, refcnt); + if (_PyType_IS_GC(Py_TYPE(self)) && !_PyObject_GC_IS_TRACKED(self)) { + _PyObject_GC_TRACK(self); + } + _PyObject_ASSERT(self, (!_PyType_IS_GC(Py_TYPE(self)) || _PyObject_GC_IS_TRACKED(self))); @@ -2088,7 +2092,29 @@ Py_ReprLeave(PyObject *obj) PyErr_Restore(error_type, error_value, error_traceback); } -/* Trashcan support. */ +/* Trashcan mechanism, thanks to Christian Tismer. + +When deallocating a container object, it's possible to trigger an unbounded +chain of deallocations, as each Py_DECREF in turn drops the refcount on "the +next" object in the chain to 0. This can easily lead to stack overflows, +especially in threads (which typically have less stack space to work with). + +We avoid this by checking the nesting level of _PyTrash_dealloc calls. + +How it works: Entering _PyTrash_dealloc increments a call-depth counter. So +long as this counter is small, the dealloc method of the type is run directly +without further ado. But if the counter gets large, it instead adds the object +to a list of objects to be deallocated later and skips calling the deallocator. +In that case, _PyTrash_dealloc returns without deallocating anything (and so +unbounded call-stack depth is avoided). + +When the call stack finishes unwinding again, the _PyTrash_dealloc function +notices this, and calls another routine to deallocate all the objects that +may have been added to the list of deferred deallocations. In effect, a +chain of N deallocations is broken into (N-1)/(_PyTrash_UNWIND_LEVEL-1) pieces, +with the call stack never exceeding a depth of _PyTrash_UNWIND_LEVEL. +*/ + #define _PyTrash_UNWIND_LEVEL 50 @@ -2101,6 +2127,11 @@ _PyTrash_thread_deposit_object(PyObject *op) { PyThreadState *tstate = _PyThreadState_GET(); _PyObject_ASSERT(op, _PyObject_IS_GC(op)); + /* untrack is done by _PyTrash_dealloc() so the following assert must be + * true. Previously Py_TRASHCAN_BEGIN() did not untrack itself and it was + * the responsibility of the users of the trashcan mechanism to ensure that + * untrack was called first. + */ _PyObject_ASSERT(op, !_PyObject_GC_IS_TRACKED(op)); _PyObject_ASSERT(op, Py_REFCNT(op) == 0); _PyGCHead_SET_PREV(_Py_AS_GC(op), tstate->trash_delete_later); @@ -2146,17 +2177,11 @@ _PyTrash_thread_destroy_chain(void) --tstate->trash_delete_nesting; } +// FIXME: next three functions are unused, might need for a stable ABI? int _PyTrash_begin(PyThreadState *tstate, PyObject *op) { - if (tstate->trash_delete_nesting >= _PyTrash_UNWIND_LEVEL) { - /* Store the object (to be deallocated later) and jump past - * Py_TRASHCAN_END, skipping the body of the deallocator */ - _PyTrash_thread_deposit_object(op); - return 1; - } - ++tstate->trash_delete_nesting; return 0; } @@ -2164,21 +2189,37 @@ _PyTrash_begin(PyThreadState *tstate, PyObject *op) void _PyTrash_end(PyThreadState *tstate) { - --tstate->trash_delete_nesting; - if (tstate->trash_delete_later && tstate->trash_delete_nesting <= 0) { - _PyTrash_thread_destroy_chain(); - } } - -/* bpo-40170: It's only be used in Py_TRASHCAN_BEGIN macro to hide - implementation details. */ int _PyTrash_cond(PyObject *op, destructor dealloc) { - return Py_TYPE(op)->tp_dealloc == dealloc; + return 0; } +static void +_PyTrash_dealloc(PyObject *op) +{ + if (_PyObject_GC_IS_TRACKED(op)) { + // The trash list re-uses the GC head pointers and so we must untrack + // the object first, if it is tracked. + _PyObject_GC_UNTRACK(op); + } + PyThreadState *tstate = PyThreadState_Get(); + if (tstate->trash_delete_nesting >= _PyTrash_UNWIND_LEVEL) { + /* Store the object (to be deallocated later) */ + _PyTrash_thread_deposit_object(op); + } + else { + ++tstate->trash_delete_nesting; + destructor dealloc = Py_TYPE(op)->tp_dealloc; + (*dealloc)(op); + --tstate->trash_delete_nesting; + if (tstate->trash_delete_later && tstate->trash_delete_nesting <= 0) { + _PyTrash_thread_destroy_chain(); + } + } +} void _Py_NO_RETURN _PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg, @@ -2239,11 +2280,17 @@ _PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg, void _Py_Dealloc(PyObject *op) { - destructor dealloc = Py_TYPE(op)->tp_dealloc; + PyTypeObject *type = Py_TYPE(op); #ifdef Py_TRACE_REFS _Py_ForgetReference(op); #endif - (*dealloc)(op); + if (_PyType_IS_GC(type)) { + _PyTrash_dealloc(op); + } + else { + destructor dealloc = type->tp_dealloc; + (*dealloc)(op); + } } diff --git a/Objects/odictobject.c b/Objects/odictobject.c index e5361da6dcdedb..e6abd1ced946a4 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1365,17 +1365,12 @@ static PyGetSetDef odict_getset[] = { static void odict_dealloc(PyODictObject *self) { - PyObject_GC_UnTrack(self); - Py_TRASHCAN_BEGIN(self, odict_dealloc) - Py_XDECREF(self->od_inst_dict); if (self->od_weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *)self); _odict_clear_nodes(self); PyDict_Type.tp_dealloc((PyObject *)self); - - Py_TRASHCAN_END } /* tp_repr */ @@ -1663,7 +1658,6 @@ typedef struct { static void odictiter_dealloc(odictiterobject *di) { - _PyObject_GC_UNTRACK(di); Py_XDECREF(di->di_odict); Py_XDECREF(di->di_current); if (di->kind & (_odict_ITER_KEYS | _odict_ITER_VALUES)) { diff --git a/Objects/setobject.c b/Objects/setobject.c index caff85c9e38939..b7b8c559fdc5cd 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -485,9 +485,6 @@ set_dealloc(PySetObject *so) setentry *entry; Py_ssize_t used = so->used; - /* bpo-31095: UnTrack is needed before calling any callbacks */ - PyObject_GC_UnTrack(so); - Py_TRASHCAN_BEGIN(so, set_dealloc) if (so->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) so); @@ -500,7 +497,6 @@ set_dealloc(PySetObject *so) if (so->table != so->smalltable) PyMem_Free(so->table); Py_TYPE(so)->tp_free(so); - Py_TRASHCAN_END } static PyObject * @@ -741,8 +737,6 @@ typedef struct { static void setiter_dealloc(setiterobject *si) { - /* bpo-31095: UnTrack is needed before calling any callbacks */ - _PyObject_GC_UNTRACK(si); Py_XDECREF(si->si_set); PyObject_GC_Del(si); } diff --git a/Objects/sliceobject.c b/Objects/sliceobject.c index 22fb7c61c354f9..f3054776366689 100644 --- a/Objects/sliceobject.c +++ b/Objects/sliceobject.c @@ -331,7 +331,6 @@ static void slice_dealloc(PySliceObject *r) { PyInterpreterState *interp = _PyInterpreterState_GET(); - _PyObject_GC_UNTRACK(r); Py_DECREF(r->step); Py_DECREF(r->start); Py_DECREF(r->stop); diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c index b7fd421196ddaa..e4cdceb55253ca 100644 --- a/Objects/tupleobject.c +++ b/Objects/tupleobject.c @@ -263,8 +263,6 @@ static void tupledealloc(PyTupleObject *op) { Py_ssize_t len = Py_SIZE(op); - PyObject_GC_UnTrack(op); - Py_TRASHCAN_BEGIN(op, tupledealloc) if (len > 0) { Py_ssize_t i = len; while (--i >= 0) { @@ -283,16 +281,11 @@ tupledealloc(PyTupleObject *op) op->ob_item[0] = (PyObject *) state->free_list[len]; state->numfree[len]++; state->free_list[len] = op; - goto done; /* return */ + return; } #endif } Py_TYPE(op)->tp_free((PyObject *)op); - -#if PyTuple_MAXSAVESIZE > 0 -done: -#endif - Py_TRASHCAN_END } static PyObject * @@ -1067,7 +1060,6 @@ typedef struct { static void tupleiter_dealloc(tupleiterobject *it) { - _PyObject_GC_UNTRACK(it); Py_XDECREF(it->it_seq); PyObject_GC_Del(it); } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 7ae50c453ed2f8..7eacf5d2c59f43 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -1381,11 +1381,6 @@ subtype_dealloc(PyObject *self) /* We get here only if the type has GC */ - /* UnTrack and re-Track around the trashcan macro, alas */ - /* See explanation at end of function for full disclosure */ - PyObject_GC_UnTrack(self); - Py_TRASHCAN_BEGIN(self, subtype_dealloc); - /* Find the nearest base with a different tp_dealloc */ base = type; while ((/*basedealloc =*/ base->tp_dealloc) == subtype_dealloc) { @@ -1399,7 +1394,7 @@ subtype_dealloc(PyObject *self) _PyObject_GC_TRACK(self); if (PyObject_CallFinalizerFromDealloc(self) < 0) { /* Resurrected */ - goto endlabel; + return; } _PyObject_GC_UNTRACK(self); } @@ -1420,7 +1415,7 @@ subtype_dealloc(PyObject *self) type->tp_del(self); if (Py_REFCNT(self) > 0) { /* Resurrected */ - goto endlabel; + return; } _PyObject_GC_UNTRACK(self); } @@ -1485,45 +1480,6 @@ subtype_dealloc(PyObject *self) Py_DECREF(type); } - endlabel: - Py_TRASHCAN_END - - /* Explanation of the weirdness around the trashcan macros: - - Q. What do the trashcan macros do? - - A. Read the comment titled "Trashcan mechanism" in object.h. - For one, this explains why there must be a call to GC-untrack - before the trashcan begin macro. Without understanding the - trashcan code, the answers to the following questions don't make - sense. - - Q. Why do we GC-untrack before the trashcan and then immediately - GC-track again afterward? - - A. In the case that the base class is GC-aware, the base class - probably GC-untracks the object. If it does that using the - UNTRACK macro, this will crash when the object is already - untracked. Because we don't know what the base class does, the - only safe thing is to make sure the object is tracked when we - call the base class dealloc. But... The trashcan begin macro - requires that the object is *untracked* before it is called. So - the dance becomes: - - GC untrack - trashcan begin - GC track - - Q. Why did the last question say "immediately GC-track again"? - It's nowhere near immediately. - - A. Because the code *used* to re-track immediately. Bad Idea. - self has a refcount of 0, and if gc ever gets its hands on it - (which can happen if any weakref callback gets invoked), it - looks like trash to gc too, and gc also tries to delete self - then. But we're already deleting self. Double deallocation is - a subtle disaster. - */ } static PyTypeObject *solid_base(PyTypeObject *type); @@ -4040,7 +3996,6 @@ type_dealloc(PyTypeObject *type) /* Assert this is a heap-allocated type object */ _PyObject_ASSERT((PyObject *)type, type->tp_flags & Py_TPFLAGS_HEAPTYPE); - _PyObject_GC_UNTRACK(type); PyErr_Fetch(&tp, &val, &tb); remove_all_subclasses(type, type->tp_bases); PyErr_Restore(tp, val, tb); @@ -8682,7 +8637,6 @@ super_dealloc(PyObject *self) { superobject *su = (superobject *)self; - _PyObject_GC_UNTRACK(self); Py_XDECREF(su->obj); Py_XDECREF(su->type); Py_XDECREF(su->obj_type); diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c index 3e6b70bf4b6f54..f5a7d6feb0b486 100644 --- a/Objects/unicodeobject.c +++ b/Objects/unicodeobject.c @@ -15632,7 +15632,6 @@ typedef struct { static void unicodeiter_dealloc(unicodeiterobject *it) { - _PyObject_GC_UNTRACK(it); Py_XDECREF(it->it_seq); PyObject_GC_Del(it); } diff --git a/Objects/unionobject.c b/Objects/unionobject.c index 80c70389ab30d6..c468baefc0e133 100644 --- a/Objects/unionobject.c +++ b/Objects/unionobject.c @@ -19,8 +19,6 @@ unionobject_dealloc(PyObject *self) { unionobject *alias = (unionobject *)self; - _PyObject_GC_UNTRACK(self); - Py_XDECREF(alias->args); Py_XDECREF(alias->parameters); Py_TYPE(self)->tp_free(self); diff --git a/Python/context.c b/Python/context.c index bf2ba93c14eb8f..580dd7ff723978 100644 --- a/Python/context.c +++ b/Python/context.c @@ -462,8 +462,6 @@ context_tp_traverse(PyContext *self, visitproc visit, void *arg) static void context_tp_dealloc(PyContext *self) { - _PyObject_GC_UNTRACK(self); - if (self->ctx_weakreflist != NULL) { PyObject_ClearWeakRefs((PyObject*)self); } diff --git a/Python/hamt.c b/Python/hamt.c index e272e8808fd956..cf91a5897b2e2f 100644 --- a/Python/hamt.c +++ b/Python/hamt.c @@ -1155,9 +1155,6 @@ hamt_node_bitmap_dealloc(PyHamtNode_Bitmap *self) Py_ssize_t len = Py_SIZE(self); Py_ssize_t i; - PyObject_GC_UnTrack(self); - Py_TRASHCAN_BEGIN(self, hamt_node_bitmap_dealloc) - if (len > 0) { i = len; while (--i >= 0) { @@ -1166,7 +1163,6 @@ hamt_node_bitmap_dealloc(PyHamtNode_Bitmap *self) } Py_TYPE(self)->tp_free((PyObject *)self); - Py_TRASHCAN_END } #ifdef Py_DEBUG @@ -1563,9 +1559,6 @@ hamt_node_collision_dealloc(PyHamtNode_Collision *self) Py_ssize_t len = Py_SIZE(self); - PyObject_GC_UnTrack(self); - Py_TRASHCAN_BEGIN(self, hamt_node_collision_dealloc) - if (len > 0) { while (--len >= 0) { @@ -1574,7 +1567,6 @@ hamt_node_collision_dealloc(PyHamtNode_Collision *self) } Py_TYPE(self)->tp_free((PyObject *)self); - Py_TRASHCAN_END } #ifdef Py_DEBUG @@ -1948,15 +1940,11 @@ hamt_node_array_dealloc(PyHamtNode_Array *self) Py_ssize_t i; - PyObject_GC_UnTrack(self); - Py_TRASHCAN_BEGIN(self, hamt_node_array_dealloc) - for (i = 0; i < HAMT_ARRAY_NODE_SIZE; i++) { Py_XDECREF(self->a_array[i]); } Py_TYPE(self)->tp_free((PyObject *)self); - Py_TRASHCAN_END } #ifdef Py_DEBUG diff --git a/Python/traceback.c b/Python/traceback.c index 204121ba66d96e..105c3b4d244e5a 100644 --- a/Python/traceback.c +++ b/Python/traceback.c @@ -168,12 +168,9 @@ static PyGetSetDef tb_getsetters[] = { static void tb_dealloc(PyTracebackObject *tb) { - PyObject_GC_UnTrack(tb); - Py_TRASHCAN_BEGIN(tb, tb_dealloc) Py_XDECREF(tb->tb_next); Py_XDECREF(tb->tb_frame); PyObject_GC_Del(tb); - Py_TRASHCAN_END } static int From a0698102a7badd2ca5b71f8a9245f068e5a8989b Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Fri, 13 Aug 2021 00:03:38 -0700 Subject: [PATCH 2/5] Remove unneeded GC TRACK/UNTRACK calls. Since _Py_Dealloc will untrack, don't need to do it in tp_dealloc. If an object is resurrected, PyObject_CallFinalizerFromDealloc() will do the track call. --- Objects/genobject.c | 3 --- Objects/object.c | 15 +++++++++------ Objects/typeobject.c | 9 --------- 3 files changed, 9 insertions(+), 18 deletions(-) diff --git a/Objects/genobject.c b/Objects/genobject.c index 569f9f85c3527c..e8ea0b545b4608 100644 --- a/Objects/genobject.c +++ b/Objects/genobject.c @@ -116,12 +116,9 @@ gen_dealloc(PyGenObject *gen) if (gen->gi_weakreflist != NULL) PyObject_ClearWeakRefs(self); - _PyObject_GC_TRACK(self); - if (PyObject_CallFinalizerFromDealloc(self)) return; /* resurrected. :( */ - _PyObject_GC_UNTRACK(self); if (PyAsyncGen_CheckExact(gen)) { /* We have to handle this case for asynchronous generators right here, because this code has to be between UNTRACK diff --git a/Objects/object.c b/Objects/object.c index 8abde96371aa21..ae5d0162ed62a9 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -249,13 +249,16 @@ PyObject_CallFinalizerFromDealloc(PyObject *self) _Py_NewReference(self); Py_SET_REFCNT(self, refcnt); - if (_PyType_IS_GC(Py_TYPE(self)) && !_PyObject_GC_IS_TRACKED(self)) { - _PyObject_GC_TRACK(self); + if (_PyType_IS_GC(Py_TYPE(self))) { + /* If we get here, it must have originally been tracked when + * _Py_Dealloc was called. Make it tracked again so it can't become + * part of a reference cycle and leak. + */ + if (!_PyObject_GC_IS_TRACKED(self)) { + _PyObject_GC_TRACK(self); + } } - _PyObject_ASSERT(self, - (!_PyType_IS_GC(Py_TYPE(self)) - || _PyObject_GC_IS_TRACKED(self))); /* If Py_REF_DEBUG macro is defined, _Py_NewReference() increased _Py_RefTotal, so we need to undo that. */ #ifdef Py_REF_DEBUG @@ -2201,7 +2204,7 @@ static void _PyTrash_dealloc(PyObject *op) { if (_PyObject_GC_IS_TRACKED(op)) { - // The trash list re-uses the GC head pointers and so we must untrack + // The trash list re-uses the GC prev pointer and so we must untrack // the object first, if it is tracked. _PyObject_GC_UNTRACK(op); } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 7eacf5d2c59f43..0d0e8d2f20dd26 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -1391,12 +1391,10 @@ subtype_dealloc(PyObject *self) has_finalizer = type->tp_finalize || type->tp_del; if (type->tp_finalize) { - _PyObject_GC_TRACK(self); if (PyObject_CallFinalizerFromDealloc(self) < 0) { /* Resurrected */ return; } - _PyObject_GC_UNTRACK(self); } /* If we added a weaklist, we clear it. Do this *before* calling tp_del, @@ -1457,13 +1455,6 @@ subtype_dealloc(PyObject *self) /* Extract the type again; tp_del may have changed it */ type = Py_TYPE(self); - /* Call the base tp_dealloc(); first retrack self if - * basedealloc knows about gc. - */ - if (_PyType_IS_GC(base)) { - _PyObject_GC_TRACK(self); - } - // Don't read type memory after calling basedealloc() since basedealloc() // can deallocate the type and free its memory. int type_needs_decref = (type->tp_flags & Py_TPFLAGS_HEAPTYPE From 692191f72b08895cd87c009070e7156998b4f201 Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Fri, 13 Aug 2021 23:52:48 -0700 Subject: [PATCH 3/5] Make _Py_Dealloc use _PyObject_IS_GC(). I'm not sure _PyType_IS_GC() is safe in case the object returns false from tp_is_gc(). --- Objects/object.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Objects/object.c b/Objects/object.c index ae5d0162ed62a9..b6c04b854fbd11 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2283,14 +2283,20 @@ _PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg, void _Py_Dealloc(PyObject *op) { - PyTypeObject *type = Py_TYPE(op); #ifdef Py_TRACE_REFS _Py_ForgetReference(op); #endif - if (_PyType_IS_GC(type)) { + // Note: it might be safe to use _PyType_IS_GC() here, for a small + // optimization. The question is if a type that implements tp_is_gc() that + // returns false can ever be deallocated. For PyType_Type, the answer + // seems to be no since those objects are statically allocated. However, + // it seems possible that some 3rd party type could have a heap allocated + // instance where tp_is_gc() returns false. + if (_PyObject_IS_GC(op)) { _PyTrash_dealloc(op); } else { + PyTypeObject *type = Py_TYPE(op); destructor dealloc = type->tp_dealloc; (*dealloc)(op); } From 6ce59ff98caf045b76751a1c65262ef222924497 Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Mon, 16 Aug 2021 10:52:49 -0700 Subject: [PATCH 4/5] Small optimization for _Py_Dealloc(). --- Objects/object.c | 91 ++++++++++++++++++++++++++---------------------- 1 file changed, 49 insertions(+), 42 deletions(-) diff --git a/Objects/object.c b/Objects/object.c index b6c04b854fbd11..8a63aa4e89017b 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -249,7 +249,7 @@ PyObject_CallFinalizerFromDealloc(PyObject *self) _Py_NewReference(self); Py_SET_REFCNT(self, refcnt); - if (_PyType_IS_GC(Py_TYPE(self))) { + if (_PyObject_IS_GC(self)) { /* If we get here, it must have originally been tracked when * _Py_Dealloc was called. Make it tracked again so it can't become * part of a reference cycle and leak. @@ -2102,16 +2102,16 @@ chain of deallocations, as each Py_DECREF in turn drops the refcount on "the next" object in the chain to 0. This can easily lead to stack overflows, especially in threads (which typically have less stack space to work with). -We avoid this by checking the nesting level of _PyTrash_dealloc calls. +We avoid this by checking the nesting level of dealloc_gc() calls. -How it works: Entering _PyTrash_dealloc increments a call-depth counter. So +How it works: Entering dealloc_gc() increments a call-depth counter. So long as this counter is small, the dealloc method of the type is run directly without further ado. But if the counter gets large, it instead adds the object to a list of objects to be deallocated later and skips calling the deallocator. -In that case, _PyTrash_dealloc returns without deallocating anything (and so +In that case, dealloc_gc() returns without deallocating anything (and so unbounded call-stack depth is avoided). -When the call stack finishes unwinding again, the _PyTrash_dealloc function +When the call stack finishes unwinding again, the dealloc_gc() function notices this, and calls another routine to deallocate all the objects that may have been added to the list of deferred deallocations. In effect, a chain of N deallocations is broken into (N-1)/(_PyTrash_UNWIND_LEVEL-1) pieces, @@ -2130,7 +2130,7 @@ _PyTrash_thread_deposit_object(PyObject *op) { PyThreadState *tstate = _PyThreadState_GET(); _PyObject_ASSERT(op, _PyObject_IS_GC(op)); - /* untrack is done by _PyTrash_dealloc() so the following assert must be + /* untrack is done by dealloc_gc()() so the following assert must be * true. Previously Py_TRASHCAN_BEGIN() did not untrack itself and it was * the responsibility of the users of the trashcan mechanism to ensure that * untrack was called first. @@ -2200,30 +2200,6 @@ _PyTrash_cond(PyObject *op, destructor dealloc) return 0; } -static void -_PyTrash_dealloc(PyObject *op) -{ - if (_PyObject_GC_IS_TRACKED(op)) { - // The trash list re-uses the GC prev pointer and so we must untrack - // the object first, if it is tracked. - _PyObject_GC_UNTRACK(op); - } - PyThreadState *tstate = PyThreadState_Get(); - if (tstate->trash_delete_nesting >= _PyTrash_UNWIND_LEVEL) { - /* Store the object (to be deallocated later) */ - _PyTrash_thread_deposit_object(op); - } - else { - ++tstate->trash_delete_nesting; - destructor dealloc = Py_TYPE(op)->tp_dealloc; - (*dealloc)(op); - --tstate->trash_delete_nesting; - if (tstate->trash_delete_later && tstate->trash_delete_nesting <= 0) { - _PyTrash_thread_destroy_chain(); - } - } -} - void _Py_NO_RETURN _PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg, const char *file, int line, const char *function) @@ -2279,6 +2255,37 @@ _PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg, Py_FatalError("_PyObject_AssertFailed"); } +static void +dealloc_gc(PyObject *op) +{ + if (_PyObject_GC_IS_TRACKED(op)) { + // The trash list re-uses the GC prev pointer and so we must untrack + // the object first, if it is tracked. + _PyObject_GC_UNTRACK(op); + } + PyThreadState *tstate = PyThreadState_Get(); + if (tstate->trash_delete_nesting >= _PyTrash_UNWIND_LEVEL) { + /* Store the object (to be deallocated later) */ + _PyTrash_thread_deposit_object(op); + } + else { + ++tstate->trash_delete_nesting; + destructor dealloc = Py_TYPE(op)->tp_dealloc; + (*dealloc)(op); + --tstate->trash_delete_nesting; + if (tstate->trash_delete_later && tstate->trash_delete_nesting <= 0) { + _PyTrash_thread_destroy_chain(); + } + } +} + +static void +dealloc_simple(PyObject *op) +{ + PyTypeObject *type = Py_TYPE(op); + destructor dealloc = type->tp_dealloc; + (*dealloc)(op); +} void _Py_Dealloc(PyObject *op) @@ -2286,23 +2293,23 @@ _Py_Dealloc(PyObject *op) #ifdef Py_TRACE_REFS _Py_ForgetReference(op); #endif - // Note: it might be safe to use _PyType_IS_GC() here, for a small - // optimization. The question is if a type that implements tp_is_gc() that - // returns false can ever be deallocated. For PyType_Type, the answer - // seems to be no since those objects are statically allocated. However, - // it seems possible that some 3rd party type could have a heap allocated - // instance where tp_is_gc() returns false. - if (_PyObject_IS_GC(op)) { - _PyTrash_dealloc(op); + // Note: using _PyType_IS_GC() here is probably not safe. A 3rd party + // extension type might have a tp_is_gc() that returns false for heap + // allocated instances. It doesn't seem likely but it's possible. + PyTypeObject *type = Py_TYPE(op); + if (!_PyType_IS_GC(type)) { + dealloc_simple(op); } else { - PyTypeObject *type = Py_TYPE(op); - destructor dealloc = type->tp_dealloc; - (*dealloc)(op); + if (type->tp_is_gc == NULL || type->tp_is_gc(op)) { + dealloc_gc(op); + } + else { + dealloc_simple(op); + } } } - PyObject ** PyObject_GET_WEAKREFS_LISTPTR(PyObject *op) { From 92653a8209c7967a5c30b884995243e3acdb089e Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Mon, 16 Aug 2021 11:46:14 -0700 Subject: [PATCH 5/5] Tidy up code, improve comments. --- Include/cpython/object.h | 7 +- Objects/object.c | 136 +++++++++++++++++---------------------- 2 files changed, 62 insertions(+), 81 deletions(-) diff --git a/Include/cpython/object.h b/Include/cpython/object.h index ca55cdee569c5d..c7518d96284ce3 100644 --- a/Include/cpython/object.h +++ b/Include/cpython/object.h @@ -456,14 +456,15 @@ PyAPI_FUNC(int) _PyObject_CheckConsistency( int check_content); -#if 1 -/* do nothing trashcan macros, for backwards compatibility */ +/* Backwards compatibility macros. The trashcan mechanism is now integrated + * with _Py_Dealloc and so these do nothing. The assert() is present to handle + * the case that a 'goto' statement precedes Py_TRASHCAN_END. */ #define Py_TRASHCAN_BEGIN(op, dealloc) \ do { #define Py_TRASHCAN_END \ assert(1); \ } while (0); -#endif + /* For backwards compatibility, these macros enable the trashcan * unconditionally */ diff --git a/Objects/object.c b/Objects/object.c index 8a63aa4e89017b..5e30a3ae567208 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2095,6 +2095,61 @@ Py_ReprLeave(PyObject *obj) PyErr_Restore(error_type, error_value, error_traceback); } +void _Py_NO_RETURN +_PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg, + const char *file, int line, const char *function) +{ + fprintf(stderr, "%s:%d: ", file, line); + if (function) { + fprintf(stderr, "%s: ", function); + } + fflush(stderr); + + if (expr) { + fprintf(stderr, "Assertion \"%s\" failed", expr); + } + else { + fprintf(stderr, "Assertion failed"); + } + fflush(stderr); + + if (msg) { + fprintf(stderr, ": %s", msg); + } + fprintf(stderr, "\n"); + fflush(stderr); + + if (_PyObject_IsFreed(obj)) { + /* It seems like the object memory has been freed: + don't access it to prevent a segmentation fault. */ + fprintf(stderr, "\n", obj); + fflush(stderr); + } + else { + /* Display the traceback where the object has been allocated. + Do it before dumping repr(obj), since repr() is more likely + to crash than dumping the traceback. */ + void *ptr; + PyTypeObject *type = Py_TYPE(obj); + if (_PyType_IS_GC(type)) { + ptr = (void *)((char *)obj - sizeof(PyGC_Head)); + } + else { + ptr = (void *)obj; + } + _PyMem_DumpTraceback(fileno(stderr), ptr); + + /* This might succeed or fail, but we're about to abort, so at least + try to provide any extra info we can: */ + _PyObject_Dump(obj); + + fprintf(stderr, "\n"); + fflush(stderr); + } + + Py_FatalError("_PyObject_AssertFailed"); +} + /* Trashcan mechanism, thanks to Christian Tismer. When deallocating a container object, it's possible to trigger an unbounded @@ -2118,7 +2173,6 @@ chain of N deallocations is broken into (N-1)/(_PyTrash_UNWIND_LEVEL-1) pieces, with the call stack never exceeding a depth of _PyTrash_UNWIND_LEVEL. */ - #define _PyTrash_UNWIND_LEVEL 50 /* Add op to the gcstate->trash_delete_later list. Called when the current @@ -2180,81 +2234,6 @@ _PyTrash_thread_destroy_chain(void) --tstate->trash_delete_nesting; } -// FIXME: next three functions are unused, might need for a stable ABI? - -int -_PyTrash_begin(PyThreadState *tstate, PyObject *op) -{ - return 0; -} - - -void -_PyTrash_end(PyThreadState *tstate) -{ -} - -int -_PyTrash_cond(PyObject *op, destructor dealloc) -{ - return 0; -} - -void _Py_NO_RETURN -_PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg, - const char *file, int line, const char *function) -{ - fprintf(stderr, "%s:%d: ", file, line); - if (function) { - fprintf(stderr, "%s: ", function); - } - fflush(stderr); - - if (expr) { - fprintf(stderr, "Assertion \"%s\" failed", expr); - } - else { - fprintf(stderr, "Assertion failed"); - } - fflush(stderr); - - if (msg) { - fprintf(stderr, ": %s", msg); - } - fprintf(stderr, "\n"); - fflush(stderr); - - if (_PyObject_IsFreed(obj)) { - /* It seems like the object memory has been freed: - don't access it to prevent a segmentation fault. */ - fprintf(stderr, "\n", obj); - fflush(stderr); - } - else { - /* Display the traceback where the object has been allocated. - Do it before dumping repr(obj), since repr() is more likely - to crash than dumping the traceback. */ - void *ptr; - PyTypeObject *type = Py_TYPE(obj); - if (_PyType_IS_GC(type)) { - ptr = (void *)((char *)obj - sizeof(PyGC_Head)); - } - else { - ptr = (void *)obj; - } - _PyMem_DumpTraceback(fileno(stderr), ptr); - - /* This might succeed or fail, but we're about to abort, so at least - try to provide any extra info we can: */ - _PyObject_Dump(obj); - - fprintf(stderr, "\n"); - fflush(stderr); - } - - Py_FatalError("_PyObject_AssertFailed"); -} - static void dealloc_gc(PyObject *op) { @@ -2287,16 +2266,17 @@ dealloc_simple(PyObject *op) (*dealloc)(op); } +/* Called by Py_DECREF() when refcount goes to zero. */ void _Py_Dealloc(PyObject *op) { #ifdef Py_TRACE_REFS _Py_ForgetReference(op); #endif + PyTypeObject *type = Py_TYPE(op); // Note: using _PyType_IS_GC() here is probably not safe. A 3rd party // extension type might have a tp_is_gc() that returns false for heap - // allocated instances. It doesn't seem likely but it's possible. - PyTypeObject *type = Py_TYPE(op); + // allocated instances. It doesn't seem likely but seems possible. if (!_PyType_IS_GC(type)) { dealloc_simple(op); }