GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-117139: A StackRef Debugging Mode by Fidget-Spinner · Pull Request #121134 · python/cpython

Conversation

@Fidget-Spinner

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

  • Issue: Set up tagged pointers in the evaluation stack #117139

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Fidget-Spinner added 2 commits June 28, 2024 23:59
@Fidget-Spinner
A StackRef Debugging Mode
ddf839e
@Fidget-Spinner
Create 2024-06-29-00-02-42.gh-issue-117139.-lKXPB.rst
0b0fa3d
@Fidget-Spinner
Fidget-Spinner requested review from corona10, ericsnowcurrently, erlend-aasland, ezio-melotti, hugovk and markshannon as code owners June 28, 2024 16:03
@bedevere-app bedevere-app Bot mentioned this pull request Jun 28, 2024
Set up tagged pointers in the evaluation stack #117139
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Jun 28, 2024
@Fidget-Spinner
fix CI
d2c7bb9
@Fidget-Spinner Fidget-Spinner added the topic-free-threading label Jun 28, 2024
@Fidget-Spinner
Fixup defines
2c6860a
@markshannon

markshannon commented Jun 28, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Fidget-Spinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner added 5 commits June 30, 2024 16:54
@Fidget-Spinner
Use a hashtable instead
ba49e58
@Fidget-Spinner
fix unused warning
18d26ce
@Fidget-Spinner
fix build
5ff2a57
@Fidget-Spinner
Fix build again
e2563c6
@Fidget-Spinner
Merge remote-tracking branch 'upstream/main' into stackref_debug_real
fe687c3
markshannon
markshannon reviewed Jul 17, 2024
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread .github/workflows/build.yml
- check_generated_files
- build_macos
- build_macos_free_threading
- build_macos_free_threading_with_stackref_debug

@markshannon markshannon Jul 17, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Fidget-Spinner Fidget-Spinner Jul 17, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread .github/workflows/reusable-macos.yml
run: make pythoninfo
- name: Tests
run: make test
# Stackref debug is 3.5x slower than normal CPython,

@markshannon markshannon Jul 17, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Fidget-Spinner Fidget-Spinner Jul 17, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner added 2 commits July 17, 2024 18:03
@Fidget-Spinner
Merge remote-tracking branch 'upstream/main' into stackref_debug_real
a6e6576
@Fidget-Spinner
cleanup
a058c34
@Fidget-Spinner

Fidget-Spinner commented Jul 17, 2024

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ericsnowcurrently ericsnowcurrently mentioned this pull request Jul 18, 2024
Make Memory-Related Tests Less Fragile #121978
Open
@ericsnowcurrently

ericsnowcurrently commented Jul 18, 2024

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Jan 16, 2025

Copy link
Copy Markdown
Member Author

This is already implemented on main by Mark.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner Fidget-Spinner closed this Jan 16, 2025
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@Fidget-Spinner @markshannon @ericsnowcurrently
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.