-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-135125: Fix Py_STACKREF_DEBUG build #139475
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-135125: Fix Py_STACKREF_DEBUG build #139475
Conversation
|
CC @markshannon |
|
This PR is just minimal fix for I suggest to make changes for this build: provide exactly those flags for indexes as for standard GIL build: And use indexes without flags as real indexes in hash table. For example, |
I provide those changes.
|
|
CC @mpage |
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.
This looks good to me. In the future I'd recommend splitting this into two PRs (and associated issues): one that is the minimal fix for the build and another that unifies the refcounting behavior. I'd like to give Mark a chance to review this as I'm not sure if he wants to preserve the refcounting behavior in the debug build. I've added him as a reviewer.
Thanks! I thought about two separate PRs but I wasn't sure about that. |
|
Since #139384 has been merged, we should add |
I've already added this function to |
Sorry, it is my oversight. |
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. Thanks!
|
@markshannon |
|
This looks good, and passes all the test on my machine with When I last run all the tests with @efimov-mikhail any idea why all the tests now pass? Have the leaks been fixed, or are we not now detecting them for some reason? |
|
Thanks for the review! On my machine I have the following failures in this mode: |
|
Those failures look very much like the ones I was seeing before. |
|
Yeah, I had All looks good now. |
|
Thanks for doing this, btw. |
|
Thanks for merge! Do we want to backport this on 3.14 or it seems redundant? |
There are two problems with PyStackRef debug builds:
_PyStackRef_FromPyObjectBorrowfunction.We should provide
Py_INCREFhere, because eachPyStackRef_CLOSEwill usePy_DECREFin this mode.PyStackRef_WrapandPyStackRef_Unwrapfunctions.Py_STACKREF_DEBUGset. #135125