Skip to content
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.

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

Replace _PyFrame_OpAlreadyRan by a check for incomplete frame #109176

Closed
iritkatriel opened this issue Sep 9, 2023 · 6 comments · Fixed by #119234
Closed

Replace _PyFrame_OpAlreadyRan by a check for incomplete frame #109176

iritkatriel opened this issue Sep 9, 2023 · 6 comments · Fixed by #119234
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@iritkatriel
Copy link
Member

iritkatriel commented Sep 9, 2023

The _PyFrame_OpAlreadyRan function in frameobject.c is not covered by any tests.

For details see faster-cpython/ideas#623.

Linked PRs

@iritkatriel iritkatriel added tests Tests in the Lib/test dir interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Sep 9, 2023
@ericsnowcurrently
Copy link
Member

Here's a demonstration of the (unlikely?) case for which _PyFrame_OpAlreadyRan() was added:

def spam(a, b, c):
    def eggs():
        return a, b, c
    return eggs

args = (1, 2, 3)
eggs1 = spam(*args)
print(eggs1.__closure__)
assert all(v is c.cell_contents for v, c in zip(args, eggs1.__closure__))

args = eggs1.__closure__
eggs2 = spam(*args)
print(eggs2.__closure__)
assert all(c not in eggs2.__closure__ for c in args)
assert all(v is c.cell_contents for v, c in zip(args, eggs2.__closure__))

However, this only needs _PyFrame_OpAlreadyRan() if it's possible to interrupt the eval loop before all MAKE_CELL instructions have executed. Per faster-cpython/ideas#623 (comment), it sounds like such a case is not possible. Thus, we might not have any way to add a test where _PyFrame_OpAlreadyRan() ever returns false. (We may end up removing _PyFrame_OpAlreadyRan() instead.)

@markshannon, could you verify that the eval loop can't be interrupted before all MAKE_CELL instructions have executed?

@ericsnowcurrently
Copy link
Member

Regardless, it would make sense to add a test that does something like the code I posted above.

@iritkatriel
Copy link
Member Author

There could perhaps also be a pure C example.

@ericsnowcurrently
Copy link
Member

Yeah, and a pure-python test that uses a tracing hook to interrupt the code as early as possible. I suppose the same is true for a test that triggers the eval breaker as early as possible.

@iritkatriel iritkatriel changed the title Missing tests for _PyFrame_OpAlreadyRan Replace _PyFrame_OpAlreadyRan by a check for incomplete frame Sep 15, 2023
@iritkatriel
Copy link
Member Author

Changed the title to reflect the decision in the discussion beginning with faster-cpython/ideas#623 (comment).

@iritkatriel iritkatriel removed the tests Tests in the Lib/test dir label May 20, 2024
iritkatriel added a commit to iritkatriel/cpython that referenced this issue May 20, 2024
…ame is complete. Pretend that cells are not set in an incomplete frame
@iritkatriel
Copy link
Member Author

@markshannon points out that this frame cannot be incomplete because it comes from a PyFrameObject (in PyFrame_GetVar). So we will just replace the check by an assertion.

iritkatriel added a commit to iritkatriel/cpython that referenced this issue May 21, 2024
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
2 participants