Re: Dynamic modules: MODULE_HANDLE_SIGNALS etc.

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

Daniel Colascione wrote:
> It's *critical* not to violate the invariants of code.

Sure, but we are discussing what those invariants should be; they are not carved 
in stone.  One possible invariant is (A) "stack overflow never happens". Another 
is (B) "if stack overflow happens, callers must tolerate being longjmped 
through". Either invariant is reasonable per se. It is a judgment call as to 
which invariant is better for Emacs. Possibly some modules will prefer (A) and 
others (B).

Take the regular expression code as an example. Suppose it has unusual 
worst-case behavior that can grow the stack in arbitrary ways (which I think it 
does though I'm not going to investigate the details right now). One way to 
address this is to rewrite the code so that it doesn't have the behavior, but 
that would be a pain; the code has been that way for decades and is crufty at 
this point and a lot of Emacs depends on its quirks. Another way to address it 
is to use a guard page or whatever on the halfway-decent platforms that support 
that sort of thing. We've chosen the latter, i.e., we've chosen invariant (B), 
and yes there are problems with this approach but it beats doing nothing and it 
beats doing (A) because nobody has had the time to do (A), assuming it's doable 
at all.

> It should be possible to replace the printfs
> in this instance with calls to write(1, "message") (which will bypass
> any output buffering) and restore async-signal-safety.

Good point. I did that with the attached patch to emacs-25. However, this 
doesn't address the Fdo_auto_save () issue in the same neighborhood.

> If a user elects to attempt auto-save in this situation, that's on him.

Sure, and Emacs already asks the user whether to auto-save in that situation, so 
this should be OK already.

> Ideally, we'd make autosave async-signal-safe, which will help in this
> handler and in the segfault hander.

Yes, that'd be good, if we didn't lose functionality thereby.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-stdio-in-SIGINT-handler.patch --]
[-- Type: text/x-diff; name="0001-Avoid-stdio-in-SIGINT-handler.patch", Size: 11744 bytes --]

From d8a33374cfa7deaf1e2fd4762c59bc6607850f65 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 3 Jan 2016 15:00:49 -0800
Subject: [PATCH] Avoid stdio in SIGINT handler

* admin/merge-gnulib (GNULIB_MODULES): Add ignore-value.
* lib/gnulib.mk, m4/gnulib-comp.m4: Regenerate.
* lib/ignore-value.h: New file, from gnulib.
* src/keyboard.c: Include it.
(write_stdout, read_stdin): New functions.
(handle_interrupt): Use them instead of printf and getchar,
and avoid fflush when handling signals.
---
 admin/merge-gnulib |  2 +-
 lib/gnulib.mk      |  9 ++++++-
 lib/ignore-value.h | 50 +++++++++++++++++++++++++++++++++++++
 m4/gnulib-comp.m4  |  2 ++
 src/keyboard.c     | 73 +++++++++++++++++++++++++++++++++++-------------------
 5 files changed, 108 insertions(+), 28 deletions(-)
 create mode 100644 lib/ignore-value.h

diff --git a/admin/merge-gnulib b/admin/merge-gnulib
index 363bb23..40b5b78 100755
--- a/admin/merge-gnulib
+++ b/admin/merge-gnulib
@@ -32,7 +32,7 @@ GNULIB_MODULES='
   dtoastr dtotimespec dup2 environ execinfo faccessat
   fcntl fcntl-h fdatasync fdopendir filemode fstatat fsync
   getloadavg getopt-gnu gettime gettimeofday gitlog-to-changelog
-  intprops largefile lstat
+  ignore-value intprops largefile lstat
   manywarnings memrchr mkostemp mktime
   pipe2 pselect pthread_sigmask putenv qcopy-acl readlink readlinkat
   sig2str socklen stat-time stdalign stddef stdio
diff --git a/lib/gnulib.mk b/lib/gnulib.mk
index 97ed5b1..b920cbb 100644
--- a/lib/gnulib.mk
+++ b/lib/gnulib.mk
@@ -21,7 +21,7 @@
 # the same distribution terms as the rest of that program.
 #
 # Generated by gnulib-tool.
-# Reproduce by: gnulib-tool --import --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=close --avoid=dup --avoid=fchdir --avoid=flexmember --avoid=fstat --avoid=malloc-posix --avoid=msvc-inval --avoid=msvc-nothrow --avoid=open --avoid=openat-die --avoid=opendir --avoid=raise --avoid=save-cwd --avoid=select --avoid=setenv --avoid=sigprocmask --avoid=stdarg --avoid=stdbool --avoid=threadlib --avoid=unsetenv --makefile-name=gnulib.mk --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca-opt binary-io byteswap c-ctype c-strcase careadlinkat close-stream count-one-bits count-trailing-zeros crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 dtoastr dtotimespec dup2 environ execinfo faccessat fcntl fcntl-h fdatasync fdopendir filemode fstatat fsync getloadavg getopt-gnu gettime gettimeofday gitlog-to-changelog intprops largefile lstat manywarnings memrchr mkostemp mktime pipe2 pselect pthread_sigmask putenv qcopy-acl readlink readlinkat sig2str socklen stat-time stdalign stddef stdio stpcpy strftime strtoimax strtoumax symlink sys_stat sys_time time time_r time_rz timegm timer-time timespec-add timespec-sub unsetenv update-copyright utimens vla warnings
+# Reproduce by: gnulib-tool --import --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=close --avoid=dup --avoid=fchdir --avoid=flexmember --avoid=fstat --avoid=malloc-posix --avoid=msvc-inval --avoid=msvc-nothrow --avoid=open --avoid=openat-die --avoid=opendir --avoid=raise --avoid=save-cwd --avoid=select --avoid=setenv --avoid=sigprocmask --avoid=stdarg --avoid=stdbool --avoid=threadlib --avoid=unsetenv --makefile-name=gnulib.mk --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca-opt binary-io byteswap c-ctype c-strcase careadlinkat close-stream count-one-bits count-trailing-zeros crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 dtoastr dtotimespec dup2 environ execinfo faccessat fcntl fcntl-h fdatasync fdopendir filemode fstatat fsync getloadavg getopt-gnu gettime gettimeofday gitlog-to-changelog ignore-value intprops largefile lstat manywarnings memrchr mkostemp mktime pipe2 pselect pthread_sigmask putenv qcopy-acl readlink readlinkat sig2str socklen stat-time stdalign stddef stdio stpcpy strftime strtoimax strtoumax symlink sys_stat sys_time time time_r time_rz timegm timer-time timespec-add timespec-sub unsetenv update-copyright utimens vla warnings
 
 
 MOSTLYCLEANFILES += core *.stackdump
@@ -567,6 +567,13 @@ EXTRA_libgnu_a_SOURCES += group-member.c
 
 ## end   gnulib module group-member
 
+## begin gnulib module ignore-value
+
+
+EXTRA_DIST += ignore-value.h
+
+## end   gnulib module ignore-value
+
 ## begin gnulib module intprops
 
 
diff --git a/lib/ignore-value.h b/lib/ignore-value.h
new file mode 100644
index 0000000..6713d96
--- /dev/null
+++ b/lib/ignore-value.h
@@ -0,0 +1,50 @@
+/* ignore a function return without a compiler warning.  -*- coding: utf-8 -*-
+
+   Copyright (C) 2008-2016 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Written by Jim Meyering, Eric Blake and Pádraig Brady.  */
+
+/* Use "ignore_value" to avoid a warning when using a function declared with
+   gcc's warn_unused_result attribute, but for which you really do want to
+   ignore the result.  Traditionally, people have used a "(void)" cast to
+   indicate that a function's return value is deliberately unused.  However,
+   if the function is declared with __attribute__((warn_unused_result)),
+   gcc issues a warning even with the cast.
+
+   Caution: most of the time, you really should heed gcc's warning, and
+   check the return value.  However, in those exceptional cases in which
+   you're sure you know what you're doing, use this function.
+
+   For the record, here's one of the ignorable warnings:
+   "copy.c:233: warning: ignoring return value of 'fchown',
+   declared with attribute warn_unused_result".  */
+
+#ifndef _GL_IGNORE_VALUE_H
+#define _GL_IGNORE_VALUE_H
+
+/* Normally casting an expression to void discards its value, but GCC
+   versions 3.4 and newer have __attribute__ ((__warn_unused_result__))
+   which may cause unwanted diagnostics in that case.  Use __typeof__
+   and __extension__ to work around the problem, if the workaround is
+   known to be needed.  */
+#if 3 < __GNUC__ + (4 <= __GNUC_MINOR__)
+# define ignore_value(x) \
+    (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))
+#else
+# define ignore_value(x) ((void) (x))
+#endif
+
+#endif
diff --git a/m4/gnulib-comp.m4 b/m4/gnulib-comp.m4
index 69920a8..27ca70a 100644
--- a/m4/gnulib-comp.m4
+++ b/m4/gnulib-comp.m4
@@ -91,6 +91,7 @@ AC_DEFUN([gl_EARLY],
   # Code from module gettimeofday:
   # Code from module gitlog-to-changelog:
   # Code from module group-member:
+  # Code from module ignore-value:
   # Code from module include_next:
   # Code from module intprops:
   # Code from module inttypes-incomplete:
@@ -905,6 +906,7 @@ AC_DEFUN([gl_FILE_LIST], [
   lib/gettimeofday.c
   lib/gl_openssl.h
   lib/group-member.c
+  lib/ignore-value.h
   lib/intprops.h
   lib/inttypes.in.h
   lib/lstat.c
diff --git a/src/keyboard.c b/src/keyboard.c
index fcafd0b..6bdfc1a 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -64,6 +64,8 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <unistd.h>
 #include <fcntl.h>
 
+#include <ignore-value.h>
+
 #ifdef HAVE_WINDOW_SYSTEM
 #include TERM_HEADER
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -10206,6 +10208,21 @@ deliver_interrupt_signal (int sig)
   deliver_process_signal (sig, handle_interrupt_signal);
 }
 
+/* Output MSG directly to standard output, without buffering.  Ignore
+   failures.  This is safe in a signal handler.  */
+static void
+write_stdout (char const *msg)
+{
+  ignore_value (write (STDOUT_FILENO, msg, strlen (msg)));
+}
+
+/* Read a byte from stdin, without buffering.  Safe in signal handlers.  */
+static int
+read_stdin (void)
+{
+  char c;
+  return read (STDIN_FILENO, &c, 1) == 1 ? c : EOF;
+}
 
 /* If Emacs is stuck because `inhibit-quit' is true, then keep track
    of the number of times C-g has been requested.  If C-g is pressed
@@ -10242,9 +10259,9 @@ handle_interrupt (bool in_signal_handler)
 	  sigemptyset (&blocked);
 	  sigaddset (&blocked, SIGINT);
 	  pthread_sigmask (SIG_BLOCK, &blocked, 0);
+	  fflush (stdout);
 	}
 
-      fflush (stdout);
       reset_all_sys_modes ();
 
 #ifdef SIGTSTP
@@ -10260,8 +10277,9 @@ handle_interrupt (bool in_signal_handler)
       /* Perhaps should really fork an inferior shell?
 	 But that would not provide any way to get back
 	 to the original shell, ever.  */
-      printf ("No support for stopping a process on this operating system;\n");
-      printf ("you can continue or abort.\n");
+      write_stdout ("No support for stopping a process"
+		    " on this operating system;\n"
+		    "you can continue or abort.\n");
 #endif /* not SIGTSTP */
 #ifdef MSDOS
       /* We must remain inside the screen area when the internal terminal
@@ -10272,46 +10290,49 @@ handle_interrupt (bool in_signal_handler)
 	 the code used for auto-saving doesn't cope with the mark bit.  */
       if (!gc_in_progress)
 	{
-	  printf ("Auto-save? (y or n) ");
-	  fflush (stdout);
-	  if (((c = getchar ()) & ~040) == 'Y')
+	  write_stdout ("Auto-save? (y or n) ");
+	  c = read_stdin ();
+	  if ((c & 040) == 'Y')
 	    {
 	      Fdo_auto_save (Qt, Qnil);
 #ifdef MSDOS
-	      printf ("\r\nAuto-save done");
-#else /* not MSDOS */
-	      printf ("Auto-save done\n");
-#endif /* not MSDOS */
+	      write_stdout ("\r\nAuto-save done");
+#else
+	      write_stdout ("Auto-save done\n");
+#endif
 	    }
-	  while (c != '\n') c = getchar ();
+	  while (c != '\n')
+	    c = read_stdin ();
 	}
       else
 	{
 	  /* During GC, it must be safe to reenable quitting again.  */
 	  Vinhibit_quit = Qnil;
+	  write_stdout
+	    (
 #ifdef MSDOS
-	  printf ("\r\n");
-#endif /* not MSDOS */
-	  printf ("Garbage collection in progress; cannot auto-save now\r\n");
-	  printf ("but will instead do a real quit after garbage collection ends\r\n");
-	  fflush (stdout);
+	     "\r\n"
+#endif
+	     "Garbage collection in progress; cannot auto-save now\r\n"
+	     "but will instead do a real quit"
+	     " after garbage collection ends\r\n");
 	}
 
 #ifdef MSDOS
-      printf ("\r\nAbort?  (y or n) ");
-#else /* not MSDOS */
-      printf ("Abort (and dump core)? (y or n) ");
-#endif /* not MSDOS */
-      fflush (stdout);
-      if (((c = getchar ()) & ~040) == 'Y')
+      write_stdout ("\r\nAbort?  (y or n) ");
+#else
+      write_stdout ("Abort (and dump core)? (y or n) ");
+#endif
+      c = read_stdin ();
+      if ((c & ~040) == 'Y')
 	emacs_abort ();
-      while (c != '\n') c = getchar ();
+      while (c != '\n')
+	c = read_stdin ();
 #ifdef MSDOS
-      printf ("\r\nContinuing...\r\n");
+      write_stdout ("\r\nContinuing...\r\n");
 #else /* not MSDOS */
-      printf ("Continuing...\n");
+      write_stdout ("Continuing...\n");
 #endif /* not MSDOS */
-      fflush (stdout);
       init_all_sys_modes ();
     }
   else
-- 
2.5.0