Member

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.

• Issue: Build fails with Py_STACKREF_DEBUG set. #135125

Member 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.

Member 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.

Contributor

Choose a reason for hiding this comment

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

Member 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.

Contributor

Since #139384 has been merged, we should add PyStackRef_IsMalformed. Do you plan to add it in this PR or in a follow-up one?

Member Author

Since #139384 has been merged, we should add PyStackRef_IsMalformed. Do you plan to add it in this PR or in a follow-up one?

I've already added this function to Include/internal/pycore_stackref.h.

Contributor

I've already added this function to Include/internal/pycore_stackref.h.

Sorry, it is my oversight.

Contributor

Choose a reason for hiding this comment

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

Member

This looks good, and passes all the test on my machine with #define Py_STACKREF_DEBUG 1 but I'm puzzled by one thing.

When I last run all the tests with #define Py_STACKREF_DEBUG 1, many moths ago before I broke them, a number of tests failed due to leaks when stopping threads.
Running today, all the tests pass.

@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?

Member Author

Thanks for the review!
It's kinda strange that all tests are passed in Py_STACKREF_DEBUG mode.
Perhaps something was compiled or run incorrectly.

On my machine I have the following failures in this mode:

15 tests failed:
    test.test_gdb.test_backtrace test.test_gdb.test_misc
    test.test_gdb.test_pretty_print
    test.test_multiprocessing_fork.test_misc
    test.test_multiprocessing_forkserver.test_misc
    test.test_multiprocessing_spawn.test_manager
    test.test_multiprocessing_spawn.test_misc test_external_inspection
    test_faulthandler test_interpreters test_profiling test_pyrepl
    test_regrtest test_repl test_threading

Member

Those failures look very much like the ones I was seeing before.
I must have done something wrong, let me try again.

Member

Yeah, I had #define Py_STACKREF_CLOSE_DEBUG 1 by mistake, which has no effect without #define Py_STACKREF_DEBUG 1.

All looks good now.

Member

Thanks for doing this, btw.

Member Author

Thanks for merge! Do we want to backport this on 3.14 or it seems redundant?

* Use the same pattern of refcounting for stackrefs as in production build