Skip to content

Conversation

efimov-mikhail
Copy link
Contributor

@efimov-mikhail efimov-mikhail commented Oct 1, 2025

There are two problems with PyStackRef debug builds:

  1. Incorrect _PyStackRef_FromPyObjectBorrow function.
    We should provide Py_INCREF here, because each PyStackRef_CLOSE will use Py_DECREF in this mode.
  2. No PyStackRef_Wrap and PyStackRef_Unwrap functions.

@efimov-mikhail
Copy link
Contributor Author

CC @markshannon

@efimov-mikhail
Copy link
Contributor Author

efimov-mikhail commented Oct 1, 2025

This PR is just minimal fix for Py_STACKREF_DEBUG build.

I suggest to make changes for this build: provide exactly those flags for indexes as for standard GIL build:

#define Py_INT_TAG 3
#define Py_TAG_INVALID 2
#define Py_TAG_REFCNT 1
#define Py_TAG_BITS 3

And use indexes without flags as real indexes in hash table.
And make all refcounts for DUP, CLOSE and Borrow exactly the same as for standard GIL build.

For example, test.test_list.ListTest.test_list_overwrite_local fails with current variant of PyStackRef debug build because refcounts differ.

PyStackRef_Wrap(void *ptr)
{
assert(ptr != NULL);
return (_PyStackRef){ .index = (uint64_t)ptr };
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it is safe to cast void* to uint64 and vice verse. Maybe we should use uintptr_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We know that Py_STACKREF_DEBUG requires 64 bit machine.
IMO, we could use uint64_t here.
But it's also possible to use uintptr_t bits as in production build, and save indexes in it.

@efimov-mikhail
Copy link
Contributor Author

This PR is just minimal fix for Py_STACKREF_DEBUG build.

I suggest to make changes for this build: provide exactly those flags for indexes as for standard GIL build:

#define Py_INT_TAG 3
#define Py_TAG_INVALID 2
#define Py_TAG_REFCNT 1
#define Py_TAG_BITS 3

And use indexes without flags as real indexes in hash table. And make all refcounts for DUP, CLOSE and Borrow exactly the same as for standard GIL build.

For example, test.test_list.ListTest.test_list_overwrite_local fails with current variant of PyStackRef debug build because refcounts differ.

I provide those changes.
IMO, this variant is better than minimal fix:

  1. Two tests test_list and test_capi don't fail with this but fail with minimal fix.
  2. We have exactly the same refcount scheme and it's easier to understand what's going on.
  3. Future changes for StackRef build will be more obvious.

@efimov-mikhail
Copy link
Contributor Author

CC @mpage

Copy link
Contributor

@mpage mpage left a 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.

@mpage mpage requested review from markshannon and sergey-miryanov and removed request for sergey-miryanov October 3, 2025 17:47
@efimov-mikhail
Copy link
Contributor Author

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.
I can make another PR with minimal fix if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants