-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[WIP Do not merge] Helpful stack overflow messages. #51514
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
Conversation
This is using libunwind, though the library is called this, it does't actually unwind the stack in our case (which is good as this preserves panic safety).
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
|
||
if let Some(format) = log_backtrace { | ||
if let Ok(mut stderr) = Stderr::new() { | ||
let _ = backtrace::print(&mut stderr, format); |
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.
Could you please print thread '{}' has overflowed its stack
here too, because the backtrace is likely to be very big, which makes it easy to miss the first time it is printed.
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.
Excellent idea.
@gilescope |
I think we need the full monty to see if there's any OS that's complaining. I.e. auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild Maybe that's only possible by merging in the PR? If so let me add @bjorn3 |
Yeah a try build can at most support Linux and macOS only at the moment. |
The backtrace implementation acquires a lock to print to stderr, so this is likely deadlock-prone (possibly only when a stack overflow occurs while a backtrace is printed). I think there are more signal safety problems in the actual backtrace implementation, too (at the very least, no regular allocations can be performed, which is why you need to enable the libbacktrace mmap allocator - this was mentioned in #51405). |
Ping from triage @kennytm! How should we move forward with this? |
Unfortunately, with the current setup we can't dist from Windows without merging it. If we decide to merge it, OP needs to fix #51514 (review) and reply to #51514 (comment). |
Am still working on this. The PR as stands seems to work if the alternative stack is not triggered (at least that's what I think is happening: the stack probe test on my machine gives a great stack overflow message if you don't allocate much memory during the recursion - in my case < 256 produced a full stack trace, where as above that the stack trace ended at the signal handler.) The signal handler's third parameter is a u_context which contains the data required to unwind while being on an alternative stack. I just need to invoke backtrace while giving it the u_context rather than it calling getcontext() itself. Alas I haven't figured out how to do this. Any hints gratefully appreciated. |
ping from triage @gilescope @kennytm what's the story with this PR? |
I think we should pull this for now. I can’t find a way to get glibc’s or
libunwind’s backtrace to work in the context of an alternative stack. They
just assume it should be called on the same thread.
I’m going to see if I can get a pure rust implementation working using
unwind-rs but it’s probably going to take me most of the summer at the pace
I can fit it in / learn at.
…On Fri, 29 Jun 2018 at 17:27, Stokhos ***@***.***> wrote:
@gilescope <https://github.com/gilescope> @kennytm
<https://github.com/kennytm> what's the story with this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51514 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAxEiO1KPF8iQs5gZOkj_e9v1XHBcSyDks5uBlVZgaJpZM4UkALf>
.
|
Great, I'm closing this to keep the PR queue clean. When you're ready you can reopen this PR, so we can review it again. |
any progress with pursuing something like this? |
Using unwind-rs and with findshlibs section support I did manage to get unwind working on OSX. |
FWIW I don't think an initial PR needs to necessarily provide better stack overflow error messages on all platforms. A PR that does so on Linux or MacOSX only would already be valuable, and things can be improved incrementally. |
This is using libunwind, though the library is called this, it does't
actually unwind the stack in our case (which is good as this preserves
panic safety).
Discussion here: #51405
Keen to see how the test fairs on the various OSes that bors uses.
(We could also do this functionality with libbacktrace or more likely move to a pure rust
implementation: #51408 once ready. )