Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot perform frame jump after handled exception #92228

Closed
dkrystki opened this issue May 3, 2022 · 32 comments
Closed

Cannot perform frame jump after handled exception #92228

dkrystki opened this issue May 3, 2022 · 32 comments
Assignees
Labels
3.11 only security fixes 3.12 bugs and security fixes release-blocker type-bug An unexpected behavior, bug, or error

Comments

@dkrystki
Copy link

dkrystki commented May 3, 2022

For given code:

def fun():
    a = 1  # <---- jump location

    try:
        b = 1 / 0
    except ZeroDivisionError as e:
        pass

    c = 3  # current location


if __name__ == "__main__":
    fun()

Jumping from current location to jump location yields ValueError: can't jump from within an exception handler

Your environment

  • CPython versions tested on: 3.11.0a7
  • Operating system and architecture: Linux q 5.4.0-42-generic x64

Works as expected for python < 3.11

@dkrystki dkrystki added the type-bug An unexpected behavior, bug, or error label May 3, 2022
@serhiy-storchaka
Copy link
Member

@markshannon

@markshannon
Copy link
Member

@dkrystki Do you have a full reproducer?
You haven't said how you are performing the jump.

@dkrystki
Copy link
Author

dkrystki commented May 4, 2022

For the presented example I performed jump using PyCharm.
Not sure if I know how to reproduce it using settrace (I can try) but under the hood PyCharm is simply setting f_lineno to perform a jump.

Edit: For my library purposes I am setting f_lineno manually and it is causing the same issue

@dkrystki
Copy link
Author

dkrystki commented May 4, 2022

I managed to make a full reproducer:

import sys

def fun():
    a = 1  # <---- jump location

    try:
        b = 1 / 0
    except ZeroDivisionError as e:
        pass

    c = 3  # current location


def trace(frame, event, arg):
    if event == "line" and frame.f_lineno == 11:
        frame.f_lineno = 4

    return trace


sys.settrace(trace)

if __name__ == "__main__":
    fun()

Traceback:

Traceback (most recent call last):
  File "<cwd>/cakeshop.py", line 24, in <module>
    fun()
    ^^^^^
  File "<cwd>/cakeshop.py", line 11, in fun
    c = 3  # current location
        ^
  File "<cwd>/cakeshop.py", line 11, in fun
    c = 3  # current location
        ^
  File "<cwd>/cakeshop.py", line 16, in trace
    frame.f_lineno = 4
    ^^^^^^^^^^^^^^
ValueError: can't jump from within an exception handler

@serhiy-storchaka
Copy link
Member

diff --git a/Lib/test/test_sys_settrace.py b/Lib/test/test_sys_settrace.py
index b1c8f6f80a..34126464bc 100644
--- a/Lib/test/test_sys_settrace.py
+++ b/Lib/test/test_sys_settrace.py
@@ -2217,6 +2217,16 @@ def test_jump_backwards_into_try_except_block(output):
             raise
         output.append(6)
 
+    @jump_test(7, 1, [1, 3, 6, 1, 3, 6])
+    def test_jump_backwards_over_try_except_block(output):
+        output.append(1)
+        try:
+            output.append(3)
+            1/0
+        except ZeroDivisionError:
+            output.append(6)
+        x = 7
+
     # 'except' with a variable creates an implicit finally block
     @jump_test(5, 7, [4], (ValueError, 'within'))
     def test_no_jump_between_except_blocks_2(output):

@serhiy-storchaka
Copy link
Member

All works if add anything above or below the assignment.

@markshannon markshannon self-assigned this May 9, 2022
@sweeneyde
Copy link
Member

For what it's worth, this is the problematic result from the reproducer.

Bytecode:

3           0 RESUME                   0

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

6           6 NOP

7           8 LOAD_CONST               1 (1)
           10 LOAD_CONST               2 (0)
           12 BINARY_OP               11 (/)
           16 STORE_FAST               1 (b)

11          18 LOAD_CONST               3 (3)
           20 STORE_FAST               3 (c)
           22 LOAD_CONST               0 (None)
           24 RETURN_VALUE
      >>   26 PUSH_EXC_INFO

8          28 LOAD_GLOBAL              0 (ZeroDivisionError)
           40 CHECK_EXC_MATCH
           42 POP_JUMP_FORWARD_IF_FALSE    13 (to 70)
           44 STORE_FAST               2 (e)

9          46 POP_EXCEPT
           48 LOAD_CONST               0 (None)
           50 STORE_FAST               2 (e)
           52 DELETE_FAST              2 (e)

11          54 LOAD_CONST               3 (3)
           56 STORE_FAST               3 (c)
           58 LOAD_CONST               0 (None)
           60 RETURN_VALUE
           62 LOAD_CONST               0 (None)
           64 STORE_FAST               2 (e)
           66 DELETE_FAST              2 (e)
           68 RERAISE                  1

8     >>   70 RERAISE                  0
      >>   72 COPY                     3
           74 POP_EXCEPT
           76 RERAISE                  1
ExceptionTable:
8 to 16 -> 26 [0]
26 to 44 -> 72 [1] lasti
62 to 70 -> 72 [1] lasti

Result from mark_stacks():

int64_t *stacks = {
     [0] =  0,  [1] =  0,  [2] =  3,  [3] =  0,  [4] =  0,
     [5] =  3,  [6] = 15,  [7] =  3,  [8] =  3,  [9] =  0,
    [10] =  3, [11] =  0, [12] =  3, [13] = -2, [14] = -2,
    [15] = -2, [16] = -2, [17] = -2, [18] = -2, [19] = -2,
    [20] = -2, [21] = -2, [22] = -2, [23] = -2, [24] = -2,
    [25] = -2, [26] = -2, [27] = -2, [28] = -2, [29] = -2,
    [30] = -2, [31] = -2, [32] = -2, [33] = -2, [34] = -2,
    [35] = -2, [36] = -2, [37] = -2, [38] = -2, [39] = -2,
};

Here, -2 means UNINITIALIZED. This is a result of the following code in mark_stacks:

        for (i = 0; i < len; i++) {
            int64_t next_stack = stacks[i];
            if (next_stack == UNINITIALIZED) {
                continue;
            }
            // ...
        }

After RETURN_VALUE, the rest of the stacks are UNINITIALIZED and remain UNINITIALIZED as they're skipped over. I don't know the correct solution.

BTW, bisected to adcd220

@markshannon
Copy link
Member

I'm making this a deferred blocker.
I don't think it needs to block the next beta, but I definitely want to fix it before release

@markshannon
Copy link
Member

We need to use the exception handling table to fill in the stacks for exception handlers.
Fiddly, but not too difficult.

@pablogsal
Copy link
Member

Bumping this to release-blocker

@iritkatriel
Copy link
Member

iritkatriel commented Jul 5, 2022

We need to use the exception handling table to fill in the stacks for exception handlers.
Fiddly, but not too difficult.

The issue here is actually not jumping from within an exception handler to code outside of the handler (I'm not sure we want to allow that). Rather, it's jumping between two points that are outside of the handler.

If it wasn't for the inline-small-exit-blocks optimisation, there would be a jump over the except block that mark_stacks follows. But the c = 3 line is duplicated, so the except case has its own copy, which is unreachable in normal control flow:

  1           0 RESUME                   0

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

  3           6 NOP

  4           8 LOAD_CONST               1 (1)
             10 LOAD_CONST               2 (0)
             12 BINARY_OP               11 (/)
             16 STORE_FAST               1 (b)

  7          18 LOAD_CONST               3 (3)          <==   c = 3 for no-exception case
             20 STORE_FAST               3 (c)
             22 LOAD_CONST               0 (None)
             24 RETURN_VALUE
        >>   26 PUSH_EXC_INFO

  5          28 LOAD_GLOBAL              0 (ZeroDivisionError)
             40 CHECK_EXC_MATCH
             42 POP_JUMP_FORWARD_IF_FALSE    13 (to 70)
             44 STORE_FAST               2 (e)

  6          46 POP_EXCEPT
             48 LOAD_CONST               0 (None)
             50 STORE_FAST               2 (e)
             52 DELETE_FAST              2 (e)

  7          54 LOAD_CONST               3 (3)            <==   c = 3 for handled-exception case
             56 STORE_FAST               3 (c)
             58 LOAD_CONST               0 (None)
             60 RETURN_VALUE
             62 LOAD_CONST               0 (None)
             64 STORE_FAST               2 (e)
             66 DELETE_FAST              2 (e)
             68 RERAISE                  1

  5     >>   70 RERAISE                  0
        >>   72 COPY                     3
             74 POP_EXCEPT
             76 RERAISE                  1

Thinking..

@iritkatriel
Copy link
Member

This confirms what I wrote above - the problem goes away if we make this change:

diff --git a/Python/compile.c b/Python/compile.c
index 5ae1e34520..5e56d4eb22 100644
--- a/Python/compile.c
+++ b/Python/compile.c
@@ -8978,7 +8978,7 @@ jump_thread(struct instr *inst, struct instr *target, int opcode)
 }
 
 /* Maximum size of basic block that should be copied in optimizer */
-#define MAX_COPY_SIZE 4
+#define MAX_COPY_SIZE 0
 
 /* Optimization */
 static int

@iritkatriel
Copy link
Member

All works if add anything above or below the assignment.

Indeed - then the size of the block exceeds the threshold for the optimisation.

@iritkatriel
Copy link
Member

This optimisation is very old, and I wanted to check that it is still worth having.

I ran pyperformance a few times on my Mac with and without it, and my results are that removing it actually makes python a little faster. This is not a shock if it's true - adding more code to save one jump per function is not an obvious improvement.

If this is repeatable then the fix for this issue is obvious.

Another option is to leave the optimisation for blocks without line numbers (the implicit 'return None's) but disable it for small blocks that do have line numbers. I want to get numbers from our benchmarking machine (which is not working for me at the moment for some reason) before we make a decision.

@tiran
Copy link
Member

tiran commented Jul 6, 2022

Since the optimization does not have a noticeable negative impact and in fact improves performance on your system, then it is reasonable to remove it and get rid of one more release blocker. In worst case we can reconsider the optimization for next beta or 3.12.

iritkatriel added a commit to iritkatriel/cpython that referenced this issue Jul 6, 2022
@iritkatriel
Copy link
Member

The PR is ready now. I went for the smaller change of disabling the optimisation only for blocks that have a line number.

@esc
Copy link
Contributor

esc commented Jul 6, 2022

This optimisation is very old, and I wanted to check that it is still worth having.

@iritkatriel : Just to double check, is this the optimization introduced in:

https://bugs.python.org/issue44626

From what I can tell this came with Python 3.10, or am I mistaken?

@iritkatriel
Copy link
Member

This optimisation is very old, and I wanted to check that it is still worth having.

@iritkatriel : Just to double check, is this the optimization introduced in:

https://bugs.python.org/issue44626

From what I can tell this came with Python 3.10, or am I mistaken?

No that PR just moved it around.

@iritkatriel
Copy link
Member

I've attached benchmark results to the PR at #94592 (comment).

miss-islington pushed a commit that referenced this issue Jul 7, 2022
…tion for blocks that have a line number (GH-94592)

Inlining of code that corresponds to source code lines, can make it hard to distinguish later between code which is only reachable from except handlers, and that which is reachable in normal control flow. This caused problems with the debugger's jump feature.

This PR turns off the inlining optimisation for code which has line numbers. We still inline things like the implicit "return None".
tiran pushed a commit to tiran/cpython that referenced this issue Jul 7, 2022
…ing' optimization for blocks that have a line number (pythonGH-94592)

Inlining of code that corresponds to source code lines, can make it hard to distinguish later between code which is only reachable from except handlers, and that which is reachable in normal control flow. This caused problems with the debugger's jump feature.

This PR turns off the inlining optimisation for code which has line numbers. We still inline things like the implicit "return None"..
(cherry picked from commit bde06e1)

Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
sidney pushed a commit to sidney/cpython that referenced this issue Jul 7, 2022
…timization for blocks that have a line number (pythonGH-94592)

Inlining of code that corresponds to source code lines, can make it hard to distinguish later between code which is only reachable from except handlers, and that which is reachable in normal control flow. This caused problems with the debugger's jump feature.

This PR turns off the inlining optimisation for code which has line numbers. We still inline things like the implicit "return None".
miss-islington pushed a commit that referenced this issue Jul 7, 2022
…ptimization for blocks that have a line number (GH-94592) (GH-94643)

Inlining of code that corresponds to source code lines, can make it hard to distinguish later between code which is only reachable from except handlers, and that which is reachable in normal control flow. This caused problems with the debugger's jump feature.

This PR turns off the inlining optimisation for code which has line numbers. We still inline things like the implicit "return None"..
(cherry picked from commit bde06e1)

Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@tiran tiran added the 3.12 bugs and security fixes label Jul 7, 2022
@esc
Copy link
Contributor

esc commented Jul 7, 2022

This optimisation is very old, and I wanted to check that it is still worth having.

@iritkatriel : Just to double check, is this the optimization introduced in:
https://bugs.python.org/issue44626
From what I can tell this came with Python 3.10, or am I mistaken?

No that PR just moved it around.

@iritkatriel : Thank you for answering my question. I have a followup questions about this. I will post the question here, but do let me know if this is not the appropriate place. I am asking, because I only discovered this bytecode optimization being triggered on Python 3.10. Maybe you can shed some light as to why I hadn't seen this before, e.g. on 3.9. Allow me to illustrate:

Using the following example:

def test(a):
    b = 0
    if a == 1:
        b = 3
    elif a == 2:
        b = 4
    else:
        b = 5
    return b

Using https://godbolt.org/ I get the following for 3.9

  2           0 LOAD_CONST               1 (0)
              2 STORE_FAST               1 (b)

  3           4 LOAD_FAST                0 (a)
              6 LOAD_CONST               2 (1)
              8 COMPARE_OP               2 (==)
             10 POP_JUMP_IF_FALSE       18

  4          12 LOAD_CONST               3 (3)
             14 STORE_FAST               1 (b)
             16 JUMP_FORWARD            18 (to 36)

  5     >>   18 LOAD_FAST                0 (a)
             20 LOAD_CONST               4 (2)
             22 COMPARE_OP               2 (==)
             24 POP_JUMP_IF_FALSE       32

  6          26 LOAD_CONST               5 (4)
             28 STORE_FAST               1 (b)
             30 JUMP_FORWARD             4 (to 36)

  8     >>   32 LOAD_CONST               6 (5)
             34 STORE_FAST               1 (b)

  9     >>   36 LOAD_FAST                1 (b)
             38 RETURN_VALUE

and then for 3.10:

  2           0 LOAD_CONST               1 (0)
              2 STORE_FAST               1 (b)

  3           4 LOAD_FAST                0 (a)
              6 LOAD_CONST               2 (1)
              8 COMPARE_OP               2 (==)
             10 POP_JUMP_IF_FALSE       10 (to 20)

  4          12 LOAD_CONST               3 (3)
             14 STORE_FAST               1 (b)

  9          16 LOAD_FAST                1 (b)
             18 RETURN_VALUE

  5     >>   20 LOAD_FAST                0 (a)
             22 LOAD_CONST               4 (2)
             24 COMPARE_OP               2 (==)
             26 POP_JUMP_IF_FALSE       18 (to 36)

  6          28 LOAD_CONST               5 (4)
             30 STORE_FAST               1 (b)

  9          32 LOAD_FAST                1 (b)
             34 RETURN_VALUE

  8     >>   36 LOAD_CONST               6 (5)
             38 STORE_FAST               1 (b)

  9          40 LOAD_FAST                1 (b)
             42 RETURN_VALUE

From what I can tell, the return block is only being inlined on Python 3.10 only?

@iritkatriel
Copy link
Member

Possibly it was added in 3.10. You can dig in the code if you want. The compiler has changed quite a lot between 3.9 and now.

@esc
Copy link
Contributor

esc commented Jul 7, 2022

Possibly it was added in 3.10. You can dig in the code if you want. The compiler has changed quite a lot between 3.9 and now.

Yes, I am aware of the changes to the compiler in 3.10. Thank you for your answer, my guess is that the code has been there for some time, but due to the refactoring in 3.10 it actually started triggering.

@markshannon
Copy link
Member

Unfortunately #94592 doesn't fix this.
The problem applies to jumping from any block that is only reachable through an exception handler.
For example, this test fails.

    @jump_test(7, 1, [1, 6, 1, 6])
    def test_jump_over_try_except_early_return(output):
        output.append(1)
        try:
            1 / 0
            return
        except ZeroDivisionError as e:
            output.append(6)
        pass # This block is dominated by the except handler

Code like this is much less likely than the original example, but I think we should fix this properly by marking the stacks for exception handlers in mark_stacks

@markshannon markshannon reopened this Jul 11, 2022
@iritkatriel
Copy link
Member

That we are not tracking the exception handler is a separate (and known) issue. The problem here was that we could not tell where an exception handler ends.

@iritkatriel
Copy link
Member

The issue of exception handlers is tracked here: #94739

@markshannon
Copy link
Member

The jump from the example I give is from the pass, which is outside of the except handler.

@iritkatriel iritkatriel reopened this Jul 11, 2022
@iritkatriel
Copy link
Member

I see, will check.

@iritkatriel
Copy link
Member

This fails too:

    @jump_test(1, 3, [1, 3])
    def test_jump_over_return(output):
        output.append(1)
        return 12
        output.append(3)

@markshannon
Copy link
Member

That should fail. output.append(3) is unreachable

@iritkatriel
Copy link
Member

You mean, the compiler can tell that it's unreachable.

If you allow the debugger to jump around you can do all kinds of impossible things (like get to the pass in your example without an exception being raised). I'm not sure what the semantics of these debugger jumps are, really.

@markshannon
Copy link
Member

I'm not sure what the semantics of these debugger jumps are, really.

No one is.

In my mind, they are:

  • Never crash the interpreter.
  • If it is possible to make the jump, then do so.
  • Present a meaningful error message if the jump is impossible.

@iritkatriel
Copy link
Member

We understand the issue, and it's the subject of #94739 so I'll close this again (the original problem reported here was resolved).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes 3.12 bugs and security fixes release-blocker type-bug An unexpected behavior, bug, or error
Projects
Development

No branches or pull requests

8 participants