GNOME Bugzilla – Bug 665446
Use g_abort() instead of abort()
Last modified: 2016-04-27 13:56:45 UTC
abort() from MSVCRT terminates the program in a way that seems not to be consistent with what abort() in other CRT libraries does. Namely, MSVCRT's abort() won't throw an exception that could be caught by gdb, it just prints "terminated in unusual way" message and dies. DebugBreak() is better, since it throws an exception, which gdb can catch, and break on it. If no debugger is present, the exception will go unhandled, and the result depends on OS settings (a debugger will be attached OR user will be given a choice of attaching a debugger or killing the process OR the process will die quietly).
Created attachment 202691 [details] [review] Use DebugBreak() instead of abort() to crash on W32 Obviously, you may have reservations against expanding glibconfig.h (especially since it does not include windows.h, which is necessary for DebugBreak to be available, and because of that a few tests did not compile without #ifdef G_OS_WIN32\n#include <windows.h>\n#endif). If you know a better way to fix this - i'm all ears.
Created attachment 232887 [details] [review] Use DebugBreak() instead of abort() to crash on W32 (v1.1) Updated the patch to be against glib-2.35.3
Created attachment 233847 [details] [review] Use DebugBreak() instead of abort() to crash on W32 (v1.2) Updated the patch to be against glib git master HEAD
Created attachment 270706 [details] [review] Define a new function g_abort() that works better than abort() from msvcrt Changed the patch. Instead of calling DebugBreak() (which requires windows.h to be included), define a new function g_abort() and use that. Patch is against current git master HEAD.
Created attachment 274813 [details] [review] Use g_abort() instead of abort() The new g_abort() function just calls abort() on systems where abort() behaves in a sane way. On other systems (read: Windows) it does its best to emulate a sane abort() behaviour. v2: same thing, just rebased on top of current git master HEAD, and attached with git bz
Review of attachment 274813 [details] [review]: See the comments. ::: glib/gnulib/vasnprintf.c @@ +43,3 @@ +#ifdef _WIN32 +#include <windows.h> +#define abort() DebugBreak() any reason to use this define here instead of g_abort? ::: glib/gutils.h @@ +229,3 @@ gchar *g_format_size_for_display (goffset size); +GLIB_AVAILABLE_IN_2_30 2_42 ?
(In reply to comment #6) > ::: glib/gnulib/vasnprintf.c > @@ +43,3 @@ > +#ifdef _WIN32 > +#include <windows.h> > +#define abort() DebugBreak() > > any reason to use this define here instead of g_abort? Because this is gnulib. I can't make it call g_abort(), since it doesn't include glib (AFAIK). I honestly can't think of a clean way to hack on gnulib that would not break pulling it from upstream. This is the least intrusive one. > > ::: glib/gutils.h > @@ +229,3 @@ > gchar *g_format_size_for_display (goffset size); > > +GLIB_AVAILABLE_IN_2_30 > > 2_42 ? Yeah, these days it'll be 2_42. This bug is, like, 2.5 years old.
Created attachment 282193 [details] [review] Use g_abort() instead of abort() The new g_abort() function just calls abort() on systems where abort() behaves in a sane way. On other systems (read: Windows) it does its best to emulate a sane abort() behaviour. v2: Updated against recent glib git master, changed availability tag
Added fanc999 to the CC list. Let's see if he has anything to add.
Review of attachment 282193 [details] [review]: Hi, I think this is something good-it would be a good idea to have this API, not only that it can notify the debugger, but also I think it makes it possible to have GLib-based test programs run and not just crash when a test fails, which AFAIK is what the GLib's test framework does on other platforms (possibly also MinGW, but I can't say this), by doing an exception handler (this would, belong to another bug though). One thing though, since this is going to seem like an API in the public headers, I think we need some documentation in here (see how the other public APIs are doing in their sources, to get an idea of what goes on there), and perhaps introspection annotations as well, if needed. With blessigs, thank you!
Review of attachment 282193 [details] [review]: In general I am not opposed to the idea, but I'd like a comment from desrt or mclasen. ::: glib/gutils.h @@ +229,3 @@ gchar *g_format_size_for_display (goffset size); +GLIB_AVAILABLE_IN_2_42 this needs update
Review of attachment 282193 [details] [review]: Without too much opinion about the impact on Windows (I leave that to Chun-wei Fan), I'm OK with this change in theory. I would prefer, however, to see it defined as a macro in order to save a million future versions of ourselves from having to type 'up' one extra time.
Created attachment 325016 [details] [review] Add g_abort() Split the patch. This part adds g_abort () as a macro.
Created attachment 325017 [details] [review] Use g_abort() instead of abort() where possible This one changes abort->g_abort
Created attachment 325019 [details] Test program Due to unusual amount of preprocessor mojo in new version, i'd like to ask fanc to test this with MSVC. Specifically - try compiling this test program. If it works, try adding/removing includes for windows.h (for DebugBreak() and ExitProcess()) and stdlib.h (for abort()) before/after the defines. Finally, remove the defines and put #include <glib.h> in their place and ensure that it still compiles. When debugger is attached, this program should break. When debugger is not attached, this program should crash.
Review of attachment 325016 [details] [review]: This is a bit insane... Allison wouldn't it be better to just declare a method on windows?
Review of attachment 325017 [details] [review]: This one looks correct to me.
Review of attachment 325016 [details] [review]: Indeed. This is exactly what I proposed. LRN cited concerns that people may be annoyed that this is a function on one platform and a macro on the others, but I don't share those concerns. I even made comparison to g_open() and friends which are implemented in exactly this way.
Review of attachment 325016 [details] [review]: Hi LRN, Here comes the Visual Stdio's take... With blessings, thank you! ::: glib/gutils.h @@ -302,0 +302,15 @@ +/* Crashes the program. */ +#ifndef G_OS_WIN32 +# define g_abort() abort () ... 12 more ... This (and the following prototypes) are going to break the build on Visual Studio, as we are trying to define prototypes that do not have the same linkage decorations (i.e. they have __declspec(dllexport) as the one shipped in the Windows SDK headers). As I am not sure whether it will cause problems for mingw-w64 headers if we add that (and there will be a quite a few macros that we will have to copy from the mingw-w64 headers as a result), perhaps I would just define WINAPI and these prototypes on !_MSC_VER, and then define the g_abort() macro for all cases, so that we don't have to deal with the nasty clashes that would occur, especially as it is likely that windows.h is going to be included before (causing a warning) or after (causing an error) including gutils.h, and it is more likely the latter case. Likewise, for abort(), if stdlib.h is included after gutils.h, the build will break with a cryptic C2059 error 'type', as the prototype here will clash with the one in stdlib.h. This does, when the item does get to build, trigger the debugger break point as expected, when running the program under the debugger.
Created attachment 325061 [details] [review] Add g_abort() v2: * Turn it into a function on W32 and a macro on other platforms.
hergertme also suggested that g_error() might be used in place of g_abort(). I investigated that (even made a new pair of patches), but it turns out that g_erorr() has a nasty side-effect where it shows a GUI message box when running on Windows, except when the program is part of a testsuite. Testsuite exception is nice, but *any* glib-based devtool is potentially screwed by this, so i'm not currently in favor of that solution. Anyway, new version is attached above, it should not have any problems.
Review of attachment 325061 [details] [review]: See comments. ::: glib/gutils.c @@ +2395,3 @@ + * g_abort: + * + * Crashes the program. A wrapper for the POSIX abort() function. @@ +2397,3 @@ + * Crashes the program. + * + * On most OSes it's a macro that expands to abort() from the C library. I would remove this line @@ +2401,3 @@ + * On Windows it is a function that makes extra effort to ensure that + * a debugger-catchable exception is thrown before the program terminates. + * I would add here also: "See your C library manual for more details about abort()." @@ +2402,3 @@ + * a debugger-catchable exception is thrown before the program terminates. + * + * Since: 2.49 2.50 ::: glib/gutils.h @@ +305,3 @@ +# define g_abort() abort () +#else +GLIB_AVAILABLE_IN_2_48 2_50
Note that a straight replacement of abort() with g_error() is very wrong in a lot of cases. This is often in lower-level code (like the threading/mutex code) that cannot call into the messages code for fear of recursion, since that code itself uses mutexes. This is noticed in the top of some of the files that are touched by the →g_abort() patch: /* The GMutex, GCond and GPrivate implementations in this file are some * of the lowest-level code in GLib. All other parts of GLib (messages, * memory, slices, etc) assume that they can freely use these facilities * without risking recursion. * * As such, these functions are NOT permitted to call any other part of * GLib. ...
Created attachment 326776 [details] [review] glib: Add 2.50 availibity macros Since i'll be adding new public APIs, it falls to me to add 2.50 macros to the appropriate places.
Created attachment 326777 [details] [review] Add g_abort() v3: * Reworded g_abort() documentation. * Changed Since: and AVAILABLE_IN to 2.50
Created attachment 326778 [details] [review] Use g_abort() instead of abort() where possible v2: * Turned two tabs into spaces to match the surrounding code
Review of attachment 326776 [details] [review]: Go ahead with these, thanks.
Review of attachment 326777 [details] [review]: Sure. Seems fine. Thanks.
Review of attachment 326778 [details] [review]: I guess the tweaks in gthread-posix.c are for the case where we use pthreads on Windows? If so, then this is OK too. Please commit.
Attachment 326776 [details] pushed as e4aaae4 - glib: Add 2.50 availibity macros Attachment 326777 [details] pushed as 5974428 - Add g_abort() Attachment 326778 [details] pushed as e47904a - Use g_abort() instead of abort() where possible
Breaks the build in GContinuous: http://build.gnome.org/continuous/buildmaster/builds/2016/04/27/27/build/ Allison, can you take this?
Should have been fixed by commit: https://git.gnome.org/browse/glib/commit/?id=2f05d1454e0ce106b23a3d9626950602bd609fea as now GLIB_VERSION_MAX_ALLOWED should be GLIB_VERSION_2_50.
Ah, now the breakage is: ../../glib/gutils.c:2408:14: error: macro "g_abort" passed 1 arguments, but takes just 0 g_abort (void) ^ ../../glib/gutils.c:2409:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token { ^
Should be fixed in master.