On 09/19/2012 09:18 AM, Eli Zaretskii wrote:

> Why is it a good idea to replace this macro with a function?

The function makes it easier to control access to volatile variables,
that is, to access them in a minimal number of times to lessen the
number of race conditions.   This requires a local variable.  This
can also be done with macros, but functions are nicer.

An advantage of using a function, is that it shrinks the code size
(Emacs's text size is about 1.3% smaller with the patch, not all due
to this part of the change).

> The macro is used quite frequently in the code; replacing
> it with a function might slow down Emacs

Thanks for mentioning that.  I benchmarked the patch, and found out
that one of my changes (to the QUIT macro) slowed things down
significantly.  I fixed that.  The patched Emacs now runs slightly
faster than the original on my benchmark.

> A function whose name is in CAPS?  Why?

I was lazy and left the callers alone.  That's easy to fix;
revised patch attached.  It renames BLOCK_INPUT_TO to block_input_to,
etc.

>   -  kill (getpid (), fatal_error_code);
>   +  raise (fatal_error_code);
>
> If there are good reasons to prefer 'raise' to 'kill' in this context,

On POSIX hosts, the change is helpful because it fixes some
race conditions.  raise (SIGABRT) is equivalent to
pthread_kill (pthread_self (), SIGABRT), and we do want the
signal to be delivered to the main thread rather than to
some other thread (which 'kill' might do).

> 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.

Sorry, I'm not following.  By "assertion violations" do you
mean eassert failure?  If so, currently:

  eassert fails.
  'die' prints "Emacs fatal error" with some details.
  fatal_error_backtrace resets SIGABRT to SIG_DFL
  fatal_error_backtrace outputs a backtrace if available
  fatal_error_backtrace unblocks SIGABRT
  fatal_error_backtrace uses 'kill' with SIGABRT to terminate Emacs

so emacs_abort is never called.  Under the proposed patch, the
actions do basically the same thing:

  eassert fails
  'die' resets SIGABRT to SIG_DFL
  'die' prints "Emacs fatal error" with some details.
  terminate_due_to_signal resets SIGABRT to SIG_DFL
  terminate_due_to_signal outputs a backtrace if available
  terminate_due_to_signal unblocks SIGABRT
  terminate_due_to_signal uses 'raise' with SIGABRT to terminate Emacs

emacs_abort is not called in either the old code or the new.

>   -  handle_on_main_thread (sig, handle_interrupt_signal);
>   +  deliver_process_signal (sig, handle_interrupt_signal);
>    }
>
> Do we really need this double indirection?

Yes, because Gnome/gtk+ threads shouldn't execute handle_interrupt_signal
at the same time that the Emacs main thread is running.  That signal
handler doesn't do an atomic action, and if its actions are done in
parallel with the Emacs main thread, bad things could happen.
There's commentary to this effect in deliver_process_signal.

>   -#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.

Emacs has been assuming C89 for a while, and C89 specifies SIGABRT,
SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM.  These days any C implementation
that has <signal.h> should have these signals defined to something.

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

Most hosts nowadays use IEEE floating point, so they do not trap on
exceptions and don't need an exception handler.  The exception handler
is problematic (it longjmps out of a signal handler, which leads
to some real problems) and should be omitted unless it's really
needed.  I'll add a comment along those lines.

>   -  return ret;
>   +  return nev;
>
> This should be in a separate commit, as it is unrelated.

OK, thanks, I did that in trunk bzr 110104 and 110106.  It still seems
to me that the windows socket hook isn't returning the proper positive
value when it should, but evidently the code's been working with it
returning 0 so I'm not sure how high a priority it is to look into
that.

Thanks for the review.  Revised patch (with above comments taken into
account) attached.  This patch is relative to trunk bzr 110110.