bpo-46841: Don't jump during `throw()` by brandtbucher · Pull Request #31968 · python/cpython
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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
SENDhas anEXTENDED_ARG. - It scans backwards, which is susceptible to accidentally reading inline cache entries as if they were valid instructions.
- It jumps from a
YIELD_VALUEinstruction, 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).
This was referenced
Mar 17, 2022I'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.
Technically, it steps back to the
SENDand then jumps.
This seems unhelpfully pedantic.
IMO, the "proper" fix for this is to wrap the
YIELD_VALUEafter theSENDin a try-except that throws the exception into the inner generator, as the only way aYIELD_VALUEcan raise is in athrow().
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_throwwill set the current exception to whatever we're throwing, then callPyEval_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 callingTOS.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_INTOwill handle three separate cases:- The
throw()call raised an exception:- It's a
StopIteration, soSET_TOP(return_value)and start the next instruction? - It's something else, so
goto error;.
- It's a
- It yielded the next value, so... push the new value, and jump back into the
yield from/awaitloop?
- The
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.
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:
Technically, it steps back to the
SENDand 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.
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.
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?