From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit Date: Tue, 31 Jan 2017 17:48:45 +0200 Message-ID: <83fujzw7qa.fsf@gnu.org> References: <20170126052541.29089.5382@vcs.savannah.gnu.org> <20170126052542.828422201BC@vcs.savannah.gnu.org> <83h94hydrs.fsf@gnu.org> <0faac5e6-d857-2afd-d05a-5fcb991419f0@cs.ucla.edu> <8337g0y34h.fsf@gnu.org> <104bf888-821f-98d0-48d3-199cf942c839@cs.ucla.edu> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1485877816 9446 195.159.176.226 (31 Jan 2017 15:50:16 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 31 Jan 2017 15:50:16 +0000 (UTC) Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org To: Paul Eggert Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Jan 31 16:50:06 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cYah4-00023i-1K for ged-emacs-devel@m.gmane.org; Tue, 31 Jan 2017 16:50:06 +0100 Original-Received: from localhost ([::1]:39124 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cYah9-0004AR-Jf for ged-emacs-devel@m.gmane.org; Tue, 31 Jan 2017 10:50:11 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:37379) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cYagG-00048O-4t for emacs-devel@gnu.org; Tue, 31 Jan 2017 10:49:17 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cYagD-00012u-1M for emacs-devel@gnu.org; Tue, 31 Jan 2017 10:49:16 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:49038) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cYagC-00012q-Tf; Tue, 31 Jan 2017 10:49:12 -0500 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:4910 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1cYagB-0003EB-2U; Tue, 31 Jan 2017 10:49:12 -0500 In-reply-to: <104bf888-821f-98d0-48d3-199cf942c839@cs.ucla.edu> (message from Paul Eggert on Mon, 30 Jan 2017 13:52:46 -0800) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:211800 Archived-At: > Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org > From: Paul Eggert > Date: Mon, 30 Jan 2017 13:52:46 -0800 > > 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. I indeed think that we should make emacs_read support quitting, and add unwind_protect calls where we currently don't. This should be safer in the long run, and also simpler. As for overhead, operations like locking a file should indeed normally be very fast, but could take perceptible time in some exceptional conditions, like networked volumes or high I/O load, in which case users may wish to interrupt that. But yes, this could be done as a separate changeset. > @@ -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. Then maybe we should add maybe_quit calls in regex.c instead? Currently, immediate_quit is non-zero all the time re_search_2 runs, so on a TTY a C-g will can stop regex.c in its tracks anywhere. I thought we wanted to make GUI sessions as responsive as TTY sessions. > @@ -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. Yes, but that is why we have gl_state-related dance in handle_interrupt, and your changes delete that part. > If Emacs quits in back_comment, it should longjmp to code that reinitializes gl_state before using it. But unwinding the Lisp stack could run some Lisp that uses syntax.c functions, before we longjmp, right? > 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. That assumes all the immediate_quit=true settings were safe. Previously, they were only in effect on TTY frames, whereas now the maybe_quit calls will be in effect everywhere, so their exposure to various use cases will be much wider. That's why I think it's prudent to take a good look at these places while we make these changes. But I don't feel I know enough about this aspect of syntax.c. Stefan, can you comment on this, please? > @@ -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. No, it will be slower. A signal handler will always react faster than any solution based on polling. A signal handler is also capable of interrupting calls to standard C library functions. It is true that we already have this issue on GUI frames, but I still feel uneasy about losing this feature. TTY frames are still quite popular, even today, in particular for remote sessions. What do others think about this? > 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. No, I don't think we can remove the SIGINT generation: if we do, there will be nothing to set the quit-flag on TTY frames. Also, the "emergency exit" feature is also based on SIGINT. The new patches still include both rarely_quit and incr_rarely_quit (in the second patchset), which I thought you decided to remove. Did you send the correct patches? Thanks.