Skip to content

Commit f650d7e

Browse files
committed
Fix another attribute read code path
1 parent 1bb2e04 commit f650d7e

1 file changed

Lines changed: 46 additions & 35 deletions

File tree

mypyc/codegen/emitfunc.py

Lines changed: 46 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,37 @@ def get_attr_expr(self, obj: str, op: GetAttr | SetAttr, decl_cl: ClassIR) -> st
401401
cast = f"({decl_cl.struct_name(self.emitter.names)} *)"
402402
return f"({cast}{obj})->{self.emitter.attr(op.attr)}"
403403

404+
def emit_load_attr_take_ref(
405+
self, dest: str, obj: str, op: GetAttr, cl: ClassIR, attr_rtype: RType, attr_expr: str
406+
) -> bool:
407+
"""Emit the load of a native attribute into 'dest', taking a new reference.
408+
409+
On free-threaded builds, reading a single reference-counted 'PyObject *' field
410+
and taking a new reference must be done atomically to avoid a use-after-free
411+
race with a concurrent setter. CPy_GetAttrRef performs the load and incref
412+
atomically and returns a new reference (or NULL if undefined), so callers must
413+
NOT emit a separate inc_ref. Return True in that case so the caller can skip it.
414+
415+
Final attributes are never rebound (no setter), so there is no concurrent writer
416+
and no use-after-free window; an owned read uses the cheaper CPy_GetAttrRefFinal
417+
(a plain load + incref). Borrowed reads keep the plain load: they are only emitted
418+
for attributes safe to borrow on free-threaded builds (Final and vec attrs -- see
419+
transform_member_expr in irbuild), whose values live as long as their container.
420+
The default (GIL) build always takes the plain-load path and increfs separately.
421+
"""
422+
use_get_attr_ref = (
423+
IS_FREE_THREADED and is_simple_refcounted_pointer(attr_rtype) and not op.is_borrowed
424+
)
425+
if use_get_attr_ref and cl.is_final_attr(op.attr):
426+
self.emitter.emit_line(f"{dest} = CPy_GetAttrRefFinal((PyObject **)&{attr_expr});")
427+
elif use_get_attr_ref:
428+
self.emitter.emit_line(
429+
f"{dest} = CPy_GetAttrRef((PyObject *){obj}, (PyObject **)&{attr_expr});"
430+
)
431+
else:
432+
self.emitter.emit_line(f"{dest} = {attr_expr};")
433+
return use_get_attr_ref
434+
404435
def visit_get_attr(self, op: GetAttr) -> None:
405436
if op.allow_error_value:
406437
self.get_attr_with_allow_error_value(op)
@@ -434,33 +465,9 @@ def visit_get_attr(self, op: GetAttr) -> None:
434465
else:
435466
# Otherwise, use direct or offset struct access.
436467
attr_expr = self.get_attr_expr(obj, op, decl_cl)
437-
# In free-threaded builds, reading a single reference-counted
438-
# 'PyObject *' field and taking a new reference must be done
439-
# atomically to avoid a use-after-free race with a concurrent setter.
440-
# CPy_GetAttrRef performs the load and incref atomically and returns a
441-
# new reference (or NULL if undefined), so no separate inc_ref is
442-
# emitted below. Borrowed reads keep the plain load; they are only
443-
# emitted for attributes that are safe to borrow on free-threaded builds
444-
# (Final and vec attrs -- see transform_member_expr in irbuild).
445-
#
446-
# Final attributes are never rebound (no setter), so there is no
447-
# concurrent writer and no use-after-free window: an owned read is a
448-
# plain load + incref (via the cheaper CPy_GetAttrRefFinal), and a
449-
# borrowed read stays a plain borrow (handled by the else branch), since
450-
# the value lives as long as its container.
451-
use_get_attr_ref = (
452-
IS_FREE_THREADED
453-
and is_simple_refcounted_pointer(attr_rtype)
454-
and not op.is_borrowed
468+
use_get_attr_ref = self.emit_load_attr_take_ref(
469+
dest, obj, op, cl, attr_rtype, attr_expr
455470
)
456-
if use_get_attr_ref and cl.is_final_attr(op.attr):
457-
self.emitter.emit_line(f"{dest} = CPy_GetAttrRefFinal((PyObject **)&{attr_expr});")
458-
elif use_get_attr_ref:
459-
self.emitter.emit_line(
460-
f"{dest} = CPy_GetAttrRef((PyObject *){obj}, (PyObject **)&{attr_expr});"
461-
)
462-
else:
463-
self.emitter.emit_line(f"{dest} = {attr_expr};")
464471
always_defined = cl.is_always_defined(op.attr)
465472
merged_branch = None
466473
if not always_defined:
@@ -513,16 +520,20 @@ def get_attr_with_allow_error_value(self, op: GetAttr) -> None:
513520
cl = rtype.class_ir
514521
attr_rtype, decl_cl = cl.attr_details(op.attr)
515522

516-
# Direct struct access without NULL check
517523
attr_expr = self.get_attr_expr(obj, op, decl_cl)
518-
self.emitter.emit_line(f"{dest} = {attr_expr};")
519-
520-
# Only emit inc_ref if not NULL
521-
if attr_rtype.is_refcounted and not op.is_borrowed:
522-
check = self.error_value_check(op, "!=")
523-
self.emitter.emit_line(f"if ({check}) {{")
524-
self.emitter.emit_inc_ref(dest, attr_rtype)
525-
self.emitter.emit_line("}")
524+
# On free-threaded builds this takes a new reference atomically (see
525+
# emit_load_attr_take_ref). CPy_GetAttrRef returns NULL when the field is
526+
# undefined, which is precisely the error value here, so the "NULL without
527+
# AttributeError" behavior is preserved (this op has error_kind ERR_NEVER).
528+
# is_simple_refcounted_pointer excludes tuples/vecs, so error_value_check's
529+
# special cases only matter on the plain-load path (where no ref was taken).
530+
if not self.emit_load_attr_take_ref(dest, obj, op, cl, attr_rtype, attr_expr):
531+
# Only emit inc_ref if not NULL
532+
if attr_rtype.is_refcounted and not op.is_borrowed:
533+
check = self.error_value_check(op, "!=")
534+
self.emitter.emit_line(f"if ({check}) {{")
535+
self.emitter.emit_inc_ref(dest, attr_rtype)
536+
self.emitter.emit_line("}")
526537

527538
def next_branch(self) -> Branch | None:
528539
if self.op_index + 1 < len(self.ops):

0 commit comments

Comments
 (0)