bpo-46841: Don't jump during throw()

[brandtbucher]

We currently perform some awkward control flow in _gen_throw to handle the case where a yield from/await sub-iterator returns a value during a throw() call.

The current code is quite sensitive to slight changes in the bytecode, and is technically invalid for three reasons:

• It doesn't properly handle the case where SEND has an EXTENDED_ARG.
• It scans backwards, which is susceptible to accidentally reading inline cache entries as if they were valid instructions.
• It jumps from a YIELD_VALUE instruction, which isn't really supposed to be possible.

Instead, this PR handles this edge-case by pushing NULL and the return value in place of the sub-iterator and its next value. SEND then handles this by adjusting the stack and jumping the same way it does when handling other return values.

This also updates the docs for SEND (which are currently lacking a bit).

https://bugs.python.org/issue46841

[markshannon]

I'm not sure that this is worthwhile, at least not in this form.

The current approach is very fragile, as you rightly point out. However, this PR moves code from a cold path generator.throw() onto the hot path of a yield from.

Although the points you raise are valid, they all safe for now (although they probably won't be in future).

It doesn't properly handle the case where SEND has an EXTENDED_ARG.

Until the compiler freely re-orders blocks, the oparg will only ever be very small as it only jumps over a couple of instructions.

It scans backwards, which is susceptible to accidentally reading inline cache entries as if they were valid instructions.

Again, it's OK because we know what instructions we are scanning over.

It jumps from a YIELD_VALUE instruction, which isn't really supposed to be possible.

Technically, it steps back to the SEND and then jumps.

---

IMO, the "proper" fix for this is to wrap the YIELD_VALUE after the SEND in a try-except that throws the exception into the inner generator, as the only way a YIELD_VALUE can raise is in a throw().

Now:

start:
    SEND end
    YIELD_VALUE
    RESUME
    JUMP_NO_INTERRUPT start
handler:
    THROW_INTO # Maybe a call to an intrinsic function: CALL_INTRINSIC "throw_into", 1
end:

With hidden try-except:

start:
    SEND end
    SETUP_FINALLY handler
    YIELD_VALUE
    POP_BLOCK
    RESUME
    JUMP_NO_INTERRUPT start
handler:
    THROW_INTO # Throws the exception into the sub-generator.
end:

This was part of the motivation for splitting out SEND in the first place.

[brandtbucher]

Technically, it steps back to the SEND and then jumps.

This seems unhelpfully pedantic.

IMO, the "proper" fix for this is to wrap the YIELD_VALUE after the SEND in a try-except that throws the exception into the inner generator, as the only way a YIELD_VALUE can raise is in a throw().

I don’t think I agree (at least not yet). It's not too clear how your suggestion would work, so just to make sure my understanding is correct:

• _gen_throw will set the current exception to whatever we're throwing, then call PyEval_EvalFrameEx(gen_frame, /*throwflag=*/1).
• This will pop TOS (which is the last thing we yielded, and we don't care about anymore) and jump to THROW_INTO, which performs the equivalent of unsetting the current exception and calling TOS.throw(exc_type, exc_value, exc_traceback).
• The above steps keep happening all the way down the chain.
• When one of the calls returns, THROW_INTO will handle three separate cases:
  • The throw() call raised an exception:
    • It's a StopIteration, so SET_TOP(return_value) and start the next instruction?
    • It's something else, so goto error;.
  • It yielded the next value, so... push the new value, and jump back into the yield from/await loop?

I'm not sure if I got that right, since a lot of this code is new to me. If so, it seems much, much more complex and expensive than what I've proposed here: clearing the sub-iterator when it returns and adding a single, easily-predictable NULL check to SEND.

[markshannon]

I hadn't considered the special case for StopIteration. THROW_INTO shouldn't have to do any more work than SEND.

If THROW_INTO were the same as SEND, except that it calls TOS.throw() instead of TOS.send(), then the following should work:

start:
    SEND end
    SETUP_FINALLY handler
    YIELD_VALUE
    POP_BLOCK
    RESUME
    JUMP_NO_INTERRUPT start
handler:
    THROW_INTO end
    RESUME
    JUMP_NO_INTERRUPT start
end:

[markshannon]

Technically, it steps back to the SEND and then jumps.

This seems unhelpfully pedantic.

No, this is an important detail.
Because it means that we know the step backwards is correct, for the reasons already given, and that we are jumping from an instruction that is a jump, so that is also OK.

[brandtbucher]

With the beta freeze looming, I strongly feel that we should go with this approach (at least for 3.11). It's a working, very minimal change to the existing code that helps other projects (like JITs, PEP 523 hooks, etc.) that need this logic to be moved back into the bytecode. The additional branch is cheap, easily predictable, and fits naturally with the existing code.

For reference, I've now tried two other approaches with virtual try/excepts. Both are considerably more complicated, and add a lot of additional instructions and logic to each yield from/await:

Attempt #1 uses a new THROW opcode in a virtual try/except. It still fails the test suite with the following error that I've spent days trying to debug:

======================================================================
ERROR: test_anext_iter (test.test_asyncgen.AsyncGenAsyncioTest.test_anext_iter) [pure-Python anext()]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/brandtbucher/cpython/Lib/test/test_asyncgen.py", line 735, in run_test
    test(py_anext)
    ^^^^^^^^^^^^^^
  File "/home/brandtbucher/cpython/Lib/test/test_asyncgen.py", line 674, in test3
    g.close()
    ^^^^^^^^^
  File "/home/brandtbucher/cpython/Lib/test/test_asyncgen.py", line 78, in anext_impl
    return await __anext__(iterator)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
RuntimeError: cannot reuse already awaited __anext__()/asend()

----------------------------------------------------------------------

I'm also not confident that the rest of the logic is 100% correct.

Attempt #2 uses a more complicated virtual try/except that catches StopIteration and extracts its value member. It does pass all tests, though.

I'll leave the decision up to you, @markshannon.

[markshannon]

Well, this didn't make it into 3.11.
For the best, IMO. The current implementation might be clunky, but it does work. Best not to rush in last minute changes.

Approach 2 passes the tests, and has fewer changes to the instruction set. That seems better than approach 1.
@brandtbucher Which do you prefer?

One other question.
How much of https://peps.python.org/pep-0380/#formal-semantics should we move into the compiler, and how much belongs in helper C code?

[brandtbucher]

Closing in favor of #96010.