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:.