GNOME Bugzilla – Bug 701800
a new approach to reporting critical errors
Last modified: 2018-05-24 15:24:14 UTC
Credit to Colin for the idea. See patches.
Created attachment 246271 [details] [review] Export __glib_assert_msg Put __glib_assert_msg in the dynamic symbol table, but not in any public headers. This variable is _not_ part of our API but this way debuggers and automated crash report utilities will be able to access this variable, even if debug symbols are not available.
Created attachment 246272 [details] [review] Add G_DEBUG=fork-criticals When a g_critical() occurs, fork() and crash inside of the child. This solves the problem of ensuring that criticals get reported as crashes while still allowing the offending process to attempt to stumble along. This is currently only enabled if we have G_DEBUG=fork-criticals but I think it should become the default.
Review of attachment 246271 [details] [review]: This looks fine; as a later optimization though, it'd be nice if on glibc platforms, we used the existing __abort_msg variable. ABRT does right now for example: /* glibc's abort() stores its message in __abort_msg variable */ args[12] = (char*)"-ex"; args[13] = (char*)"print (char*)__abort_msg"; args[14] = (char*)"-ex"; args[15] = (char*)"print (char*)__glib_assert_msg";
Comment on attachment 246272 [details] [review] Add G_DEBUG=fork-criticals Evil. (In an "Evil Geniuses for a Better Tomorrow" kind of way :-) > This is currently only enabled if we have G_DEBUG=fork-criticals but I > think it should become the default. Look at your ~/.cache/gdm/session.log. The world is not ready. >+ /* We fork the critical and wait for it asynchronously. This is >+ * because it may take a while for the forked process to dump core and >+ * get collected by system crash reporting programs. We don't want to >+ * block while that's happening. Why not? The program just did a bad thing. It deserves to be punished.
(In reply to comment #3) > This looks fine; as a later optimization though, it'd be nice if on glibc > platforms, we used the existing __abort_msg variable. See 3658727cfa0eca8c66bc2cdff46992099caf0acd. (In reply to comment #4) > Why not? The program just did a bad thing. It deserves to be punished. What if the program is gnome-shell? (I don't think this part is true, but...) what if part of this involves popping a dialog and waiting for the compositor to respond? Your system just effectively locked up.
(In reply to comment #4) > Look at your ~/.cache/gdm/session.log. The world is not ready. This is not about making criticals fatal -- only about making them more visible. The world ought to be ready for that, at least... Until we figure out some way to get this problem onto the radar (since it clearly isn't now), we will continue to have session.log full of criticals...
Review of attachment 246272 [details] [review]: It'd be nice to have G_DEBUG=fork-warnings too, but that doesn't have to be this patch. ::: glib/gmessages.c @@ +663,3 @@ + +GLIB_VAR const gchar *__glib_critical_msg; +const gchar *__glib_critical_msg; Why add another variable that crash catchers will have to look for?
(In reply to comment #7) > Why add another variable that crash catchers will have to look for? Martin wants (from apport) to be able to easily tell the difference between criticals and "real" crashes. btw: TODO: figure out how to disable this stuff when running under a debugger... the fork() would not exactly be the most helpful thing in the world in this case.
(In reply to comment #8) > (In reply to comment #7) > > Why add another variable that crash catchers will have to look for? > > Martin wants (from apport) to be able to easily tell the difference between > criticals and "real" crashes. Ok, makes sense. > btw: TODO: figure out how to disable this stuff when running under a > debugger... the fork() would not exactly be the most helpful thing in the world > in this case. You could just use G_BREAKPOINT() instead of abort(), like the main g_logv() internals do.
(In reply to comment #9) > You could just use G_BREAKPOINT() instead of abort(), like the main g_logv() > internals do. Er nevermind, that doesn't help. /proc/self/status does contain TracerPid.
(In reply to comment #10) > Er nevermind, that doesn't help. /proc/self/status does contain TracerPid. Problem with this is that upstart uses ptrace to emulate cgroups in userspace... being traced doesn't always mean being debugged. That being said, I'm not sure that we should bother working around such bizarre behaviour... Also: Linux-only.
I think we should just require an environment variable set while debugging to disable this feature.... ptrace-detection is going to be too problematic.
Attachment 246271 [details] pushed as e70250b - Export __glib_assert_msg
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/712.