From: Paul Eggert <eggert@cs.ucla.edu>
To: Eli Zaretskii <eliz@gnu.org>
Cc: 12471@debbugs.gnu.org, lekktu@gmail.com
Subject: bug#12471: Avoid some signal-handling races, and simplify.
Date: Fri, 21 Sep 2012 00:42:45 -0700 [thread overview]
Message-ID: <505C1A75.9030509@cs.ucla.edu> (raw)
In-Reply-To: <837grozizw.fsf@gnu.org>
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);
}
next prev parent reply other threads:[~2012-09-21 7:42 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-18 23:39 bug#12471: Avoid some signal-handling races, and simplify Paul Eggert
2012-09-19 16:18 ` Eli Zaretskii
2012-09-20 6:07 ` Paul Eggert
2012-09-20 17:44 ` Eli Zaretskii
2012-09-21 7:42 ` Paul Eggert [this message]
2012-09-21 8:31 ` Eli Zaretskii
2012-09-21 17:26 ` Paul Eggert
2012-09-21 17:38 ` Eli Zaretskii
2012-09-21 18:13 ` Paul Eggert
2012-09-21 18:27 ` Eli Zaretskii
2012-09-21 19:59 ` Paul Eggert
2012-09-22 8:02 ` Eli Zaretskii
2012-09-22 8:47 ` Paul Eggert
2012-09-22 9:10 ` Eli Zaretskii
2012-09-22 9:40 ` Paul Eggert
2012-09-22 10:07 ` Eli Zaretskii
2012-09-22 10:55 ` Paul Eggert
2012-09-22 11:16 ` Eli Zaretskii
2012-09-22 20:28 ` Stefan Monnier
2012-09-22 21:55 ` Paul Eggert
2012-09-23 3:55 ` Eli Zaretskii
2012-09-23 3:52 ` Eli Zaretskii
2012-09-19 16:45 ` Jan Djärv
2012-09-19 19:58 ` Paul Eggert
2012-09-20 6:27 ` Jan Djärv
2012-09-19 21:36 ` Andy Moreton
2012-09-23 9:36 ` bug#12471: installed into trunk Paul Eggert
2012-09-23 15:22 ` Andy Moreton
2012-09-23 16:23 ` Andy Moreton
2012-09-23 17:37 ` Juanma Barranquero
2012-09-23 17:37 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=505C1A75.9030509@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=12471@debbugs.gnu.org \
--cc=eliz@gnu.org \
--cc=lekktu@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.