[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit
From: |
Paul Eggert |
Subject: |
Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit |
Date: |
Mon, 30 Jan 2017 13:52:46 -0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 |
Thanks for reviewing the patches. I installed the little patch for delq
as that's relatively independent and was not commented on. Attached is a
revised set of patches. The first two of are the same as before
(rebased), and the third patch attempts to address several of your
comments directly. The other comments are remarked on below.
On 01/30/2017 07:33 AM, Eli Zaretskii wrote:
I don't understand why we need both this and
emacs_read, which cannot be interrupted. Why not have just emacs_read
which can be interrupted, and use that all over?
For example, filelock.c's read_lock_data opens a file, uses emacs_read
to read it, and then closes the file. If read_lock_data used
emacs_read_quit it might process a quit, which would skip the close and
leak a file descriptor.
The read_lock_data issue could be fixed by having it call
record_unwind_protect_int (close_file_unwind, fd) before calling
emacs_read. Possibly all these dicey uses of emacs_read could be fixed
in a similar way. However, that would be a bigger and more-intrusive
change, and in the read_lock_data case it arguably would be overkill and
I wanted to keep the patch smaller. I used emacs_read_quit only in
places that I verified were safe, and stuck with emacs_read when I
wasn't sure, or where more-intrusive changes would be needed.
@@ -1252,6 +1256,7 @@ search_buffer (Lisp_Object string, ptrdiff_t pos,
ptrdiff_t pos_byte,
return (n);
}
n++;
+ maybe_quit ();
}
while (n > 0)
{
regex.c calls maybe_quit internally, so why do we need this additional
call?
The regex code does not always call maybe_quit. For example, without
this additional call, (re-search-forward "[[:alpha:]]" nil nil
most-positive-fixnum) would loop indefinitely in a buffer containing
only alphabetic characters on a 64-bit platform.
@@ -724,6 +725,8 @@ back_comment (ptrdiff_t from, ptrdiff_t from_byte,
ptrdiff_t stop,
that determines quote parity to the comment-end. */
while (from != stop)
{
+ incr_rarely_quit (&quit_count);
+
Is it safe to quit from back_comment? It manipulates a global
variable gl_state, and I don't see unwind-protect calls anywhere in
sight.
It should be OK. The current master sets immediate_quit=true in
back_comment's callers (both scan_lists and Fforward_comment), so
current master already lets back_comment quit. If Emacs quits in
back_comment, it should longjmp to code that reinitializes gl_state
before using it. This also applies to the other places you mentioned.
The idea is to insert maybe_quit calls in code that was already subject
to immediate_quit=true in the current master, so it should be safe to quit.
@@ -10445,30 +10433,12 @@ handle_interrupt (bool in_signal_handler)
}
else
{
- /* If executing a function that wants to be interrupted out of
- and the user has not deferred quitting by binding `inhibit-quit'
- then quit right away. */
- if (immediate_quit && NILP (Vinhibit_quit))
- {
- struct gl_state_s saved;
-
- immediate_quit = false;
- pthread_sigmask (SIG_SETMASK, &empty_mask, 0);
- saved = gl_state;
- quit ();
- gl_state = saved;
- }
- else
- { /* Else request quit when it's safe. */
- int count = NILP (Vquit_flag) ? 1 : force_quit_count + 1;
- force_quit_count = count;
- if (count == 3)
- {
- immediate_quit = true;
- Vinhibit_quit = Qnil;
- }
- Vquit_flag = Qt;
- }
+ /* Request quit when it's safe. */
+ int count = NILP (Vquit_flag) ? 1 : force_quit_count + 1;
+ force_quit_count = count;
+ if (count == 3)
+ Vinhibit_quit = Qnil;
+ Vquit_flag = Qt;
}
This loses the feature whereby C-g on a TTY would quit much faster.
Why is this a good idea?
Speed is not a problem, as C-g (with the proposed changes) should quit
just as fast on a TTY as it already does under X, and it's been working
that way under X for some time. The good idea here is to simplify the
analysis of C code, so that reviewers no longer have to worry about
random asynchronous longjmps that depend on settings of global
variables, something that is a real pain to reason about. Instead,
quitting will work the same on a TTY as it does on a GUI, making
maintenance easier overall.
And if it is a good idea, why do we still
generate SIGINT on C-g (and force GDB to handle SIGINT specially to
support that)?
Inertia, I think. Having C-g generate SIGINT made sense when we had
immediate_quit. I expect that it is a useless appendage now, and that in
a later patch we can change Emacs so that C-g no longer generates SIGINT
but is instead processed like any other input character.
0001-Remove-immediate_quit.patch
Description: Source code patch
0002-Revamp-quitting-and-fix-infloops.patch
Description: Source code patch
0003-Fix-quitting-bug-when-buffers-are-frozen.patch
Description: Source code patch
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Stefan Monnier, 2017/01/26
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Paul Eggert, 2017/01/26
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Eli Zaretskii, 2017/01/26
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Eli Zaretskii, 2017/01/29
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Paul Eggert, 2017/01/29
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Eli Zaretskii, 2017/01/30
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit,
Paul Eggert <=
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Eli Zaretskii, 2017/01/31
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Stefan Monnier, 2017/01/31
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Paul Eggert, 2017/01/31