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

code.replace(co_code=new_code) no longer catch exceptions on Python 3.11 #91341

Closed
vstinner opened this issue Apr 1, 2022 · 10 comments
Closed
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@vstinner
Copy link
Member

vstinner commented Apr 1, 2022

BPO 47185
Nosy @gvanrossum, @gpshead, @vstinner, @markshannon

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2022-04-05.22:35:28.589>
created_at = <Date 2022-04-01.09:31:33.790>
labels = ['interpreter-core', '3.11']
title = 'code.replace(co_code=new_code) no longer catch exceptions on Python 3.11'
updated_at = <Date 2022-04-05.23:22:21.211>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2022-04-05.23:22:21.211>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2022-04-05.22:35:28.589>
closer = 'gvanrossum'
components = ['Interpreter Core']
creation = <Date 2022-04-01.09:31:33.790>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 47185
keywords = []
message_count = 10.0
messages = ['416479', '416499', '416501', '416503', '416518', '416637', '416825', '416827', '416829', '416832']
nosy_count = 5.0
nosy_names = ['gvanrossum', 'gregory.p.smith', 'vstinner', 'Mark.Shannon', 'dom1310df']
pr_nums = []
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue47185'
versions = ['Python 3.11']

@vstinner
Copy link
Member Author

vstinner commented Apr 1, 2022

Since bpo-40222 "Zero cost exception handling", code object created by from bytecode with code.replace(co_code=new_code) no longer catch exceptions on Python 3.11, unless an exception table is set explicitly.

Example:
---

def f():
    try:
        print("raise")
        raise ValueError
    except ValueError:
        print("except")
    else:
        print("else")
    print("exit func")

def g(): pass

if 1:
    code = f.__code__
    g.__code__ = g.__code__.replace(
        co_code=code.co_code,
        co_consts=code.co_consts,
        co_names=code.co_names,
        co_flags=code.co_flags,
        co_stacksize=code.co_stacksize)
else:
    g.__code__ = f.__code__  # this code path works on Python 3.11

g()

Output with Python 3.10 (ok):
---

raise
except
exit func

Output with Python 3.11 (oops):
---

raise
Traceback (most recent call last):
  ...
ValueError

Would it make sense to automatically compute co_exceptiontable on code.replace(co_code=new_code)? If it's computed automatically, I don't know if it makes still sense to call code.replace(co_code=new_code, co_exceptiontable=new_table).

It seems like currently, the only implementation to build an exception table lives in Python/compile.c which compiles AST to bytecode. It cannot be reused for code.replace() which takes bytecode as input, not AST.

--

If code.replace() is not updated to recompute co_exceptiontable, at least, it would be nice to document the bpo-40222 changes in What's New in Python 3.11 and in the CodeType documentation:

@vstinner vstinner added 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Apr 1, 2022
@gvanrossum
Copy link
Member

How would you compute the exception table from the bytecode? There are no clues in the bytecode about where the try and except blocks are.

@vstinner
Copy link
Member Author

vstinner commented Apr 1, 2022

How would you compute the exception table from the bytecode? There are no clues in the bytecode about where the try and except blocks are.

Disassemble the bytecode to rebuild basic blocks and detect which ones are except blocks. I don't know how the exception table works :-) It's just a guess.

Or do you think that this job should be done by the caller?

@vstinner
Copy link
Member Author

vstinner commented Apr 1, 2022

python-dev thread:
https://mail.python.org/archives/list/python-dev@python.org/thread/KWSPCLXDHBAP2U4LBSMLQEOC7LREDMPB/

Mark wrote:

"You can pass the exception table the same way you pass all the other arguments. The exception table depends on the code, but that is nothing new. The bytecode library already recomputes the consts, names, etc."

Constants and names are easy to build, it's just an array and the bytecode refers to their index.

Building the exception table is more complicated. It's nice that the format is documented in https://github.com/python/cpython/blob/main/Objects/exception_handling_notes.txt but it would be more convenient to put it in the regular Python documentation (docs.python.org), no? I discovered that file by mistake with filename completion in my editor while looking for Objects/exceptions.c :-)

@vstinner
Copy link
Member Author

vstinner commented Apr 1, 2022

Guido (msg416498)

Surely the bigger issue is that the contents of new_code itself must be totally different? Also there are other tables that need to be adjusted if you really do change co_code, e.g. the debugging tables.

Do you consider that .replace() must reject changing co_code if other tables are not updated?

Debugging tables are not strictly required just to *execute* code, no?

If you consider that the caller *must* update co_exceptiontable, replace() must raise an exception in this case, to prevent creating a code object which would behave in a strange way (broken exception handling).

If someone really wants testing an empty exception table just for fun, it would still be possible to pass co_exceptiontable=b''.

My concern is more about people upgrading to Python 3.11 and who "suddenly" don't get their exceptions handled anymore. I would prefer catching such bug at the replace() call, rather than having to execute the code (and only notice the bug in production? oops).

@gvanrossum
Copy link
Member

[Victor]

Do you consider that .replace() must reject changing co_code if other tables are not updated?

I simply don't believe it can always do that correctly, so I believe it should not do it.

Debugging tables are not strictly required just to *execute* code, no?

No, but if they are wrong crashes might happen when they are consulted. At the very least they would confuse users.

If you consider that the caller *must* update co_exceptiontable, replace() must raise an exception in this case, to prevent creating a code object which would behave in a strange way (broken exception handling).

No. There are a zillion use cases. If you are using .replace() you are taking responsibility for the result.

If someone really wants testing an empty exception table just for fun, it would still be possible to pass co_exceptiontable=b''.

My concern is more about people upgrading to Python 3.11 and who "suddenly" don't get their exceptions handled anymore. I would prefer catching such bug at the replace() call, rather than having to execute the code (and only notice the bug in production? oops).

Where would these people get the value that they're using to replace co_code? Surely if they are generating bytecode it will already be broken. Pretty much all instructions are different in 3.11.

@gvanrossum
Copy link
Member

This idea just cannot work. Take these two functions:

def f():
    foo()
    try:
        bar()
    except:
        pass

def g():
    try:
        foo()
        bar()
    except:
        pass

Using dis to look at their disassembly, the only hint that in f(), the call to foo() is outside the try block and in g() it is inside it is the presence of some NOP opcodes. The actual demarcation of where the try blocks start and end is exclusively determined by the exception table.

It just doesn't make sense to try to validate that correct parameters are being passed in when you are modifying co_code and friends.

@vstinner
Copy link
Member Author

vstinner commented Apr 5, 2022

>>> def f():
...     foo()
...     try:
...         bar()
...     except:
...         pass
... 
>>> def g():
...     try:
...         foo()
...         bar()
...     except:
...         pass
... 

>>> dis.dis(f)
  1           0 RESUME                   0

2 2 LOAD_GLOBAL 1 (NULL + foo)
14 PRECALL 0
18 CALL 0
28 POP_TOP

3 30 NOP

4 32 LOAD_GLOBAL 3 (NULL + bar)
44 PRECALL 0
48 CALL 0
58 POP_TOP
60 LOAD_CONST 0 (None)
62 RETURN_VALUE
>> 64 PUSH_EXC_INFO

5 66 POP_TOP

6 68 POP_EXCEPT
70 LOAD_CONST 0 (None)
72 RETURN_VALUE
>> 74 COPY 3
76 POP_EXCEPT
78 RERAISE 1
ExceptionTable:
32 to 58 -> 64 [0]
64 to 66 -> 74 [1] lasti

>>> dis.dis(g)
  1           0 RESUME                   0

2 2 NOP

3 4 LOAD_GLOBAL 1 (NULL + foo)
16 PRECALL 0
20 CALL 0
30 POP_TOP

4 32 LOAD_GLOBAL 3 (NULL + bar)
44 PRECALL 0
48 CALL 0
58 POP_TOP
60 LOAD_CONST 0 (None)
62 RETURN_VALUE
>> 64 PUSH_EXC_INFO

5 66 POP_TOP

6 68 POP_EXCEPT
70 LOAD_CONST 0 (None)
72 RETURN_VALUE
>> 74 COPY 3
76 POP_EXCEPT
78 RERAISE 1
ExceptionTable:
4 to 58 -> 64 [0]
64 to 66 -> 74 [1] lasti

Oh, I didn't follow recent bytecode changes. Ok, now I see that it is not longer possible to build the exception table just from the bytecode. The purpose of the exception table is to handle exceptions: the opcodes related to exception handles are simply gone in Python 3.11.

I was thinking about looking for things like PUSH_EXC_INFO or POP_EXCEPT, but as Guido shows, it doesn't work: the start of the "try" block cannot be detected in the bytecode anymore in Python 3.11.

If code.replace() is not updated to recompute co_exceptiontable, at least, it would be nice to document the bpo-40222 changes in What's New in Python 3.11 and in the CodeType documentation

You closed the issue. I understand that you don't want to document CodeType.replace() changes neither. Users of this API should follow Python development and notice that their code no longer works with Python 3.11.

@gvanrossum
Copy link
Member

If you think the changes to .replace() should be documented just open a new bpo. You made this issue about your various proposals to change .replace().

@vstinner
Copy link
Member Author

vstinner commented Apr 5, 2022

Ok sure, I created bpo-47236 "Document types.CodeType.replace() changes about co_exceptiontable".

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Development

No branches or pull requests

2 participants