Repository navigation
GH-128534: Fix behavior of branch monitoring for async for - #130847
markshannon merged 10 commits into
Conversation
Sorry, something went wrong.
… source and are included in co_branches
| @@ -0,0 +1,2 @@ | |||
| Ensure that both and left branches have same source for ``async for`` loops. | |||
There was a problem hiding this comment.
grammar
Sorry, something went wrong.
|
|
||
|
|
||
| // The offset (in code units) of the END_SEND from the SEND in the `yield from` sequence. | ||
| #define END_SEND_OFFSET 5 |
There was a problem hiding this comment.
What is this 5? Is it related to the size of the RESUME and JUMP instructions between the YIELD and END_SEND?
Sorry, something went wrong.
There was a problem hiding this comment.
It's the distance from the SEND to the END_SEND in the instructions code units in the yield from/await sequence
Sorry, something went wrong.
There was a problem hiding this comment.
Can we assert that the instruction at that distance is a SEND?
Sorry, something went wrong.
| instr->i_oparg = target->i_offset; | ||
| if (instr->i_oparg < offset) { | ||
| if (instr->i_opcode == END_ASYNC_FOR) { | ||
| // Monitoring needs the target to be the END_SEND |
There was a problem hiding this comment.
I think this comment can be made clearer. We are not setting the target here, so I think what the comment is saying is "since the target is the end_send, we set the offset differently in this case". Right?
Sorry, something went wrong.
| } | ||
| maxdepth = Py_MAX(maxdepth, depth + effects.max); | ||
| if (HAS_TARGET(instr->i_opcode)) { | ||
| if (HAS_TARGET(instr->i_opcode) && instr->i_opcode != END_ASYNC_FOR) { |
There was a problem hiding this comment.
Are you skipping END_ASYNC_FOR here because its target is not instr->i_target? Apart from that the assertion should hold.
Sorry, something went wrong.
There was a problem hiding this comment.
Its skipped because there is no flow control edge from the END_ASYNC_FOR to its "target". The "target" is where the control flow comes from, which makes sense for co_branches but not otherwise.
Sorry, something went wrong.
This PR: