runtime: signal_unix.crash() does not trigger crash reporters on iOS #20392
Comments
|
Hi @steeve. Sadly, the files in DiagnosticReports are not core dumps. Those are just text logs with some basic information about crashes. I just checked, and core dumps on OS X 10.12.5 (which appear in |
|
Thank you for looking into this @aclements Indeed they are not core dumps, but I am not able to reproduce the humongous core dump you talk about with the example program I have pasted up there. Could you give me more details about how to generate them? |
At a minimum, you have to set
Can you say more about how crash reporting works on iOS? E.g., if it's going to have the same core file problem as Darwin, then I don't think we can enable this. |
|
Indeed I may not have set the Most crash reporters (Crashlytics, Sentry, etc...) on iOS work by hooking When I commented the code the crash reporting worked, but the traceback was wrong (that's another story). I would be very surprised that |
|
/cc @crawshaw @eliasnaur |
aclements
changed the title
|
That's funny, I'm sitting with a similar problem on Android. Could you try https://go-review.googlesource.com/c/49590/ ? As per Ian's comments, it might not be the correct fix, but will tell us if the problem is the same. |
|
hey @eliasnaur, sure, will do |
|
CL https://golang.org/cl/49590 mentions this issue. |
|
You probably need to apply https://go-review.googlesource.com/c/49770 as well, to make 49590 effective on darwin/arm64. |
aclements
added
the
early-in-cycle
Before this CL, whenever the Go runtime wanted to kill its own process with a signal dieFromSignal would reset the signal handler to _SIG_DFL. Unfortunately, if any signal handler were installed before the Go runtime initialized, it wouldn't be invoked either. Instead, use whatever signal handler was installed before initialization. The motivating use case is Crashlytics on Android. Before this CL, Crashlytics would not consider a crash from a panic() since the corresponding SIGABRT never reached its signal handler. Updates #11382 Updates #20392 (perhaps even fixes it) Fixes #19389 Change-Id: I0c8633329433b45cbb3b16571bea227e38e8be2e Reviewed-on: https://go-review.googlesource.com/49590 Run-TryBot: Elias Naur <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
|
@steeve, both CLs are now in, hopefully fixing this issue. |
|
Change https://golang.org/cl/55032 mentions this issue: |
The dieFromSignal runtime function attempts to forward crashing signals to a signal handler registered before the runtime was initialized, if any. However, on Darwin, a special signal handler trampoline is invoked, even for non-Go signal handlers. Clear the crashing signal's handlingSig entry to ensure sigtramp forwards the signal. Fixes the darwin/386 builder. Updates #20392 Updates #19389 Change-Id: I441a3d30c672cdb21ed6d8f1e1322d7c0e5b9669 Reviewed-on: https://go-review.googlesource.com/55032 Run-TryBot: Elias Naur <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
|
@steeve, can you confirm whether Go on tip now triggers crash reporters on iOS? |
aclements
added
the
WaitingForInfo
|
Okay, just got the time to properly test this. Unfortunately it does not work. Here is my test code: import (
"runtime/debug"
)
func init() {
debug.SetTraceback("crash")
}
func panicOnNullValue() {
var c []int
c[0] = 0
}
func panicForReal() {
panic("DID PANIC")
}
func Panic() {
panicOnNullValue()
}
func Panic2() {
panicForReal()
}Both of these do not generate crash reports on iOS (while they correctly dump to The good news it, it does generate a I believe this may be because the sig handlers might be set too early by Go on iOS, since you need to install them with in the Application delegate (application |
|
Another thing: if I manually raise @IBAction func buttonUp(_ sender: Any) {
Crash.CrashPrint()
raise(SIGABRT)
}Whereas this does not: @IBAction func buttonUp(_ sender: Any) {
Crash.CrashPrint()
Crash.CrashPanic2()
}Note that The weird thing is that |
|
Ping @ianlancetaylor, @bcmills. |
|
After reading 5500c9c in detail, I do believe this is because of a race between Crashlytics init and Go. Crashlytics installs its handlers when asked (in the app main), but I think the Go runtime is initialized before (being a static library). Since the handlers are saved at runtime init, this may be too early. I suspect this works on Android because of the trampoline. Sorry I did not have time to check this earlier... |
|
There is probably a second change we need to do on top of https://golang.org/cl/55032: we should first try raising the signal without reinstalling the |
|
Change https://golang.org/cl/57291 mentions this issue: |
|
Happy to report that CL 57291 properly makes crash detectors (Crashlytics in my case) detect the crash ! However, the stack trace is lost: I am using the same code as above, that is calling The trace should be: The framework was generated with: I'm thinking it may be due to missing debug/symbols information ? |
|
Glad to hear it works! Please raise a new issue for the missing stack trace. It is a problem on Android as well, and is orthogonal to this issue. It could be as simple as missing framepointers on arm/arm64, in which case https://groups.google.com/forum/#!topic/golang-dev/v1TxCJNemPY sounds like good news, at least for arm64. |
|
That's great, will do! |
CL 49590 made it possible for external signal handlers to catch signals from a crashing Go process. This CL extends that support to handlers registered after the Go runtime has initialized. Updates #20392 (and possibly fix it). Change-Id: I18eccd5e958a505f4d1782a7fc51c16bd3a4ff9c Reviewed-on: https://go-review.googlesource.com/57291 Run-TryBot: Elias Naur <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
|
CL 57291 was reworked before going in. Can you verify crashes are still propagated as you expect on tip? If so, you can close this issue. |
|
Ping @steeve. Can you re-test with tip to make sure the reworked CL still fixes the issue? Thanks! |
|
Will do!
On Wed 8 Nov 2017 at 23:16, Austin Clements ***@***.***> wrote:
Ping @steeve <https://github.com/steeve>. Can you re-test with tip to
make sure the reworked CL still fixes the issue? Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20392 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIY-xbJY85z3fLVEASTe3TxrVWD591Oks5s0ihQgaJpZM4NdrE6>
.
--
twitter.com/steeve
github.com/steeve
linkd.in/smorin
|
|
Confirmed working! However, there is still the issue regarding the stack traces which are all wrong: |
|
@steeve, great! Thanks for checking. I'm going to close this issue as fixed. Could you open a new issue about the stack traces? Thanks! |
|
@aclements just opened #22716 |
What version of Go are you using (
go version)?What operating system and processor architecture are you using (
go env)?What did you do?
Run this sample program:
What did you expect to see?
SIGABRTWhat did you see instead?
exit(2)Notes
According to
go/src/runtime/signal_unix.go
Line 450 in c20e545
I traced it back to 5146a93#diff-51306f0b403ee6e698224ae3ae071c21R101, which is 4 years ago.
I did try to comment this code to see if I would still get the same behaviour as described by the commit/code, and turns out I do not.
Running the above program on
go version devel +c20e545 Wed May 17 01:30:51 2017 +0000 darwin/amd64, OS X crash reported correctly identified the crash and the file is pretty small:So I think this code is outdated and can be dropped, thus honouring the
tracebackCrashflag properly. However, I'm not sure since when.All I can say is that it works fine on my OS X install, which is
10.12.4. So at minimum the code could disable the silencing starting this10.12perhaps:The text was updated successfully, but these errors were encountered: