From a400a06e24c3d0ec1611879574e20863470c7335 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Tue, 2 Jan 2024 00:45:18 +0900 Subject: [PATCH 1/6] gh-111926: Set up basic sementics of weakref API for freethreading --- Include/internal/pycore_weakref.h | 62 ++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index eacbe14c903289..fa21ed07bea93c 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -8,49 +8,75 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif +#include "pycore_object.h" // _Py_REF_IS_MERGED() #include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION() -static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) { +static inline int _is_dead(PyObject *obj) +{ + if (obj == NULL) { + return 0; + } + // Explanation for the Py_REFCNT() check: when a weakref's target is part + // of a long chain of deallocations which triggers the trashcan mechanism, + // clearing the weakrefs can be delayed long after the target's refcount + // has dropped to zero. In the meantime, code accessing the weakref will + // be able to "see" the target object even though it is supposed to be + // unreachable. See issue gh-60806. +#if defined(Py_GIL_DISABLED) + int refcount = Py_REFCNT(obj); + if (refcount == 0) { + // In freethreading CPython, Py_REFCNT is only estimated count, + // so need to check shared refcount value too. + Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&obj->ob_ref_shared); + return _Py_REF_IS_MERGED(shared); + } + return 0; +#else + return (Py_REFCNT(obj) == 0); +#endif +} + +static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) +{ assert(PyWeakref_Check(ref_obj)); + PyObject *ret = NULL; + Py_BEGIN_CRITICAL_SECTION(ref_obj); PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); PyObject *obj = ref->wr_object; if (obj == Py_None) { // clear_weakref() was called - return NULL; + goto end; } - // Explanation for the Py_REFCNT() check: when a weakref's target is part - // of a long chain of deallocations which triggers the trashcan mechanism, - // clearing the weakrefs can be delayed long after the target's refcount - // has dropped to zero. In the meantime, code accessing the weakref will - // be able to "see" the target object even though it is supposed to be - // unreachable. See issue gh-60806. - Py_ssize_t refcnt = Py_REFCNT(obj); - if (refcnt == 0) { - return NULL; + if (_is_dead(obj)) { + goto end; } - assert(refcnt > 0); - return Py_NewRef(obj); + assert(Py_REFCNT(obj) > 0); + ret = Py_NewRef(obj); +end: + Py_END_CRITICAL_SECTION(); + return ret; } -static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) { +static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) +{ assert(PyWeakref_Check(ref_obj)); - int is_dead; + int ret = 0; Py_BEGIN_CRITICAL_SECTION(ref_obj); PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); PyObject *obj = ref->wr_object; if (obj == Py_None) { // clear_weakref() was called - is_dead = 1; + ret = 1; } else { // See _PyWeakref_GET_REF() for the rationale of this test - is_dead = (Py_REFCNT(obj) == 0); + ret = _is_dead(obj); } Py_END_CRITICAL_SECTION(); - return is_dead; + return ret; } extern Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head); From 5c755a884965069e1224e353329bc3a9a26e1572 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Tue, 2 Jan 2024 00:58:46 +0900 Subject: [PATCH 2/6] fix compiler warn --- Include/internal/pycore_weakref.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index fa21ed07bea93c..4ce9fdc4889220 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -23,7 +23,7 @@ static inline int _is_dead(PyObject *obj) // be able to "see" the target object even though it is supposed to be // unreachable. See issue gh-60806. #if defined(Py_GIL_DISABLED) - int refcount = Py_REFCNT(obj); + Py_ssize_t refcount = Py_REFCNT(obj); if (refcount == 0) { // In freethreading CPython, Py_REFCNT is only estimated count, // so need to check shared refcount value too. From 32b612f04c39a490063f711a0f0cf9d21bd27ff1 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Tue, 2 Jan 2024 01:20:36 +0900 Subject: [PATCH 3/6] fix --- Include/internal/pycore_weakref.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 4ce9fdc4889220..0233546369c352 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -52,8 +52,9 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) if (_is_dead(obj)) { goto end; } - +#if !defined(Py_GIL_DISABLED) assert(Py_REFCNT(obj) > 0); +#endif ret = Py_NewRef(obj); end: Py_END_CRITICAL_SECTION(); From 1662b5d03ea087afd4143db015f5b09a1190799e Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 3 Jan 2024 21:59:34 +0900 Subject: [PATCH 4/6] Update Include/internal/pycore_weakref.h Co-authored-by: Sam Gross --- Include/internal/pycore_weakref.h | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 0233546369c352..df5775e7273daa 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -23,14 +23,8 @@ static inline int _is_dead(PyObject *obj) // be able to "see" the target object even though it is supposed to be // unreachable. See issue gh-60806. #if defined(Py_GIL_DISABLED) - Py_ssize_t refcount = Py_REFCNT(obj); - if (refcount == 0) { - // In freethreading CPython, Py_REFCNT is only estimated count, - // so need to check shared refcount value too. - Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&obj->ob_ref_shared); - return _Py_REF_IS_MERGED(shared); - } - return 0; + Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&obj->ob_ref_shared); + return shared == _Py_REF_SHARED(0, _Py_REF_MERGED); #else return (Py_REFCNT(obj) == 0); #endif From 8b75804873293e742e4b3def788f48e453e47cf6 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 3 Jan 2024 22:00:24 +0900 Subject: [PATCH 5/6] Address code review --- Include/internal/pycore_weakref.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index df5775e7273daa..c47cf55c14ae1c 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -13,9 +13,6 @@ extern "C" { static inline int _is_dead(PyObject *obj) { - if (obj == NULL) { - return 0; - } // Explanation for the Py_REFCNT() check: when a weakref's target is part // of a long chain of deallocations which triggers the trashcan mechanism, // clearing the weakrefs can be delayed long after the target's refcount From 72baa0fe1e9fd7a4b4d2094e2ef4745db29b292f Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 3 Jan 2024 22:01:39 +0900 Subject: [PATCH 6/6] Sort header --- Include/internal/pycore_weakref.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index c47cf55c14ae1c..dea267b49039e7 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -8,8 +8,8 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif -#include "pycore_object.h" // _Py_REF_IS_MERGED() #include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION() +#include "pycore_object.h" // _Py_REF_IS_MERGED() static inline int _is_dead(PyObject *obj) {