[PATCH 21.5] assert_failed should not return

[Jerry James]

PATCH 21.5

I am sending this to xemacs-beta as well as xemacs-patches because I
expect that not everyone will be totally sanguine about this patch.
Nevertheless, I think it is a good idea.

Here is the rationale.  With the current code, the assert() macro can
return even when its argument evaluates to false.  This can happen if
fatal_error_in_progress is true when assert_failed() is entered.
Somewhere back in the mists of time, somebody had the idea that we
shouldn't shut down in that case, because user data might be lost.
Guess what?  It's lost anyway.  If we return from assert_failed(), then
the universe makes no sense to the code that called assert().  We just
determined that an assumption that code relies on does not hold.
Anything can happen.  In many cases, that "anything" is a segfault.  My
contention is that assert_failed() should not return under ANY
circumstances, otherwise we're heading into random behavior land.

FWIW, emacs behaves in the way I consider sensible.  If you trigger an
assertion failure while already handling an assertion failure or fatal
error, emacs shuts down immediately.

I'm not entirely certain that returning from a failed assert is the
highly dubious kludge to which Jamie refers; if anyone knows otherwise,
I'll leave those comments in place.


src/ChangeLog addition:

2007-09-24  Jerry James  <james at xemacs.org>

	* lisp.h: Declare that assert_failed does not return.  Drop
	unnecessary declaration of really_abort.
	* emacs.c: Ditto.
	* emacs.c (assert_failed): Don't return when
	fatal_error_in_progress.  Just exit.


xemacs-21.5 source patch:
Diff command:   cvs -q diff -uN
Files affected: src/emacs.c
===================================================================
RCS src/lisp.h
===================================================================
RCS

Index: src/lisp.h
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/lisp.h,v
retrieving revision 1.148
diff -d -u -r1.148 lisp.h
--- src/lisp.h	2007/08/21 20:32:37	1.148
+++ src/lisp.h	2007/09/25 03:11:16
@@ -1037,16 +1037,12 @@
    time the assert checks take is measurable so let's not include them
    in production binaries.

-   If ASSERTIONS_DONT_ABORT defined, we will continue after assertion
-   failures.
-
    assert_at_line() is used for asserts inside of inline functions called
    from error-checking macros.  If we're not tricky, we just get the file
    and line of the inline function, which is not very useful. */

-/* Highly dubious kludge */
-/*   (thanks, Jamie, I feel better now -- ben) */
-MODULE_API void assert_failed (const Ascbyte *, int, const Ascbyte *);
+MODULE_API DECLARE_DOESNT_RETURN (assert_failed (const Ascbyte *, int,
+						 const Ascbyte *));
 #define ABORT() (assert_failed (__FILE__, __LINE__, "ABORT()"))

 #ifdef USE_ASSERTIONS
@@ -4244,7 +4240,6 @@
 extern int suppress_early_error_handler_backtrace;
 void debug_break (void);
 int debug_can_access_memory (void *ptr, Bytecount len);
-DECLARE_DOESNT_RETURN (really_abort (void));
 void zero_out_command_line_status_vars (void);

 /* Defined in emodules.c */
Index: src/emacs.c
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/emacs.c,v
retrieving revision 1.168
diff -d -u -r1.168 emacs.c
--- src/emacs.c	2006/06/21 17:30:36	1.168
+++ src/emacs.c	2007/09/25 03:11:17
@@ -3885,13 +3885,6 @@
 /* abnormal shutdowns: assertion failures */
 /* -------------------------------------- */

-/* This flag is useful to define if you're under a debugger; this way, you
-   can put a breakpoint of assert_failed() and debug multiple problems
-   in one session without having to recompile. */
-/* #define ASSERTIONS_DONT_ABORT */
-
-/* This highly dubious kludge ... shut up Jamie, I'm tired of your slagging. */
-
 /* Nonzero if handling an assertion failure. (Bumped by one each time
    we recursively hit such a failure.) */
 static int in_assert_failed;
@@ -3907,20 +3900,15 @@
 /* This is called when an assert() fails or when ABORT() is called -- both
    of those are defined in the preprocessor to an expansion involving
    assert_failed(). */
-void
+DOESNT_RETURN
 assert_failed (const Ascbyte *file, int line, const Ascbyte *expr)
 {
-  /* If we're already crashing, let's not crash again.  This might be
-     critical to getting auto-saving working properly. */
-  if (fatal_error_in_progress)
-    return;
-
   /* We are extremely paranoid so we sensibly deal with recursive
      assertion failures. */
   in_assert_failed++;
   inhibit_non_essential_conversion_operations = 1;

-  if (in_assert_failed >= 4)
+  if (fatal_error_in_progress || in_assert_failed >= 4)
     _exit (-1);
   else if (in_assert_failed == 3)
     {
@@ -3958,7 +3946,6 @@
      get the auto-save behavior, which may be extremely important if you
      were in the middle of doing something */
   /* debugging_breakpoint (); */
-#if !defined (ASSERTIONS_DONT_ABORT)
 #if defined (_MSC_VER) || defined (CYGWIN)
   /* In VC++, calling abort() directly just seems to exit, in a way we can't
      trap. (#### The docs say it does raise (SIGABRT), which we should be
@@ -3982,12 +3969,8 @@
   * ((int *) 0) = 666;
   /* RaiseException (STATUS_ASSERTION_FAILURE, EXCEPTION_NONCONTINUABLE, 0,
 	             0); */
-#else
-  really_abort ();
 #endif /* defined (_MSC_VER) || defined (CYGWIN) */
-#endif /* !defined (ASSERTIONS_DONT_ABORT) */
-  inhibit_non_essential_conversion_operations = 0;
-  in_assert_failed = 0;
+  abort ();
 }

 /* -------------------------------------- */
@@ -4686,9 +4669,3 @@
 }

 #endif
-
-DOESNT_RETURN
-really_abort (void)
-{
-  abort ();
-}

-- 
Jerry James
http://loganjerry.googlepages.com/