Skip to content

Comments

gh-131527: Stackref debug borrow checker#140599

Merged
mpage merged 13 commits intopython:mainfrom
efimov-mikhail:issue-131527-stackref_debug_borrow_checker
Nov 5, 2025
Merged

gh-131527: Stackref debug borrow checker#140599
mpage merged 13 commits intopython:mainfrom
efimov-mikhail:issue-131527-stackref_debug_borrow_checker

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
Closed
@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
mpage
mpage approved these changes Nov 5, 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.

LGTM - thanks!

@mpage mpage merged commit 3cb1ab0 into python:main Nov 5, 2025
83 of 85 checks passed
@efimov-mikhail
Copy link
Member Author

efimov-mikhail commented Nov 5, 2025 •
edited
Loading

Thanks a lot for merge, @mpage!

Maybe you'll be interesting in another stackref related PR?
#139717

StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
@efimov-mikhail @mpage @StanFromIreland
pythongh-131527: Stackref debug borrow checker (python#140599)
a1ddfca 
Add borrow checking to the stackref debug mode

---------

Co-authored-by: mpage <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mpage mpage mpage approved these changes

@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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@efimov-mikhail @mpage