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.
| 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.
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