From 06c9d7e23de595e8ef84a8967ae1be6124f84cf2 Mon Sep 17 00:00:00 2001 From: Robert O'Shea Date: Fri, 29 Jul 2022 20:17:44 +0100 Subject: [PATCH 1/5] gh-95348: Split clear_weakref into two functions --- Objects/weakrefobject.c | 70 ++++++++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index cf89a9231d204d..36b935036f45c7 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -46,38 +46,54 @@ new_weakref(PyObject *ob, PyObject *callback) } -/* This function clears the passed-in reference and removes it from the - * list of weak references for the referent. This is the only code that - * removes an item from the doubly-linked list of weak references for an - * object; it is also responsible for clearing the callback slot. +/* This function is responsible for clearing the callback slot. When using this + * It must be called before calling remove_weakref, otherwise a segmentation fault will + * occur at build time. */ static void clear_weakref(PyWeakReference *self) { PyObject *callback = self->wr_callback; - - if (self->wr_object != Py_None) { - PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); - - if (*list == self) - /* If 'self' is the end of the list (and thus self->wr_next == NULL) - then the weakref list itself (and thus the value of *list) will - end up being set to NULL. */ - *list = self->wr_next; - self->wr_object = Py_None; - if (self->wr_prev != NULL) - self->wr_prev->wr_next = self->wr_next; - if (self->wr_next != NULL) - self->wr_next->wr_prev = self->wr_prev; - self->wr_prev = NULL; - self->wr_next = NULL; - } if (callback != NULL) { Py_DECREF(callback); self->wr_callback = NULL; } } +/* This function removes the pass in reference from the list of weak references + * for the referent. This should be called from remove_weakref_fromt_referent + * as it will pass the required list argument. + */ +static void +remove_weakref(PyWeakReference *self, PyWeakReference **list) +{ + if (*list == self) + /* If 'self' is the end of the list (and thus self->wr_next == NULL) + then the weakref list itself (and thus the value of *list) will + end up being set to NULL. */ + *list = self->wr_next; + self->wr_object = Py_None; + if (self->wr_prev != NULL) + self->wr_prev->wr_next = self->wr_next; + if (self->wr_next != NULL) + self->wr_next->wr_prev = self->wr_prev; + self->wr_prev = NULL; + self->wr_next = NULL; +} + +/* This function removes the pass in reference from the list of weak references + * for the referent. This should be called after clear_weakref, otherwise a + * segmentation fault will occur at build time. + */ +static void +remove_weakref_from_referent(PyWeakReference *self) +{ + if (self->wr_object != Py_None) { + PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); + remove_weakref(self, list); + } +} + /* Cyclic gc uses this to *just* clear the passed-in reference, leaving * the callback intact and uncalled. It must be possible to call self's * tp_dealloc() after calling this, so self has to be left in a sane enough @@ -100,6 +116,7 @@ _PyWeakref_ClearRef(PyWeakReference *self) callback = self->wr_callback; self->wr_callback = NULL; clear_weakref(self); + remove_weakref_from_referent(self); self->wr_callback = callback; } @@ -108,6 +125,7 @@ weakref_dealloc(PyObject *self) { PyObject_GC_UnTrack(self); clear_weakref((PyWeakReference *) self); + remove_weakref_from_referent((PyWeakReference *) self); Py_TYPE(self)->tp_free(self); } @@ -124,6 +142,7 @@ static int gc_clear(PyWeakReference *self) { clear_weakref(self); + remove_weakref_from_referent(self); return 0; } @@ -562,6 +581,7 @@ proxy_dealloc(PyWeakReference *self) if (self->wr_callback != NULL) PyObject_GC_UnTrack((PyObject *)self); clear_weakref(self); + remove_weakref_from_referent(self); PyObject_GC_Del(self); } @@ -958,8 +978,11 @@ PyObject_ClearWeakRefs(PyObject *object) /* Remove the callback-less basic and proxy references */ if (*list != NULL && (*list)->wr_callback == NULL) { clear_weakref(*list); - if (*list != NULL && (*list)->wr_callback == NULL) + remove_weakref_from_referent(*list); + if (*list != NULL && (*list)->wr_callback == NULL) { clear_weakref(*list); + remove_weakref_from_referent(*list); + } } if (*list != NULL) { PyWeakReference *current = *list; @@ -972,6 +995,7 @@ PyObject_ClearWeakRefs(PyObject *object) current->wr_callback = NULL; clear_weakref(current); + remove_weakref_from_referent(current); if (callback != NULL) { if (Py_REFCNT((PyObject *)current) > 0) { handle_callback(current, callback); @@ -1002,6 +1026,7 @@ PyObject_ClearWeakRefs(PyObject *object) } current->wr_callback = NULL; clear_weakref(current); + remove_weakref_from_referent(current); current = next; } for (i = 0; i < count; ++i) { @@ -1036,5 +1061,6 @@ _PyStaticType_ClearWeakRefs(PyTypeObject *type) weaklist before clearing its wr_object and wr_callback. That is how we're able to loop over the list. */ clear_weakref((PyWeakReference *)*list); + remove_weakref_from_referent((PyWeakReference *)*list); } } From 40f247f598540b6225c3c30699b52ccdb2e7b7c7 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Fri, 29 Jul 2022 19:35:31 +0000 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst diff --git a/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst b/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst new file mode 100644 index 00000000000000..5bb8e868f8d9c1 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst @@ -0,0 +1,3 @@ +Split `clear_weakref` into two functions: `clear_weakref` +and `remove_weakref`. Abstracts `remove_weakref` with +`remove_weakref_from_referent`. Patch by Robert O'Shea. From 8a24c4f4df70fc7e2d3d97b87a03bf22dcb4a73e Mon Sep 17 00:00:00 2001 From: Robert O'Shea Date: Fri, 29 Jul 2022 20:42:20 +0100 Subject: [PATCH 3/5] gh-95348 Update NEWS blub --- .../C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst b/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst index 5bb8e868f8d9c1..cc518a9aa44069 100644 --- a/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst +++ b/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst @@ -1,3 +1,4 @@ -Split `clear_weakref` into two functions: `clear_weakref` -and `remove_weakref`. Abstracts `remove_weakref` with -`remove_weakref_from_referent`. Patch by Robert O'Shea. +Split ``clear_weakref`` into two functions: ``clear_weakref`` +and ``remove_weakref``. Abstracts ``remove_weakref`` with +``remove_weakref_from_referent`` in weakrefobject.c. +Patch by Robert O'Shea. From 355625aaeea38daf65b8a540bc6c0cc50551cea2 Mon Sep 17 00:00:00 2001 From: Robert O'Shea Date: Mon, 1 Aug 2022 18:37:44 +0100 Subject: [PATCH 4/5] gh-95348: Update new functions in weakrefobject.c --- Objects/weakrefobject.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 36b935036f45c7..454c0f18d92480 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -47,36 +47,41 @@ new_weakref(PyObject *ob, PyObject *callback) /* This function is responsible for clearing the callback slot. When using this - * It must be called before calling remove_weakref, otherwise a segmentation fault will - * occur at build time. + * function it must be called before calling remove_weakref_from_referent, + * otherwise a segmentation fault will occur at build time. */ static void clear_weakref(PyWeakReference *self) { PyObject *callback = self->wr_callback; + if (callback != NULL) { Py_DECREF(callback); self->wr_callback = NULL; } } -/* This function removes the pass in reference from the list of weak references - * for the referent. This should be called from remove_weakref_fromt_referent - * as it will pass the required list argument. +/* This function removes the pass in reference from the list of weak + * references for the referent. This should be called from + * remove_weakref_fromt_referent as it will pass the required list + * argument. */ static void remove_weakref(PyWeakReference *self, PyWeakReference **list) { - if (*list == self) + if (*list == self) { /* If 'self' is the end of the list (and thus self->wr_next == NULL) then the weakref list itself (and thus the value of *list) will end up being set to NULL. */ *list = self->wr_next; + } self->wr_object = Py_None; - if (self->wr_prev != NULL) + if (self->wr_prev != NULL) { self->wr_prev->wr_next = self->wr_next; - if (self->wr_next != NULL) + } + if (self->wr_next != NULL) { self->wr_next->wr_prev = self->wr_prev; + } self->wr_prev = NULL; self->wr_next = NULL; } From 1e8e522261b4de0aaf122846bd2977ea05dc09f8 Mon Sep 17 00:00:00 2001 From: Robert O'Shea Date: Fri, 5 Aug 2022 18:57:58 +0100 Subject: [PATCH 5/5] gh-95348: Update weakrefobject to use new function --- ...2-07-29-19-35-30.gh-issue-95348.p5CDaH.rst | 4 -- Objects/weakrefobject.c | 56 +++++++++---------- 2 files changed, 27 insertions(+), 33 deletions(-) delete mode 100644 Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst diff --git a/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst b/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst deleted file mode 100644 index cc518a9aa44069..00000000000000 --- a/Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst +++ /dev/null @@ -1,4 +0,0 @@ -Split ``clear_weakref`` into two functions: ``clear_weakref`` -and ``remove_weakref``. Abstracts ``remove_weakref`` with -``remove_weakref_from_referent`` in weakrefobject.c. -Patch by Robert O'Shea. diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 454c0f18d92480..c88ed4a55ba5e4 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -46,56 +46,53 @@ new_weakref(PyObject *ob, PyObject *callback) } -/* This function is responsible for clearing the callback slot. When using this - * function it must be called before calling remove_weakref_from_referent, - * otherwise a segmentation fault will occur at build time. +/* This function clears the passed-in reference. It must have been + * from the weaklist already. (See weaklist_remove_weakref().) */ static void clear_weakref(PyWeakReference *self) { PyObject *callback = self->wr_callback; + self->wr_object = Py_None; + if (callback != NULL) { Py_DECREF(callback); self->wr_callback = NULL; } } -/* This function removes the pass in reference from the list of weak - * references for the referent. This should be called from - * remove_weakref_fromt_referent as it will pass the required list - * argument. +/* This function removes the passed-in reference from the list of weak + * references for the referent. */ static void -remove_weakref(PyWeakReference *self, PyWeakReference **list) +weaklist_remove_weakref(PyWeakReference **list, PyWeakReference *ref) { - if (*list == self) { - /* If 'self' is the end of the list (and thus self->wr_next == NULL) + if (*list == ref) { + /* If 'ref' is the end of the list (and thus ref->wr_next == NULL) then the weakref list itself (and thus the value of *list) will end up being set to NULL. */ - *list = self->wr_next; + *list = ref->wr_next; } - self->wr_object = Py_None; - if (self->wr_prev != NULL) { - self->wr_prev->wr_next = self->wr_next; + if (ref->wr_prev != NULL) { + ref->wr_prev->wr_next = ref->wr_next; } - if (self->wr_next != NULL) { - self->wr_next->wr_prev = self->wr_prev; + if (ref->wr_next != NULL) { + ref->wr_next->wr_prev = ref->wr_prev; } - self->wr_prev = NULL; - self->wr_next = NULL; + ref->wr_prev = NULL; + ref->wr_next = NULL; } /* This function removes the pass in reference from the list of weak references - * for the referent. This should be called after clear_weakref, otherwise a - * segmentation fault will occur at build time. + * for the referent. */ static void remove_weakref_from_referent(PyWeakReference *self) { if (self->wr_object != Py_None) { PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); - remove_weakref(self, list); + weaklist_remove_weakref(list, self); } } @@ -120,8 +117,8 @@ _PyWeakref_ClearRef(PyWeakReference *self) /* Preserve and restore the callback around clear_weakref. */ callback = self->wr_callback; self->wr_callback = NULL; - clear_weakref(self); remove_weakref_from_referent(self); + clear_weakref(self); self->wr_callback = callback; } @@ -129,8 +126,8 @@ static void weakref_dealloc(PyObject *self) { PyObject_GC_UnTrack(self); - clear_weakref((PyWeakReference *) self); remove_weakref_from_referent((PyWeakReference *) self); + clear_weakref((PyWeakReference *) self); Py_TYPE(self)->tp_free(self); } @@ -146,8 +143,8 @@ gc_traverse(PyWeakReference *self, visitproc visit, void *arg) static int gc_clear(PyWeakReference *self) { - clear_weakref(self); remove_weakref_from_referent(self); + clear_weakref(self); return 0; } @@ -983,10 +980,10 @@ PyObject_ClearWeakRefs(PyObject *object) /* Remove the callback-less basic and proxy references */ if (*list != NULL && (*list)->wr_callback == NULL) { clear_weakref(*list); - remove_weakref_from_referent(*list); + weaklist_remove_weakref(list, *list); if (*list != NULL && (*list)->wr_callback == NULL) { clear_weakref(*list); - remove_weakref_from_referent(*list); + weaklist_remove_weakref(list, *list); } } if (*list != NULL) { @@ -1000,7 +997,7 @@ PyObject_ClearWeakRefs(PyObject *object) current->wr_callback = NULL; clear_weakref(current); - remove_weakref_from_referent(current); + weaklist_remove_weakref(list, current); if (callback != NULL) { if (Py_REFCNT((PyObject *)current) > 0) { handle_callback(current, callback); @@ -1031,7 +1028,7 @@ PyObject_ClearWeakRefs(PyObject *object) } current->wr_callback = NULL; clear_weakref(current); - remove_weakref_from_referent(current); + weaklist_remove_weakref(list, current); current = next; } for (i = 0; i < count; ++i) { @@ -1066,6 +1063,7 @@ _PyStaticType_ClearWeakRefs(PyTypeObject *type) weaklist before clearing its wr_object and wr_callback. That is how we're able to loop over the list. */ clear_weakref((PyWeakReference *)*list); - remove_weakref_from_referent((PyWeakReference *)*list); + weaklist_remove_weakref((PyWeakReference **)list, + (PyWeakReference *)list); } }