8000 gh-117139: A StackRef Debugging Mode by Fidget-Spinner · Pull Request #121134 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-117139: A StackRef Debugging Mode #121134

8000 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
Closed
wants to merge 11 commits into from
Closed

gh-117139: A StackRef Debugging Mode #121134

wants to merge 11 commits into from

Conversation

Fidget-Spinner
Copy link
Member
@Fidget-Spinner Fidget-Spinner commented Jun 28, 2024 •
edited
Loading

This enforces HPy-like semantics in CPython's eval loop. It's checked at runtime with a new handle table. This catches errors like double decref, introducing dead references to the stack, etc.

See https://github.com/faster-cpython/ideas/blob/2beefd4da2bf956c837e8fb7097b7a43c53f5735/3.14/stackref_semantics.md
for more info.

I set up a single runner that just runs python -m test test_asyncio as this mode is 2.5x slower than normal CPython. That is usually enough to catch almost all bugs. Besides, this mode is also not fully compatible with all our tests that introspect CPython.

This PR depends on #121127 being merged first.

@bedevere-app bedevere-app bot mentioned this pull request Jun 28, 2024
Closed
@markshannon
Copy link
Member
markshannon commented Jun 28, 2024 •
edited
Loading

I don't think you need the live flag in the stack ref. The presence of the bits in the mapping implies liveness.
That way the _PyStackRef struct can remain unchanged.

Something like this:

THE_MAP: dict[uintptr_t, object] = {}
THE_COUNTER = 0

class StackRef:
    bits: uintptr_t

    def __init__(self, obj):
        THE_COUNTER += 1
        self.bits = THE_COUNTER
        THE_MAP[self.bits] = obj
       
    def close(self):
        del THE_MAP[self.bits]

    def dup(ref: StackRef) -> StackRef:
        obj = THE_MAP[ref.bits]
        return StackRef(obj)

    def steal_obj(ref: StackRef) -> object:
        return THE_MAP.pop(ref.bits)
      
    def steal_ref(ref: StackRef) -> StackRef:
        return StackRef.from_obj_steal(self.steal_obj())
  
    @classmethod
    def from_obj_steal(obj: object) -> StackRef:
        res = StackRef(obj)
        Py_DECREF(object)
        return res

    @classmethod
    def from_obj_new(obj: object) -> StackRef:
        return StackRef(obj)

The hashtable in hashtable.c should provide the basics of the mapping.

markshannon
markshannon reviewed Jul 17, 2024
View reviewed changes
Copy link
Member
@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

How much extra effort would it be do this for the default build?

Can you try this on the full set of tests, just to see if it would be acceptable on CI?

.github/workflows/build.yml
@@ -551,6 +564,7 @@ jobs:
- check_generated_files
- build_macos
- build_macos_free_threading
- build_macos_free_threading_with_stackref_debug
Copy link
Member

Choose a reason for hiding this comment

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

Could we do this for normal builds as well?
The linux machines are cheapest, so we should probably use those.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't because normal builds require all the inline functions to be macros #121263. And these are too complicated to fit into a single macro.

.github/workflows/reusable-macos.yml
--prefix=/opt/python-dev \
--with-openssl="$(brew --prefix openssl@3.0)"
- name: Build CPython
run: make -j8
- name: Display build info
run: make pythoninfo
- name: Tests
run: make test
# Stackref debug is 3.5x slower than normal CPython,
Copy link
Member

Choose a reason for hiding this comment

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

How about using the set of tests for profiling? It is about 40 tests instead of the usual ~400, so would give decent coverage without being too slow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry how do I run those specific tests? What command with python -m test?

@Fidget-Spinner
Copy link
Member Author

Can you try this on the full set of tests, just to see if it would be acceptable on CI?

I did. It fails

    test.test_gdb.test_pretty_print test_capi test_exceptions
    test_regrtest test_repl test_sys test_tracemalloc

mainly because they count allocations/test out of memory situations, and the hashtable adds random allocations which breaks their careful calculations.

On my computer on free threaded builds with the stackref debugging mode, it takes 5 minutes to run the whole test suite. macOS is the fastest/"cheapest" runner we have right now because Cirrus Labs is sponsoring bare-metal runners IIRC. Without stackref debug mode, it takes 2 min 7 s. So roughly a 2.5x slowdown. Factoring that in, the fastest Cirruslabs runners takes 6 min to complete on this PR. So it should take 15 minutes to run the whole test suite. We would need to skip those tests above too, as those are known failures.

@ericsnowcurrently ericsnowcurrently mentioned this pull request Jul 18, 2024
Open
@ericsnowcurrently
Copy link
Member

mainly because they count allocations/test out of memory situations

That's too bad. I wonder what it would take to fix those tests to be less fragile (tightly coupled to fine details)? I've opened gh-121978 so we can consider that separately.

@Fidget-Spinner
Copy link
Member Author
8000

This is already implemented on main by Mark.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon markshannon left review comments

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

@ezio-melotti ezio-melotti Awaiting requested review from ezio-melotti ezio-melotti is a code owner

@hugovk hugovk Awaiting requested review from hugovk hugovk is a code owner

Assignees
No one assigned
Labels
awaiting core review topic-free-threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Fidget-Spinner @markshannon @ericsnowcurrently
0