GH-128534: Fix behavior of branch monitoring for async for#130847
GH-128534: Fix behavior of branch monitoring for async for#130847markshannon merged 10 commits intopython:mainfrom
async for#130847Conversation
… source and are included in co_branches
markshannon
requested review from
ericsnowcurrently and
iritkatriel
as code owners
| @@ -0,0 +1,2 @@ | |||
| Ensure that both and left branches have same source for ``async for`` loops. | |||
|
|
||
|
|
||
| // 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?
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
There was a problem hiding this comment.
Can we assert that the instruction at that distance is a SEND?
Python/assemble.c
Outdated
| 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?
| } | ||
| 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.
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.
2 participants
This PR:
co_branches.async for#128534