gh-149965: rename ptr_wise_atomic_memmove to _PyObject_ptr_wise_atomic_memmove#151271
gh-149965: rename ptr_wise_atomic_memmove to _PyObject_ptr_wise_atomic_memmove#151271clin1234 wants to merge 20 commits into
ptr_wise_atomic_memmove to _PyObject_ptr_wise_atomic_memmove#151271Conversation
…FW3ZD.rst Co-authored-by: Sergey Miryanov <sergey.miryanov@gmail.com>
…ternal helper and add unit tests Extract the duplicated static `ptr_wise_atomic_memmove` from `Modules/_elementtree.c` and `Objects/listobject.c` into a shared `static inline _Py_ptr_wise_atomic_memmove(PyObject *a, ...)` in `Include/internal/pycore_object.h`. The first parameter is generalised from a type-specific pointer to `PyObject *` since only `PyObject *`- level operations (`_Py_IsOwnedByCurrentThread`, `_PyObject_GC_IS_SHARED`) were ever performed on it. `listobject.c` previously embedded `_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED` inside the local helper. That assertion is preserved by moving it explicitly to each of the four call sites in `listobject.c` (which are all called under a critical section). `_elementtree.c`'s open question about whether a critical section is needed remains unanswered, so no assertion is added there. Unit tests are added in `Modules/_testinternalcapi/test_ptr_wise_memmove.c` covering: dest < src (forward copy), dest > src (backward copy), dest == src (no-op), overlapping ranges, and the single-owner fast path. The single-owner test explicitly clears the GC SHARED bit to guard against freelist reuse leaving the bit set from a sibling test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Commented out critical section assertion for ElementObject.
…e-149965.edk2ut.rst
This reverts commit a3a7eee.
clin1234
changed the title
ptr_wise_atomic_memmove use within element_ass_subscrptr_wise_atomic_memmove within element_ass_subscr
|
@ZeroIntensity, I've reverted the header change and unit test additions, so my current PR is focused on only one file. |
|
I think you misunderstood what I meant. You can move the function to a header (and in fact, you should, because otherwise we're duplicating code), but it was weird to do it in its own PR. Just combine this PR with #151255 and it'll be fine. |
clin1234
requested review from
a team,
AA-Turner,
emmatyping,
erlend-aasland and
itamaro
as code owners
|
@ZeroIntensity, mind taking a look again? |
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Can you add a multithreaded test stressing this path?
| // Pointer-by-pointer memmove for PyObject** arrays that is safe for shared | ||
| // objects in Py_GIL_DISABLED builds. Locking is the caller's responsibility. | ||
| static inline void | ||
| _Py_ptr_wise_atomic_memmove(PyObject *a, PyObject **dest, PyObject **src, |
There was a problem hiding this comment.
We should probably prefix this with _PyObject instead of plain _Py.
| _Py_ptr_wise_atomic_memmove( | ||
| (PyObject *)self, |
There was a problem hiding this comment.
To avoid needing to cast on every use, we can turn this into a macro that uses _PyObject_CAST. Here's an outline:
#define _Py_ptr_wise_atomic_memmove(a, dest, src, n) _Py_ptr_wise_atomic_memmove(_PyObject_CAST(a), dest, src, n)
Where would the multithreaded test lie within the source tree, considering that it's an internal function not exported to the public headers? |
clin1234
changed the title
ptr_wise_atomic_memmove within element_ass_subscrptr_wise_atomic_memmove to _PyObject_ptr_wise_atomic_memmove
|
Do you think a News fragment is needed here? |
I meant for the
Yes; |
2 participants
See also capi-workgroup/decisions#106