gh-127604: Add C stack dumps to faulthandler
#128159
Conversation
ZeroIntensity
requested review from
erlend-aasland and
corona10
as code owners
|
@ZeroIntensity If you ever use the web UI, don't forget to replace my tabs with spaces... hopefully your IDE does it but I see that I have tabs instead of spaces on my suggestions (I've corrected them but just double-check) |
Co-authored-by: Bénédikt Tran <[email protected]>
…thon into c-faulthandler
Co-authored-by: Bénédikt Tran <[email protected]>
…thon into c-faulthandler
ZeroIntensity
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit e79e661 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
There was a problem hiding this comment.
Nice! I did start playing around with an implementation myself, and looks like we made a couple different implementation decisions:
Instead of adding a dump_c_stack function, I added a kwarg to dump_traceback. I think I prefer your approach here, since a user may want to see the c backtrace but doesn't care about the python traceback. Though that does clash somewhat with it being a kwarg to enable (and register/dump_traceback_later).
I named my kwarg c_backtrace, though I think I prefer c_stack to avoid backtrace/traceback confusion.
I also added a kwarg to register(), and dump_traceback_later could also get it.
I made all functions raise an exception if passed c_stack=True and it wasn't available, and added a global faulthandler.HAS_C_BACKTRACE. I'm not sure what the best approach is here.
| faulthandler_stack_dump_impl(int fd) | ||
| { | ||
| PUTS(fd, " <cannot get C stack on this system>\n"); | ||
| } |
There was a problem hiding this comment.
could make this error message more informative, mentioning glibc and/or execinfo.h
edit: although bsd makes this more complicated... smh
There was a problem hiding this comment.
I don't think it's worth it. faulthandler is just the first step to debugging a crash, nobody is going to try and switch systems to enable the C stack (they'll just use valgrind or something like that).
There was a problem hiding this comment.
In addition, switching system may also hide the failure (the issue could come from the platform they are working on)
| PyObject *file = NULL; | ||
| int all_threads = 1; | ||
| int fd; | ||
| int c_stack = 1; |
There was a problem hiding this comment.
I don't think it should be defaulted to True, especially when only available on a subset of systems.
There was a problem hiding this comment.
Eh, I don't think it matters. People don't use faulthandler for compatibility reasons, they just enable it for debugging. I feel like not enabling the C stack trace by default will just add an extra, annoying step for users.
There was a problem hiding this comment.
I'm also in favor of keeping the stack trace.
especially when only available on a subset of systems
Well.. I'd say it's available on many popular systems except FreeBSD/CentoS. Ubuntu and Fedora being supported are engouh IMO, at least for many users.
Is it available on macOS by the way?
There was a problem hiding this comment.
Is it available on macOS by the way?
Probably, but I haven't checked.
| @@ -93,18 +93,20 @@ def check_error(self, code, lineno, fatal_error, *, | |||
| fd=None, know_current_thread=True, | |||
| py_fatal_error=False, | |||
| garbage_collecting=False, | |||
| c_stack=True, | |||
There was a problem hiding this comment.
We probably want tests for:
c_stack=False- checking for "cannot get C stack..." on unsupported systems
dump_c_stack
There was a problem hiding this comment.
checking for "cannot get C stack..." on unsupported systems
That's checked by the regex, there's a case for <whatever> messages in there.
This only adds it for glibc right now. I think it's possible to implement it for Windows with
StackWalk64, but I'd prefer to get a solid working implementation for other systems before I dive into that.faulthandler#127604📚 Documentation preview 📚: https://cpython-previews--128159.org.readthedocs.build/en/128159/library/faulthandler.html#dumping-the-c-stack