-
Notifications
You must be signed in to change notification settings - Fork 15
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
Log backtrace on sigsegv and sigbus signals #22
Conversation
I guess I should notify the next handler. Rust handles segfaults to detect and report stack overflows: rust-lang/rust#31333 |
To do this robustly we need a separate implementation for Windows and Unix so this doesn't seem worth it. I added a note about that. I also added a note about R's segfault handler. It doesn't override our handler because we set |
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.
LGTM!
// Register segfault handler to get a backtrace. Should be after | ||
// initialising `log!`. Note that R will not override this handler | ||
// because we set `R_SignalHandlers` to 0 before startup. | ||
stdext::traps::register_trap_handlers(); |
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.
Do we want to disable this in release builds?
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.
Would there be a downside to leave it on? On the upside it could be useful for R developers to get a backtrace of their native code on crash.
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.
I think you're right -- let's leave it on.
d2b3aab
to
65b72a9
Compare
Requires posit-dev/positron#708 otherwise the backtrace doesn't show up.