From a307780823cc0a5e8e755a33da7cf36360485798 Mon Sep 17 00:00:00 2001 From: Piotr Sawicki Date: Mon, 19 Oct 2026 12:05:45 +0200 Subject: [PATCH 1/3] [mypyc] Access tuple attributes under critical section --- mypyc/codegen/emit.py | 81 ++++++++++- mypyc/codegen/emitclass.py | 46 ++++-- mypyc/codegen/emitfunc.py | 48 ++++++- mypyc/test-data/run-classes.test | 50 +++++++ mypyc/test-data/run-threading.test | 221 +++++++++++++++++++++++++++++ mypyc/test/test_emit.py | 43 ++++++ mypyc/test/test_emitclass.py | 90 +++++++++++- mypyc/test/test_emitfunc.py | 180 +++++++++++++++++++++-- 8 files changed, 729 insertions(+), 30 deletions(-) diff --git a/mypyc/codegen/emit.py b/mypyc/codegen/emit.py index c747aa6628f6..70b2c5b53a18 100644 --- a/mypyc/codegen/emit.py +++ b/mypyc/codegen/emit.py @@ -471,7 +471,14 @@ def _emit_attr_bitmap_update( index = cl.bitmap_attrs.index(attr) mask = 1 << (index & (BITMAP_BITS - 1)) bitmap = self.attr_bitmap_expr(obj, cl, index) - if clear: + if self.target_python.free_threaded and not cl.attrs_are_thread_confined(): + # Scalar and tuple attributes share bitmap words. All concurrent + # loads and RMWs must be atomic. + if clear: + self.emit_line(f"_Py_atomic_and_uint32(&{bitmap}, ~{mask}U);") + else: + self.emit_line(f"_Py_atomic_or_uint32(&{bitmap}, {mask});") + elif clear: self.emit_line(f"{bitmap} &= ~{mask};") else: self.emit_line(f"{bitmap} |= {mask};") @@ -497,9 +504,79 @@ def emit_undefined_attr_check( bit = 1 << (index & (BITMAP_BITS - 1)) attr = self.bitmap_field(index) obj_expr = f"({cl.struct_name(self.names)} *){obj}" - check = f"{check} && !(({obj_expr})->{attr} & {bit})" + bitmap = f"({obj_expr})->{attr}" + if self.target_python.free_threaded and not cl.attrs_are_thread_confined(): + bitmap = f"_Py_atomic_load_uint32(&{bitmap})" + check = f"{check} && !({bitmap} & {bit})" self.emit_line(f"if ({check}) {{") + def emit_tuple_attr_load( + self, + dest: str, + attr_expr: str, + obj: str, + rtype: RTuple, + cl: ClassIR, + attr: str, + *, + borrow: bool = False, + ) -> str: + """Snapshot a tuple attribute and its defined state while holding the owner lock. + + Secure references before unlocking: tuple fields can otherwise tear or be + reclaimed by a concurrent setter. The bitmap must be read under the same lock. + """ + undefined = self.temp_name() + self.emit_line(f"char {undefined} = 0;") + self.emit_line(f"Py_BEGIN_CRITICAL_SECTION({obj});") + self.emit_line(f"{dest} = {attr_expr};") + if not cl.is_always_defined(attr) or rtype.is_refcounted: + self.emit_undefined_attr_check(rtype, dest, "==", obj, attr, cl) + self.emit_line(f"{undefined} = 1;") + self.emit_line("}") + if rtype.is_refcounted and not borrow: + self.emit_line(f"if (!{undefined}) {{") + self.emit_inc_ref(dest, rtype) + self.emit_line("}") + self.emit_line("Py_END_CRITICAL_SECTION();") + return undefined + + def emit_tuple_attr_store( + self, + src: str, + attr_expr: str, + obj: str, + rtype: RTuple, + cl: ClassIR, + attr: str, + *, + delete: bool = False, + is_init: bool = False, + ) -> None: + """Publish a tuple under the owner lock, releasing old references outside it.""" + release_old = rtype.is_refcounted and not is_init + old = self.temp_name() + if release_old: + self.emit_line(f"{self.ctype(rtype)} {old};") + self.emit_line(f"Py_BEGIN_CRITICAL_SECTION({obj});") + if release_old: + self.emit_line(f"{old} = {attr_expr};") + if delete: + self.set_undefined_value(attr_expr, rtype) + else: + self.emit_line(f"{attr_expr} = {src};") + if rtype.error_overlap and not cl.is_always_defined(attr): + if delete: + self.emit_attr_bitmap_clear(obj, rtype, cl, attr) + else: + self.emit_attr_bitmap_set(src, obj, rtype, cl, attr) + self.emit_line("Py_END_CRITICAL_SECTION();") + if release_old: + # Refcounted tuple types have a disjoint undefined value (no bitmap). + self.emit_line(f"if ({self.error_value_check(rtype, old, '!=')}) {{") + self.emit_dec_ref(old, rtype) + self.emit_line("}") + def error_value_check(self, rtype: RType, value: str, compare: str) -> str: if isinstance(rtype, RTuple): return self.tuple_undefined_check_cond( diff --git a/mypyc/codegen/emitclass.py b/mypyc/codegen/emitclass.py index 467e2d4b947c..f2e4c5cdb601 100644 --- a/mypyc/codegen/emitclass.py +++ b/mypyc/codegen/emitclass.py @@ -1422,14 +1422,30 @@ def generate_getter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N # values is benign. always_defined = cl.is_always_defined(attr) and not rtype.is_refcounted + tuple_snapshot = ( + emitter.target_python.free_threaded + and isinstance(rtype, RTuple) + and attr not in cl.final_attributes + ) + if tuple_snapshot: + assert isinstance(rtype, RTuple) + emitter.emit_line(f"{emitter.ctype(rtype)} tmp;") + undefined = emitter.emit_tuple_attr_load("tmp", attr_expr, "self", rtype, cl, attr) + attr_expr = "tmp" if not always_defined: - emitter.emit_undefined_attr_check(rtype, attr_expr, "==", "self", attr, cl, unlikely=True) + if tuple_snapshot: + emitter.emit_line(f"if (unlikely({undefined})) {{") + else: + emitter.emit_undefined_attr_check( + rtype, attr_expr, "==", "self", attr, cl, unlikely=True + ) emitter.emit_line("PyErr_SetString(PyExc_AttributeError,") emitter.emit_line(f' "attribute {repr(attr)} of {repr(cl.name)} undefined");') emitter.emit_line("return NULL;") emitter.emit_line("}") - emitter.emit_inc_ref(f"self->{attr_field}", rtype) - emitter.emit_box(f"self->{attr_field}", "retval", rtype, declare_dest=True) + if not tuple_snapshot: + emitter.emit_inc_ref(attr_expr, rtype) + emitter.emit_box(attr_expr, "retval", rtype, declare_dest=True) emitter.emit_line("return retval;") emitter.emit_line("}") @@ -1540,17 +1556,25 @@ def emit_decref_old_value() -> None: # Take ownership of the incoming value before releasing the old one. In # particular, a failed cast must leave the attribute and its reference intact. emitter.emit_inc_ref("tmp", rtype) - emit_decref_old_value() - emitter.emit_line(f"self->{attr_field} = tmp;") - if rtype.error_overlap and not always_defined: - emitter.emit_attr_bitmap_set("tmp", "self", rtype, cl, attr) + if emitter.target_python.free_threaded and isinstance(rtype, RTuple): + emitter.emit_tuple_attr_store("tmp", f"self->{attr_field}", "self", rtype, cl, attr) + else: + emit_decref_old_value() + emitter.emit_line(f"self->{attr_field} = tmp;") + if rtype.error_overlap and not always_defined: + emitter.emit_attr_bitmap_set("tmp", "self", rtype, cl, attr) if deletable: emitter.emit_line("} else {") - emit_decref_old_value() - emitter.set_undefined_value(f"self->{attr_field}", rtype) - if rtype.error_overlap: - emitter.emit_attr_bitmap_clear("self", rtype, cl, attr) + if emitter.target_python.free_threaded and isinstance(rtype, RTuple): + emitter.emit_tuple_attr_store( + "", f"self->{attr_field}", "self", rtype, cl, attr, delete=True + ) + else: + emit_decref_old_value() + emitter.set_undefined_value(f"self->{attr_field}", rtype) + if rtype.error_overlap: + emitter.emit_attr_bitmap_clear("self", rtype, cl, attr) emitter.emit_line("}") emitter.emit_line("return 0;") emitter.emit_line("}") diff --git a/mypyc/codegen/emitfunc.py b/mypyc/codegen/emitfunc.py index 354059508d3c..7e6968b89d5a 100644 --- a/mypyc/codegen/emitfunc.py +++ b/mypyc/codegen/emitfunc.py @@ -526,15 +526,30 @@ def visit_get_attr(self, op: GetAttr) -> None: else: # Otherwise, use direct or offset struct access. attr_expr = self.get_attr_expr(obj, op, decl_cl) - use_get_attr_ref = self.emit_load_attr_take_ref( - dest, obj, op, cl, attr_rtype, attr_expr - ) + tuple_undefined = None + if ( + self.emitter.target_python.free_threaded + and isinstance(attr_rtype, RTuple) + and not cl.is_final_attr(op.attr) + and not cl.attrs_are_thread_confined() + ): + tuple_undefined = self.emitter.emit_tuple_attr_load( + dest, attr_expr, obj, attr_rtype, cl, op.attr, borrow=op.is_borrowed + ) + use_get_attr_ref = True + else: + use_get_attr_ref = self.emit_load_attr_take_ref( + dest, obj, op, cl, attr_rtype, attr_expr + ) always_defined = cl.is_always_defined(op.attr) merged_branch = None if not always_defined: - self.emitter.emit_undefined_attr_check( - attr_rtype, dest, "==", obj, op.attr, cl, unlikely=True - ) + if tuple_undefined is not None: + self.emitter.emit_line(f"if (unlikely({tuple_undefined})) {{") + else: + self.emitter.emit_undefined_attr_check( + attr_rtype, dest, "==", obj, op.attr, cl, unlikely=True + ) branch = self.next_branch() if branch is not None: if ( @@ -580,6 +595,16 @@ def get_attr_with_allow_error_value(self, op: GetAttr) -> None: attr_rtype, decl_cl = cl.attr_details(op.attr) attr_expr = self.get_attr_expr(obj, op, decl_cl) + if ( + self.emitter.target_python.free_threaded + and isinstance(attr_rtype, RTuple) + and not cl.is_final_attr(op.attr) + and not cl.attrs_are_thread_confined() + ): + self.emitter.emit_tuple_attr_load( + dest, attr_expr, obj, attr_rtype, cl, op.attr, borrow=op.is_borrowed + ) + return # On free-threaded builds this takes a new reference atomically (see # emit_load_attr_take_ref). CPy_GetAttrRef returns NULL and CPyTagged_GetAttr # returns CPY_INT_TAG when the field is undefined, which is precisely the error @@ -671,6 +696,17 @@ def visit_set_attr(self, op: SetAttr) -> None: ) if op.error_kind == ERR_FALSE: self.emitter.emit_line(f"{dest} = 1;") + elif ( + self.emitter.target_python.free_threaded + and isinstance(attr_rtype, RTuple) + and not cl.attrs_are_thread_confined() + ): + attr_expr = self.get_attr_expr(obj, op, decl_cl) + self.emitter.emit_tuple_attr_store( + src, attr_expr, obj, attr_rtype, cl, op.attr, is_init=op.is_init + ) + if op.error_kind == ERR_FALSE: + self.emitter.emit_line(f"{dest} = 1;") else: # ...and struct access for normal attributes. attr_expr = self.get_attr_expr(obj, op, decl_cl) diff --git a/mypyc/test-data/run-classes.test b/mypyc/test-data/run-classes.test index bf127e5d37e1..a67062dd78ea 100644 --- a/mypyc/test-data/run-classes.test +++ b/mypyc/test-data/run-classes.test @@ -9514,3 +9514,53 @@ def test_trait() -> None: set_k(w, BIG + 1) assert get_k(w) == BIG + 1 assert w.k == BIG + 1 + + +[case testFinalTupleAttributeOwnedBorrowedInherited] +from typing import Final + +class Base: + def __init__(self, item: object, number: int) -> None: + self.refs: Final[tuple[object, tuple[object, int]]] = (item, (item, number)) + self.numbers: Final[tuple[float, float]] = (1.0, 2.0) + +class Child(Base): + pass + +def owned_base(box: Base) -> tuple[object, tuple[object, int]]: + return box.refs + +def owned_child(box: Child) -> tuple[object, tuple[object, int]]: + return box.refs + +def borrowed_base(box: Base) -> object: + return box.refs[1][0] + +def borrowed_child(box: Child) -> object: + return box.refs[1][0] + +def read_numbers(box: Child, count: int) -> float: + result = 0.0 + for _ in range(count): + result += box.numbers[0] + box.numbers[1] + return result + +[file driver.py] +import sys +from native import Base, Child, owned_base, owned_child, borrowed_base, borrowed_child, read_numbers + +item = object() +number = 1 << 100 +for box in [Base(item, number), Child(item, number)]: + before = sys.getrefcount(item) + for _ in range(100): + assert owned_base(box) == (item, (item, number)) + assert borrowed_base(box) is item + # Interpreted access uses the defining-class Final getset descriptor. + assert box.refs == (item, (item, number)) + assert sys.getrefcount(item) == before +child = Child(item, number) +for _ in range(100): + assert owned_child(child) == (item, (item, number)) + assert borrowed_child(child) is item +assert read_numbers(child, 100) == 300.0 diff --git a/mypyc/test-data/run-threading.test b/mypyc/test-data/run-threading.test index 896c36e4cb93..7ae693ca40b7 100644 --- a/mypyc/test-data/run-threading.test +++ b/mypyc/test-data/run-threading.test @@ -392,3 +392,224 @@ def test_release_unlocked() -> None: lock.release() with assertRaises(RuntimeError): lock.release() + +[case testConcurrentNativeTupleAttributeGetSetDelete] +from threading import Barrier, Thread +from typing import Any +from mypy_extensions import trait + +class Item: + def __init__(self, value: int) -> None: + self.value = value + +@trait +class RefView: + refs: tuple[Item, tuple[Item, int]] + +class Box(RefView): + __deletable__ = ["refs", "numbers"] + refs: tuple[Item, tuple[Item, int]] + numbers: tuple[float, float] + + def __init__(self) -> None: + self.refs = (Item(0), (Item(0), 0)) + self.numbers = (0.0, 0.0) + +def native_writer(box: Box, count: int) -> None: + for i in range(count): + box.refs = (Item(i), (Item(i), i)) + box.numbers = (float(i), float(i)) + +def dynamic_writer(box: Any, count: int) -> None: + for i in range(count): + box.refs = (Item(i), (Item(i), i)) + box.numbers = (float(i), float(i)) + +def native_reader(box: Box, count: int) -> None: + for _ in range(count): + try: + refs = box.refs + except AttributeError: + pass + else: + assert refs[0].value == refs[1][0].value == refs[1][1] + try: + numbers = box.numbers + except AttributeError: + pass + else: + assert numbers[0] == numbers[1] + +def trait_reader(box: RefView, count: int) -> None: + for _ in range(count): + try: + refs = box.refs + except AttributeError: + pass + else: + assert refs[0].value == refs[1][0].value == refs[1][1] + +def dynamic_reader(box: Any, count: int) -> None: + for _ in range(count): + try: + refs = box.refs + except AttributeError: + pass + else: + assert refs[0].value == refs[1][0].value == refs[1][1] + try: + numbers = box.numbers + except AttributeError: + pass + else: + assert numbers[0] == numbers[1] + +def native_deleter(box: Box, count: int) -> None: + for _ in range(count): + try: + del box.refs + except AttributeError: + pass + try: + del box.numbers + except AttributeError: + pass + +def dynamic_deleter(box: Any, count: int) -> None: + for _ in range(count): + try: + del box.refs + except AttributeError: + pass + try: + del box.numbers + except AttributeError: + pass + +def test_concurrent_tuple_attribute_get_set_delete() -> None: + # Native and descriptor access must share the owner lock. New Items on each + # store also exercise acquiring references before the old tuple is reclaimed. + # Collect worker exceptions so assertions in a thread fail the test runner. + box = Box() + # This valid tuple overlaps the undefined sentinel and needs the bitmap. + box.numbers = (-113.0, -113.0) + assert box.numbers == (-113.0, -113.0) + dynamic: Any = box + assert dynamic.numbers == (-113.0, -113.0) + del dynamic.numbers + dynamic.numbers = (-113.0, -113.0) + assert box.numbers == (-113.0, -113.0) + workers = [native_writer, dynamic_writer, native_reader, dynamic_reader, + native_deleter, dynamic_deleter, trait_reader] + barrier = Barrier(len(workers)) + errors: list[BaseException] = [] + def run(worker: Any) -> None: + try: + barrier.wait() + worker(box, 10_000) + except BaseException as error: + errors.append(error) + threads = [Thread(target=run, args=(worker,)) for worker in workers] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + assert not errors, errors + +[case testConcurrentTupleScalarAttributeBitmap] +from threading import Barrier, Thread +from typing import Any +from testutil import assertRaises + +class Box: + __deletable__ = ["pair", "flag"] + pair: tuple[float, float] + flag: float + + def __init__(self) -> None: + self.pair = (-113.0, -113.0) + self.flag = -113.0 + +def pair_worker(box: Box, count: int) -> None: + dynamic: Any = box + for _ in range(count): + del dynamic.pair + # Only this worker touches pair. Scalar bitmap updates must neither + # restore a deleted pair's bit nor erase a successfully assigned bit. + with assertRaises(AttributeError): + pair = box.pair + box.pair = (-113.0, -113.0) + assert box.pair == (-113.0, -113.0) + del box.pair + dynamic.pair = (-113.0, -113.0) + assert dynamic.pair == (-113.0, -113.0) + +def scalar_worker(box: Box, count: int) -> None: + dynamic: Any = box + for _ in range(count): + del box.flag + box.flag = -113.0 + del dynamic.flag + dynamic.flag = -113.0 + +def test_concurrent_tuple_scalar_attribute_bitmap() -> None: + box = Box() + barrier = Barrier(2) + errors: list[BaseException] = [] + def run(worker: Any) -> None: + try: + barrier.wait() + worker(box, 100_000) + except BaseException as error: + errors.append(error) + threads = [Thread(target=run, args=(worker,)) for worker in [pair_worker, scalar_worker]] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + assert not errors, errors + +[case testConcurrentTupleAttributeExplicitReinitialize] +from typing import Any + +class Box: + def __init__(self, value: float) -> None: + # A wide, nonrefcounted tuple makes tearing easy to observe without + # depending on the ownership semantics of repeated refcounted __init__. + self.values = (value, value, value, value, value, value, value, value, value, value, \ + value, value, value, value, value, value, value, value, value, value, value, value, \ + value, value, value, value, value, value, value, value, value, value) + +def writer(box: Box, count: int) -> None: + for i in range(count): + Box.__init__(box, float(i)) + +def reader(box: Box, count: int) -> None: + for _ in range(count): + values = box.values + assert all(value == values[0] for value in values), values + +def dynamic_reader(box: Any, count: int) -> None: + for _ in range(count): + values = box.values + assert all(value == values[0] for value in values), values + +[file driver.py] +from threading import Barrier, Thread +from native import Box, writer, reader, dynamic_reader + +box = Box(0.0) +barrier = Barrier(3) +errors = [] +def run(worker): + try: + barrier.wait() + worker(box, 100_000) + except BaseException as error: + errors.append(error) +threads = [Thread(target=run, args=(worker,)) for worker in [writer, reader, dynamic_reader]] +for thread in threads: + thread.start() +for thread in threads: + thread.join() +assert not errors, errors diff --git a/mypyc/test/test_emit.py b/mypyc/test/test_emit.py index 987724cf975b..6c28fc3eb112 100644 --- a/mypyc/test/test_emit.py +++ b/mypyc/test/test_emit.py @@ -10,6 +10,7 @@ RTuple, RUnion, bool_rprimitive, + int64_rprimitive, int_rprimitive, list_rprimitive, none_rprimitive, @@ -35,6 +36,48 @@ def setUp(self) -> None: def test_label(self) -> None: assert self.emitter.label(BasicBlock(4)) == "CPyL4" + def test_private_frame_bitmap_access(self) -> None: + cl = self.instance_a.class_ir + cl.attributes = {"value": int64_rprimitive} + cl.bitmap_attrs = ["value"] + cl.is_generated = True + cl.has_private_generator_frame = True + cl.has_running_flag = True + cl.needs_getseters = False + emitter = Emitter( + EmitterContext( + NameGenerator([["mod"]]), True, target_python=TargetPython((3, 14), True) + ) + ) + # Exposing the frame or removing its execution guard requires atomics again. + for changed in ( + None, + "needs_getseters", + "has_running_flag", + "has_private_generator_frame", + ): + with self.subTest(changed=changed): + if changed is not None: + previous = getattr(cl, changed) + setattr(cl, changed, not previous) + emitter.fragments.clear() + emitter.emit_attr_bitmap_set("value", "obj", int64_rprimitive, cl, "value") + emitter.emit_attr_bitmap_clear("obj", int64_rprimitive, cl, "value") + emitter.emit_undefined_attr_check( + int64_rprimitive, "value", "==", "obj", "value", cl + ) + generated = "".join(emitter.fragments) + if changed is None: + assert "((mod___AObject *)obj)->bitmap |= 1;" in generated + assert "((mod___AObject *)obj)->bitmap &= ~1;" in generated + assert "!(((mod___AObject *)obj)->bitmap & 1)" in generated + assert "_Py_atomic_" not in generated + else: + assert "_Py_atomic_or_uint32" in generated + assert "_Py_atomic_and_uint32" in generated + assert "_Py_atomic_load_uint32" in generated + setattr(cl, changed, previous) + def test_reg(self) -> None: names: dict[Value, str] = {self.n: "n"} emitter = Emitter(self.context, names) diff --git a/mypyc/test/test_emitclass.py b/mypyc/test/test_emitclass.py index 42f5ce2bb7bf..2467667ecfeb 100644 --- a/mypyc/test/test_emitclass.py +++ b/mypyc/test/test_emitclass.py @@ -12,7 +12,13 @@ slot_key, ) from mypyc.ir.class_ir import ClassIR -from mypyc.ir.rtypes import int32_rprimitive, int_rprimitive, object_rprimitive +from mypyc.ir.rtypes import ( + RTuple, + float_rprimitive, + int32_rprimitive, + int_rprimitive, + object_rprimitive, +) from mypyc.namegen import NameGenerator from mypyc.options import TargetPython @@ -84,6 +90,88 @@ def test_free_threaded_int_attribute_getter_and_setter(self) -> None: assert "CPyTagged_SetAttr((PyObject *)self, &self->_n, CPY_INT_TAG);" in generated assert "CPyTagged_DECREF" not in generated + def test_tuple_attribute_getter_and_setter_lock(self) -> None: + cl = ClassIR("A", "mod") + rtype = RTuple([object_rprimitive, object_rprimitive]) + cl.attributes = {"t": rtype} + cl.deletable = ["t"] + cl.mro = cl.base_mro = [cl] + emitter = Emitter( + EmitterContext( + NameGenerator([["mod"]]), True, target_python=TargetPython((3, 14), True) + ) + ) + generate_getter(cl, "t", rtype, emitter) + getter = "".join(emitter.fragments) + assert getter.index("Py_BEGIN_CRITICAL_SECTION(self)") < getter.index("tmp = self->_t;") + assert getter.index("CPy_INCREF(tmp.f1)") < getter.index("Py_END_CRITICAL_SECTION()") + assert getter.index("Py_END_CRITICAL_SECTION()") < getter.index("PyErr_SetString") + emitter.fragments.clear() + generate_setter(cl, "t", rtype, emitter) + setter = "".join(emitter.fragments) + assert setter.count("Py_BEGIN_CRITICAL_SECTION(self)") == 2 + assert setter.count("Py_END_CRITICAL_SECTION()") == 2 + assert setter.index("self->_t = tmp;") < setter.index("Py_END_CRITICAL_SECTION()") + assert setter.index("Py_END_CRITICAL_SECTION()") < setter.index("CPy_DECREF(") + + def test_tuple_attribute_bitmap_is_locked(self) -> None: + cl = ClassIR("A", "mod") + rtype = RTuple([float_rprimitive, float_rprimitive]) + cl.attributes = {"t": rtype} + cl.deletable = ["t"] + cl.bitmap_attrs = ["t"] + cl.mro = cl.base_mro = [cl] + emitter = Emitter( + EmitterContext( + NameGenerator([["mod"]]), True, target_python=TargetPython((3, 14), True) + ) + ) + generate_getter(cl, "t", rtype, emitter) + getter = "".join(emitter.fragments) + assert getter.index("Py_BEGIN_CRITICAL_SECTION(self)") < getter.index("->bitmap") + assert getter.index("->bitmap") < getter.index("Py_END_CRITICAL_SECTION()") + assert "_Py_atomic_load_uint32" in getter + emitter.fragments.clear() + generate_setter(cl, "t", rtype, emitter) + setter = "".join(emitter.fragments) + assert "_Py_atomic_or_uint32" in setter + assert "_Py_atomic_and_uint32" in setter + for section in setter.split("Py_BEGIN_CRITICAL_SECTION(self);")[1:]: + assert section.index("->bitmap") < section.index("Py_END_CRITICAL_SECTION()") + + def test_scalar_attribute_bitmap_access_is_atomic(self) -> None: + cl = ClassIR("A", "mod") + cl.attributes = {"flag": float_rprimitive} + cl.deletable = ["flag"] + cl.bitmap_attrs = ["flag"] + cl.mro = cl.base_mro = [cl] + emitter = Emitter( + EmitterContext( + NameGenerator([["mod"]]), True, target_python=TargetPython((3, 14), True) + ) + ) + generate_getter(cl, "flag", float_rprimitive, emitter) + generate_setter(cl, "flag", float_rprimitive, emitter) + generated = "".join(emitter.fragments) + assert "_Py_atomic_load_uint32(&((mod___AObject *)self)->bitmap)" in generated + assert "_Py_atomic_or_uint32(&((mod___AObject *)self)->bitmap, 1)" in generated + assert "_Py_atomic_and_uint32(&((mod___AObject *)self)->bitmap, ~1U)" in generated + assert "->bitmap |=" not in generated + assert "->bitmap &=" not in generated + + def test_final_tuple_attribute_getter_uses_plain_load(self) -> None: + cl = ClassIR("A", "mod") + rtype = RTuple([object_rprimitive, object_rprimitive]) + cl.attributes = {"t": rtype} + cl.final_attributes = {"t"} + cl.mro = cl.base_mro = [cl] + emitter = Emitter(EmitterContext(NameGenerator([["mod"]]), True)) + generate_getter(cl, "t", rtype, emitter) + getter = "".join(emitter.fragments) + assert "CRITICAL_SECTION" not in getter + assert "CPy_INCREF(self->_t.f0)" in getter + assert "CPy_INCREF(self->_t.f1)" in getter + 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 550531976a07..f42bf5653e6d 100644 --- a/mypyc/test/test_emitfunc.py +++ b/mypyc/test/test_emitfunc.py @@ -615,7 +615,8 @@ def test_get_attr_merged(self) -> None: skip_next=True, ) - def test_get_attr_with_bitmap(self) -> None: + def test_get_attr_with_bitmap_gil(self) -> None: + self.use_gil_target() self.assert_emit( GetAttr(self.r, "i1", 1), """cpy_r_r0 = ((mod___AObject *)cpy_r_r)->_i1; @@ -625,16 +626,136 @@ def test_get_attr_with_bitmap(self) -> None: """, ) - def test_get_attr_nullable_with_tuple(self) -> None: + def test_get_attr_with_bitmap_free_threaded(self) -> None: + self.use_free_threaded_target() + bitmap = "_Py_atomic_load_uint32(&((mod___AObject *)cpy_r_r)->bitmap)" self.assert_emit( - GetAttr(self.r, "t", 1, allow_error_value=True), - """cpy_r_r0 = ((mod___AObject *)cpy_r_r)->_t; - if (cpy_r_r0.f0 != NULL) { - CPy_INCREF(cpy_r_r0.f0); - CPy_INCREF(cpy_r_r0.f1); - } + GetAttr(self.r, "i1", 1), + f"""cpy_r_r0 = ((mod___AObject *)cpy_r_r)->_i1; + if (unlikely(cpy_r_r0 == -113) && !({bitmap} & 1)) {{ + PyErr_SetString(PyExc_AttributeError, "attribute 'i1' of 'A' undefined"); + }} + """, + ) + + def test_get_attr_nullable_with_tuple_free_threaded(self) -> None: + self.use_free_threaded_target() + expected = """char __tmp1 = 0; + Py_BEGIN_CRITICAL_SECTION(cpy_r_r); + cpy_r_r0 = ((mod___AObject *)cpy_r_r)->_t; + if (cpy_r_r0.f0 == NULL) { + __tmp1 = 1; + } + if (!__tmp1) { + CPy_INCREF(cpy_r_r0.f0); + CPy_INCREF(cpy_r_r0.f1); + } + Py_END_CRITICAL_SECTION(); + """ + self.assert_emit(GetAttr(self.r, "t", 1, allow_error_value=True), expected) + + def test_get_tuple_attr_free_threaded(self) -> None: + self.use_free_threaded_target() + self.assert_emit( + GetAttr(self.r, "t", 1), + """char __tmp1 = 0; + Py_BEGIN_CRITICAL_SECTION(cpy_r_r); + cpy_r_r0 = ((mod___AObject *)cpy_r_r)->_t; + if (cpy_r_r0.f0 == NULL) { + __tmp1 = 1; + } + if (!__tmp1) { + CPy_INCREF(cpy_r_r0.f0); + CPy_INCREF(cpy_r_r0.f1); + } + Py_END_CRITICAL_SECTION(); + if (unlikely(__tmp1)) { + PyErr_SetString(PyExc_AttributeError, "attribute 't' of 'A' undefined"); + } + """, + ) + + def test_set_tuple_attr_free_threaded(self) -> None: + self.use_free_threaded_target() + assert isinstance(self.r.type, RInstance) + src = Register(self.r.type.class_ir.attributes["t"], "tuple") + self.registers.append(src) + self.assert_emit( + SetAttr(self.r, "t", src, 1), + """tuple_T2OO __tmp1; + Py_BEGIN_CRITICAL_SECTION(cpy_r_r); + __tmp1 = ((mod___AObject *)cpy_r_r)->_t; + ((mod___AObject *)cpy_r_r)->_t = cpy_r_tuple; + Py_END_CRITICAL_SECTION(); + if (__tmp1.f0 != NULL) { + CPy_DECREF(__tmp1.f0); + CPy_DECREF(__tmp1.f1); + } + cpy_r_r0 = 1; + """, + ) + + def test_tuple_attr_initializer_store_free_threaded(self) -> None: + self.use_free_threaded_target() + assert isinstance(self.r.type, RInstance) + src = Register(self.r.type.class_ir.attributes["t"], "tuple") + self.registers.append(src) + op = SetAttr(self.r, "t", src, 1) + op.is_init = True + # Lock even an initializer store, but preserve its ownership transfer: + # no incref of the source and no decref of a previous field value. + self.assert_emit( + op, + """Py_BEGIN_CRITICAL_SECTION(cpy_r_r); + ((mod___AObject *)cpy_r_r)->_t = cpy_r_tuple; + Py_END_CRITICAL_SECTION(); + cpy_r_r0 = 1; """, ) + cl = self.r.type.class_ir + cl.has_private_generator_frame = True + cl.has_running_flag = True + cl.needs_getseters = False + cl.is_generated = True + # Private generator frames remain protected by their running flag. + self.assert_emit( + op, + """((mod___AObject *)cpy_r_r)->_t = cpy_r_tuple; + cpy_r_r0 = 1; + """, + ) + + def test_final_tuple_attr_reads(self) -> None: + assert isinstance(self.r.type, RInstance) + base = self.r.type.class_ir + base.final_attributes.add("t") + for inherited in (False, True): + if inherited: + cl = ClassIR("Sub", "mod") + cl.base = base + cl.mro = [cl, base] + self.r.type = RInstance(cl) + struct = "mod___SubObject" + else: + struct = "mod___AObject" + for borrow in (False, True): + for nullable in (False, True): + expected = f"cpy_r_r0 = (({struct} *)cpy_r_r)->_t;\n" + if not nullable: + expected += """if (unlikely(cpy_r_r0.f0 == NULL)) { + PyErr_SetString(PyExc_AttributeError, "attribute 't' of 'A' undefined"); + """.replace("of 'A'", "of 'Sub'" if inherited else "of 'A'") + expected += "} else {\n" if not borrow else "}\n" + elif not borrow: + expected += "if (cpy_r_r0.f0 != NULL) {\n" + if not borrow: + expected += """CPy_INCREF(cpy_r_r0.f0); + CPy_INCREF(cpy_r_r0.f1); + }""" + self.assert_emit( + GetAttr(self.r, "t", 1, borrow=borrow, allow_error_value=nullable), + expected, + ) def test_set_attr(self) -> None: self.use_gil_target() @@ -704,7 +825,8 @@ def test_set_attr_non_refcounted_no_error(self) -> None: """, ) - def test_set_attr_with_bitmap(self) -> None: + def test_set_attr_with_bitmap_gil(self) -> None: + self.use_gil_target() # For some rtypes the error value overlaps a valid value, so we need # to use a separate bitmap to track defined attributes. self.assert_emit( @@ -726,7 +848,31 @@ def test_set_attr_with_bitmap(self) -> None: """, ) - def test_set_attr_init_with_bitmap(self) -> None: + def test_set_attr_with_bitmap_free_threaded(self) -> None: + self.use_free_threaded_target() + # For some rtypes the error value overlaps a valid value, so we need + # to use a separate bitmap to track defined attributes. + self.assert_emit( + SetAttr(self.r, "i1", self.i64, 1), + """if (unlikely(cpy_r_i64 == -113)) { + _Py_atomic_or_uint32(&((mod___AObject *)cpy_r_r)->bitmap, 1); + } + ((mod___AObject *)cpy_r_r)->_i1 = cpy_r_i64; + cpy_r_r0 = 1; + """, + ) + self.assert_emit( + SetAttr(self.r, "i2", self.i32, 1), + """if (unlikely(cpy_r_i32 == -113)) { + _Py_atomic_or_uint32(&((mod___AObject *)cpy_r_r)->bitmap, 2); + } + ((mod___AObject *)cpy_r_r)->_i2 = cpy_r_i32; + cpy_r_r0 = 1; + """, + ) + + def test_set_attr_init_with_bitmap_gil(self) -> None: + self.use_gil_target() op = SetAttr(self.r, "i1", self.i64, 1) op.is_init = True self.assert_emit( @@ -739,6 +885,20 @@ def test_set_attr_init_with_bitmap(self) -> None: """, ) + def test_set_attr_init_with_bitmap_free_threaded(self) -> None: + self.use_free_threaded_target() + op = SetAttr(self.r, "i1", self.i64, 1) + op.is_init = True + self.assert_emit( + op, + """if (unlikely(cpy_r_i64 == -113)) { + _Py_atomic_or_uint32(&((mod___AObject *)cpy_r_r)->bitmap, 1); + } + ((mod___AObject *)cpy_r_r)->_i1 = cpy_r_i64; + cpy_r_r0 = 1; + """, + ) + def test_dict_get_item(self) -> None: self.assert_emit( CallC( From 6141ddd5fd1114a265a27f76390207bd9d9c68a7 Mon Sep 17 00:00:00 2001 From: Piotr Sawicki Date: Mon, 19 Oct 2026 15:36:47 +0200 Subject: [PATCH 2/3] Address comments --- mypyc/codegen/emit.py | 9 ++++--- mypyc/codegen/emitclass.py | 6 ++--- mypyc/codegen/emitfunc.py | 6 ++--- mypyc/test/test_emitfunc.py | 48 +++++++++++++------------------------ 4 files changed, 28 insertions(+), 41 deletions(-) diff --git a/mypyc/codegen/emit.py b/mypyc/codegen/emit.py index 70b2c5b53a18..354eade50434 100644 --- a/mypyc/codegen/emit.py +++ b/mypyc/codegen/emit.py @@ -510,7 +510,7 @@ def emit_undefined_attr_check( check = f"{check} && !({bitmap} & {bit})" self.emit_line(f"if ({check}) {{") - def emit_tuple_attr_load( + def emit_tuple_attr_load_locked( self, dest: str, attr_expr: str, @@ -541,7 +541,7 @@ def emit_tuple_attr_load( self.emit_line("Py_END_CRITICAL_SECTION();") return undefined - def emit_tuple_attr_store( + def emit_tuple_attr_store_locked( self, src: str, attr_expr: str, @@ -553,7 +553,10 @@ def emit_tuple_attr_store( delete: bool = False, is_init: bool = False, ) -> None: - """Publish a tuple under the owner lock, releasing old references outside it.""" + """Assign a tuple attribute attr to src under the owner lock. + + References to the old value of the attribute are released outside the lock. + """ release_old = rtype.is_refcounted and not is_init old = self.temp_name() if release_old: diff --git a/mypyc/codegen/emitclass.py b/mypyc/codegen/emitclass.py index f2e4c5cdb601..7d949bc35759 100644 --- a/mypyc/codegen/emitclass.py +++ b/mypyc/codegen/emitclass.py @@ -1430,7 +1430,7 @@ def generate_getter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N if tuple_snapshot: assert isinstance(rtype, RTuple) emitter.emit_line(f"{emitter.ctype(rtype)} tmp;") - undefined = emitter.emit_tuple_attr_load("tmp", attr_expr, "self", rtype, cl, attr) + undefined = emitter.emit_tuple_attr_load_locked("tmp", attr_expr, "self", rtype, cl, attr) attr_expr = "tmp" if not always_defined: if tuple_snapshot: @@ -1557,7 +1557,7 @@ def emit_decref_old_value() -> None: # particular, a failed cast must leave the attribute and its reference intact. emitter.emit_inc_ref("tmp", rtype) if emitter.target_python.free_threaded and isinstance(rtype, RTuple): - emitter.emit_tuple_attr_store("tmp", f"self->{attr_field}", "self", rtype, cl, attr) + emitter.emit_tuple_attr_store_locked("tmp", f"self->{attr_field}", "self", rtype, cl, attr) else: emit_decref_old_value() emitter.emit_line(f"self->{attr_field} = tmp;") @@ -1567,7 +1567,7 @@ def emit_decref_old_value() -> None: if deletable: emitter.emit_line("} else {") if emitter.target_python.free_threaded and isinstance(rtype, RTuple): - emitter.emit_tuple_attr_store( + emitter.emit_tuple_attr_store_locked( "", f"self->{attr_field}", "self", rtype, cl, attr, delete=True ) else: diff --git a/mypyc/codegen/emitfunc.py b/mypyc/codegen/emitfunc.py index 7e6968b89d5a..07412dfbf73c 100644 --- a/mypyc/codegen/emitfunc.py +++ b/mypyc/codegen/emitfunc.py @@ -533,7 +533,7 @@ def visit_get_attr(self, op: GetAttr) -> None: and not cl.is_final_attr(op.attr) and not cl.attrs_are_thread_confined() ): - tuple_undefined = self.emitter.emit_tuple_attr_load( + tuple_undefined = self.emitter.emit_tuple_attr_load_locked( dest, attr_expr, obj, attr_rtype, cl, op.attr, borrow=op.is_borrowed ) use_get_attr_ref = True @@ -601,7 +601,7 @@ def get_attr_with_allow_error_value(self, op: GetAttr) -> None: and not cl.is_final_attr(op.attr) and not cl.attrs_are_thread_confined() ): - self.emitter.emit_tuple_attr_load( + self.emitter.emit_tuple_attr_load_locked( dest, attr_expr, obj, attr_rtype, cl, op.attr, borrow=op.is_borrowed ) return @@ -702,7 +702,7 @@ def visit_set_attr(self, op: SetAttr) -> None: and not cl.attrs_are_thread_confined() ): attr_expr = self.get_attr_expr(obj, op, decl_cl) - self.emitter.emit_tuple_attr_store( + self.emitter.emit_tuple_attr_store_locked( src, attr_expr, obj, attr_rtype, cl, op.attr, is_init=op.is_init ) if op.error_kind == ERR_FALSE: diff --git a/mypyc/test/test_emitfunc.py b/mypyc/test/test_emitfunc.py index f42bf5653e6d..490a0ae2d205 100644 --- a/mypyc/test/test_emitfunc.py +++ b/mypyc/test/test_emitfunc.py @@ -675,6 +675,22 @@ def test_get_tuple_attr_free_threaded(self) -> None: """, ) + def test_get_tuple_attr_final_free_threaded(self) -> None: + self.use_free_threaded_target() + assert isinstance(self.r.type, RInstance) + self.r.type.class_ir.final_attributes.add("t") + self.assert_emit( + GetAttr(self.r, "t", 1), + """cpy_r_r0 = ((mod___AObject *)cpy_r_r)->_t; + if (unlikely(cpy_r_r0.f0 == NULL)) { + PyErr_SetString(PyExc_AttributeError, "attribute 't' of 'A' undefined"); + } else { + CPy_INCREF(cpy_r_r0.f0); + CPy_INCREF(cpy_r_r0.f1); + } + """, + ) + def test_set_tuple_attr_free_threaded(self) -> None: self.use_free_threaded_target() assert isinstance(self.r.type, RInstance) @@ -725,38 +741,6 @@ def test_tuple_attr_initializer_store_free_threaded(self) -> None: """, ) - def test_final_tuple_attr_reads(self) -> None: - assert isinstance(self.r.type, RInstance) - base = self.r.type.class_ir - base.final_attributes.add("t") - for inherited in (False, True): - if inherited: - cl = ClassIR("Sub", "mod") - cl.base = base - cl.mro = [cl, base] - self.r.type = RInstance(cl) - struct = "mod___SubObject" - else: - struct = "mod___AObject" - for borrow in (False, True): - for nullable in (False, True): - expected = f"cpy_r_r0 = (({struct} *)cpy_r_r)->_t;\n" - if not nullable: - expected += """if (unlikely(cpy_r_r0.f0 == NULL)) { - PyErr_SetString(PyExc_AttributeError, "attribute 't' of 'A' undefined"); - """.replace("of 'A'", "of 'Sub'" if inherited else "of 'A'") - expected += "} else {\n" if not borrow else "}\n" - elif not borrow: - expected += "if (cpy_r_r0.f0 != NULL) {\n" - if not borrow: - expected += """CPy_INCREF(cpy_r_r0.f0); - CPy_INCREF(cpy_r_r0.f1); - }""" - self.assert_emit( - GetAttr(self.r, "t", 1, borrow=borrow, allow_error_value=nullable), - expected, - ) - def test_set_attr(self) -> None: self.use_gil_target() self.assert_emit( From d4cecb1afe74d2ee01d829054dd10186fcff9f12 Mon Sep 17 00:00:00 2001 From: Piotr Sawicki Date: Mon, 19 Oct 2026 16:07:19 +0200 Subject: [PATCH 3/3] Fix comment --- mypyc/codegen/emit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mypyc/codegen/emit.py b/mypyc/codegen/emit.py index 354eade50434..eb82fb601428 100644 --- a/mypyc/codegen/emit.py +++ b/mypyc/codegen/emit.py @@ -553,7 +553,7 @@ def emit_tuple_attr_store_locked( delete: bool = False, is_init: bool = False, ) -> None: - """Assign a tuple attribute attr to src under the owner lock. + """Assign src to a tuple attribute attr under the owner lock. References to the old value of the attribute are released outside the lock. """