From 6ee5b1618c5b876a6812ef8633c2322a0dae6d85 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Sat, 18 Jul 2026 14:10:49 +0200 Subject: [PATCH 1/8] [mypyc] New implementation for free-threading attribute get/set --- mypyc/codegen/emitclass.py | 14 +++-- mypyc/codegen/emitfunc.py | 20 ++++--- mypyc/lib-rt/pythonsupport.c | 29 +++++---- mypyc/lib-rt/pythonsupport.h | 94 +++++++++--------------------- mypyc/test-data/run-threading.test | 63 ++++++++++++++++++++ mypyc/test/test_emitclass.py | 28 ++++++++- mypyc/test/test_emitfunc.py | 24 ++++++++ 7 files changed, 177 insertions(+), 95 deletions(-) diff --git a/mypyc/codegen/emitclass.py b/mypyc/codegen/emitclass.py index b15c48092ce57..f6115124c68fa 100644 --- a/mypyc/codegen/emitclass.py +++ b/mypyc/codegen/emitclass.py @@ -1190,8 +1190,8 @@ def generate_getter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N attr_expr = f"self->{attr_field}" if IS_FREE_THREADED and is_simple_refcounted_pointer(rtype): - # In free-threaded builds, load the attribute and take a new reference - # atomically to avoid a use-after-free race with a concurrent setter. + # In free-threaded builds, load the attribute and take a new reference with + # an optimistic validated incref to avoid racing with a concurrent setter. # CPy_GetAttrRef returns NULL if the attribute is undefined (NULL field), # which is exactly the error/undefined value for a 'PyObject *' field. # @@ -1204,7 +1204,7 @@ def generate_getter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N if attr in cl.final_attributes: getattr_ref = f"CPy_GetAttrRefFinal((PyObject **)&{attr_expr})" else: - getattr_ref = f"CPy_GetAttrRef((PyObject **)&{attr_expr})" + getattr_ref = f"CPy_GetAttrRef((PyObject *)self, (PyObject **)&{attr_expr})" emitter.emit_line(f"PyObject *retval = {getattr_ref};") emitter.emit_line("if (unlikely(retval == NULL)) {") emitter.emit_line("PyErr_SetString(PyExc_AttributeError,") @@ -1267,10 +1267,14 @@ def generate_setter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N emitter.emit_cast("value", "tmp", rtype, declare_dest=True) emitter.emit_lines("if (!tmp)", " return -1;") emitter.emit_inc_ref("tmp", rtype) - emitter.emit_line(f"CPy_SetAttrRef((PyObject **)&self->{attr_field}, tmp);") + emitter.emit_line( + f"CPy_SetAttrRef((PyObject *)self, (PyObject **)&self->{attr_field}, tmp);" + ) if deletable: emitter.emit_line("} else {") - emitter.emit_line(f"CPy_SetAttrRef((PyObject **)&self->{attr_field}, NULL);") + emitter.emit_line( + f"CPy_SetAttrRef((PyObject *)self, (PyObject **)&self->{attr_field}, NULL);" + ) emitter.emit_line("}") emitter.emit_line("return 0;") emitter.emit_line("}") diff --git a/mypyc/codegen/emitfunc.py b/mypyc/codegen/emitfunc.py index 78746e1434331..812ea3d34376c 100644 --- a/mypyc/codegen/emitfunc.py +++ b/mypyc/codegen/emitfunc.py @@ -407,10 +407,11 @@ def emit_load_attr_take_ref( """Emit the load of a native attribute into 'dest', taking a new reference. On free-threaded builds, reading a single reference-counted 'PyObject *' field - and taking a new reference must be done atomically to avoid a use-after-free - race with a concurrent setter. CPy_GetAttrRef performs the load and incref - atomically and returns a new reference (or NULL if undefined), so callers must - NOT emit a separate inc_ref. Return True in that case so the caller can skip it. + and taking a new reference must be synchronized to avoid a use-after-free race + with a concurrent setter. CPy_GetAttrRef uses an optimistic validated incref + with a locked fallback and returns a new reference (or NULL if undefined), so + callers must NOT emit a separate inc_ref. Return True in that case so the caller + can skip it. Final attributes are never rebound (no setter), so there is no concurrent writer and no use-after-free window; an owned read uses the cheaper CPy_GetAttrRefFinal @@ -425,7 +426,9 @@ def emit_load_attr_take_ref( if use_get_attr_ref and cl.is_final_attr(op.attr): self.emitter.emit_line(f"{dest} = CPy_GetAttrRefFinal((PyObject **)&{attr_expr});") elif use_get_attr_ref: - self.emitter.emit_line(f"{dest} = CPy_GetAttrRef((PyObject **)&{attr_expr});") + self.emitter.emit_line( + f"{dest} = CPy_GetAttrRef((PyObject *){obj}, (PyObject **)&{attr_expr});" + ) else: self.emitter.emit_line(f"{dest} = {attr_expr};") return use_get_attr_ref @@ -591,8 +594,11 @@ def visit_set_attr(self, op: SetAttr) -> None: # CPy_InitAttrRef). self.emitter.emit_line(f"CPy_InitAttrRef((PyObject **)&{attr_expr}, {src});") else: - # Atomically swap in the new value and reclaim the old one. - self.emitter.emit_line(f"CPy_SetAttrRef((PyObject **)&{attr_expr}, {src});") + # Replace the value under the owner's critical section and reclaim + # the old one with a normal decref. + self.emitter.emit_line( + f"CPy_SetAttrRef((PyObject *){obj}, (PyObject **)&{attr_expr}, {src});" + ) if op.error_kind == ERR_FALSE: self.emitter.emit_line(f"{dest} = 1;") else: diff --git a/mypyc/lib-rt/pythonsupport.c b/mypyc/lib-rt/pythonsupport.c index a8f5a4f4ad4ea..c7255baa92693 100644 --- a/mypyc/lib-rt/pythonsupport.c +++ b/mypyc/lib-rt/pythonsupport.c @@ -6,23 +6,20 @@ #include "pythonsupport.h" #ifdef Py_GIL_DISABLED -// Cold slow path of CPy_GetAttrRef (declared in pythonsupport.h). Reached only -// when the inline fast-path try-incref fails: the value is owned by another -// thread, so taking a reference requires an atomic shared-refcount operation. -// Kept out-of-line so the inline fast path stays small. -// -// First try the lock-free shared-refcount CAS. If the value has not had -// maybe-weakref set yet (for example, it was published by CPy_InitAttrRef), force -// a cross-thread reference via _Py_NewRefWithLock, which cannot fail and sets -// maybe-weakref so subsequent reads take the fast path. The value was already -// observed in the field by CPy_GetAttrRef; CPy_SetAttrRef's QSBR-delayed decref -// keeps any replaced value alive long enough for this reader. +// Cold slow path of CPy_GetAttrRef (declared in pythonsupport.h). The optimistic +// try-incref failed, either because the field changed or because an unflagged +// cross-thread value could not be increfed safely. Reload under the same +// per-object lock that protects CPy_SetAttrRef, then take a reference while the +// value is guaranteed to remain in the field. _Py_XNewRefWithLock also sets +// maybe-weakref lazily, so later cross-thread reads generally take the lock-free +// fast path. CPy_NOINLINE -PyObject *CPy_GetAttrRefSlow(PyObject *v) { - if (_Py_TryIncRefShared(v)) { - return v; - } - return _Py_NewRefWithLock(v); // sets maybe-weakref; cannot fail +PyObject *CPy_GetAttrRefSlow(PyObject *owner, PyObject **field) { + PyObject *v; + Py_BEGIN_CRITICAL_SECTION(owner); + v = _Py_XNewRefWithLock((PyObject *)_Py_atomic_load_ptr_relaxed(field)); + Py_END_CRITICAL_SECTION(); + return v; } #endif diff --git a/mypyc/lib-rt/pythonsupport.h b/mypyc/lib-rt/pythonsupport.h index 33c5a596d1747..2b0ab22cab067 100644 --- a/mypyc/lib-rt/pythonsupport.h +++ b/mypyc/lib-rt/pythonsupport.h @@ -44,30 +44,28 @@ extern "C" { // // On free-threaded builds a plain load followed by an incref races with a // concurrent setter that may decref the old value to zero and free it before the -// incref runs (use-after-free). CPy_SetAttrRef avoids that by reclaiming old -// values through QSBR-delayed decref, so a value observed in the field remains -// safe to touch while this reader is running. +// incref runs (use-after-free). We avoid that with an optimistic try-incref that +// validates the field. If that fails, the slow path takes the same per-object +// lock as CPy_SetAttrRef and reloads the field before taking a reference. // -// Only the hot case is inlined here: an incref of a value owned by this thread or -// immortal, via '_Py_TryIncrefFast' (no CAS, no loop). Everything colder -- the -// cross-thread shared-refcount CAS and the _Py_NewRefWithLock fallback -- lives -// out-of-line in 'CPy_GetAttrRefSlow'. Splitting it this way keeps each call -// site's fast path small enough to inline, which is measurably faster than -// letting the compiler auto-out-line the whole helper (that merges every read -// site's branch history into one shared copy and mispredicts). It is only used in +// The hot path is lock-free. _Py_TryIncrefCompare handles values owned by this +// thread and immortal values cheaply, and uses a shared-refcount CAS followed by +// field validation when possible. An unflagged cross-thread value takes the slow +// path once; _Py_NewRefWithLock sets maybe-weakref lazily, so subsequent reads can +// generally use the lock-free shared-refcount path. It is only used in // free-threaded builds; the default (GIL) build keeps the plain load + incref // generated inline by mypyc. -PyObject *CPy_GetAttrRefSlow(PyObject *v); +PyObject *CPy_GetAttrRefSlow(PyObject *owner, PyObject **field); -static inline PyObject *CPy_GetAttrRef(PyObject **field) { +static inline PyObject *CPy_GetAttrRef(PyObject *owner, PyObject **field) { PyObject *v = (PyObject *)_Py_atomic_load_ptr_acquire(field); if (v == NULL) { return NULL; } - if (_Py_TryIncrefFast(v)) { + if (_Py_TryIncrefCompare(field, v)) { return v; } - return CPy_GetAttrRefSlow(v); + return CPy_GetAttrRefSlow(owner, field); } // Read a native attribute that is a single reference-counted 'PyObject *' field @@ -94,57 +92,23 @@ static inline PyObject *CPy_GetAttrRefFinal(PyObject **field) { return v; } -// Reclaim the previous value of a native attribute after it has been replaced. -// -// CPy_GetAttrRef reads the field optimistically without holding any lock the -// writer also takes, so a reader can load the old pointer and then try to take a -// reference after this store. The old value must therefore stay alive until every -// thread has passed a quiescent point, which is exactly what a QSBR-deferred -// decref guarantees. So all mortal old values are -// reclaimed via _PyObject_XDecRefDelayed, matching CPython's own replace-a-slot -// paths (e.g. _PyObject_SetDict / _PyObject_SetManagedDict). -// -// We deliberately do NOT take a "local refcount > 1, owned by this thread" fast -// path: dropping the field's reference is not the only decref of the object, so a -// non-freeing local decrement here does not prevent an unrelated reference holder -// from driving the object to zero (a plain, non-deferred Py_DECREF -> _Py_Dealloc) -// while an in-flight reader still holds the stale pointer -- a use-after-free that -// only QSBR deferral closes. Immortal objects are never freed, so skipping their -// decref entirely is safe and avoids queuing a no-op onto the delayed-free list. -static inline void CPy_DecRefAttrOld(PyObject *op) { - if (op == NULL) { - return; - } - if (_Py_IsImmortal(op)) { - return; - } - _PyObject_XDecRefDelayed(op); -} - // Set a native attribute that is a single reference-counted 'PyObject *' field, // stealing the reference to 'value' (which may be NULL to delete the attribute) // and safely reclaiming the previous value. // -// Memory safety does NOT depend on SetMaybeWeakref here, so (unlike an earlier -// version) we do not call it. Two things keep this safe: -// - The old value is reclaimed via CPy_DecRefAttrOld, a QSBR-deferred decref, so -// it cannot be freed while an in-flight CPy_GetAttrRef still holds the stale -// pointer. -// - A concurrent cross-thread reader whose inline fast-path try-incref fails on -// an unflagged 'value' still cannot fail and never blocks on this writer: -// CPy_GetAttrRef falls into CPy_GetAttrRefSlow, which is fully lock-free. It -// retries with a lock-free shared-refcount CAS (_Py_TryIncRefShared) and, only -// if that also fails, forces a reference via _Py_NewRefWithLock, which cannot -// fail and lazily sets maybe-weakref so later cross-thread reads take the CAS. -// This mirrors CPy_InitAttrRef, which already omits SetMaybeWeakref for the same -// reason. Setting the flag here would only be a possible performance tuning knob -// (it would let that first cross-thread reader succeed on the cheaper -// _Py_TryIncRefShared CAS instead of falling through to _Py_NewRefWithLock); it is -// not needed for correctness. The atomic exchange publishes the new pointer and -// hands back the old one without a writer/writer race. -static inline void CPy_SetAttrRef(PyObject **field, PyObject *value) { - PyObject *old = (PyObject *)_Py_atomic_exchange_ptr(field, value); - CPy_DecRefAttrOld(old); +// The owner's critical section serializes writers and synchronizes them with +// CPy_GetAttrRef's fallback path. A lock-free reader either secures a reference +// and validates the field, or fails and reloads it under this lock. Thus once the +// replacement is published, the old field reference can be decrefed normally; +// no QSBR-delayed decref is needed. Decref outside the critical section so an +// arbitrary destructor does not run while the owner is locked. +static inline void CPy_SetAttrRef(PyObject *owner, PyObject **field, PyObject *value) { + PyObject *old; + Py_BEGIN_CRITICAL_SECTION(owner); + old = (PyObject *)_Py_atomic_load_ptr_relaxed(field); + _Py_atomic_store_ptr_release(field, value); + Py_END_CRITICAL_SECTION(); + Py_XDECREF(old); } // Initialize a native attribute that is known to be previously undefined (NULL), @@ -158,10 +122,10 @@ static inline void CPy_SetAttrRef(PyObject **field, PyObject *value) { // returned), and that publication carries the release barrier making all the // construction stores visible. A relaxed store therefore suffices. // -// Unlike CPy_SetAttrRef, this deliberately does NOT call SetMaybeWeakref (its CAS -// is pure overhead here, ~+2.6ns per fresh store, and construction-heavy code -// pays it on every attribute of every new object). The cost is moved off this hot -// path onto CPy_GetAttrRef's cold slow path, which sets maybe-weakref lazily on +// This deliberately does NOT call SetMaybeWeakref (its CAS is pure overhead here, +// ~+2.6ns per fresh store, and construction-heavy code pays it on every attribute +// of every new object). CPy_SetAttrRef likewise leaves the flag unset. The cost is +// moved onto CPy_GetAttrRef's cold slow path, which sets maybe-weakref lazily on // the first cross-thread read that needs it. static inline void CPy_InitAttrRef(PyObject **field, PyObject *value) { _Py_atomic_store_ptr_relaxed(field, value); diff --git a/mypyc/test-data/run-threading.test b/mypyc/test-data/run-threading.test index 8af8755b76c44..45c4cc63b1f5f 100644 --- a/mypyc/test-data/run-threading.test +++ b/mypyc/test-data/run-threading.test @@ -1,5 +1,68 @@ # Test cases for librt.threading (compile and run) +[case testConcurrentNativeAttributeGetSet] +from typing import Any +import threading +import sys + +class Item: + def __init__(self, value: int) -> None: + self.value = value + +class Box: + def __init__(self) -> None: + self.item = Item(0) + +def native_writer(box: Box, count: int) -> None: + for i in range(count): + box.item = Item(i) + +def native_reader(box: Box, count: int) -> None: + for _ in range(count): + item = box.item + assert item.value >= 0 + +def dynamic_writer(box: Any, count: int) -> None: + for i in range(count): + box.item = Item(i) + +def dynamic_reader(box: Any, count: int) -> None: + for _ in range(count): + item = box.item + assert item.value >= 0 + +def test_attribute_set_decrefs_old_value_immediately() -> None: + box = Box() + dynamic_sys: Any = sys + old = box.item + before = dynamic_sys.getrefcount(old) + box.item = Item(1) + assert dynamic_sys.getrefcount(old) == before - 1 + + dynamic: Any = box + old = box.item + before = dynamic_sys.getrefcount(old) + dynamic.item = Item(2) + assert dynamic_sys.getrefcount(old) == before - 1 + +def test_concurrent_native_attribute_get_set() -> None: + # Native access exercises the direct generated calls. Access through Any uses + # the generated getset descriptor. On a free-threaded build all four workers + # race, repeatedly dropping the field's last reference to the previous Item. + box = Box() + count = 10_000 + threads = [ + threading.Thread(target=native_writer, args=(box, count)), + threading.Thread(target=native_reader, args=(box, count)), + threading.Thread(target=dynamic_writer, args=(box, count)), + threading.Thread(target=dynamic_reader, args=(box, count)), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + assert box.item.value >= 0 + [case testLockBasics_librt] from typing import Any from testutil import assertRaises diff --git a/mypyc/test/test_emitclass.py b/mypyc/test/test_emitclass.py index 9c3cd02d1100c..149798e98de44 100644 --- a/mypyc/test/test_emitclass.py +++ b/mypyc/test/test_emitclass.py @@ -1,11 +1,19 @@ from __future__ import annotations import unittest +from unittest.mock import patch from mypyc.analysis.attrdefined import detect_undefined_bitmap -from mypyc.codegen.emitclass import getter_name, setter_name, slot_key +from mypyc.codegen.emit import Emitter, EmitterContext +from mypyc.codegen.emitclass import ( + generate_getter, + generate_setter, + getter_name, + setter_name, + slot_key, +) from mypyc.ir.class_ir import ClassIR -from mypyc.ir.rtypes import int32_rprimitive +from mypyc.ir.rtypes import int32_rprimitive, object_rprimitive from mypyc.namegen import NameGenerator @@ -36,6 +44,22 @@ def test_getter_name(self) -> None: assert getter_name(cls, "down", generator) == "testing___SomeClass_get_down" + def test_free_threaded_ref_attribute_getter_and_setter_use_owner(self) -> None: + cl = ClassIR("A", "mod") + cl.attributes = {"o": object_rprimitive} + cl.deletable = ["o"] + cl.mro = cl.base_mro = [cl] + emitter = Emitter(EmitterContext(NameGenerator([["mod"]]), True)) + + with patch("mypyc.codegen.emitclass.IS_FREE_THREADED", True): + generate_getter(cl, "o", object_rprimitive, emitter) + generate_setter(cl, "o", object_rprimitive, emitter) + + generated = "".join(emitter.fragments) + assert "CPy_GetAttrRef((PyObject *)self, (PyObject **)&self->_o)" in generated + assert "CPy_SetAttrRef((PyObject *)self, (PyObject **)&self->_o, tmp)" in generated + assert "CPy_SetAttrRef((PyObject *)self, (PyObject **)&self->_o, NULL)" in generated + def test_bitmap_attrs_stable_across_repeat_analysis(self) -> None: # Regression: detect_undefined_bitmap used to mutate cl.bitmap_attrs # in place, so under separate=True (one SCC per group) a shared base diff --git a/mypyc/test/test_emitfunc.py b/mypyc/test/test_emitfunc.py index 00cbe603a9b91..6fe82824ed08d 100644 --- a/mypyc/test/test_emitfunc.py +++ b/mypyc/test/test_emitfunc.py @@ -1,6 +1,7 @@ from __future__ import annotations import unittest +from unittest.mock import patch from mypy.test.helpers import assert_string_arrays_equal from mypyc.codegen.emit import Emitter, EmitterContext @@ -121,6 +122,7 @@ def add_local(name: str, rtype: RType) -> Register: ir.attributes = { "x": bool_rprimitive, "y": int_rprimitive, + "o": object_rprimitive, "i1": int64_rprimitive, "i2": int32_rprimitive, "t": RTuple([object_rprimitive, object_rprimitive]), @@ -487,6 +489,18 @@ def test_get_attr_non_refcounted(self) -> None: """, ) + def test_get_attr_ref_free_threaded(self) -> None: + with patch("mypyc.codegen.emitfunc.IS_FREE_THREADED", True): + self.assert_emit( + GetAttr(self.r, "o", 1), + """\ + cpy_r_r0 = CPy_GetAttrRef((PyObject *)cpy_r_r, (PyObject **)&((mod___AObject *)cpy_r_r)->_o); + if (unlikely(cpy_r_r0 == NULL)) { + PyErr_SetString(PyExc_AttributeError, "attribute 'o' of 'A' undefined"); + } + """, + ) + def test_get_attr_merged(self) -> None: op = GetAttr(self.r, "y", 1) branch = Branch(op, BasicBlock(8), BasicBlock(9), Branch.IS_ERROR) @@ -538,6 +552,16 @@ def test_set_attr(self) -> None: """, ) + def test_set_attr_ref_free_threaded(self) -> None: + with patch("mypyc.codegen.emitfunc.IS_FREE_THREADED", True): + self.assert_emit( + SetAttr(self.r, "o", self.o, 1), + """\ + CPy_SetAttrRef((PyObject *)cpy_r_r, (PyObject **)&((mod___AObject *)cpy_r_r)->_o, cpy_r_o); + cpy_r_r0 = 1; + """, + ) + def test_set_attr_non_refcounted(self) -> None: self.assert_emit( SetAttr(self.r, "x", self.b, 1), From 6086005d88201c4b79635ee3ac2c45c2e08169c4 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Sat, 18 Jul 2026 14:53:42 +0200 Subject: [PATCH 2/8] Optimize attribute read further --- mypyc/lib-rt/mypyc_util.h | 3 +++ mypyc/lib-rt/pythonsupport.c | 28 +++++++++++++++++----------- mypyc/lib-rt/pythonsupport.h | 29 ++++++++++++++++------------- 3 files changed, 36 insertions(+), 24 deletions(-) diff --git a/mypyc/lib-rt/mypyc_util.h b/mypyc/lib-rt/mypyc_util.h index d7a3eb3214bed..4048fe93aa92c 100644 --- a/mypyc/lib-rt/mypyc_util.h +++ b/mypyc/lib-rt/mypyc_util.h @@ -17,10 +17,13 @@ #if defined(__clang__) || defined(__GNUC__) #define CPy_NOINLINE __attribute__((noinline)) +#define CPy_COLD __attribute__((cold)) #elif defined(_MSC_VER) #define CPy_NOINLINE __declspec(noinline) +#define CPy_COLD #else #define CPy_NOINLINE +#define CPy_COLD #endif #ifndef Py_GIL_DISABLED diff --git a/mypyc/lib-rt/pythonsupport.c b/mypyc/lib-rt/pythonsupport.c index c7255baa92693..11347a572a1ac 100644 --- a/mypyc/lib-rt/pythonsupport.c +++ b/mypyc/lib-rt/pythonsupport.c @@ -6,20 +6,26 @@ #include "pythonsupport.h" #ifdef Py_GIL_DISABLED -// Cold slow path of CPy_GetAttrRef (declared in pythonsupport.h). The optimistic -// try-incref failed, either because the field changed or because an unflagged -// cross-thread value could not be increfed safely. Reload under the same -// per-object lock that protects CPy_SetAttrRef, then take a reference while the -// value is guaranteed to remain in the field. _Py_XNewRefWithLock also sets -// maybe-weakref lazily, so later cross-thread reads generally take the lock-free -// fast path. +// Cold slow path of CPy_GetAttrRef (declared in pythonsupport.h). First try the +// lock-free shared-refcount path and validate that the field still contains the +// value. If that fails, reload under the same per-object lock that protects +// CPy_SetAttrRef and take a reference while the value is guaranteed to remain in +// the field. _Py_XNewRefWithLock also sets maybe-weakref lazily, so later +// cross-thread reads generally use the lock-free shared-refcount path. CPy_NOINLINE -PyObject *CPy_GetAttrRefSlow(PyObject *owner, PyObject **field) { - PyObject *v; +CPy_COLD +PyObject *CPy_GetAttrRefSlow(PyObject *v, PyObject *owner, PyObject **field) { + if (_Py_TryIncRefShared(v)) { + if (v == (PyObject *)_Py_atomic_load_ptr(field)) { + return v; + } + Py_DECREF(v); + } + PyObject *result; Py_BEGIN_CRITICAL_SECTION(owner); - v = _Py_XNewRefWithLock((PyObject *)_Py_atomic_load_ptr_relaxed(field)); + result = _Py_XNewRefWithLock((PyObject *)_Py_atomic_load_ptr_relaxed(field)); Py_END_CRITICAL_SECTION(); - return v; + return result; } #endif diff --git a/mypyc/lib-rt/pythonsupport.h b/mypyc/lib-rt/pythonsupport.h index 2b0ab22cab067..6e186f52b143a 100644 --- a/mypyc/lib-rt/pythonsupport.h +++ b/mypyc/lib-rt/pythonsupport.h @@ -44,28 +44,31 @@ extern "C" { // // On free-threaded builds a plain load followed by an incref races with a // concurrent setter that may decref the old value to zero and free it before the -// incref runs (use-after-free). We avoid that with an optimistic try-incref that -// validates the field. If that fails, the slow path takes the same per-object -// lock as CPy_SetAttrRef and reloads the field before taking a reference. +// incref runs (use-after-free). We avoid that with an optimistic try-incref, +// validating the field when taking a shared reference. If no reference can be +// secured that way, the fallback takes the same per-object lock as CPy_SetAttrRef +// and reloads the field before taking a reference. // -// The hot path is lock-free. _Py_TryIncrefCompare handles values owned by this -// thread and immortal values cheaply, and uses a shared-refcount CAS followed by -// field validation when possible. An unflagged cross-thread value takes the slow -// path once; _Py_NewRefWithLock sets maybe-weakref lazily, so subsequent reads can -// generally use the lock-free shared-refcount path. It is only used in -// free-threaded builds; the default (GIL) build keeps the plain load + incref -// generated inline by mypyc. -PyObject *CPy_GetAttrRefSlow(PyObject *owner, PyObject **field); +// The hot path is lock-free and intentionally small: _Py_TryIncrefFast handles +// values owned by this thread and immortal values. Everything colder -- the +// shared-refcount CAS, field validation, and per-object lock fallback -- is in +// CPy_GetAttrRefSlow. The cold annotation also prevents those arguments and +// branches from bloating the caller's hot path. An unflagged cross-thread value +// takes the locked slow path once; _Py_NewRefWithLock sets maybe-weakref lazily, +// so subsequent reads generally succeed through the lock-free shared-refcount +// path. The default (GIL) build keeps the plain load + incref generated inline by +// mypyc. +CPy_COLD PyObject *CPy_GetAttrRefSlow(PyObject *v, PyObject *owner, PyObject **field); static inline PyObject *CPy_GetAttrRef(PyObject *owner, PyObject **field) { PyObject *v = (PyObject *)_Py_atomic_load_ptr_acquire(field); if (v == NULL) { return NULL; } - if (_Py_TryIncrefCompare(field, v)) { + if (likely(_Py_TryIncrefFast(v))) { return v; } - return CPy_GetAttrRefSlow(owner, field); + return CPy_GetAttrRefSlow(v, owner, field); } // Read a native attribute that is a single reference-counted 'PyObject *' field From 8442238ac005a70733b6e408c9d4d87c6e32686a Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Sat, 18 Jul 2026 14:57:49 +0200 Subject: [PATCH 3/8] Update comments --- mypyc/lib-rt/pythonsupport.c | 13 +++++++------ mypyc/lib-rt/pythonsupport.h | 15 ++++++++------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/mypyc/lib-rt/pythonsupport.c b/mypyc/lib-rt/pythonsupport.c index 11347a572a1ac..42e91a54646ec 100644 --- a/mypyc/lib-rt/pythonsupport.c +++ b/mypyc/lib-rt/pythonsupport.c @@ -6,12 +6,13 @@ #include "pythonsupport.h" #ifdef Py_GIL_DISABLED -// Cold slow path of CPy_GetAttrRef (declared in pythonsupport.h). First try the -// lock-free shared-refcount path and validate that the field still contains the -// value. If that fails, reload under the same per-object lock that protects -// CPy_SetAttrRef and take a reference while the value is guaranteed to remain in -// the field. _Py_XNewRefWithLock also sets maybe-weakref lazily, so later -// cross-thread reads generally use the lock-free shared-refcount path. +// Cold slow path of CPy_GetAttrRef (declared in pythonsupport.h). First try to +// acquire a shared reference without locking, then validate that the field still +// contains v. If validation fails, drop the provisional reference. If the shared +// incref or validation fails, take the owner's critical section, reload the field, +// and take a reference while the value cannot be replaced. _Py_XNewRefWithLock +// also sets maybe-weakref lazily, so later cross-thread reads generally use the +// lock-free shared-refcount path. CPy_NOINLINE CPy_COLD PyObject *CPy_GetAttrRefSlow(PyObject *v, PyObject *owner, PyObject **field) { diff --git a/mypyc/lib-rt/pythonsupport.h b/mypyc/lib-rt/pythonsupport.h index 6e186f52b143a..71054ee203760 100644 --- a/mypyc/lib-rt/pythonsupport.h +++ b/mypyc/lib-rt/pythonsupport.h @@ -54,7 +54,7 @@ extern "C" { // shared-refcount CAS, field validation, and per-object lock fallback -- is in // CPy_GetAttrRefSlow. The cold annotation also prevents those arguments and // branches from bloating the caller's hot path. An unflagged cross-thread value -// takes the locked slow path once; _Py_NewRefWithLock sets maybe-weakref lazily, +// takes the locked slow path once; _Py_XNewRefWithLock sets maybe-weakref lazily, // so subsequent reads generally succeed through the lock-free shared-refcount // path. The default (GIL) build keeps the plain load + incref generated inline by // mypyc. @@ -79,7 +79,7 @@ static inline PyObject *CPy_GetAttrRef(PyObject *owner, PyObject **field) { // use-after-free race that CPy_GetAttrRef guards against cannot happen: the field // holds a strong reference for the object's whole lifetime, and any thread reading // it necessarily holds 'self', which keeps the value alive. So the try-incref + -// _Py_NewRefWithLock fallback are unnecessary here -- a plain load + Py_INCREF is +// _Py_XNewRefWithLock fallback are unnecessary here -- a plain load + Py_INCREF is // safe. A cross-thread Py_INCREF is an unconditional // atomic add on ob_ref_shared, so (unlike CPy_GetAttrRef's try-incref) it needs no // maybe-weakref and has no slow path. The load is relaxed rather than acquire: the @@ -100,11 +100,12 @@ static inline PyObject *CPy_GetAttrRefFinal(PyObject **field) { // and safely reclaiming the previous value. // // The owner's critical section serializes writers and synchronizes them with -// CPy_GetAttrRef's fallback path. A lock-free reader either secures a reference -// and validates the field, or fails and reloads it under this lock. Thus once the -// replacement is published, the old field reference can be decrefed normally; -// no QSBR-delayed decref is needed. Decref outside the critical section so an -// arbitrary destructor does not run while the owner is locked. +// CPy_GetAttrRef's fallback path. A lock-free reader either secures a local or +// immortal reference directly, or secures a shared reference and validates the +// field. Otherwise it reloads the field under this lock. Thus once the replacement +// is published, the old field reference can be decrefed normally; no QSBR-delayed +// decref is needed. Decref outside the critical section so an arbitrary destructor +// does not run while the owner is locked. static inline void CPy_SetAttrRef(PyObject *owner, PyObject **field, PyObject *value) { PyObject *old; Py_BEGIN_CRITICAL_SECTION(owner); From 54f5644742c309d0b644fad8e07dd41656eadf62 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 19 Aug 2026 14:52:38 +0100 Subject: [PATCH 4/8] Fix tests --- mypyc/test/test_emitfunc.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/mypyc/test/test_emitfunc.py b/mypyc/test/test_emitfunc.py index 6fe82824ed08d..5bfc921ecfa71 100644 --- a/mypyc/test/test_emitfunc.py +++ b/mypyc/test/test_emitfunc.py @@ -1,12 +1,11 @@ from __future__ import annotations import unittest -from unittest.mock import patch from mypy.test.helpers import assert_string_arrays_equal from mypyc.codegen.emit import Emitter, EmitterContext from mypyc.codegen.emitfunc import FunctionEmitterVisitor, generate_native_function -from mypyc.common import HAVE_IMMORTAL, PLATFORM_SIZE +from mypyc.common import HAVE_IMMORTAL, IS_FREE_THREADED, PLATFORM_SIZE from mypyc.ir.class_ir import ClassIR from mypyc.ir.func_ir import FuncDecl, FuncIR, FuncSignature, RuntimeArg from mypyc.ir.ops import ( @@ -490,7 +489,7 @@ def test_get_attr_non_refcounted(self) -> None: ) def test_get_attr_ref_free_threaded(self) -> None: - with patch("mypyc.codegen.emitfunc.IS_FREE_THREADED", True): + if IS_FREE_THREADED: self.assert_emit( GetAttr(self.r, "o", 1), """\ @@ -553,7 +552,7 @@ def test_set_attr(self) -> None: ) def test_set_attr_ref_free_threaded(self) -> None: - with patch("mypyc.codegen.emitfunc.IS_FREE_THREADED", True): + if IS_FREE_THREADED: self.assert_emit( SetAttr(self.r, "o", self.o, 1), """\ From 1db4eda1f59ed4e7d71cd3d2d1ce0598f1eba3bc Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 2 Sep 2026 16:31:46 +0100 Subject: [PATCH 5/8] Fix test on compiled build --- mypyc/test/test_emitclass.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/mypyc/test/test_emitclass.py b/mypyc/test/test_emitclass.py index 149798e98de44..4e34f35ef464e 100644 --- a/mypyc/test/test_emitclass.py +++ b/mypyc/test/test_emitclass.py @@ -1,7 +1,6 @@ from __future__ import annotations import unittest -from unittest.mock import patch from mypyc.analysis.attrdefined import detect_undefined_bitmap from mypyc.codegen.emit import Emitter, EmitterContext @@ -12,6 +11,7 @@ setter_name, slot_key, ) +from mypyc.common import IS_FREE_THREADED from mypyc.ir.class_ir import ClassIR from mypyc.ir.rtypes import int32_rprimitive, object_rprimitive from mypyc.namegen import NameGenerator @@ -44,16 +44,18 @@ def test_getter_name(self) -> None: assert getter_name(cls, "down", generator) == "testing___SomeClass_get_down" + @unittest.skipUnless(IS_FREE_THREADED, "requires a free threaded build") def test_free_threaded_ref_attribute_getter_and_setter_use_owner(self) -> None: + # Note: We can't monkey patch IS_FREE_THREADED to test this on a build with + # the GIL enabled, since monkey patching doesn't work if mypyc is compiled. cl = ClassIR("A", "mod") cl.attributes = {"o": object_rprimitive} cl.deletable = ["o"] cl.mro = cl.base_mro = [cl] emitter = Emitter(EmitterContext(NameGenerator([["mod"]]), True)) - with patch("mypyc.codegen.emitclass.IS_FREE_THREADED", True): - generate_getter(cl, "o", object_rprimitive, emitter) - generate_setter(cl, "o", object_rprimitive, emitter) + generate_getter(cl, "o", object_rprimitive, emitter) + generate_setter(cl, "o", object_rprimitive, emitter) generated = "".join(emitter.fragments) assert "CPy_GetAttrRef((PyObject *)self, (PyObject **)&self->_o)" in generated From efebdaab4b0aea17b4f8cd3c432efe9fcce47014 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 2 Sep 2026 17:50:49 +0100 Subject: [PATCH 6/8] Refactor tests --- mypyc/test/test_emitfunc.py | 38 +++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/mypyc/test/test_emitfunc.py b/mypyc/test/test_emitfunc.py index 5bfc921ecfa71..adaafbf7af9cc 100644 --- a/mypyc/test/test_emitfunc.py +++ b/mypyc/test/test_emitfunc.py @@ -488,17 +488,19 @@ def test_get_attr_non_refcounted(self) -> None: """, ) + # Note: We can't monkey patch IS_FREE_THREADED to test this on a build with the + # GIL enabled, since monkey patching doesn't work if mypyc is compiled. + @unittest.skipUnless(IS_FREE_THREADED, "requires a free threaded build") def test_get_attr_ref_free_threaded(self) -> None: - if IS_FREE_THREADED: - self.assert_emit( - GetAttr(self.r, "o", 1), - """\ - cpy_r_r0 = CPy_GetAttrRef((PyObject *)cpy_r_r, (PyObject **)&((mod___AObject *)cpy_r_r)->_o); - if (unlikely(cpy_r_r0 == NULL)) { - PyErr_SetString(PyExc_AttributeError, "attribute 'o' of 'A' undefined"); - } - """, - ) + self.assert_emit( + GetAttr(self.r, "o", 1), + """\ + cpy_r_r0 = CPy_GetAttrRef((PyObject *)cpy_r_r, (PyObject **)&((mod___AObject *)cpy_r_r)->_o); + if (unlikely(cpy_r_r0 == NULL)) { + PyErr_SetString(PyExc_AttributeError, "attribute 'o' of 'A' undefined"); + } + """, + ) def test_get_attr_merged(self) -> None: op = GetAttr(self.r, "y", 1) @@ -551,15 +553,15 @@ def test_set_attr(self) -> None: """, ) + @unittest.skipUnless(IS_FREE_THREADED, "requires a free threaded build") def test_set_attr_ref_free_threaded(self) -> None: - if IS_FREE_THREADED: - self.assert_emit( - SetAttr(self.r, "o", self.o, 1), - """\ - CPy_SetAttrRef((PyObject *)cpy_r_r, (PyObject **)&((mod___AObject *)cpy_r_r)->_o, cpy_r_o); - cpy_r_r0 = 1; - """, - ) + self.assert_emit( + SetAttr(self.r, "o", self.o, 1), + """\ + CPy_SetAttrRef((PyObject *)cpy_r_r, (PyObject **)&((mod___AObject *)cpy_r_r)->_o, cpy_r_o); + cpy_r_r0 = 1; + """, + ) def test_set_attr_non_refcounted(self) -> None: self.assert_emit( From f66060bcb9394a8c25bcaaeefe7d7ba502910ed7 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 2 Sep 2026 17:51:46 +0100 Subject: [PATCH 7/8] Add test for concurrent attribute deletes --- mypyc/test-data/run-threading.test | 67 ++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/mypyc/test-data/run-threading.test b/mypyc/test-data/run-threading.test index 45c4cc63b1f5f..b169a6d133ec5 100644 --- a/mypyc/test-data/run-threading.test +++ b/mypyc/test-data/run-threading.test @@ -63,6 +63,73 @@ def test_concurrent_native_attribute_get_set() -> None: thread.join() assert box.item.value >= 0 +[case testConcurrentNativeAttributeDelete] +from typing import Any +import threading + +class Item: + def __init__(self, value: int) -> None: + self.value = value + +class DelBox: + __deletable__ = ["item"] + + def __init__(self) -> None: + self.item = Item(0) + +def native_setter(box: DelBox, count: int) -> None: + for i in range(count): + box.item = Item(i) + +def native_deleter(box: DelBox, count: int) -> None: + for _ in range(count): + try: + del box.item + except AttributeError: + pass + +def native_reader(box: DelBox, count: int) -> None: + for _ in range(count): + try: + item = box.item + except AttributeError: + continue + assert item.value >= 0 + +def dynamic_deleter(box: Any, count: int) -> None: + for _ in range(count): + try: + del box.item + except AttributeError: + pass + +def dynamic_reader(box: Any, count: int) -> None: + for _ in range(count): + try: + item = box.item + except AttributeError: + continue + assert item.value >= 0 + +def test_concurrent_native_attribute_delete() -> None: + # A deleted attribute is the only way a read that already observed a live value + # can still fail: the optimistic incref can miss and the locked reload then sees + # NULL, so the read raises AttributeError (see CPy_GetAttrRefSlow). Readers here + # must therefore tolerate AttributeError, but never see a freed or torn value. + box = DelBox() + count = 10_000 + threads = [ + threading.Thread(target=native_setter, args=(box, count)), + threading.Thread(target=native_deleter, args=(box, count)), + threading.Thread(target=native_reader, args=(box, count)), + threading.Thread(target=dynamic_deleter, args=(box, count)), + threading.Thread(target=dynamic_reader, args=(box, count)), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + [case testLockBasics_librt] from typing import Any from testutil import assertRaises From a84646bf7d647f9b4b1d0186600c124db1e968d2 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 2 Sep 2026 18:55:57 +0100 Subject: [PATCH 8/8] Some comment updates --- mypyc/codegen/emitclass.py | 16 +++++----- mypyc/codegen/emitfunc.py | 22 +++++++------- mypyc/lib-rt/pythonsupport.c | 8 ++++- mypyc/lib-rt/pythonsupport.h | 57 +++++++++++++++++++++++++++++++----- 4 files changed, 78 insertions(+), 25 deletions(-) diff --git a/mypyc/codegen/emitclass.py b/mypyc/codegen/emitclass.py index f6115124c68fa..4427d70f51afb 100644 --- a/mypyc/codegen/emitclass.py +++ b/mypyc/codegen/emitclass.py @@ -1197,8 +1197,9 @@ def generate_getter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N # # Final attributes are never rebound (no setter), so there is no concurrent # writer to race with: a plain load + incref is safe. Use the cheaper - # CPy_GetAttrRefFinal, which skips the try-incref and _Py_NewRefWithLock - # slow path entirely (an unconditional Py_INCREF needs no maybe-weakref). + # CPy_GetAttrRefFinal, which skips the try-incref and the locked + # _Py_XNewRefWithLock fallback entirely (an unconditional Py_INCREF needs no + # maybe-weakref). # This getter is generated per defining class, so a direct membership test # matches the read-only getset table above (no need to walk the MRO). if attr in cl.final_attributes: @@ -1253,11 +1254,12 @@ def generate_setter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N emitter.emit_line("}") if IS_FREE_THREADED and is_simple_refcounted_pointer(rtype): - # In free-threaded builds, publish the new value atomically via - # CPy_SetAttrRef so a concurrent reader (see CPy_GetAttrRef) never sees a - # torn pointer or a freed old value. CPy_SetAttrRef steals its value and - # reclaims the old one, so we cast/type-check the incoming value, take a - # new reference (the setter only borrows 'value'), then hand it over. + # In free-threaded builds, publish the new value via CPy_SetAttrRef, which + # takes the owner's critical section so a concurrent reader (see + # CPy_GetAttrRef) can always secure a reference to the value it observes, + # even though the old value is decrefed right away. CPy_SetAttrRef steals its + # value and reclaims the old one, so we cast/type-check the incoming value, + # take a new reference (the setter only borrows 'value'), then hand it over. # A NULL value deletes the attribute (reclaims the old value, stores NULL). if deletable: emitter.emit_line("if (value != NULL) {") diff --git a/mypyc/codegen/emitfunc.py b/mypyc/codegen/emitfunc.py index 812ea3d34376c..28e6110361e9d 100644 --- a/mypyc/codegen/emitfunc.py +++ b/mypyc/codegen/emitfunc.py @@ -409,9 +409,10 @@ def emit_load_attr_take_ref( On free-threaded builds, reading a single reference-counted 'PyObject *' field and taking a new reference must be synchronized to avoid a use-after-free race with a concurrent setter. CPy_GetAttrRef uses an optimistic validated incref - with a locked fallback and returns a new reference (or NULL if undefined), so - callers must NOT emit a separate inc_ref. Return True in that case so the caller - can skip it. + with a locked fallback and returns a new reference (or NULL if undefined, which + on a free-threaded build includes an attribute deleted by another thread while + the read was in flight), so callers must NOT emit a separate inc_ref. Return + True in that case so the caller can skip it. Final attributes are never rebound (no setter), so there is no concurrent writer and no use-after-free window; an owned read uses the cheaper CPy_GetAttrRefFinal @@ -583,15 +584,16 @@ def visit_set_attr(self, op: SetAttr) -> None: self.emitter.emit_error_check(tmp, ret_type, f"{dest} = 0;") elif IS_FREE_THREADED and is_simple_refcounted_pointer(attr_rtype): # In free-threaded builds, publishing a single reference-counted - # 'PyObject *' field must be atomic so a concurrent reader (see - # CPy_GetAttrRef) never observes a torn pointer or a freed value. - # Both helpers steal the reference to src. + # 'PyObject *' field must be synchronized with concurrent readers (see + # CPy_GetAttrRef): the store is atomic, and CPy_SetAttrRef additionally + # takes the owner's critical section, which is what lets it decref the + # old value right away. Both helpers steal the reference to src. attr_expr = self.get_attr_expr(obj, op, decl_cl) if op.is_init: - # The attribute is known to be previously undefined (NULL), so - # there is no old value to reclaim; a relaxed store suffices - # (self's later publication provides the release barrier -- see - # CPy_InitAttrRef). + # The attribute is known to be previously undefined (NULL) and self + # can't have leaked yet, so there is no old value to reclaim and no + # competing writer; a relaxed store suffices (self's later publication + # provides the release barrier -- see CPy_InitAttrRef). self.emitter.emit_line(f"CPy_InitAttrRef((PyObject **)&{attr_expr}, {src});") else: # Replace the value under the owner's critical section and reclaim diff --git a/mypyc/lib-rt/pythonsupport.c b/mypyc/lib-rt/pythonsupport.c index 42e91a54646ec..d06d734ff1f4d 100644 --- a/mypyc/lib-rt/pythonsupport.c +++ b/mypyc/lib-rt/pythonsupport.c @@ -12,7 +12,13 @@ // incref or validation fails, take the owner's critical section, reload the field, // and take a reference while the value cannot be replaced. _Py_XNewRefWithLock // also sets maybe-weakref lazily, so later cross-thread reads generally use the -// lock-free shared-refcount path. +// lock-free shared-refcount path. Returns NULL if the field is NULL on reload. +// +// The reload is deliberately relaxed, where CPython 3.14 uses a consume load in the +// equivalent locked read (_PyObject_TryGetInstanceAttribute): acquiring the owner's +// critical section already pairs with CPy_SetAttrRef's release store, and a value +// published by CPy_InitAttrRef is ordered by the publication of the owner itself +// (see CPy_InitAttrRef). CPy_NOINLINE CPy_COLD PyObject *CPy_GetAttrRefSlow(PyObject *v, PyObject *owner, PyObject **field) { diff --git a/mypyc/lib-rt/pythonsupport.h b/mypyc/lib-rt/pythonsupport.h index 71054ee203760..b97d19e14dc9d 100644 --- a/mypyc/lib-rt/pythonsupport.h +++ b/mypyc/lib-rt/pythonsupport.h @@ -47,7 +47,30 @@ extern "C" { // incref runs (use-after-free). We avoid that with an optimistic try-incref, // validating the field when taking a shared reference. If no reference can be // secured that way, the fallback takes the same per-object lock as CPy_SetAttrRef -// and reloads the field before taking a reference. +// and reloads the field before taking a reference. This mirrors CPython's own +// instance attribute read (in CPython 3.14: _Py_TryIncrefCompare, then a locked +// reload -- see _PyObject_TryGetInstanceAttribute in Objects/dictobject.c). +// +// The load and _Py_TryIncrefFast can therefore touch a 'v' that a concurrent +// CPy_SetAttrRef has already decrefed to zero and freed, since that decref is a +// plain Py_XDECREF. Two invariants make that safe, and they are what let +// CPy_SetAttrRef free the old value immediately instead of deferring it. Both were +// verified against CPython 3.14 and depend on interpreter internals, so they must +// be rechecked when adding support for a new Python version: +// - Reading the header of a freed object cannot fault. All three object heaps +// set 'page_use_qsbr' (see Python/pystate.c), so a freed block's page is not +// unmapped or handed to another size class while any thread is attached. +// - _Py_TryIncrefFast cannot succeed on stale memory. The current thread does +// not allocate between the field load and the try-incref, so the block cannot +// have been reused for an object owned by this thread; a freed block reads +// ob_tid as 0 or as mimalloc's free-list pointer (which overwrites only the +// first word, i.e. ob_tid) and ob_ref_local as 0, so neither the +// owned-by-this-thread test nor the immortal test can fire. +// If the block was reused for a live object at the same address, that object is +// either the field's current value (so returning it is correct) or the field +// validation fails and the provisional reference is dropped again, leaving its +// refcount unchanged. ob_ref_shared of a freed block is 0 or _Py_REF_MERGED, so +// _Py_TryIncRefShared fails on it and the reader falls into the locked path. // // The hot path is lock-free and intentionally small: _Py_TryIncrefFast handles // values owned by this thread and immortal values. Everything colder -- the @@ -58,6 +81,11 @@ extern "C" { // so subsequent reads generally succeed through the lock-free shared-refcount // path. The default (GIL) build keeps the plain load + incref generated inline by // mypyc. +// +// Note that observing a non-NULL 'v' does not guarantee a non-NULL result: 'v' is +// only a provisional read, and if the attribute is deleted before a reference to it +// can be secured, this returns NULL and the caller raises AttributeError. That is a +// legal outcome for a racing read, since it is ordered after the delete. CPy_COLD PyObject *CPy_GetAttrRefSlow(PyObject *v, PyObject *owner, PyObject **field); static inline PyObject *CPy_GetAttrRef(PyObject *owner, PyObject **field) { @@ -80,9 +108,9 @@ static inline PyObject *CPy_GetAttrRef(PyObject *owner, PyObject **field) { // holds a strong reference for the object's whole lifetime, and any thread reading // it necessarily holds 'self', which keeps the value alive. So the try-incref + // _Py_XNewRefWithLock fallback are unnecessary here -- a plain load + Py_INCREF is -// safe. A cross-thread Py_INCREF is an unconditional -// atomic add on ob_ref_shared, so (unlike CPy_GetAttrRef's try-incref) it needs no -// maybe-weakref and has no slow path. The load is relaxed rather than acquire: the +// safe. A cross-thread Py_INCREF is an unconditional atomic add on ob_ref_shared +// (CPython 3.14), so (unlike CPy_GetAttrRef's try-incref) it needs no maybe-weakref +// and has no slow path. The load is relaxed rather than acquire: the // reader reached 'self' through a synchronization edge (self's own publication) // that already ordered the construction stores before it, exactly as with // CPy_InitAttrRef's relaxed store. Relaxed keeps it TSan-clean at zero cost (plain @@ -103,9 +131,24 @@ static inline PyObject *CPy_GetAttrRefFinal(PyObject **field) { // CPy_GetAttrRef's fallback path. A lock-free reader either secures a local or // immortal reference directly, or secures a shared reference and validates the // field. Otherwise it reloads the field under this lock. Thus once the replacement -// is published, the old field reference can be decrefed normally; no QSBR-delayed -// decref is needed. Decref outside the critical section so an arbitrary destructor -// does not run while the owner is locked. +// is published, the old field reference can be decrefed immediately, even if a +// reader is still dereferencing the old value (see CPy_GetAttrRef for why touching +// a freed value there is safe). Decref outside the critical section so an +// arbitrary destructor does not run while the owner is locked -- the equivalent in +// CPython 3.14 (store_instance_attr_lock_held) decrefs with the lock still held. +// +// Caveat (based on CPython 3.14 internals): this takes 'owner->ob_mutex', which for +// a native class with a built-in base is the same mutex CPython uses for that +// container's own critical sections, and CPython runs arbitrary Python code under it +// (e.g. _PyDict_SetItem_LockHeld calls __hash__/__eq__ while holding CS(dict)). +// Re-entering a critical section on the same object is only free when the held +// section is the top-most one; with an unrelated section still active in between, +// the thread parks, detaches, has this mutex released by +// _PyCriticalSection_SuspendAll, then re-locked by _PyCriticalSection_Resume, and +// parks again. Reaching that needs a callback under +// two nested critical sections to assign an attribute of the outer object; the +// helpers here can't cause it on their own, since they only ever hold this one +// mutex (so there is also no lock-ordering deadlock). static inline void CPy_SetAttrRef(PyObject *owner, PyObject **field, PyObject *value) { PyObject *old; Py_BEGIN_CRITICAL_SECTION(owner);