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

[Eli Zaretskii]

> Cc: address@hidden, address@hidden
> From: Paul Eggert <address@hidden>
> Date: Sun, 29 Jan 2017 15:05:50 -0800
> 
> The idea of the attached code is to fix the problems I recently introduced in 
> this area, along with some longstanding related bugs, e.g, (defun foo () (nth 
> most-positive-fixnum '#1=(1 . #1#))) when byte-compiled on a 64-bit X 
> platform (currently Emacs hangs and cannot be C-g'ed out of).

Thanks, some comments follow:

  +   On error, set errno to a value other than EINTR, and return -1.  */
  +static ptrdiff_t
  +emacs_nointr_read (int fd, void *buf, ptrdiff_t nbyte, bool interruptible)

The "nointr" part in the name of the function seems to be in
contradiction to what the function actually does.

More generally, 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?  I've reviewed the
places where you left the call to emacs_read, and I don't see why
those would be "unsafe" for C-g.

  @@ -198,7 +198,6 @@ call_process_cleanup (Lisp_Object buffer)
       {
         kill (-synch_process_pid, SIGINT);
         message1 ("Waiting for process to die...(type C-g again to kill it 
instantly)");
  -      maybe_quit ();
         wait_for_termination (synch_process_pid, 0, 1);

I think it would be good to add a comment here saying that
wait_for_termination will quit if the user hits C-g there.

  +INLINE void
  +rarely_quit (unsigned short int count)
  +{
  +  if (! (count & (QUIT_COUNT_HEURISTIC - 1)))
  +    maybe_quit ();
  +}
  +
  +/* Increment *QUIT_COUNT and rarely quit.  */
  +
  +INLINE void
  +incr_rarely_quit (unsigned short int *quit_count)
  +{
  +  rarely_quit (++*quit_count);
  +}

Does it really pay off to have two almost identical functions?  Why
not have just rarely_quit, and increment the counter by hand where we
need that?

  @@ -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?

  @@ -1296,6 +1301,7 @@ search_buffer (Lisp_Object string, ptrdiff_t pos, 
ptrdiff_t pos_byte,
                return (0 - n);
              }
            n--;
  +       maybe_quit ();
          }
   #ifdef REL_ALLOC
         r_alloc_inhibit_buffer_relocation (0);

This can quit without calling r_alloc_inhibit_buffer_relocation, which
will leave ralloc.c in a state where it doesn't do relocations, which
is a crash waiting to happen.

  @@ -637,6 +636,7 @@ find_defun_start (ptrdiff_t pos, ptrdiff_t pos_byte)
          }
         /* Move to beg of previous line.  */
         scan_newline (PT, PT_BYTE, BEGV, BEGV_BYTE, -2, 1);
  +      incr_rarely_quit (&quit_count);
       }

     /* Record what we found, for the next try.  */

scan_newline calls maybe_quit internally, so the call to
incr_rarely_quit shouldn't be necessary, I think.

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

  @@ -1445,6 +1448,7 @@ scan_words (register ptrdiff_t from, register EMACS_INT 
count)
              break;
            if (code == Sword)
              break;
  +       rarely_quit (from);
          }
         /* Now CH0 is a character which begins a word and FROM is the
            position of the next character.  */

Same here (and in a few more places in scan_words where you added such
calls).

  @@ -2183,17 +2194,22 @@ skip_syntaxes (bool forwardp, Lisp_Object string, 
Lisp_Object lim)
                      stop = endp;
                    }
                  UPDATE_SYNTAX_TABLE_BACKWARD (pos - 1);
  -             prev_p = p;
  -             while (--p >= stop && ! CHAR_HEAD_P (*p));
  +
  +             unsigned char *prev_p = p;
  +             do
  +               p--;
  +             while (stop <= p && ! CHAR_HEAD_P (*p));
  +
                  c = STRING_CHAR (p);
                  if (! fastmap[SYNTAX (c)])
                    break;
                  pos--, pos_byte -= prev_p - p;
  +             rarely_quit (pos);
                }

Same here.

Same issue in forw_comment and in scan_lists.

  @@ -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?  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)?

Thanks again for working on this.