Created attachment 259465 [details] [review]
gtestutils: fix g_test_set_nonfatal_assertions()

g_test_set_nonfatal_assertions() was a no-op, because
g_assertion_message() wasn't actually checking the
test_nonfatal_assertions flag. Fix that and add a test.

Also, g_test_set_nonfatal_assertions() has to set test_mode_fatal to
FALSE as well, or else a failed assertion will cause the test program
to abort at the end of the failed test.

Also, belatedly add this and the new g_assert_* methods to the docs.

Created attachment 259466 [details] [review]
gtestutils: add g_assert_nonnull() to go with g_assert_null()

Review of attachment 259465 [details] [review]:

::: glib/gtestutils.c
@@ +2292,1 @@
   __glib_assert_msg = (char*) malloc (strlen (s) + 1);

You've created a use-after-free here for the case where we still do have fatal assertions.

Review of attachment 259466 [details] [review]:

Okay.

Hmm, there's a bit of a mess now.

It seems Stef has introduced _g_log_set_exit_on_fatal and _g_log_abort in the meantime, which kinda overlaps this. And if you look around, we have

g_assertion_message call _g_log_abort and
g_assertion_message_expr call abort, right next to each other

:-(

Created attachment 261317 [details] [review]
gtestutils: fix g_test_set_nonfatal_assertions()

g_test_set_nonfatal_assertions() was a no-op, because
g_assertion_message() wasn't actually checking the
test_nonfatal_assertions flag. Fix that and add a test.

Also, g_test_set_nonfatal_assertions() has to set test_mode_fatal to
FALSE as well, or else a failed assertion will cause the test program
to abort at the end of the failed test.

Also, belatedly add this and the new g_assert_* methods to the docs.

Created attachment 261318 [details] [review]
gtestutils: add g_assert_nonnull() to go with g_assert_null()

Created attachment 261319 [details] [review]
Reorganize the "don't dump core from test subprocesses" code.

g_test_init() was calling _g_messages_set_exit_on_fatal() from
subprocesses, to make fatal log messages call _exit() rather than
abort(), but the function name is sort of confusing, and we don't
really need it anyway, since g_log() can just call g_test_subprocess()
instead and decide for itself.

Likewise, update g_assertion_message() to do the check itself, rather
than calling into gmessages to do it, and fix
g_assertion_message_expr() to also check whether it should exit or
abort. (Previously it always called abort(), although this didn't
actually matter since that was dead code until
test_nonfatal_assertions was added.)

(In reply to comment #0)
> Also, while trying to port some libsoup tests, I kept wanting
> g_assert_nonnull() to go with g_assert_null(), so I added that. (Or
> should it be "non_null"?)

Since we're talking about "nonfatal" assertions, it should be "nonnull" I guess.

(In reply to comment #3)
> You've created a use-after-free here for the case where we still do have fatal
> assertions.

fixed

(In reply to comment #5)
> Hmm, there's a bit of a mess now.

It's not as bad as it looked at first glance, and the third commit should clean up what mess there is.

Review of attachment 261318 [details] [review]:

sure, ok

Review of attachment 261317 [details] [review]:

::: docs/reference/glib/glib-sections.txt
@@ +2981,1 @@
 

oops, thanks for catching this oversight

::: glib/gtestutils.c
@@ +2291,2 @@
   __glib_assert_msg = (char*) malloc (strlen (s) + 1);
   strcpy (__glib_assert_msg, s);

I wonder if it wouldn't be better to use a small amount of static memory for this

Review of attachment 261317 [details] [review]:

.

Review of attachment 261319 [details] [review]:

::: glib/gmessages-private.h
@@ -26,3 @@
-
-G_GNUC_INTERNAL void
-_g_log_set_exit_on_fatal (void);

G_GNUC_INTERNAL is no longer needed - we switched to a model where exporting is explicit.

::: glib/gtestutils.c
@@ +2306,3 @@
   g_free (s);
+
+  if (test_in_subprocess)

Would be good to have a comment here explaining that this is about avoiding coredumps

(In reply to comment #13)
> -G_GNUC_INTERNAL void
> -_g_log_set_exit_on_fatal (void);
> 
> G_GNUC_INTERNAL is no longer needed

That code is being removed, not added.

> Would be good to have a comment here explaining that this is about avoiding
> coredumps

Added a comment there and in gmessages and pushed.

commit e53caad4f139e2df0b34dae5864576796dda514e
Author: Dan Winship <danw@gnome.org>
Date:   Wed Nov 27 10:57:43 2013 -0500

    Fix a warning about _g_log_abort()
    
    G_BREAKPOINT is not noreturn, so make it so that we abort() if it
    returns, to make _g_log_abort() be properly noreturn again.


This completely breaks the breakpoint log_abort() usage (which I am using extensively, to skip some warnings/criticals (especially those deprecated created by third party libraries, such as gtk/gio ;).

reopening

um? can you clarify exactly what you're doing? It sounds vastly unsupported.

Created attachment 263394 [details] [review]
gmessages: make _g_log_abort() do only breakpoints again

Commit e53caad4 makes _g_log_abort() noreturn by calling abort()
unconditionally.

However, it is useful to be able to skip some log_abort() with a
debugger, to reach a point of interest. Revert back to previous
behaviour. Make g_assert_warning() noreturn by calling abort().

(In reply to comment #16)
> um? can you clarify exactly what you're doing? It sounds vastly unsupported.

run a glib program under gdb with SPICE_ABORT_LEVEL=2 for ex, and skip a few warnings until I reach the one I want to fix.

If _g_log_abort() always abort() I don't see the point of using breakpoint in the first place.

Sorry, I meant G_DEBUG=fatal_warnings (I have the 2 in the same alias)

oh, I thought you meant you were *programmatically* skipping fatal warnings or something. Yes, assuming that glib compiles without warnings after your patch, it looks good.

Attachment 263394 [details] pushed as bff76bc - gmessages: make _g_log_abort() do only breakpoints again