[mypyc] Make attribute access memory safe on free-threaded builds#21705
[mypyc] Make attribute access memory safe on free-threaded builds#21705JukkaL wants to merge 14 commits into
Conversation
|
cc @msullivan |
| if (value != NULL) { | ||
| _PyObject_SetMaybeWeakref(value); | ||
| } |
There was a problem hiding this comment.
I think that this should not be required for correctness anymore. It looks like the original implementation of GetAttrRef had a loop on _Py_TryIncrefCompare, which requires that maybe-weakref be set in order to succeed, but the current version calls _Py_NewRefWithLock, which does not require that, so we should be good. (I think that change was made so that init wouldn't need to call _PyObject_SetMaybeWeakref, but it also means set shouldn't need to either.)
There was a problem hiding this comment.
The reasoning sounds correct, and removing the call reduced the self check regression by about 1% from ~6% to ~5%, which is nice. Done.
| if (_Py_TryIncrefCompare(field, v)) { | ||
| return v; | ||
| } | ||
| Py_BEGIN_CRITICAL_SECTION(self); |
There was a problem hiding this comment.
I don't think that the Py_BEGIN_CRITICAL_SECTION can possibly be doing anything useful here, since the writer doesn't take one.
The slow paths for list/dict reads that use Py_BEGIN_CRITICAL_SECTION rely on the write side taking a critical section.
There was a problem hiding this comment.
Yes, this seems redundant. Removed.
| if (_Py_TryIncrefCompare(field, v)) { | ||
| return v; | ||
| } |
There was a problem hiding this comment.
I don't think that we need to use _Py_TryIncrefCompare or do a re-load of the field, with the current implementation. I think we should be able to use just _Py_TryIncRefShared.
In particular, I think that the dict/list paths that rely on validating that the values haven't changed are a different code path than this one in which the objects do actually get freed but the allocator makes sure they don't get reused for different kinds of objects, so the refcounts stay refcounts.
We're doing a different approach based on _PyObject_XDecRefDelayed, so I don't think that the reloading-and-checking helps any.
There was a problem hiding this comment.
This sounds like a good idea, though I'm not sure if the impact can be seen in microbenchmarks. Done.
|
The simplified implementation still fixes all the memory safety issues I've been able to reproduce on master. |
msullivan
left a comment
msullivan
left a comment
There was a problem hiding this comment.
This looks pretty good.
The one concern is that doing deferred decrefs is a behavioral change (stuff won't get freed eagerly), and I think it'll happen even on thread-local objects.
Not sure if there's any decent way to fix it without a lock though; I'll ponder about it some though
| emitter.emit_line("{") | ||
| attr_expr = f"self->{attr_field}" | ||
|
|
||
| if IS_FREE_THREADED and is_simple_refcounted_pointer(rtype): |
There was a problem hiding this comment.
What is done in the non is_simple_refcounted_pointer case?
| // 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 fast-path try-incref fails on an | ||
| // unflagged 'value' does not spin: CPy_GetAttrRef falls into its | ||
| // _Py_NewRefWithLock fallback (CPy_GetAttrRefSlow), which cannot fail and | ||
| // sets maybe-weakref lazily on that first read. |
There was a problem hiding this comment.
Classic claude talking about old code; I'd probably trim this up
2 participants
This fixes memory unsafety when there is a race condition related to attribute get/set operations on a free-threaded build, for simple reference-based attributes only (
PyObject *). This includes attributes with primitive types likelist,setandstranddict, and native class types.The main idea is to use delayed decref for the old attribute value when assigning to an attribute. There are detailed comments explaining the approach in detail.
This causes a ~5% slowdown in self check when using 3.14t. Currently it looks like this is unavoidable if we want to fix memory unsafety.
Remaining work includes fixing attributes with fixed-length tuple types and tagged integer types (in follow-up PRs).
I used coding agent assist heavily.
Work on mypyc/mypyc#1203.