bpo-39114: Fix tracing of except handlers with name binding

[pablogsal]

https://bugs.python.org/issue39114

[pablogsal]

@nedbat Can you confirm if this fixes your issue?

[pablogsal]

@markshannon Can you check if this looks right to you? I am not completely sure what was originally this guarding against (not sure what the comment Make sure that we trace line after exception is referring to) but looks like this is what is making the trace function emit incorrect lines after the artificial finally of the try-except with name to delete de name.

[nedbat]

@pablogsal Thanks, I'll take a look at it. Right now, this change is failing other tests of mine. We should talk about how to add more regression tests to the Python test suite so that we don't have to have these kinds of bug reports in the first place.

As an example, shouldn't this pull request add some tests to confirm the fix?

[nedbat]

@pablogsal good news: looks like the coverage.py tests that fail with this change are testing the same things that the current test failure here is testing.

[pablogsal]

As an example, shouldn't this pull request add some tests to confirm the fix?

Yes, but for now I am not sure that the fix is even correct so I still didn't make tests for it :)

[pablogsal]

@nedbat Can you check your test suite with this version of the PR? If Mark confirms that this looks good to him, I can add some tests.

[nedbat]

@pablogsal Thanks. This PR fixes the two problems reported in bpo-39114. But it seems to lose the tracing for some finally clauses.

/tmp/pr17769.py:

import linecache, sys

def trace(frame, event, arg):
    if frame.f_code.co_filename == __file__:
        lineno = frame.f_lineno
        print("{} {}: {}".format(event[:4], lineno, linecache.getline(__file__, lineno).rstrip()))
    return trace

def f():
    a = 0; b = 0
    try:
        a = 1
        try:
            raise Exception("foo")
        finally:
            b = 123
    except:
        a = 99
    assert a == 99 and b == 123

print(sys.version)
sys.settrace(trace)
f()

In Python 3.8 and 3.9a2, line 16 (the finally clause) is reported. With this PR, it is not:

% python3.8 /tmp/pr17769.py
3.8.1 (default, Dec 19 2019, 08:38:38)
[Clang 10.0.0 (clang-1000.10.44.4)]
call 9: def f():
line 10:     a = 0; b = 0
line 11:     try:
line 12:         a = 1
line 13:         try:
line 14:             raise Exception("foo")
exce 14:             raise Exception("foo")
line 16:             b = 123
line 17:     except:
line 18:         a = 99
line 19:     assert a == 99 and b == 123
retu 19:     assert a == 99 and b == 123

% python3.9 /tmp/pr17769.py
3.9.0a2 (default, Dec 19 2019, 08:42:29)
[Clang 10.0.0 (clang-1000.10.44.4)]
call 9: def f():
line 10:     a = 0; b = 0
line 11:     try:
line 12:         a = 1
line 13:         try:
line 14:             raise Exception("foo")
exce 14:             raise Exception("foo")
line 16:             b = 123
line 17:     except:
line 18:         a = 99
line 19:     assert a == 99 and b == 123
retu 19:     assert a == 99 and b == 123

% cpython /tmp/pr17769.py
3.9.0a2+ (heads/pr/17769:398fe55bea, Dec 31 2019, 13:52:37)
[Clang 10.0.0 (clang-1000.10.44.4)]
call 9: def f():
line 10:     a = 0; b = 0
line 11:     try:
line 12:         a = 1
line 13:         try:
line 14:             raise Exception("foo")
exce 14:             raise Exception("foo")
line 17:     except:
line 18:         a = 99
line 19:     assert a == 99 and b == 123
retu 19:     assert a == 99 and b == 123

[pablogsal]

Thanks, @nedbat. Is very unfortunate that we don't have enough coverage for the trace functionality to detect this when iterating over changes in the compiler....

I will try to find a solution for this in the meantime but @markshannon is more familiar with the changes in this code and probably knows what we are missing. Certainly we need to improve the test suite of sys_settrace.

[pablogsal]

I have updated the PR with a new test covering the last case you reported. @nedbat Is there any way I can try your test suite to make sure I am not missing something?

[nedbat]

@pablogsal The tests all pass with this change, thanks! We can work on how to run the coverage.py test suite locally. In this case, I had already xfail'ed these tests on 3.9, so it was a little involved to see them run again.

[markshannon]

Thanks @pablogsal.
LGTM. I think this is the best we can do without any large changes, but it is papering over the cracks a bit.

The root problem is that we have no way to express that some sequence of instructions does not correspond to any actual source.
I think the simplest way to do that would be to give those instructions a line number of zero and have sys.setrace ignore line zero.

[nedbat]

@markshannon I'm not sure what you mean by "sequence of instructions does not correspond to any actual source." What's the example of that here?

[pablogsal]

"sequence of instructions does not correspond to any actual source."

This is because:

try:
   ...
except Exception as e:
   ...

Really generates this code

try:
   ...
except Exception as e:
    try:
        ...
    finally:
         e = None
         del e

The reason is to avoid cycles in the gc with the exception name and keep alive entire stacks. The bytecode for the finally that gets generated have no real source lines and is what is generating the weird traces because the way unwinding and frame blocks work.

This is just one example, there are more cases when we need to generate bytecode that does not correspond to "real" user code.