GNU bug report logs - #12471
Avoid some signal-handling races, and simplify.

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: lekktu <at> gmail.com, 12471 <at> debbugs.gnu.org
Subject: bug#12471: Avoid some signal-handling races, and simplify.
Date: Wed, 19 Sep 2012 19:18:41 +0300

> Date: Tue, 18 Sep 2012 16:39:18 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: Eli Zaretskii <eliz <at> gnu.org>, Juanma Barranquero <lekktu <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.