[Top][All Lists]

[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);
   +      maybe_quit ();
         while (n > 0)

regex.c calls maybe_quit internally, so why do we need this additional

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

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)
   -      /* 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: 0001-Remove-immediate_quit.patch
Description: Source code patch

Attachment: 0002-Revamp-quitting-and-fix-infloops.patch
Description: Source code patch

Attachment: 0003-Fix-quitting-bug-when-buffers-are-frozen.patch
Description: Source code patch

reply via email to

[Prev in Thread] Current Thread [Next in Thread]