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

[Paul Eggert]

On 09/20/2012 10:44 AM, Eli Zaretskii wrote:
> The only volatile variable that is involved is
> interrupt_input_blocked, which is a global variable.  How does a
> function help in this regard?

UNBLOCK_INPUT (the macro in the current trunk) accesses that variable
four times in the usual case.  unblock_input (the function in the
proposed patch) accesses it just twice.  This lessens the number of
race conditions and makes the code run a bit faster.  (It doesn't
eliminate the races, but that's OK, one bug fix at a time.)

unblock_input does fewer accesses by using a local variable 'level'.
Local variables like that are cleaner when they're in functions.  One
can put them into macros too, but then one has the problem of capture.
It's a relatively minor thing; it could still be done with a macro.
But since the whole thing basically needed to be rewritten anyway, I
figured we might as well do it more cleanly, with a fucntion.

> can we use 'pthread_kill' as you show above, instead of 'raise'?

That would not work on older POSIXish hosts that lack pthread_kill.
In contrast, 'raise' should be portable to all C89-or-better hosts.

> On Windows, 'kill' is redefined to call 'emacs_abort', when its
> first arg is Emacs's own PID and the second arg is SIGABRT.

For consistency, how about if Windows also defines 'raise' to call
'emacs_abort', when the signal is SIGABRT?  That should fix the
problem, and it shouldn't require any change to the mainline code.  It
could be something as simple as this in a Windows-specific include
file, perhaps:

        #define raise(sig) kill (getpid (), sig)

> And I don't want to redefine yet another library function, if it can
> be avoided.

But it does seem the simpler approach here, overall.

> I think this feature of producing a backtrace on a fatal
> error will be hated by both users and Emacs maintainers,
> but that's another subject for another thread.

> Can we do better than that?  Ideally, there should be a single
> function, the signal handler, which does everything.

I tried that initially but the code was bulkier and more error-prone.
Each handler needed to have an extra local variable (to save errno)
and to call two auxiliary-library functions, one for setup and the
other for teardown.  In contrast, the proposed approach requires no
additional locals and one auxiliary-library function.  It's true that
it requires two functions for each per-process signal, one for the
thread catching the signal and one for the main process, but I find
that actually simpler, since it makes it clearer which handler code is
run where.  Anyway, pointers to signal handlers are standard practice
in POSIXish code, and it's not particularly exotic to pass such a
pointer around or to call the handler via the pointer.

> Or maybe it would work to set up things so that no non-main threads
> ever get any signals

Yes, that was my first thought too!  I'd rather do that; it'd be simpler.

> is there some pthread_* function that can
> setup a callback to run each time a thread is created?

Unfortunately not, which is why I settled with the proposed approach


> Given that SIGFPE will not happen due to calculations, how about
> changing its handler to call fatal_error_backtrace, rather than signal
> a Lisp-level error?

Yes, that makes sense, and it's what the proposed patch already
does on IEEE hosts.  But I see that the code isn't very clear
about that.  I'll replace it with the following, which I hope
makes it clearer.

  /* Typically SIGFPE is thread-specific and is fatal, like SIGILL.
     But on a non-IEEE host SIGFPE can come from a trap in the Lisp
     interpreter's floating point operations, so treat SIGFPE as an
     arith-error if it arises in the main thread.  */
  if (IEEE_FLOATING_POINT)
    sigaction (SIGFPE, &thread_fatal_action, 0);
  else
    {
      emacs_sigaction_init (&action, deliver_arith_signal);
      sigaction (SIGFPE, &action, 0);
    }