KEMBAR78
gh-135125: Fix Py_STACKREF_DEBUG build by efimov-mikhail · Pull Request #139475 · python/cpython · GitHub
Skip to content

Conversation

efimov-mikhail
Copy link
Member

@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
Member Author

CC @markshannon

@efimov-mikhail
Copy link
Member 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.

@efimov-mikhail
Copy link
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.

@efimov-mikhail
Copy link
Member 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
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.

@sergey-miryanov
Copy link
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?

@efimov-mikhail
Copy link
Member Author

efimov-mikhail commented Oct 23, 2025

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.

@sergey-miryanov
Copy link
Contributor

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

Sorry, it is my oversight.

Copy link
Contributor

@sergey-miryanov sergey-miryanov left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@efimov-mikhail
Copy link
Member Author

@markshannon
Could you please take a look at this?

@markshannon
Copy link
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?

@efimov-mikhail
Copy link
Member Author

efimov-mikhail commented Oct 23, 2025

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

@markshannon
Copy link
Member

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

@markshannon
Copy link
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.

@markshannon markshannon merged commit 918a9ac into python:main Oct 23, 2025
53 checks passed
@markshannon
Copy link
Member

Thanks for doing this, btw.

@efimov-mikhail
Copy link
Member Author

efimov-mikhail commented Oct 23, 2025

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

@efimov-mikhail efimov-mikhail deleted the issue_135379_stackref_debug_fixes branch October 23, 2025 16:53
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.

4 participants