gh-128182: Switch ctypes to solely critical sections
#132133
Conversation
Modules/_ctypes/_ctypes.c
Outdated
| } else { | ||
| LOCK_PTR(self); | ||
| Py_BEGIN_CRITICAL_SECTION(self); | ||
| *((void **)self->b_ptr) = dst->b_ptr; |
There was a problem hiding this comment.
Can you explain again why we can't use locked_deref_assign here? is it because of pointer rules? (because here, we're just re-assigning the stuff right?)
There was a problem hiding this comment.
I would like to get rid of locked_deref_assign in future so using it less is better. Also that reacquires the critical sections so will cause perf loss
| @@ -3347,13 +3366,15 @@ _PyCData_set(ctypes_state *st, | |||
| if (err == -1) | |||
| return NULL; | |||
| if (err) { | |||
| locked_memcpy_from(ptr, src, size); | |||
| Py_BEGIN_CRITICAL_SECTION(src); | |||
| memcpy(ptr, src->b_ptr, size); | |||
There was a problem hiding this comment.
So the critical section should be held longer? because GetKeepedObjects() is calling PyCData_GetContainer() which itself uses Py_BEGIN_CRITICAL_SECTION(src), so aren't we having a re-entrant critical section lock?
There was a problem hiding this comment.
Yes, which critical sections are allowed to do 😄
We can't release the lock before calling GetKeepedObjects, but I guess we could call GetKeepedObjects_lock_held. Edit: nevermind, that's not a real function.
|
The existing usage of For example, on this line it acquires the critical section after reading the self->b_size so it is not safe. cpython/Modules/_ctypes/_ctypes.c Line 2218 in 0a10b45 |
|
Yeah, I'll remove that too. |
|
All commit authors signed the Contributor License Agreement. |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Got rid of I didn't expect the Spanish Inquisition. |
|
Nobody expects the Spanish Inquisition! @kumaraditya303: please review the changes made to this pull request. |
…#132133) Co-authored-by: Kumar Aditya <[email protected]>
Removes the
LOCK_PTR/UNLOCK_PTRmacros, and fixes two races related to missing locks.ctypespointer writes are not thread safe #128182