GH-128534: Fix behavior of branch monitoring for async for
#130847
Conversation
… 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. | |||
| @@ -632,6 +632,10 @@ makecode(_PyCompile_CodeUnitMetadata *umd, struct assembler *a, PyObject *const_ | |||
| return co; | |||
| } | |||
|
|
|||
|
|
|||
| // 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
| @@ -670,7 +674,11 @@ resolve_jump_offsets(instr_sequence *instrs) | |||
| if (OPCODE_HAS_JUMP(instr->i_opcode)) { | |||
| instruction *target = &instrs->s_instrs[instr->i_target]; | |||
| 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?
| @@ -849,7 +849,7 @@ calculate_stackdepth(cfg_builder *g) | |||
| goto error; | |||
| } | |||
| 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.
This PR:
co_branches.async for#128534