Do we need the _PyFrame_OpAlreadyRan check in frameobject.c? #623

Activity

1. ericsnowcurrently commented on Sep 8, 2023

   

   ericsnowcurrently

   on Sep 8, 2023

   Collaborator

   More actions

   tl;dr We probably could drop _PyFrame_OpAlreadyRan(), but it would be regression in correctness, though the faulty corner cases seem quite unlikely. I'd tend toward leaving things as they are.

   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 _PyFrame_OpAlreadyRan().

   Context

   Relevant public API:

   • PyFrame_FastToLocals()
   • PyFrame_FastToLocalsWithError()
   • PyEval_GetLocals()
   • PyFrame_GetVar()
   • PyFrame_GetVarString()
   • PyFrame_LocalsToFast()
   • no-arg super()

   The following are changes are relevant:

   • gh-26396 - adds MAKE_CELL and interleaves the fastlocals
     • python/cpython@362088d - actually adds _PyFrame_OpAlreadyRan()
   • gh-26587 - earlier opportunity to drop _PyFrame_OpAlreadyRan() (?)
     • python/cpython@0e1cbc4 - stops using _PyFrame_OpAlreadyRan() in no-arg super()
     • python/cpython@bdb12a7 - simplifies the comment from the previous commit

   There are some essential points to keep in mind here:

   • before gh-26396, cells were always created (and populated for args) before the eval loop started executing the function
   • that PR introduced the MAKE_CELL instructions and the CO_FAST_CELL entry type for the fastlocals array
   • each CO_FAST_CELL entry in the fastlocals array is always set via a distinct MAKE_CELL instruction
   • (basically) no other instructions execute before all the MAKE_CELL instructions have run
   • MAKE_CELL takes the existing value in the fastlocals array entry (usually NULL) and replaces it with a new cell object, initialized to the original value
   • if that original value was a cell then it is stored in the new cell unchanged, like any other object would be; MAKE_CELL always creates a new cell and initializes it with the old value, even if a cell
   • there are (were?) only 2 ways a CO_FAST_CELL entry in the fastlocals array may be non-NULL before the corresponding MAKE_CELL instruction:
     1. set by PyFrame_LocalsToFast() (after the eval loop starts but before the corresponding MAKE_CELL)
     2. an arg
   • after an entry's MAKE_CELL executes, that entry not be modified again; only the value in that entry's cell will be modified (PyFrame_LocalsToFast() respects this constraint)
   • there are only 2 places the value in a CO_FAST_CELL entry in the fastlocals array is read directly before its MAKE_CELL executes:
     1. PyFrame_FastToLocals()
     2. no-arg super()
   • once that MAKE_CELL runs, only the value in the cell at that entry is used, never the cell itself (both of the above respect this constraint)
   • the only other place that fastlocals array entry is used is in the eval loop, and only after all MAKE_CELL instructions have run (hence only the value in the cell is read/written)

   (expand for details about how `_PyFrame_OpAlreadyRan()` was/is used)

   • PyFrame_FastToLocals() (used by the call trampoline and for INTRINSIC_IMPORT_STAR)
     • when fastlocals array entry kind is CO_FAST_CELL
       • if entry value is non-NULL
         • if the value is a cell object
           • if the MAKE_CELL instruction already ran
             • use the value held by the cell (ignore it if NULL)
           • else use the value as-is (passed in as an arg or set via PyFrame_LocalsToFast())
         • else use the value as-is (passed in as an arg or set via PyFrame_LocalsToFast())
       • else ignore the entry
   • PyFrame_LocalsToFast() (used by the call trampoline and for INTRINSIC_IMPORT_STAR)
     • when fastlocals array entry kind is CO_FAST_CELL
       • if entry value is non-NULL
         • if that value is a cell object
           • if the MAKE_CELL instruction already ran
             • replace the value in the cell
           • else replace the value in the fastlocals entry (passed in as an arg or set via PyFrame_LocalsToFast())
         • else replace the value in the fastlocals entry (passed in as an arg or set via PyFrame_LocalsToFast())
       • else ignore the entry
   • no-arg super() calls (no longer happens, as of gh-26587)
     • if the first fastlocals array entry is a cell
       • if the MAKE_CELL instruction already ran
         • use the value held by the cell for self (raise if NULL)
       • else raise
     • else raise

   Note that, since gh-26396, we have changed the machinery and data structures for frames, and refactored the code related to PyFrame_FastToLocals(). We've also added a few new related C-API functions, which involved factoring out the code using _PyFrame_OpAlreadyRan() into a helper function.

   Why _PyFrame_OpAlreadyRan()?

   With all of that in mind, we needed a way to tell if MAKE_CELL had run for a fastlocals array entry yet, but really only when the current value of the entry was a cell object. That's the exceptional case--where someone passed a cell to a function where that arg was itself was meant to be a cell (or likewise set a cell using PyFrame_LocalsToFast() before MAKE_CELL ran).

   To deal with this, we would need to know in PyFrame_FastToLocals() and no-arg super() if we should use the cell itself or use the value it's holding. Likewise we would need know in PyFrame_LocalsToFast() if we should replace the entry's current value (a cell) with the new value or put the new value into the cell.

   _PyFrame_OpAlreadyRan() was more or less a solution for dealing with this exceptional case.
   python/cpython@362088d (from gh-26396) demonstrates the connection.

   (FYI, gh-26396 left the code in a situation where every (?) cell var would have two entries in the fastlocals array (the second of them was never used). This was fixed a week later in gh-26587. That situation and the fix are certainly related to the fundamental motivation to _PyFrame_OpAlreadyRan(). However, it was a separate matter and the fix did not impact the state of things relative to _PyFrame_OpAlreadyRan().)

   (I'll also note that, since I added _PyFrame_OpAlreadyRan(), we have changed the conditions under which the "eval breaker" gets checked, which is a mechanism by which the eval loop can be interrupted separately from tracing/debugging. Currently there isn't any way for the eval breaker to lead to the exceptional situation.)

   Drop _PyFrame_OpAlreadyRan()?

   The question at hand is, do we still need _PyFrame_OpAlreadyRan()?

   In retrospect, we didn't really need _PyFrame_OpAlreadyRan() for no-arg super(). It's highly unlikely that anyone would pass a cell in as the self argument, especially since it is passed implicitly for instance methods. It's also fairly unlikely that anyone would replace self via PyFrame_LocalsToFast() while debugging/tracing before the first instruction (MAKE_CELL for the self entry, which is always the first one). Consequently, we actually stopped using _PyFrame_OpAlreadyRan() for no-arg super() in gh-26587 (python/cpython@0e1cbc4).

   (@markshannon pointed out in a review comment that we should drop _PyFrame_OpAlreadyRan(), but I'm pretty sure at this point that he was referring to no longer exporting the symbol, which I did fix in that PR.)

   As to our use of _PyFrame_OpAlreadyRan() in PyFrame_LocalsToFast() and PyFrame_FastToLocals() (and now PyEval_GetLocals(), PyFrame_GetVar(), etc.), I'm still not sure. 🤪 It depends on the likelihood of the following happening:

   • a cell ends up in a CO_FAST_CELL entry in the fastlocals array
     • someone passes a cell object to a function where that arg variable is itself a cell
     • someone sets a cell via PyFrame_LocalsToFast() in the short interval between which the eval loop starts and MAKE_CELL executes
   • the distinction actually maters to someone

   The likelihood of the PyFrame_LocalsToFast() case seems fairly remote, but I'm not certain about the arg case (though it doesn't seem super likely that people are writing functions that take a cell object as an arg which is then used in an inner function).
2. iritkatriel commented on Sep 9, 2023

   

   iritkatriel

   on Sep 9, 2023

   CollaboratorAuthor

   More actions

   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).

   👍React with 👍1Reacted by Eric Snow
3. markshannon commented on Sep 12, 2023

   

   markshannon

   on Sep 12, 2023

   Member

   More actions

   I'm fairly sure we can just remove _PyFrame_OpAlreadyRan().

   It is always true if the frame is complete and _PyFrame_OpAlreadyRan() cannot be called if the frame is incomplete.
4. ericsnowcurrently commented on Sep 13, 2023

   

   ericsnowcurrently

   on Sep 13, 2023

   Collaborator

   More actions

   The following directly or indirectly depend on _PyFrame_OpAlreadyRan() to know if a cell object in the fastlocals array was an arg (or set externally¹) vs. set by MAKE_CELL:

   • PyFrame_FastToLocals()
   • PyFrame_FastToLocalsWithError()
   • PyFrame_GetVar()
   • PyFrame_GetVarString()
   • PyEval_GetLocals()
   • builtins.locals() (AKA _PyEval_GetFrameLocals())
   • _PyFrame_FastToLocalsWithError()
   • _PyFrame_GetLocals()
   • (setter) PyFrame_LocalsToFast()
   • (setter) _PyFrame_LocalsToFast()

   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:

   • eval breaker
   • tracing

   The former won't happen before all the MAKE_CELL instructions execute, due to the limited places we check (loops, calls). It sounds like the latter won't be a problem because tracing doesn't start until RESUME, which always follows all the MAKE_CELL instructions. It may be worth having asserts, to replace _PyFrame_OpAlreadyRan(), that verify the frame is already "complete" in each case.

   Footnotes

   1. A cell object could be set externally in the fastlocals array with PyFrame_LocalsToFast(), etc. FWIU, there really isn't any other way. ↩
5. gvanrossum commented on Sep 14, 2023

   

   gvanrossum

   on Sep 14, 2023

   Collaborator

   More actions

   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.

   👍React with 👍1Reacted by Eric Snow
6. ericsnowcurrently commented on Sep 15, 2023

   

   ericsnowcurrently

   on Sep 15, 2023

   Collaborator

   More actions

   So before MAKE_CELL runs, the corresponding fastlocals entry is considered "Not set", even if populated with an initial value? I'm fine with that.
7. gvanrossum commented on Sep 15, 2023

   

   gvanrossum

   on Sep 15, 2023

   Collaborator

   More actions

   So before MAKE_CELL runs, the corresponding fastlocals entry is considered "Not set", even if populated with an initial value? I'm fine with that.

   Only if it's "problematic", i.e. if the kind is CO_FAST_CELL and the value is indeed a cell (since that's the only case where _PyFrame_OpAlreadyRan() is needed to disambiguate).

   A lesser-known line of the Zen of Python applies: "In the face of ambiguity, refuse the temptation to guess."

   👍React with 👍1Reacted by Eric Snow
8. iritkatriel

   closed this as completedon May 20, 2024