Make panic handling more robust #28631
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
| // If this is a double panic, make sure that we print a backtrace | ||
| // for this panic. Otherwise only print it if logging is enabled. | ||
| if panics >= 2 || backtrace::log_enabled() { | ||
| log_panic(obj, file, line) |
There was a problem hiding this comment.
I think that this wants to be called unconditionally in all cases actually, backtrace::log_enabled was actually just intended to guard the backtrace emission, not the panic message itself.
For example, if you have a panicking program with this patch and don't have RUST_BACKTRACE=1, this means that no output will happen. (e.g. see failing Travis build for an example)
Move the panic logging code to a function separate from `on_panic` and simplify the code to decide whether the backtrace should be logged.
Move the panic handling logic from the `unwind` module to `panicking` and use a panic counter to distinguish between normal state, panics and double panics.
The double-panic `abort` is run after the logging code, to provide feedback in case of a double-panic. This means that if the panic logging fails with a panic, the `abort` might never be reached. This should not normally occur, but if the `on_panic` function detects more than 2 panics, aborting *before* logging makes panic handling somewhat more robust, as it avoids an infinite recursion, which would eventually crash the process, but also make the problem harder to debug. This handles the FIXME about what to do if the thread printing panics.
ranma42
force-pushed
the
robust-panic
branch
from
7d23165 to
54c0231
Compare
|
I updated the commits to fix the behaviour as pointed out by @alexcrichton and to improve the commit messages a bit. In particular now it states explicitly that the FIXME ranma42@54c0231#diff-dec9d6f114f706b628f5fcc9222f739dL55 now is handled. |
bors
added a commit
that referenced
this pull request
Sep 26, 2015
This is mainly to avoid infinite recursion and make debugging more convenient in the anomalous case in which `on_panic` panics. I encountered such issues while changing libstd to debug/fix part of #28129. While writing this I was wondering about which functions belong to `panicking` and which to `unwind`. I placed them in this way mostly because of convenience, but I would strongly appreciate guidance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is mainly to avoid infinite recursion and make debugging more convenient in the anomalous case in which
on_panicpanics.I encountered such issues while changing libstd to debug/fix part of #28129.
While writing this I was wondering about which functions belong to
panickingand which tounwind.I placed them in this way mostly because of convenience, but I would strongly appreciate guidance.