Certain sys.monitoring "not taken" branches in a `for` loop not showing correctly

[jaltmayerpizzorno]

Bug report

Bug description:

With the code from PR 122564, the destinations of the "not taken" branches out of lines 2 and 3 show as going out of scope, rather than the correct line 6. Additionally, the ("taken") branch into if's block also shows as going out of scope:

def foo():
 for v in [1, 2]:
 if v > 0:
 x += v

 return x

Results:

 1 0 RESUME 0

 2 2 LOAD_CONST 1 ((1, 2))
 4 GET_ITER
 L1: 6 FOR_ITER 18 (to L3)
 10 NOT_TAKEN
 12 STORE_FAST 0 (v)

 3 14 LOAD_FAST 0 (v)
 16 LOAD_CONST 2 (0)
 18 COMPARE_OP 148 (bool(>))
 22 POP_JUMP_IF_TRUE 2 (to L2)
 26 JUMP_BACKWARD 12 (to L1)
 L2: 30 NOT_TAKEN

 4 32 LOAD_FAST_CHECK 1 (x)
 34 LOAD_FAST 0 (v)
 36 BINARY_OP 13 (+=)
 40 STORE_FAST 1 (x)
 42 JUMP_BACKWARD 20 (to L1)

 2 L3: 46 END_FOR
 48 POP_TOP

 6 50 LOAD_FAST_CHECK 1 (x)
 52 RETURN_VALUE
'foo' branches:
 ex10.py 2:13-2:19 "[1, 2]" (foo@6) -> 2:8-2:9 "v" (foo@12) [for -> block]
 ex10.py 2:13-2:19 "[1, 2]" (foo@6) -> 2:13-2:19 "[1, 2]" (foo@46) [for -> out]
 ex10.py 3:11-3:16 "v > 0" (foo@22) -> 3:11-3:16 "v > 0" (foo@28) [if/while -> out]
 ex10.py 3:11-3:16 "v > 0" (foo@22) -> 3:11-3:16 "v > 0" (foo@30) [if/while -> out]

@markshannon @nedbat

CPython versions tested on:

CPython main branch

Operating systems tested on:

macOS

[markshannon]

There's a few things going on here.
First:
The jump in if v > 0: is miscompiled to

 22 POP_JUMP_IF_TRUE 2 (to L2)
 26 JUMP_BACKWARD 12 (to L1)
 L2: 30 NOT_TAKEN

Conceptually the this is a single instruction: POP_JUMP_IF_TRUE_BACKWARD L1.
We only have one backward jump, JUMP_BACKWARD, so the assembler converts POP_JUMP_IF_TRUE_BACKWARD label
to

 POP_JUMP_IF_TRUE over
 JUMP_BACKWARD label
over:

We need to account for NOT_TAKEN here (which probably means emitting NOT_TAKEN in the assembler).
Even then, the location of the instruction following NOT_TAKEN is likely not the location of label, but that of POP_JUMP_IF_TRUE.

We don't want to prevent the compiler from generating efficient code, so we needs a means to handle these complex cases, so we'll need to handle this sys.monitoring somehow.

Since this isn't bug until we add the new branch events, I'm marking this as a feature request.

[jaltmayerpizzorno]

Updating the disassembly for the current code in pull/122564; the branches around the if are still unclear.

 1 0 RESUME 0

 2 2 LOAD_CONST 1 (0)
 4 STORE_FAST 0 (x)

 3 6 LOAD_CONST 2 ((1, 2))
 8 GET_ITER
 L1: 10 FOR_ITER 18 (to L3)
 14 NOT_TAKEN
 16 STORE_FAST 1 (v)

 4 18 LOAD_FAST 1 (v)
 20 LOAD_CONST 1 (0)
 22 COMPARE_OP 148 (bool(>))
 26 POP_JUMP_IF_TRUE 3 (to L2)
 30 NOT_TAKEN
 32 JUMP_BACKWARD 13 (to L1)

 -- L2: 36 NOP

 5 38 LOAD_FAST_LOAD_FAST 1 (x, v)
 40 BINARY_OP 13 (+=)
 44 STORE_FAST 0 (x)
 46 JUMP_BACKWARD 20 (to L1)

 3 L3: 50 END_FOR
 52 POP_TOP

 7 54 LOAD_FAST 0 (x)
 56 RETURN_VALUE

'foo' branches:
 ex10.py 3:13-3:19 "[1, 2]" (foo@10) -> 3:8-3:9 "v" (foo@16) [for -> block]
 ex10.py 3:13-3:19 "[1, 2]" (foo@10) -> 3:13-3:19 "[1, 2]" (foo@50) [for -> out]
 ex10.py 4:11-4:16 "v > 0" (foo@26) -> 4:11-4:16 "v > 0" (foo@32) [if/while -> out]
 ex10.py 4:11-4:16 "v > 0" (foo@26) -> (start_line=None) (foo@36) [(dst.start_line=None)]

[markshannon]

Latest disassembly (with offsets) on main:

 1 0 RESUME 0

 2 2 LOAD_CONST 0 ((1, 2))
 4 GET_ITER
 L1: 6 FOR_ITER 19 (to L3)
 10 NOT_TAKEN
 12 STORE_FAST 0 (v)

 3 14 LOAD_FAST 0 (v)
 16 LOAD_SMALL_INT 0
 18 COMPARE_OP 148 (bool(>))
 22 POP_JUMP_IF_TRUE 3 (to L2)
 26 NOT_TAKEN
 28 JUMP_BACKWARD 13 (to L1)

 -- L2: 32 NOP

 4 34 LOAD_FAST_CHECK 1 (x)
 36 LOAD_FAST 0 (v)
 38 BINARY_OP 13 (+=)
 42 STORE_FAST 1 (x)
 44 JUMP_BACKWARD 21 (to L1)

 2 L3: 48 END_FOR
 50 POP_TOP

 6 52 LOAD_FAST_CHECK 1 (x)
 54 RETURN_VALUE

I don't know why the NOP at label L2 is left in, rather than being merged into the following line, but that doesn't make the generated code wrong.

The branch from 22 to L2 (32) jumps to an instruction with no line number, but execution will immediately proceed to 34 (line 4).
The "not taken" branch can likewise be followed from 26 to 28 and back to L1 (offset 6) on line 2.
In other words the "taken" branch goes from line 3 to 4 and the "not taken" branch goes from line 3 to line 2.

[markshannon]

On latest main the disassembly now looks like this:

 1 0 RESUME 0

 2 2 LOAD_CONST 0 ((1, 2))
 4 GET_ITER
 L1: 6 FOR_ITER 17 (to L3)
 10 STORE_FAST 0 (v)

 3 12 LOAD_FAST 0 (v)
 14 LOAD_SMALL_INT 0
 16 COMPARE_OP 148 (bool(>))
 20 POP_JUMP_IF_TRUE 3 (to L2)
 24 NOT_TAKEN
 26 JUMP_BACKWARD 12 (to L1)

 4 L2: 30 LOAD_FAST_CHECK 1 (x)
 32 LOAD_FAST 0 (v)
 34 BINARY_OP 13 (+=)
 38 STORE_FAST 1 (x)
 40 JUMP_BACKWARD 19 (to L1)

 2 L3: 44 END_FOR
 46 POP_ITER

 6 48 LOAD_FAST_CHECK 1 (x)
 50 RETURN_VALUE

And the branches:

>>> list(foo.__code__.co_branches())
[(6, 10, 48), (20, 26, 30)]

which, to me, looks correct.
@jaltmayerpizzorno does this work for you?

[jaltmayerpizzorno]

Hi @markshannon, thank you. That 20->26 branch maps the same location in source and destination,

3:11-3:16 "v > 0" (foo@20) -> 3:11-3:16 "v > 0" (foo@26)

which I think we previously said means an exit from the function?

Anyway, I have been increasingly thinking that we may be asking for too much... we could maybe just report on the number of branches taken/existing per line, and give up on filtering out clause branches. @nedbat, what do you think?

[jaltmayerpizzorno]

BTW, I missed initializing x in the example:

def foo():
 x = 0
 for v in [1, 2]:
 if v > 0:
 x += v

 return x

If we continue on this, let's please use this version instead.

[nedbat]

give up on filtering out clause branches.

I don't know what this means. It feels like we are close to getting the data we want.

[jaltmayerpizzorno]

give up on filtering out clause branches.

I don't know what this means. It feels like we are close to getting the data we want.

I mean filtering out the branches such as the one between the A and B in if A and B:.