bug#12471: Avoid some signal-handling races, and simplify.

[Eli Zaretskii]

> Date: Tue, 18 Sep 2012 16:39:18 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Eli Zaretskii <eliz@gnu.org>, Juanma Barranquero <lekktu@gmail.com>
> 
> Attached is a patch to fix some of the problems that I found, and to
> simplify nearby signal-handling code.

Thanks.  I have a few questions/comments:

  -#define UNBLOCK_INPUT                                \
  -  do                                         \
  -    {                                                \
  -      --interrupt_input_blocked;             \
  -      if (interrupt_input_blocked == 0)              \
  -     {                                       \
  -       if (interrupt_input_pending)          \
  -         reinvoke_input_signal ();           \
  -       if (pending_atimers)                  \
  -         do_pending_atimers ();              \
  -     }                                       \
  -      else if (interrupt_input_blocked < 0)  \
  -     emacs_abort ();                         \
  -    }                                                \
  -  while (0)
  +extern void unblock_input (void);

Why is it a good idea to replace this macro with a function?  The
macro is used quite frequently in the code; replacing it with a
function might slow down Emacs, and for what good reason?

  +extern void UNBLOCK_INPUT_TO (int);

A function whose name is in CAPS?  Why?
 
   /* Report a fatal error due to signal SIG, output a backtrace of at
      most BACKTRACE_LIMIT lines, and exit.  */
   _Noreturn void
  -fatal_error_backtrace (int sig, int backtrace_limit)
  +terminate_due_to_signal (int sig, int backtrace_limit)
   {
     fatal_error_code = sig;
  -  signal (sig, SIG_DFL);

     TOTALLY_UNBLOCK_INPUT;

  @@ -316,9 +302,8 @@
       }

     /* Signal the same code; this time it will really be fatal.
  -     Remember that since we're in a signal handler, the signal we're
  -     going to send is probably blocked, so we have to unblock it if we
  -     want to really receive it.  */
  +     Since we're in a signal handler, the signal is blocked, so we
  +     have to unblock it if we want to really receive it.  */
   #ifndef MSDOS
     {
       sigset_t unblocked;
  @@ -328,7 +313,7 @@
     }
   #endif

  -  kill (getpid (), fatal_error_code);
  +  raise (fatal_error_code);

If there are good reasons to prefer 'raise' to 'kill' in this context,
can we please special-case SIGABRT here and call 'emacs_abort'
directly, at least for hosts that cannot produce the backtrace?
Otherwise, assertion violations will not end up in 'emacs_abort',
AFAICS.
 
   static void
   deliver_interrupt_signal (int sig)
   {
  -  handle_on_main_thread (sig, handle_interrupt_signal);
  +  deliver_process_signal (sig, handle_interrupt_signal);
   }

Do we really need this double indirection?  Why not call
deliver_process_signal directly (here and elsewhere)?

  -#ifdef SIGTERM
         parse_signal ("term", SIGTERM);
  -#endif

I don't understand why did you remove the conditional.  This won't
compile if SIGTERM isn't defined.  Did you assume that it is always
available?

  +  if (! IEEE_FLOATING_POINT)
  +    sigaddset (&action->sa_mask, SIGFPE);

Why is IEEE_FLOATING_POINT, which detects the representation of FP
numbers, related to SIGFPE?

  -# ifdef SIGTERM
         sys_siglist[SIGTERM] = "Terminated";
  -# endif

This won't compile if SIGTERM is not defined.

  +  maybe_fatal_sig (SIGTERM);

Same here.

  +  sigaction (SIGTERM, &process_fatal_action, 0);

And here.

  +  if (! IEEE_FLOATING_POINT)
  +    {
  +      emacs_sigaction_init (&action, deliver_arith_signal);
  +      sigaction (SIGFPE, &action, 0);
  +    }

See above: why?

  -  return ret;
  +  return nev;

This should be in a separate commit, as it is unrelated.