-
Notifications
You must be signed in to change notification settings - Fork 49
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
Do we need the _PyFrame_OpAlreadyRan check in frameobject.c? #623
Comments
tl;dr We probably could drop As to tests, at the bottom here I've outlined the cases where things would get weird. I don't think it would crash though. I do expect we have a test coverage gap relative to ContextRelevant public API:
The following are changes are relevant:
There are some essential points to keep in mind here:
(expand for details about how `_PyFrame_OpAlreadyRan()` was/is used)
Note that, since gh-26396, we have changed the machinery and data structures for frames, and refactored the code related to Why
|
Thanks @ericsnowcurrently , that's helpful. I think we need to add tests covering those edge cases. I'll create an issue on cpython repo for that. I am also wondering if there is a more direct way to check whether the Cell came from a MAKE_CELL, than to look at the instruction pointer. We could, for instance, have a field on the cell to save the version of the function whose MAKE_CELL created it. I would also consider renaming this function to makecell_already_ran, and make it specific to MAKE_CELL. I don't think we can trust it in general, for other opcodes, given the instruction pointer can be moved around in the debugger almost arbitrarily (I don't think it can be moved to before the MAKE_CELLs though). |
I'm fairly sure we can just remove It is always true if the frame is complete and |
The following directly or indirectly depend on
I could only think of two realistic ways that the eval loop could be interrupted to run external code, such that one of the above might be invoked:
The former won't happen before all the Footnotes
|
Thanks, Eric! It makes sense that we should just check whether the frame is complete. I'm not sure an assert would be wise -- in a no-GIL world C code inspecting another thread's frame stack using internal APIs (like some debugging tool) could potentially trip such an assert. It might be safer to just pretend a cell variable is not set at all when we ask for its value while the frame is incomplete. "Not set" should always be a valid outcome for these APIs. |
So before |
Only if it's "problematic", i.e. if the kind is A lesser-known line of the Zen of Python applies: "In the face of ambiguity, refuse the temptation to guess." |
There are two places in frameobject.c where there is a check like:
if (PyCell_Check(value) && _PyFrame_OpAlreadyRan(f, MAKE_CELL, i))
(added by @ericsnowcurrently in python/cpython@631f993).
No tests fail if I replace
_PyFrame_OpAlreadyRan(...)
bytrue
, and preceded byFrom inspecting the code, there are only two places where a
PyCell
can be created: inMAKE_CELL
, and from Python viatypes.CellType()
. Is the check intended to catch cells created from Python in this way? If so, can be write a test for it? Otherwise, can we just checkPyCell_Check(value)
and remove the_PyFrame_OpAlreadyRan
function?The text was updated successfully, but these errors were encountered: