fix: throwing repr caused a segfault #2389
Conversation
include/pybind11/pybind11.h
Outdated
| try { | ||
| msg += pybind11::repr(args_[ti]); | ||
| } catch (const error_already_set&) { | ||
| msg += pybind11::repr(args_[ti].get_type()); |
There was a problem hiding this comment.
The repr produced is pretty ugly, but better than a segfault. Maybe should indicate it's an instance of the class?
> m.simple_bool_passthrough(MyRepr())
E TypeError: simple_bool_passthrough(): incompatible function arguments. The following argument types are supported:
E 1. (arg0: bool) -> bool
E
E Invoked with: <class 'test_exceptions.test_invalid_repr.<locals>.MyRepr'>
test_exceptions.py:162: TypeError
There was a problem hiding this comment.
Yes, it should probably say it's an instance.
This is the sort of situation where Python would chain exceptions ("While handling this error, another error occurred..."). but there's no C API for that. Although I wonder if it's worth indicating that an exception occurred in repr().
There was a problem hiding this comment.
It doesn't seem like this information is always available - some compilers are not showing it. Maybe just a "" message would work?
There was a problem hiding this comment.
Maybe discard_as_unraisable() does make sense here. It would get the stack trace and exception from __repr__ out, and would not be compiler-dependent.
There was a problem hiding this comment.
Okay, we could use it then; I've added an unchanging <repr raised Error> currently but would be happy to change it.
There was a problem hiding this comment.
We could potentially put the exception as string into <repr raised str(exc)>, but maybe we'll start some infinite recursion, then?
There was a problem hiding this comment.
There is a C API for chaining exceptions, see #2112
There was a problem hiding this comment.
What I meant was, for their own reasons, the Python core developers chose to not automate chaining exceptions in C or provide an API to make it easier to do.
https://www.python.org/dev/peps/pep-0490/
It's hard to tell what to make of the situation, in terms of what they think C extensions to do.
We potentially have exceptions for each argument to the function, and each of those could involve an individual chain of exceptions - potentially there's a tree of exceptions. I think chaining these exceptions to each other could muddy the waters because it could end up linking the exceptions from attempts to repr() different arguments. Write unraisable here gets the exception out, and then we print a message complaining that we can't repr() the arguments.
|
I currently just have a simple |
|
Nah, that's probably fine enough. If this happens, it's because there's a mistake anyway, right? We just don't want stuff to segfault. |
|
Is this related to #1716, though? |
|
I think it would solve that segfault, but the error message would not be as ideal, I'd think? |
|
nice, I vaguely remember running into this in a project of mine, but I never got around to fixing it. |
|
I think this can go in, then, and the messaging can be improved later if it's needed? |
Closes #2383 , thanks to hint from @jbarlow83 .