From 066f365fb32b031c46a898581f9c297b156608dc Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Tue, 21 May 2024 00:11:07 +0200 Subject: [PATCH 01/17] Add test to reproduce data race on PyMember_Get/Set --- Lib/test/test_free_threading/test_slots.py | 43 ++++++++++++++++++++++ Tools/tsan/suppressions_free_threading.txt | 4 +- 2 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 Lib/test/test_free_threading/test_slots.py diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py new file mode 100644 index 00000000000000..d76207e8779f78 --- /dev/null +++ b/Lib/test/test_free_threading/test_slots.py @@ -0,0 +1,43 @@ +import threading +from test.support import threading_helper +from unittest import TestCase + + +class Spam: + __slots__ = [ + "cheese", + "eric", + ] + + def __init__(self): + self.cheese = 0 + self.eric = "" + + +spam = Spam() + + +def writer(): + for _ in range(1_000): + spam.eric = "idle" + spam.cheese += 1 + + +def reader(): + spam.eric + spam.cheese + + +@threading_helper.requires_working_threading() +class TestList(TestCase): + + def test_slots(self): + threads = [threading.Thread(target=writer), *[ + threading.Thread(target=reader) + for _ in range(3) + ]] + + for thread in threads: + thread.start() + for thread in threads: + thread.join() diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index dfa4a1fe9ca438..b6fec7a8fd5431 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -30,7 +30,7 @@ race:_PyParkingLot_Park race_top:_add_to_weak_set race_top:_in_weak_set race_top:_mi_heap_delayed_free_partial -race_top:_PyEval_EvalFrameDefault +#race_top:_PyEval_EvalFrameDefault race_top:_PyImport_AcquireLock race_top:_PyImport_ReleaseLock # https://gist.github.com/mpage/0a24eb2dd458441ededb498e9b0e5de8 @@ -41,8 +41,6 @@ race_top:gc_restore_tid race_top:insertdict race_top:lookup_tp_dict race_top:mi_heap_visit_pages -race_top:PyMember_GetOne -race_top:PyMember_SetOne race_top:new_reference race_top:set_contains_key race_top:set_inheritable From cc2858d4471397623895bf6f60bdfeb5b7d7c6b9 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Tue, 21 May 2024 20:53:37 +0200 Subject: [PATCH 02/17] fix race for case `Py_T_OBJECT_EX` --- .../internal/pycore_pyatomic_ft_wrappers.h | 13 +++++ Lib/test/test_free_threading/test_slots.py | 48 +++++++++---------- Python/structmember.c | 6 +-- 3 files changed, 39 insertions(+), 28 deletions(-) diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index bc6aba56cf9fc7..ec853971bddb18 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -59,6 +59,8 @@ extern "C" { _Py_atomic_store_uint16_relaxed(&value, new_value) #define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) \ _Py_atomic_store_uint32_relaxed(&value, new_value) +#define FT_ATOMIC_EXCHANGE_PTR(value, new_value) \ + _Py_atomic_exchange_ptr(value, new_value) #else #define FT_ATOMIC_LOAD_PTR(value) value @@ -83,6 +85,17 @@ extern "C" { #define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) value = new_value +static inline void * +_Py_non_atomic_exchange_ptr(void **value, void *new_value) +{ + void *current = *value; + *value = new_value; + return current; +} + +#define FT_ATOMIC_EXCHANGE_PTR(value, new_value) \ + _Py_non_atomic_exchange_ptr(value, new_value) + #endif #ifdef __cplusplus diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py index d76207e8779f78..e64b77e23f2963 100644 --- a/Lib/test/test_free_threading/test_slots.py +++ b/Lib/test/test_free_threading/test_slots.py @@ -5,39 +5,37 @@ class Spam: __slots__ = [ - "cheese", - "eric", + "eggs", ] def __init__(self): - self.cheese = 0 - self.eric = "" + self.eggs = 0 -spam = Spam() - - -def writer(): - for _ in range(1_000): - spam.eric = "idle" - spam.cheese += 1 - - -def reader(): - spam.eric - spam.cheese +def run_in_threads(targets): + """A decorator to run some functions in separate threads""" + threads = [ + threading.Thread(target=target) + for target in targets + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() @threading_helper.requires_working_threading() class TestList(TestCase): - def test_slots(self): - threads = [threading.Thread(target=writer), *[ - threading.Thread(target=reader) - for _ in range(3) - ]] + def test_str(self): + spam = Spam() + + def writer(): + for _ in range(1_000): + spam.eggs = str(int(spam.eggs) + 1) + + def reader(): + for _ in range(1_000): + spam.eggs - for thread in threads: - thread.start() - for thread in threads: - thread.join() + run_in_threads([writer, reader, reader, reader]) diff --git a/Python/structmember.c b/Python/structmember.c index ba881d18a0973d..e1316cf1de220b 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -4,6 +4,7 @@ #include "Python.h" #include "pycore_abstract.h" // _PyNumber_Index() #include "pycore_long.h" // _PyLong_IsNegative() +#include "pycore_pyatomic_ft_wrappers.h" PyObject * @@ -75,7 +76,7 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) Py_INCREF(v); break; case Py_T_OBJECT_EX: - v = *(PyObject **)addr; + v = FT_ATOMIC_LOAD_PTR(*addr); if (v == NULL) { PyObject *obj = (PyObject *)obj_addr; PyTypeObject *tp = Py_TYPE(obj); @@ -281,8 +282,7 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) break; case _Py_T_OBJECT: case Py_T_OBJECT_EX: - oldv = *(PyObject **)addr; - *(PyObject **)addr = Py_XNewRef(v); + oldv = FT_ATOMIC_EXCHANGE_PTR(addr, Py_XNewRef(v)); Py_XDECREF(oldv); break; case Py_T_CHAR: { From cff8b44dd6cb54c4a20368ff2c1454f9ec4b5c64 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Tue, 21 May 2024 21:13:14 +0200 Subject: [PATCH 03/17] hide segafulting test --- Lib/test/test_free_threading/test_slots.py | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py index e64b77e23f2963..eeeb3807a723f4 100644 --- a/Lib/test/test_free_threading/test_slots.py +++ b/Lib/test/test_free_threading/test_slots.py @@ -8,8 +8,8 @@ class Spam: "eggs", ] - def __init__(self): - self.eggs = 0 + def __init__(self, initial_value): + self.eggs = initial_value def run_in_threads(targets): @@ -25,10 +25,10 @@ def run_in_threads(targets): @threading_helper.requires_working_threading() -class TestList(TestCase): +class TestSlots(TestCase): def test_str(self): - spam = Spam() + spam = Spam("0") def writer(): for _ in range(1_000): @@ -39,3 +39,17 @@ def reader(): spam.eggs run_in_threads([writer, reader, reader, reader]) + + # this segfaults + # def test_int(self): + # spam = Spam(1) + # + # def writer(): + # for _ in range(1_000_000): + # spam.eggs += 1 + # + # def reader(): + # for _ in range(1_000_000): + # spam.eggs + # + # run_in_threads([writer, writer, reader, reader]) From a14f324e6dcbae85d65c5e67014972e8826fee63 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Tue, 21 May 2024 23:14:58 +0200 Subject: [PATCH 04/17] test segafults with many iterations --- Lib/test/test_free_threading/test_slots.py | 54 +++++++++++++--------- Python/structmember.c | 3 ++ 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py index eeeb3807a723f4..a1f9abd698c6f7 100644 --- a/Lib/test/test_free_threading/test_slots.py +++ b/Lib/test/test_free_threading/test_slots.py @@ -3,17 +3,11 @@ from unittest import TestCase -class Spam: - __slots__ = [ - "eggs", - ] - - def __init__(self, initial_value): - self.eggs = initial_value +# import _testcapi def run_in_threads(targets): - """A decorator to run some functions in separate threads""" + """Run `targets` in separate threads""" threads = [ threading.Thread(target=target) for target in targets @@ -27,29 +21,45 @@ def run_in_threads(targets): @threading_helper.requires_working_threading() class TestSlots(TestCase): - def test_str(self): - spam = Spam("0") + def test_object(self): + class Spam: + __slots__ = [ + "eggs", # the Py_T_OBJECT_EX type is missing in + # Modules/_testcapi/structmember.c + ] + + def __init__(self, initial_value): + self.eggs = initial_value + + spam = Spam(0) + iters = 1_000_000 def writer(): - for _ in range(1_000): - spam.eggs = str(int(spam.eggs) + 1) + for _ in range(iters): + spam.eggs += 1 def reader(): - for _ in range(1_000): - spam.eggs + for _ in range(iters): + eggs = spam.eggs + assert type(eggs) is int + assert 0 <= eggs <= iters run_in_threads([writer, reader, reader, reader]) - # this segfaults - # def test_int(self): - # spam = Spam(1) + # def test_bool(self): + # spam_old = _testcapi._test_structmembersType_OldAPI() + # spam_new = _testcapi._test_structmembersType_NewAPI() # # def writer(): - # for _ in range(1_000_000): - # spam.eggs += 1 + # for _ in range(1_000): + # spam_old.T_BOOL = True + # spam_old.T_BOOL = False # separate code path for True/False + # spam_new.T_BOOL = True + # spam_new.T_BOOL = False # # def reader(): - # for _ in range(1_000_000): - # spam.eggs + # for _ in range(1_000): + # spam_old.T_BOOL + # spam_new.T_BOOL # - # run_in_threads([writer, writer, reader, reader]) + # run_in_threads([writer, reader, reader, reader]) diff --git a/Python/structmember.c b/Python/structmember.c index e1316cf1de220b..326aad1c389d92 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -5,6 +5,7 @@ #include "pycore_abstract.h" // _PyNumber_Index() #include "pycore_long.h" // _PyLong_IsNegative() #include "pycore_pyatomic_ft_wrappers.h" +#include "pycore_object.h" PyObject * @@ -93,6 +94,7 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) v = PyLong_FromUnsignedLongLong(*(unsigned long long *)addr); break; case _Py_T_NONE: + // doesn't require free-threading code path v = Py_NewRef(Py_None); break; default: @@ -282,6 +284,7 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) break; case _Py_T_OBJECT: case Py_T_OBJECT_EX: + _PyObject_SetMaybeWeakref(v); oldv = FT_ATOMIC_EXCHANGE_PTR(addr, Py_XNewRef(v)); Py_XDECREF(oldv); break; From 1ee30915c09a4dbb3db78b7318e67531d7b99807 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Tue, 21 May 2024 23:30:38 +0200 Subject: [PATCH 05/17] retry path for free threading builds --- Python/structmember.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Python/structmember.c b/Python/structmember.c index 326aad1c389d92..1dbaf5d6b62877 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -77,6 +77,7 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) Py_INCREF(v); break; case Py_T_OBJECT_EX: +#ifndef Py_GIL_DISABLED v = FT_ATOMIC_LOAD_PTR(*addr); if (v == NULL) { PyObject *obj = (PyObject *)obj_addr; @@ -86,6 +87,19 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) tp->tp_name, l->name); } Py_XINCREF(v); +#else + do { + v = FT_ATOMIC_LOAD_PTR(*addr); + if (v == NULL) { + PyObject * obj = (PyObject *) obj_addr; + PyTypeObject *tp = Py_TYPE(obj); + PyErr_Format(PyExc_AttributeError, + "'%.200s' object has no attribute '%s'", + tp->tp_name, l->name); + break; + } + } while (!_Py_TryIncref(v)); // if refcount == 0: retry +#endif break; case Py_T_LONGLONG: v = PyLong_FromLongLong(*(long long *)addr); From 7aa039624f608e6bf113e7942c718a125fc864ba Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Tue, 21 May 2024 23:39:00 +0200 Subject: [PATCH 06/17] comment --- Python/structmember.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Python/structmember.c b/Python/structmember.c index 1dbaf5d6b62877..43b0b9c382400c 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -298,7 +298,8 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) break; case _Py_T_OBJECT: case Py_T_OBJECT_EX: - _PyObject_SetMaybeWeakref(v); + _PyObject_SetMaybeWeakref(v); // without setting _Py_REF_MAYBE_WEAKREF, + // we may get an infinite loop in PyMember_GetOne oldv = FT_ATOMIC_EXCHANGE_PTR(addr, Py_XNewRef(v)); Py_XDECREF(oldv); break; From 419929d8aa02b1e28411a166bcad5e1fe0c0ac00 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Tue, 21 May 2024 23:51:30 +0200 Subject: [PATCH 07/17] self-review --- Python/structmember.c | 5 +++++ Tools/tsan/suppressions_free_threading.txt | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Python/structmember.c b/Python/structmember.c index 43b0b9c382400c..d65a4bc1f754eb 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -298,9 +298,14 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) break; case _Py_T_OBJECT: case Py_T_OBJECT_EX: +#ifndef Py_GIL_DISABLED + oldv = *(PyObject **)addr; + *(PyObject **)addr = Py_XNewRef(v); +#else _PyObject_SetMaybeWeakref(v); // without setting _Py_REF_MAYBE_WEAKREF, // we may get an infinite loop in PyMember_GetOne oldv = FT_ATOMIC_EXCHANGE_PTR(addr, Py_XNewRef(v)); +#endif Py_XDECREF(oldv); break; case Py_T_CHAR: { diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index b6fec7a8fd5431..2cc2ea82b846f9 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -30,7 +30,7 @@ race:_PyParkingLot_Park race_top:_add_to_weak_set race_top:_in_weak_set race_top:_mi_heap_delayed_free_partial -#race_top:_PyEval_EvalFrameDefault +race_top:_PyEval_EvalFrameDefault race_top:_PyImport_AcquireLock race_top:_PyImport_ReleaseLock # https://gist.github.com/mpage/0a24eb2dd458441ededb498e9b0e5de8 From ce49aeade42e21d0a887b40a146e4b88c33aa9a8 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Wed, 22 May 2024 16:48:40 +0200 Subject: [PATCH 08/17] revert change in default build --- Python/structmember.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/structmember.c b/Python/structmember.c index d65a4bc1f754eb..c7624f86b9cc6b 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -78,7 +78,7 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) break; case Py_T_OBJECT_EX: #ifndef Py_GIL_DISABLED - v = FT_ATOMIC_LOAD_PTR(*addr); + v = *(PyObject **)addr; if (v == NULL) { PyObject *obj = (PyObject *)obj_addr; PyTypeObject *tp = Py_TYPE(obj); From 8639eae0260c034f652460a934fd58c47533bad8 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Wed, 22 May 2024 18:31:20 +0200 Subject: [PATCH 09/17] missing NULL check --- Python/structmember.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Python/structmember.c b/Python/structmember.c index c7624f86b9cc6b..31b2d6d4dff829 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -91,7 +91,7 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) do { v = FT_ATOMIC_LOAD_PTR(*addr); if (v == NULL) { - PyObject * obj = (PyObject *) obj_addr; + PyObject *obj = (PyObject *)obj_addr; PyTypeObject *tp = Py_TYPE(obj); PyErr_Format(PyExc_AttributeError, "'%.200s' object has no attribute '%s'", @@ -302,7 +302,9 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) oldv = *(PyObject **)addr; *(PyObject **)addr = Py_XNewRef(v); #else - _PyObject_SetMaybeWeakref(v); // without setting _Py_REF_MAYBE_WEAKREF, + if (v != NULL) + _PyObject_SetMaybeWeakref(v); + // without setting _Py_REF_MAYBE_WEAKREF, // we may get an infinite loop in PyMember_GetOne oldv = FT_ATOMIC_EXCHANGE_PTR(addr, Py_XNewRef(v)); #endif From f30cb2023a277b780b38b1679bd568f887352e03 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Tue, 4 Jun 2024 18:30:12 +0200 Subject: [PATCH 10/17] use critical section instead of relying on weakref flag --- Python/structmember.c | 60 ++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/Python/structmember.c b/Python/structmember.c index 31b2d6d4dff829..b0b1dc3406b210 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -6,6 +6,24 @@ #include "pycore_long.h" // _PyLong_IsNegative() #include "pycore_pyatomic_ft_wrappers.h" #include "pycore_object.h" +#include "pycore_critical_section.h" + + +static inline PyObject * +_PyMember_GetOneObject(const char *addr, const char *obj_addr, PyMemberDef *l) +{ + PyObject *v = FT_ATOMIC_LOAD_PTR(*(PyObject **) addr); + if (v == NULL) { + PyObject *obj = (PyObject *) obj_addr; + PyTypeObject *tp = Py_TYPE(obj); + PyErr_Format(PyExc_AttributeError, + "'%.200s' object has no attribute '%s'", + tp->tp_name, l->name); + } + return v; +} + + PyObject * @@ -78,27 +96,16 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) break; case Py_T_OBJECT_EX: #ifndef Py_GIL_DISABLED - v = *(PyObject **)addr; - if (v == NULL) { - PyObject *obj = (PyObject *)obj_addr; - PyTypeObject *tp = Py_TYPE(obj); - PyErr_Format(PyExc_AttributeError, - "'%.200s' object has no attribute '%s'", - tp->tp_name, l->name); - } + v = _PyMember_GetOneObject(addr, obj_addr, l); Py_XINCREF(v); #else - do { - v = FT_ATOMIC_LOAD_PTR(*addr); - if (v == NULL) { - PyObject *obj = (PyObject *)obj_addr; - PyTypeObject *tp = Py_TYPE(obj); - PyErr_Format(PyExc_AttributeError, - "'%.200s' object has no attribute '%s'", - tp->tp_name, l->name); - break; + v = _PyMember_GetOneObject(addr, obj_addr, l); + if (v != NULL && !_Py_TryIncrefCompare((PyObject **) addr, v)) { + Py_BEGIN_CRITICAL_SECTION((PyObject *)obj_addr); + v = _PyMember_GetOneObject(addr, obj_addr, l); + Py_XINCREF(v); + Py_END_CRITICAL_SECTION(); } - } while (!_Py_TryIncref(v)); // if refcount == 0: retry #endif break; case Py_T_LONGLONG: @@ -135,6 +142,7 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) return -1; } + PyObject *obj = (PyObject *) addr; addr += l->offset; if ((l->flags & Py_READONLY)) @@ -298,15 +306,13 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) break; case _Py_T_OBJECT: case Py_T_OBJECT_EX: -#ifndef Py_GIL_DISABLED - oldv = *(PyObject **)addr; - *(PyObject **)addr = Py_XNewRef(v); -#else - if (v != NULL) - _PyObject_SetMaybeWeakref(v); - // without setting _Py_REF_MAYBE_WEAKREF, - // we may get an infinite loop in PyMember_GetOne - oldv = FT_ATOMIC_EXCHANGE_PTR(addr, Py_XNewRef(v)); +#ifdef Py_GIL_DISABLED + Py_BEGIN_CRITICAL_SECTION(obj); +#endif + oldv = FT_ATOMIC_LOAD_PTR(*(PyObject **)addr); + FT_ATOMIC_STORE_PTR(*(PyObject **)addr, Py_XNewRef(v)); +#ifdef Py_GIL_DISABLED + Py_END_CRITICAL_SECTION(); #endif Py_XDECREF(oldv); break; From 99b1bce542cbc2892e1540de9d38947478c51c87 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Wed, 5 Jun 2024 10:23:49 +0200 Subject: [PATCH 11/17] review --- .../internal/pycore_pyatomic_ft_wrappers.h | 13 -------- Python/structmember.c | 32 ++++++++----------- Tools/tsan/suppressions_free_threading.txt | 1 - 3 files changed, 14 insertions(+), 32 deletions(-) diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index ec853971bddb18..bc6aba56cf9fc7 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -59,8 +59,6 @@ extern "C" { _Py_atomic_store_uint16_relaxed(&value, new_value) #define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) \ _Py_atomic_store_uint32_relaxed(&value, new_value) -#define FT_ATOMIC_EXCHANGE_PTR(value, new_value) \ - _Py_atomic_exchange_ptr(value, new_value) #else #define FT_ATOMIC_LOAD_PTR(value) value @@ -85,17 +83,6 @@ extern "C" { #define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) value = new_value -static inline void * -_Py_non_atomic_exchange_ptr(void **value, void *new_value) -{ - void *current = *value; - *value = new_value; - return current; -} - -#define FT_ATOMIC_EXCHANGE_PTR(value, new_value) \ - _Py_non_atomic_exchange_ptr(value, new_value) - #endif #ifdef __cplusplus diff --git a/Python/structmember.c b/Python/structmember.c index b0b1dc3406b210..b2244da836c295 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -4,13 +4,12 @@ #include "Python.h" #include "pycore_abstract.h" // _PyNumber_Index() #include "pycore_long.h" // _PyLong_IsNegative() -#include "pycore_pyatomic_ft_wrappers.h" -#include "pycore_object.h" +#include "pycore_object.h" // _Py_TryIncrefCompare(), FT_ATOMIC_*() #include "pycore_critical_section.h" static inline PyObject * -_PyMember_GetOneObject(const char *addr, const char *obj_addr, PyMemberDef *l) +member_get_object(const char *addr, const char *obj_addr, PyMemberDef *l) { PyObject *v = FT_ATOMIC_LOAD_PTR(*(PyObject **) addr); if (v == NULL) { @@ -23,9 +22,6 @@ _PyMember_GetOneObject(const char *addr, const char *obj_addr, PyMemberDef *l) return v; } - - - PyObject * PyMember_GetOne(const char *obj_addr, PyMemberDef *l) { @@ -96,16 +92,18 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) break; case Py_T_OBJECT_EX: #ifndef Py_GIL_DISABLED - v = _PyMember_GetOneObject(addr, obj_addr, l); + v = member_get_object(addr, obj_addr, l); Py_XINCREF(v); #else - v = _PyMember_GetOneObject(addr, obj_addr, l); - if (v != NULL && !_Py_TryIncrefCompare((PyObject **) addr, v)) { - Py_BEGIN_CRITICAL_SECTION((PyObject *)obj_addr); - v = _PyMember_GetOneObject(addr, obj_addr, l); - Py_XINCREF(v); + v = member_get_object(addr, obj_addr, l); + if (v != NULL) { + if (!_Py_TryIncrefCompare((PyObject **) addr, v)) { + Py_BEGIN_CRITICAL_SECTION((PyObject *) obj_addr); + v = member_get_object(addr, obj_addr, l); + Py_XINCREF(v); Py_END_CRITICAL_SECTION(); } + } #endif break; case Py_T_LONGLONG: @@ -142,7 +140,9 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) return -1; } +#ifdef Py_GIL_DISABLED PyObject *obj = (PyObject *) addr; +#endif addr += l->offset; if ((l->flags & Py_READONLY)) @@ -306,14 +306,10 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) break; case _Py_T_OBJECT: case Py_T_OBJECT_EX: -#ifdef Py_GIL_DISABLED Py_BEGIN_CRITICAL_SECTION(obj); -#endif - oldv = FT_ATOMIC_LOAD_PTR(*(PyObject **)addr); - FT_ATOMIC_STORE_PTR(*(PyObject **)addr, Py_XNewRef(v)); -#ifdef Py_GIL_DISABLED + oldv = *(PyObject **)addr; + FT_ATOMIC_STORE_PTR_RELEASE(*(PyObject **)addr, Py_XNewRef(v)); Py_END_CRITICAL_SECTION(); -#endif Py_XDECREF(oldv); break; case Py_T_CHAR: { diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index 9ef3de8679e739..d6b1981df67a61 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -32,7 +32,6 @@ race_top:_PyType_HasFeature race_top:assign_version_tag race_top:insertdict race_top:lookup_tp_dict -race_top:mi_heap_visit_pages race_top:new_reference race_top:set_contains_key # https://gist.github.com/colesbury/d13d033f413b4ad07929d044bed86c35 From 8f273f7d5354bd092c8776e2d4d736ec72d84058 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Wed, 5 Jun 2024 10:27:03 +0200 Subject: [PATCH 12/17] move up --- Python/structmember.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Python/structmember.c b/Python/structmember.c index b2244da836c295..5ded2b6220ac9d 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -91,11 +91,10 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) Py_INCREF(v); break; case Py_T_OBJECT_EX: -#ifndef Py_GIL_DISABLED v = member_get_object(addr, obj_addr, l); +#ifndef Py_GIL_DISABLED Py_XINCREF(v); #else - v = member_get_object(addr, obj_addr, l); if (v != NULL) { if (!_Py_TryIncrefCompare((PyObject **) addr, v)) { Py_BEGIN_CRITICAL_SECTION((PyObject *) obj_addr); From 798c2f03d2433cce5f4c74fc847897212167b36b Mon Sep 17 00:00:00 2001 From: Daniele Parmeggiani <8658291+dpdani@users.noreply.github.com> Date: Fri, 7 Jun 2024 17:25:31 +0200 Subject: [PATCH 13/17] suggested new formatting syntax Co-authored-by: Erlend E. Aasland --- Python/structmember.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/structmember.c b/Python/structmember.c index 5ded2b6220ac9d..b0f0a0a17075bf 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -16,8 +16,8 @@ member_get_object(const char *addr, const char *obj_addr, PyMemberDef *l) PyObject *obj = (PyObject *) obj_addr; PyTypeObject *tp = Py_TYPE(obj); PyErr_Format(PyExc_AttributeError, - "'%.200s' object has no attribute '%s'", - tp->tp_name, l->name); + "'%N' object has no attribute '%s'", + tp, l->name); } return v; } From cb698b32217a66fe89e3beacab06cd32c5b6479a Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Fri, 7 Jun 2024 18:29:36 +0200 Subject: [PATCH 14/17] fix test --- Lib/test/test_descr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_descr.py b/Lib/test/test_descr.py index 7742f075285602..b0f86317bfecf6 100644 --- a/Lib/test/test_descr.py +++ b/Lib/test/test_descr.py @@ -1314,7 +1314,7 @@ class X(object): # Inherit from object on purpose to check some backwards compatibility paths class X(object): __slots__ = "a" - with self.assertRaisesRegex(AttributeError, "'X' object has no attribute 'a'"): + with self.assertRaisesRegex(AttributeError, "'test.test_descr.ClassPropertiesAndMethods.test_slots..X' object has no attribute 'a'"): X().a # Test string subclass in `__slots__`, see gh-98783 From 737eb09212029ac639e8e1501566043490782f59 Mon Sep 17 00:00:00 2001 From: parmeggiani Date: Fri, 7 Jun 2024 18:30:59 +0200 Subject: [PATCH 15/17] remove commented code --- Lib/test/test_free_threading/test_slots.py | 24 +--------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py index a1f9abd698c6f7..b4076522b4341d 100644 --- a/Lib/test/test_free_threading/test_slots.py +++ b/Lib/test/test_free_threading/test_slots.py @@ -3,9 +3,6 @@ from unittest import TestCase -# import _testcapi - - def run_in_threads(targets): """Run `targets` in separate threads""" threads = [ @@ -24,8 +21,7 @@ class TestSlots(TestCase): def test_object(self): class Spam: __slots__ = [ - "eggs", # the Py_T_OBJECT_EX type is missing in - # Modules/_testcapi/structmember.c + "eggs", ] def __init__(self, initial_value): @@ -45,21 +41,3 @@ def reader(): assert 0 <= eggs <= iters run_in_threads([writer, reader, reader, reader]) - - # def test_bool(self): - # spam_old = _testcapi._test_structmembersType_OldAPI() - # spam_new = _testcapi._test_structmembersType_NewAPI() - # - # def writer(): - # for _ in range(1_000): - # spam_old.T_BOOL = True - # spam_old.T_BOOL = False # separate code path for True/False - # spam_new.T_BOOL = True - # spam_new.T_BOOL = False - # - # def reader(): - # for _ in range(1_000): - # spam_old.T_BOOL - # spam_new.T_BOOL - # - # run_in_threads([writer, reader, reader, reader]) From 15fdffef6fe5ee6ea18295976864c4ef7a3c05aa Mon Sep 17 00:00:00 2001 From: Daniele Parmeggiani <8658291+dpdani@users.noreply.github.com> Date: Tue, 11 Jun 2024 16:21:37 +0200 Subject: [PATCH 16/17] good nit Co-authored-by: Erlend E. Aasland --- Python/structmember.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Python/structmember.c b/Python/structmember.c index b0f0a0a17075bf..d5e7ab83093dc8 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -13,11 +13,9 @@ member_get_object(const char *addr, const char *obj_addr, PyMemberDef *l) { PyObject *v = FT_ATOMIC_LOAD_PTR(*(PyObject **) addr); if (v == NULL) { - PyObject *obj = (PyObject *) obj_addr; - PyTypeObject *tp = Py_TYPE(obj); PyErr_Format(PyExc_AttributeError, - "'%N' object has no attribute '%s'", - tp, l->name); + "'%T' object has no attribute '%s'", + (PyObject *)obj_addr, l->name); } return v; } From 1cd2bb0a9e47d2ebe26c98c8329ba91321ee0735 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 17 Jun 2024 14:20:30 -0400 Subject: [PATCH 17/17] Update Lib/test/test_free_threading/test_slots.py --- Lib/test/test_free_threading/test_slots.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py index b4076522b4341d..758f74f54d0b56 100644 --- a/Lib/test/test_free_threading/test_slots.py +++ b/Lib/test/test_free_threading/test_slots.py @@ -28,7 +28,7 @@ def __init__(self, initial_value): self.eggs = initial_value spam = Spam(0) - iters = 1_000_000 + iters = 20_000 def writer(): for _ in range(iters):