-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
GH-140061: Use _PyObject_IsUniquelyReferenced to check if object is uniquely referenced #140062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-140061: Use _PyObject_IsUniquelyReferenced to check if object is uniquely referenced #140062
Conversation
I think it should skip news, but not sure about backporting. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the reference is owned by the eval loop, we need to use PyUnstable_Object_IsUniqueReferencedTemporary
instead. I think there are a few cases of that here.
cc @colesbury
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You need to be careful about changing
assert()
, especially when the code paths can be called by extensions. - There are a number of conditions like
Py_REFCNT(result) > 1
that need to be changed, such as initertooslmodule.c
@ZeroIntensity @colesbury Thanks for review! I have fixed your suggestions, but it fails in TSAN(free-threading). Can you take a look? |
It seems that me and Kumar broke that earlier (see #140138), sorry! You can ignore it for now and we'll rebase this once we get it fixed on main. |
@ZeroIntensity Thanks! |
Is this PR ready for a review? |
@vstinner Yes, it is ready to review. If @colesbury don't have any comments, of course. |
Ok, I click on [Ready for review] button in this case :-) |
Ough, I forgot that I turned it to draft :( |
Remaining code using $ git grep 'Py_REFCNT.*\(==\|<\|<=\|>\|>=\) *1'
Doc/c-api/object.rst: :c:expr:`Py_REFCNT(op) == 1`.
Doc/c-api/object.rst: is only used by this thread. :c:expr:`Py_REFCNT(op) == 1` is **not**
Doc/whatsnew/3.14.rst: a replacement for ``Py_REFCNT(op) == 1`` on :term:`free threaded
Include/internal/pycore_object.h: return Py_REFCNT(ob) == 1;
Modules/_ctypes/_ctypes.c: assert (Py_REFCNT(obj) == 1);
Modules/_functoolsmodule.c: assert(Py_REFCNT(args) == 1);
Modules/_io/bytesio.c: * Py_REFCNT(buf) == 1, exports == 0.
Modules/_io/bytesio.c: * Py_REFCNT(buf) > 1. exports == 0,
Modules/_io/bytesio.c: * exports > 0. Py_REFCNT(buf) == 1, any modifications are forbidden.
Modules/_testcapi/object.c: assert(Py_REFCNT(obj) == 1);
Modules/_testcapi/object.c: assert(Py_REFCNT(obj) == 1);
Modules/_testcapi/object.c: assert(Py_REFCNT(obj) == 1); \
Modules/_testcapimodule.c: assert(Py_REFCNT(obj) == 1);
Modules/itertoolsmodule.c: assert (npools==0 || Py_REFCNT(result) == 1);
Modules/itertoolsmodule.c: assert(r == 0 || Py_REFCNT(result) == 1);
Modules/itertoolsmodule.c: assert(r == 0 || Py_REFCNT(result) == 1);
Modules/itertoolsmodule.c: assert(r == 0 || Py_REFCNT(result) == 1);
Objects/listobject.c: assert(Py_REFCNT(self) == 1 || PyMutex_IsLocked(&_PyObject_CAST(self)->ob_mutex));
Objects/longobject.c: assert(Py_REFCNT(z) == 1);
Objects/longobject.c: assert(_PyLong_IsZero(z) || Py_REFCNT(z) == 1);
Objects/longobject.c: assert(Py_REFCNT(z) == 1);
Objects/longobject.c: assert(Py_REFCNT(obj) == 1);
Objects/longobject.c: assert(Py_REFCNT(obj) == 1);
Objects/object.c: CHECK(Py_REFCNT(op) >= 1);
Objects/object.c: _PyObject_ASSERT(name, Py_REFCNT(name) >= 1);
Objects/setobject.c: assert(Py_REFCNT(so) == 1);
Objects/typeobject.c: CHECK(Py_REFCNT(type) >= 1);
Objects/unicodeobject.c: assert(Py_REFCNT(unicode) == 1);
Objects/unicodeobject.c: assert(Py_REFCNT(unicode) == 1);
Python/ceval.c: assert(Py_REFCNT(locals) > 1);
Python/gc_free_threading.c: _PyObject_ASSERT(op, Py_REFCNT(op) > 1); There are documentation and assertions. Assertions should continue to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As I understand @colesbury - yes. |
Co-authored-by: Victor Stinner <[email protected]>
Thanks @sergey-miryanov! |
Thanks @sergey-miryanov for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…bjects are uniquely referenced (pythongh-140062) The previous `Py_REFCNT(x) == 1` checks can have data races in the free threaded build. `_PyObject_IsUniquelyReferenced(x)` is a more conservative check that is safe in the free threaded build and is identical to `Py_REFCNT(x) == 1` in the default GIL-enabled build. (cherry picked from commit 32c2649) Co-authored-by: Sergey Miryanov <[email protected]>
GH-140157 is a backport of this pull request to the 3.14 branch. |
…objects are uniquely referenced (gh-140062) (gh-140157) The previous `Py_REFCNT(x) == 1` checks can have data races in the free threaded build. `_PyObject_IsUniquelyReferenced(x)` is a more conservative check that is safe in the free threaded build and is identical to `Py_REFCNT(x) == 1` in the default GIL-enabled build. (cherry picked from commit 32c2649) Co-authored-by: Sergey Miryanov <[email protected]>
Thanks everyone for review! |
There are a lot of places where
Py_REFCNT(..)==1
used. IIUC it is not correct forno-gil
version.Py_REFCNT(x) == 1
for free-threaded compatibility #140061