Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 4952 bytes --]

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.

[-- Attachment #2: 0001-Remove-immediate_quit.patch --]
[-- Type: application/x-patch, Size: 34434 bytes --]

[-- Attachment #3: 0002-Revamp-quitting-and-fix-infloops.patch --]
[-- Type: application/x-patch, Size: 49937 bytes --]

[-- Attachment #4: 0003-Fix-quitting-bug-when-buffers-are-frozen.patch --]
[-- Type: application/x-patch, Size: 16986 bytes --]