(exc, val, tb) triplet to one value

[iritkatriel]

@markshannon suggested to change the (exc, val, tb) triplets on the interpreter that represent exceptions into one: if it's a normalized exception, just the exception itself. If it isn't, make it a 3-tuple of the values.

The advantage is that 3 POPS and pushes become one, and the code is simplified in various places.

[iritkatriel]

Pushing a frame to the tb when unwinding will be a bit slower - it would require a type check to see if it's an exception or a tuple, (and if it's a tuple it would require creating a new one with the new tb?)

This is not thought to be on any hot path.

[gvanrossum]

What are the sources of unnormalized triples? Maybe we can address those, and have it always be either an exception or `None`?

[iritkatriel]

What are the sources of unnormalized triples? Maybe we can address those, and have it always be either an exception or None?

I think the source could be any PyErr_Restore call, so we probably need to deprecate it.

[gvanrossum]

I think the source could be any PyErr_Restore call, so we probably need to deprecate it.

Could not PyErr_Restore() normalize it?

Then again, I may be mistaken, but IIRC there are two uses for unnormalized exceptions:

• So we can add to the traceback chain without having to update the exception's __traceback__ attribute each time.
• So we can avoid instantiating an exception that was raised by class (e.g. raise RuntimeError), in case there's some C code that catches it.

These uses are both optimizations, and losing them would be a little sad (though who knows, maybe it really doesn't matter any more).

[iritkatriel]

Could not PyErr_Restore() normalize it?

I was thinking it raises if the exception is not normalized but of course it can't really do that. So yes, it would have to normalize. Is this not a semantic change to a public API?

[gvanrossum]

I was thinking it raises if the exception is not normalized but of course it can't really do that. So yes, it would have to normalize. Is this not a semantic change to a public API?

Hm, who knows. PyErr_Save()/Restore() is ancient, and it's in the Stable ABI. So I'm guessing probably not worth disturbing unless you have a really good reason.

[iritkatriel]

Some thought after making the interpreter crash many times:

Perhaps we should separate the curexc and exc_info cases. ISTM that there isn't much benefit in changing curexc (it's not what gets pushed and popped on the stack) and it is also tricker (one issue is that it is set in places where it is not possible to report an error, so we should not allocate a new tuple there).

On the other hand, exc_info is what will impact the eval stack (and the complexity of implementing except*). exc_info should always be normalized, and its traceback is no longer growing. So it should be enough to just have the exception value.

One snag is PyErr_SetExcInfo, which lets you do whatever you want. And you can set exc_info to a bad state:

>>> import _testcapi
>>> _testcapi.set_exc_info(TypeError, ValueError('blah'), None)
(None, None, None)
>>> import sys
>>> sys.exc_info()
(<class 'TypeError'>, ValueError('blah'), None)

However, the second arg must be an exception so you can't pass in an unnormalized triplet:

>>> _testcapi.set_exc_info(TypeError, 'blah', None)
Assertion failed: (PyExceptionInstance_Check(exc)), function _PyBaseExceptionObject_cast, file exceptions.c, line 321.
zsh: abort      ./python.exe

So, you can only pass in inconsistent data, which will not be normalized by the interpreter. Can we assume that this is not a feature?

History: PyErr_SetExcInfo was added in 3.3 under bpo-14098, and It is documented as something that should be used to save and restore the exc_info: https://docs.python.org/3/c-api/exceptions.html#c.PyErr_SetExcInfo.

The motivation for adding it was for cython to implement stuff around exceptions in a way which is compatible with other python implementations. cython uses it in:

(1) https://github.com/TeslaProject/external_chromium_org/blob/0b2c88af2b79c51467aeddc00f116d826db3b941/third_party/cython/src/Cython/Utility/Exceptions.c#L301
and in this case it normalizes the exception in line 267. (Upstream: https://github.com/cython/cython/blob/master/Cython/Utility/Exceptions.c#L301)

(2) A few places in coroutines to swap exceptions around. It's harder to see where they are normalized but if they originate from the interpreter's exc_info then they should be.

[markshannon]

Perhaps we should separate the curexc and exc_info cases.

I thought that they already were. But if they are not, then yes we definitely should.

It might help to use more precise terminology. I know that I have struggled in the past to tell which exception some C code is dealing with.

IIRC, C++ uses the term "in flight" to describe an exception that was raised (thrown) but has not yet been handled (caught).
The docs for sys.exc_info() say "This function returns a tuple of three values that give information about the exception that is currently being handled."

So it might help to use the term "in flight" and "handled" to described the two different exceptions.
Also, given the sys.exc_info() exceptions form a stack, maybe "topmost handled"?

<rant>
The whole concept of setting a "current exception" as a side-channel, as opposed to returning error codes (in C), is bad design but it is what we have (at least for now), so we have to handle it.
</rant>

[iritkatriel]

As discussed at the meeting:

All buildbot tests pass with this assertion: python/cpython#29518 , so this is strong indication that the exc_type of exc_info is indeed redundant and should be removed. I tried to do this, but it's complicated because the fact that exc_info has 3 items on the stack is implicitly hard-coded in many places. It is pretty clear in ceval.c, because there are variable names indicating what is being popped and pushed. But in compile.c there are ADDOP(c, POP_TOP); sequences that are related to exceptions without any indication. There are also constants in a few places that count exc_info entries (but again this is not explicit). Getting anything wrong just makes the interpreter crash (usually a bit later) and is often hard to debug. So it will take some careful analysis to come up with a way to do this correctly and make a PR which is clear enough that a reviewer would want to approve it.

Regarding the tb field, an assertion similar to the above showed that the __traceback__ on the exc_value is not always the same as the exc_traceback. In one case I looked at, the traceback on the exception had one additional frame (whose tb_next was equal to exc_traceback). This needs to be investigated to see how it happens, it could be a bug or it could be that the test was directly modifying the exception's traceback.

In summary - I think it is definitely worth sorting this out (ideally end up with just exc_value on the stack). But it's not a quick fix, so I will do except* first and come back to this afterwards.

[ericsnowcurrently]

there are ADDOP(c, POP_TOP); sequences that are related to exceptions without any indication.
There are also constants in a few places that count exc_info entries (but again this is not explicit).

So, if I understood that correctly, there are a number of places where the code explicitly deals with the 3 objects in question, but it isn't easy to identify those places because that code doesn't clearly relate to exc_info (and there aren't sufficient comments or other markers to help narrow it down). Is that right? I certainly believe that being the case and feel your pain. 😕

Regarding the tb field, an assertion similar to the above showed that the __traceback__ on the exc_value is not always the same as the exc_traceback.

Yuck!

In summary - I think it is definitely worth sorting this out (ideally end up with just exc_value on the stack).

+1

But it's not a quick fix, so I will do except* first and come back to this afterwards.

smart

[iritkatriel]

So, if I understood that correctly, there are a number of places where the code explicitly deals with the 3 objects in question, but it isn't easy to identify those places because that code doesn't clearly relate to exc_info (and there aren't sufficient comments or other markers to help narrow it down). Is that right?

Yes, this is what I mean. I think together with the opcode documentation we should be able to identify what's what, but I find compile.c harder to follow than ceval.c.

[iritkatriel]

I looked into some of the tests that make a traceback-is-redundant assertion to fail. It's things like this, where an exception is raised (not reraised) while it is being handled. The traceback on the exception gets updated, but the tb from the first raise does not.

>>> E = TypeError
>>> try:
...   raise E
... except Exception as e:
...   try:
...     raise E 
...   except:
...     raise e
... 
Assertion failed: (tb == exc_tb), function _assert_traceback_is_redundant, file ceval.c, line 1126.
zsh: abort      ./python.exe

[iritkatriel]

So at the moment this script:

import sys

E = TypeError
try:
   raise E
except Exception as e:
   print('1-- ', sys.exc_info()[2], e.__traceback__)
   try:
      try:
         raise E 
      except:
         raise e
   except:
      pass
   print('2-- ', sys.exc_info()[2], e.__traceback__)

Produces:

1--  <traceback object at 0x10e907f70> <traceback object at 0x10e907f70>
2--  <traceback object at 0x10e907f70> <traceback object at 0x10e9240f0>

And if we stop pushing the traceback to the stack then sys.exc_info()[2] will return the e.__traceback__ value, which is not always the same.

[iritkatriel]

I checked the documentation and I don't see any mention of what should happen when an exception is raised (with 'raise e' rather than raise) while it is being handled. I think it's ok if we change behaviour in this edge case. If it's a problem for someone there is an easy workaround - to call sys.exc_info() at the beginning of the except: body and save the value to be used later.

[gvanrossum]

I think the conclusion is that after

try:
  raise E()
except E as e:
  e.__traceback__ = <something else>
  print(sys.exc_info()[2])

then sys.exc_info()[2] isn't equal to e.__traceback__. I think that's fine, it's a veritable edge case.