FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-131527: Stackref debug borrow checker by efimov-mikhail · Pull Request #140599 · python/cpython · GitHub

/ cpython Public

gh-131527: Stackref debug borrow checker - #140599

Merged
mpage merged 13 commits into
python:mainfrom
efimov-mikhail:issue-131527-stackref_debug_borrow_checker
Nov 5, 2025
ConversationCommits (13)ChecksFiles changed
Merged

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

Conversation

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

Copy link
Copy Markdown
Member

bedevere-app Bot mentioned this pull request Oct 25, 2025
Closed

efimov-mikhail commented Oct 25, 2025

Copy link
Copy Markdown
Member Author

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

mpage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Include/internal/pycore_stackref.h Outdated
Comment thread Python/stackrefs.c Outdated
Comment thread Python/stackrefs.c
Comment thread Python/stackrefs.c
efimov-mikhail requested a review from mpage November 4, 2025 09:08
mpage approved these changes Nov 5, 2025
View reviewed changes

mpage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM - thanks!

mpage merged commit 3cb1ab0 into python:main Nov 5, 2025
83 of 85 checks passed

efimov-mikhail commented Nov 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

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
pythongh-131527: Stackref debug borrow checker (python#140599)
a1ddfca 
Add borrow checking to the stackref debug mode

---------

Co-authored-by: mpage <mpage@meta.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mpage mpage approved these changes
Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner
sergey-miryanov Awaiting requested review from sergey-miryanov
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

Back | FazBrowse Home | New Git URL