Do we need the _PyFrame_OpAlreadyRan check in frameobject.c?

[iritkatriel]

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 https://github.com/python/cpython/commit/631f9938b1604d4f893417ec339b9e0fa9196fb1).

No tests fail if I replace _PyFrame_OpAlreadyRan(...) by true, and preceded by

    if (PyCell_Check(value)) {
        assert(_PyFrame_OpAlreadyRan(frame, MAKE_CELL, i));
    }

From inspecting the code, there are only two places where a PyCell can be created: in MAKE_CELL, and from Python via types.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 check PyCell_Check(value) and remove the _PyFrame_OpAlreadyRan function?

[ericsnowcurrently]

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
  • https://github.com/python/cpython/pull/26396/commits/362088d6f208c99305f5e7693e975c4158e3bd47 - actually adds _PyFrame_OpAlreadyRan()
• gh-26587 - earlier opportunity to drop _PyFrame_OpAlreadyRan() (?)
  • https://github.com/python/cpython/pull/26587/commits/0e1cbc48c38becd8b13ad26fc3e956d64729e186 - stops using _PyFrame_OpAlreadyRan() in no-arg super()
  • https://github.com/python/cpython/pull/26587/commits/bdb12a79eb1a6655642b07ec6daf3c2d533a73cf - 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](https://github.com/python/cpython/pull/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.
https://github.com/python/cpython/pull/26396/commits/362088d6f208c99305f5e7693e975c4158e3bd47 (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 (https://github.com/python/cpython/pull/26587/commits/0e1cbc48c38becd8b13ad26fc3e956d64729e186).

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

[iritkatriel]

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

[markshannon]

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.

[ericsnowcurrently]

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[^1]) 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.

[^1]: A cell object could be set externally in the fastlocals array with PyFrame_LocalsToFast(), etc. FWIU, there really isn't any other way.

[gvanrossum]

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.

[ericsnowcurrently]

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.

[gvanrossum]

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