Skip to content

gh-131527: Stackref debug borrow checker #140599

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

gh-131527: Stackref debug borrow checker #140599

wants to merge 12 commits into from
+110 −14

Conversation

@efimov-mikhail
Copy link
Member

@efimov-mikhail efimov-mikhail commented Oct 25, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Oct 25, 2025
Open
@efimov-mikhail
Copy link
Member Author

efimov-mikhail commented Oct 25, 2025

It works fine and generally follows the sketch in the issue.
But I don't follow chains of borrows and check only the nearest borrowed_from stackref.

There are 17 failed tests with this PR in Py_STACKREF_DEBUG mode.

17 tests failed:
    test.test_gdb.test_backtrace test.test_gdb.test_misc
    test.test_gdb.test_pretty_print test.test_io.test_general
    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_frame test_interpreters test_profiling
    test_pyrepl test_regrtest test_repl test_threading

Compare with
#139475 (comment)

Here test.test_io.test_general is a bit flaky, but test_frame is a real find by new borrow checker.

-> % ./python -m unittest -v test.test_frame.TestFrameLocals.test_overwrite_locals
test_overwrite_locals (test.test_frame.TestFrameLocals.test_overwrite_locals) ... Fatal Python error: _Py_stackref_close: StackRef with ID 2401080 closed while borrows 1 refs at Objects/frameobject.c:288. Opened at Python/generated_cases.c.h:1239

There's no real problem here but we can rewrite it a little to prevent this error and make code related to frame->f_overwritten_fast_locals clearer.

mpage
mpage reviewed Nov 4, 2025
View reviewed changes
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.

Nice! Just some minor inline comments/suggestions. Feel free to push back if you don't agree.

@efimov-mikhail efimov-mikhail requested a review from mpage November 4, 2025 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner

@sergey-miryanov sergey-miryanov Awaiting requested review from sergey-miryanov

@markshannon markshannon Awaiting requested review from markshannon

@mpage mpage Awaiting requested review from mpage

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@efimov-mikhail @mpage