Skip to content

GH-140061: Use _PyObject_IsUniquelyReferenced to check if object is uniquely referenced#140062

Merged
colesbury merged 13 commits into
python:mainfrom
sergey-miryanov:140061-fix-checking-object-uniq-ref
Oct 15, 2025
Merged

GH-140061: Use _PyObject_IsUniquelyReferenced to check if object is uniquely referenced#140062
colesbury merged 13 commits into
python:mainfrom
sergey-miryanov:140061-fix-checking-object-uniq-ref

Conversation

@sergey-miryanov

@sergey-miryanov sergey-miryanov commented Oct 13, 2025

Copy link
Copy Markdown
Contributor

There are a lot of places where Py_REFCNT(..)==1 used. IIUC it is not correct for no-gil version.

@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

I think it should skip news, but not sure about backporting.

@ZeroIntensity

ZeroIntensity commented Oct 14, 2025

Copy link
Copy Markdown
Member

skip news isn't right -- Py_REFCNT(op) == 1 is an actual bug on the free-threaded build (and sometimes the GILful build), at least when using it for optimizations.

@ZeroIntensity ZeroIntensity left a comment

Copy link
Copy Markdown
Member

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

@colesbury colesbury self-requested a review October 14, 2025 15:59

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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 in itertooslmodule.c

Comment thread Modules/_functoolsmodule.c Outdated
Comment thread Modules/_testcapi/object.c Outdated
Comment thread Modules/_testcapi/object.c Outdated
Comment thread Objects/dictobject.c Outdated
Comment thread Objects/unicodeobject.c Outdated
Comment thread Objects/longobject.c Outdated
Comment thread Objects/longobject.c Outdated
Comment thread Objects/listobject.c Outdated
@sergey-miryanov sergey-miryanov marked this pull request as draft October 14, 2025 18:39
@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

@ZeroIntensity @colesbury Thanks for review! I have fixed your suggestions, but it fails in TSAN(free-threading). Can you take a look?

@ZeroIntensity

Copy link
Copy Markdown
Member

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.

@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

@ZeroIntensity Thanks!

@vstinner

Copy link
Copy Markdown
Member

Is this PR ready for a review?

@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

@vstinner Yes, it is ready to review. If @colesbury don't have any comments, of course.

@vstinner vstinner marked this pull request as ready for review October 15, 2025 12:33
@vstinner

Copy link
Copy Markdown
Member

Ok, I click on [Ready for review] button in this case :-)

@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

Ough, I forgot that I turned it to draft :(

@vstinner

Copy link
Copy Markdown
Member

Remaining code using Py_REFCNT() with 1:

$ 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 Py_REFCNT(), right?

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread Objects/longobject.c Outdated
Comment thread Modules/_elementtree.c Outdated
@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

Assertions should continue to use Py_REFCNT(), right?

As I understand @colesbury - yes.

Co-authored-by: Victor Stinner <vstinner@python.org>
@colesbury colesbury added the needs backport to 3.14 bugs and security fixes label Oct 15, 2025
@colesbury

Copy link
Copy Markdown
Contributor

Thanks @sergey-miryanov!

@colesbury colesbury merged commit 32c2649 into python:main Oct 15, 2025
47 checks passed
@miss-islington-app

Copy link
Copy Markdown

Thanks @sergey-miryanov for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 15, 2025
…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 <sergey.miryanov@gmail.com>
@bedevere-app

bedevere-app Bot commented Oct 15, 2025

Copy link
Copy Markdown

GH-140157 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Oct 15, 2025
colesbury pushed a commit that referenced this pull request Oct 15, 2025
…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 <sergey.miryanov@gmail.com>
@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

Thanks everyone for review!

StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants