[PATCH 21.5] assert_failed should not return

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(a)xemacs.org&gt; * 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/ _______________________________________________ XEmacs-Beta mailing list XEmacs-Beta(a)xemacs.org http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta