-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Made signal handling more uniform during crashes. #82163
Conversation
You forgot to put a blank line between line 77 and line 78, small stuff but just to keep the code clean. |
Added the space, how's that? |
Sorry i meant a blank line. |
Does SIGABRT also need to be handled? See #77286. |
In general, Godot's crash handler is several steps removed from being signal-safe. Handling more signals right now means more opportunities to mishandle them and cause lockups. The default behavior of fatal signals is to simply crash, which is very safe. However it comes with the tradeoff of not providing any stack trace information for the user. Ideally, Godot's crash handler would first be made signal-safe, then all signals that could be reasonably caused by a programmer's code should print a stack-trace to help identify what's wrong. tldr Not right now, but yes after improvements have been made to the crash handler. |
I've created a new issue to address the lack of signal safety here for further discussion: #82418 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
Thanks! And congrats for your first merged Godot contribution 🎉 |
Fixes #82102
I've learned that signal is heavily implementation specific. According to the cppreference docs:
This means that we won't know if further exceptions during crash handling will be ignored, cause loops, lockups, or crashes since it's completely implementation specific.
It also states
I'm not sure if sigaction is a good fit for godot, since godot is multi-platform and sigaction is not. And it's nice keeping the crash handlers somewhat similar in design when possible.
So now for my proposed solution: a very simple fix is to enforce the behavior of signal by starting the crash handler with
This prevents further fatal errors from initiating patty-cake with the kernel for most implementations. The trade-off being that we still won't know if the implementation is currently ignoring any of the signals.
I think making it work for all implementations would require a reassessment for using sigaction.
Regardless, this change should make the crash handlers more sane without being complicated. Tested it and it works okay: