bug#12316: Simplify redefinition of 'abort'.

[Paul Eggert]

On 08/31/2012 07:29 AM, Eli Zaretskii wrote:

> why not simply replace all the calls
> to 'abort' with calls to 'emacs_abort'?  Isn't that cleaner?

Yes, I prefer that, too.  I avoided that only because
it bloated the patch.  But it's easy enough to add, and
the attached patch does that.

>> To keep in sync with that, the MS-Windows part of the patch
>> renames w32_abort to emacs_abort.
>
> I don't see this part in your patches.  Where is it?

Sorry, I missed one place.  Fixed in attached patch.
This also should fix the HAVE_NTGUI issue you reported in
a separate email.

> with your changes, only those calls to 'abort' that
> originate within Emacs sources will wind up at 'emacs_abort'.

Yes, that's part of the idea.  Intercepting other libraries'
call to 'abort' tends to break things.  We've caught some of
the breakages and have disabled them with NO_ABORT but it's
safer to avoid the problem in the first place.  Doing this
will also make it a bit easier for me to add a new feature,
namely backtraces on production aborts.

> A standard header should not include another
> standard header

That guideline is needed for glibc for conformance reasons,
and it used to be important long ago when multiple inclusion
of standard headers was a problem, but these days it's not
necessary and is even somewhat counterproductive in
portability emulations.  We violate that guideline all the
time in gnulib (and therefore in Emacs) and it's not a
problem.  However, if you prefer using the glibc style
for the Microsoft port that's easy to do I've revised
the patch to do that.

The attached patch is fairly long because of the abort ->
emacs_abort thing, so here's a precis that skips the boring
parts.

=== modified file 'ChangeLog'
--- ChangeLog   2012-08-28 16:01:59 +0000
+++ ChangeLog   2012-08-31 03:18:42 +0000
@@ -1,3 +1,8 @@
+2012-08-31  Paul Eggert  <eggert@cs.ucla.edu>
+
+       Simplify redefinition of 'abort' (Bug#12316).
+       * configure.ac (NO_ABRT): Remove.
+
 2012-08-26  Paul Eggert  <eggert@cs.ucla.edu>

        * configure.ac (CFLAGS): Prefer -g3 to -g if -g3 works

=== modified file 'configure.ac'
--- configure.ac        2012-08-28 16:01:59 +0000
+++ configure.ac        2012-08-30 18:23:15 +0000
@@ -3321,12 +3321,6 @@
     AC_DEFINE(BROKEN_PTY_READ_AFTER_EAGAIN, 1, [Define on FreeBSD to
       work around an issue when reading from a PTY.])
     ;;
-
-  dnl Define the following so emacs symbols will not conflict with those
-  dnl in the System framework.  Otherwise -prebind will not work.
-  darwin)
-    AC_DEFINE(NO_ABORT, 1, [Do not define abort in emacs.c.])
-    ;;
 esac

 case $opsys in

=== modified file 'nt/ChangeLog'
--- nt/ChangeLog        2012-08-28 16:01:59 +0000
+++ nt/ChangeLog        2012-08-31 16:26:56 +0000
@@ -1,3 +1,8 @@
+2012-08-31  Paul Eggert  <eggert@cs.ucla.edu>
+
+       Simplify redefinition of 'abort' (Bug#12316).
+       * inc/ms-w32.h (w32_abort) [HAVE_NTGUI]: Remove.
+
 2012-08-22  Juanma Barranquero  <lekktu@gmail.com>

        * config.nt: Sync with autogen/config.in.

=== modified file 'nt/inc/ms-w32.h'
--- nt/inc/ms-w32.h     2012-08-07 11:03:48 +0000
+++ nt/inc/ms-w32.h     2012-08-30 22:25:36 +0000
@@ -334,16 +334,7 @@
 #include <malloc.h>
 #endif

-/* stdlib.h must be included after redefining malloc & friends, but
-   before redefining abort.  Isn't library redefinition funny?  */
 #include <stdlib.h>
-
-/* Redefine abort.  */
-#ifdef HAVE_NTGUI
-#define abort  w32_abort
-extern _Noreturn void w32_abort (void);
-#endif
-
 #include <sys/stat.h>

 /* Define for those source files that do not include enough NT system files.  
*/

=== modified file 'nt/inc/unistd.h'
--- nt/inc/unistd.h     2011-02-27 19:48:31 +0000
+++ nt/inc/unistd.h     2012-08-31 16:31:23 +0000
@@ -3,8 +3,12 @@
 #ifndef _UNISTD_H
 #define _UNISTD_H

+/* On Microsoft platforms, <stdlib.h> declares 'environ'; on POSIX
+   platforms, <unistd.h> does.  Every file in Emacs that includes
+   <unistd.h> also includes <stdlib.h>, so there's no need to declare
+   'environ' here.  */
+
 extern ssize_t readlink (const char *, char *, size_t);
 extern int symlink (char const *, char const *);

 #endif /* _UNISTD_H */
-

=== modified file 'src/.gdbinit'
--- src/.gdbinit        2012-08-20 17:32:31 +0000
+++ src/.gdbinit        2012-08-30 19:23:45 +0000
@@ -1222,14 +1222,9 @@
   set $tem = (struct Lisp_String *) $ptr
   set $tem = (char *) $tem->data

-  # Don't let abort actually run, as it will make stdio stop working and
-  # therefore the `pr' command above as well.
-  if $tem[0] == 'w' && $tem[1] == 'i' && $tem[2] == 'n' && $tem[3] == 'd'
-    # The windows-nt build replaces abort with its own function.
-    break w32_abort
-  else
-    break abort
-  end
+  # Don't let emacs_abort actually run, as it will make stdio stop
+  # working and therefore the 'pr' command above as well.
+  break emacs_abort
 end

 # x_error_quitter is defined only on X.  But window-system is set up

=== modified file 'src/ChangeLog'
--- src/ChangeLog       2012-08-31 10:53:19 +0000
+++ src/ChangeLog       2012-08-31 16:34:18 +0000
@@ -1,3 +1,23 @@
+2012-08-31  Paul Eggert  <eggert@cs.ucla.edu>
+
+       Simplify redefinition of 'abort' (Bug#12316).
+       Do not try to redefine the 'abort' function.  Instead, redo
+       the code so that it calls 'emacs_abort' rather than 'abort'.
+       This removes the need for the NO_ABORT configure-time macro
+       and makes it easier to change the abort code to do a backtrace.
+       * .gdbinit: Just stop at emacs_abort, not at w32_abort or abort.
+       * emacs.c (abort) [!DOS_NT && !NO_ABORT]:
+       Remove; sysdep.c's emacs_abort now takes its place.
+       * lisp.h (emacs_abort): New decl.  All calls from Emacs code to
+       'abort' changed to use 'emacs_abort'.
+       * msdos.c (dos_abort) [defined abort]: Remove; not used.
+       (abort) [!defined abort]: Rename to ...
+       (emacs_abort): ... new name.
+       * sysdep.c (emacs_abort) [!HAVE_NTGUI]: New function, taking
+       the place of the old 'abort' in emacs.c.
+       * w32.c, w32fns.c (abort): Do not #undef.
+       * w32.c (emacs_abort): Rename from w32_abort.
+
 2012-08-31  Dmitry Antipov  <dmantipov@yandex.ru>

        Remove mark_ttys function and fix tty_display_info initialization.

=== modified file 'src/conf_post.h'
--- src/conf_post.h     2012-08-20 16:48:10 +0000
+++ src/conf_post.h     2012-08-31 14:47:20 +0000
@@ -178,9 +178,6 @@
 #endif

 #include <string.h>
-/* If you think about removing the line below, note that the
-   MS-Windows build relies on it for declaration of 'environ' needed
-   by a few source files.  */
 #include <stdlib.h>

 #if __GNUC__ >= 3  /* On GCC 3.0 we might get a warning.  */

=== modified file 'src/emacs.c'
--- src/emacs.c 2012-08-25 06:38:43 +0000
+++ src/emacs.c 2012-08-31 14:58:40 +0000
@@ -345,22 +345,6 @@
 }
 #endif

-/* We define abort, rather than using it from the library,
-   so that GDB can return from a breakpoint here.
-   MSDOS has its own definition in msdos.c.  */
-
-#if ! defined (DOS_NT) && ! defined (NO_ABORT)
-
-void
-abort (void)
-{
-  kill (getpid (), SIGABRT);
-  /* This shouldn't be executed, but it prevents a warning.  */
-  exit (1);
-}
-#endif
-
-
 /* Code for dealing with Lisp access to the Unix command line.  */

 static void

=== modified file 'src/msdos.c'
--- src/msdos.c 2012-08-21 10:21:04 +0000
+++ src/msdos.c 2012-08-31 14:56:42 +0000
@@ -4215,26 +4215,8 @@
 }
 #endif

-#ifdef abort
-#undef abort
 void
-dos_abort (char *file, int line)
-{
-  char buffer1[200], buffer2[400];
-  int i, j;
-
-  sprintf (buffer1, "<EMACS FATAL ERROR IN %s LINE %d>", file, line);
-  for (i = j = 0; buffer1[i]; i++) {
-    buffer2[j++] = buffer1[i];
-    buffer2[j++] = 0x70;
-  }
-  dosmemput (buffer2, j, (int)ScreenPrimary);
-  ScreenSetCursor (2, 0);
-  abort ();
-}
-#else
-void
-abort (void)
+emacs_abort (void)
 {
   dos_ttcooked ();
   ScreenSetCursor (10, 0);
@@ -4250,7 +4232,6 @@
 #endif /* __DJGPP_MINOR__ >= 2 */
   exit (2);
 }
-#endif

 void
 syms_of_msdos (void)

=== modified file 'src/sysdep.c'
--- src/sysdep.c        2012-08-18 02:49:24 +0000
+++ src/sysdep.c        2012-08-30 22:25:36 +0000
@@ -1838,6 +1838,15 @@
 }
 #endif
 
+#ifndef HAVE_NTGUI
+/* Using emacs_abort lets GDB return from a breakpoint here.  */
+void
+emacs_abort (void)
+{
+  (abort) ();
+}
+#endif
+
 int
 emacs_open (const char *path, int oflag, int mode)
 {

=== modified file 'src/w32.c'
--- src/w32.c   2012-08-26 10:29:37 +0000
+++ src/w32.c   2012-08-31 15:03:25 +0000
@@ -6640,8 +6640,7 @@
                      buffer,
                      "Emacs Abort Dialog",
                      MB_OK | MB_ICONEXCLAMATION | MB_TASKMODAL);
-      /* Use the low-level Emacs abort. */
-#undef abort
+         /* Use the low-level system abort. */
          abort ();
        }
       else

=== modified file 'src/w32fns.c'
--- src/w32fns.c        2012-08-18 06:06:39 +0000
+++ src/w32fns.c        2012-08-31 15:04:03 +0000
@@ -7194,10 +7194,8 @@
   syms_of_w32uniscribe ();
 }

-#undef abort
-
 void
-w32_abort (void)
+emacs_abort (void)
 {
   int button;
   button = MessageBox (NULL,

no-abort.txt
Description: Text document