From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc. Date: Sat, 13 Jan 2024 11:39:29 +0200 Message-ID: <83cyu5h8ry.fsf@gnu.org> References: <46480759b6d89b5a4864e8ee1b986817366a56e5.camel@timruffing.de> <83wmsmucuo.fsf@gnu.org> <1b3fa12138838a3fe5643a9e76a65d32a677e34d.camel@timruffing.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33788"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 68272@debbugs.gnu.org To: Tim Ruffing , Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Jan 13 10:41:10 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rOaVp-0008dI-UG for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 13 Jan 2024 10:41:10 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rOaVl-00020S-DU; Sat, 13 Jan 2024 04:41:05 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rOaVk-0001zw-8Y for bug-gnu-emacs@gnu.org; Sat, 13 Jan 2024 04:41:04 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rOaVj-00013G-UX for bug-gnu-emacs@gnu.org; Sat, 13 Jan 2024 04:41:03 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rOaVi-0008GI-CY for bug-gnu-emacs@gnu.org; Sat, 13 Jan 2024 04:41:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 13 Jan 2024 09:41:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 68272 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 68272-submit@debbugs.gnu.org id=B68272.170513883531709 (code B ref 68272); Sat, 13 Jan 2024 09:41:02 +0000 Original-Received: (at 68272) by debbugs.gnu.org; 13 Jan 2024 09:40:35 +0000 Original-Received: from localhost ([127.0.0.1]:38448 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rOaVG-0008FL-Qu for submit@debbugs.gnu.org; Sat, 13 Jan 2024 04:40:35 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:54466) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rOaVF-0008F9-81 for 68272@debbugs.gnu.org; Sat, 13 Jan 2024 04:40:34 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rOaV3-0000bS-1c; Sat, 13 Jan 2024 04:40:27 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-version:References:Subject:In-Reply-To:To:From: Date; bh=e9AV3QT9Hcc8mOkZrQqzNMgY6QguXJDBMLne4JeS9oA=; b=h0nelmsdfvEOPDlfrX8D dkq7EOhuxmijD9NVrQSNWjG0BUOx11VSb0WJT7aUEAmmEadnvkeFoj53MRBT3CkOKTB6nlnh2Yaur imuUfq7edyX9E2/D6TTg/S4USpzsi6MjGOkZSdlSxxm0KlXyQFZIrDkFHdzpbsH5Z2GVq7THsISNS ESHspVTmNcjGfeQbUvenNvDlQsvZN4JkeDQnmxczO42WE8L8TijSSV+VObAd4cz14CQz0V6G3PoWN KSJU8vl7KLvU+vMJ8T6yGL6EZHETQdW/URqUwWQ7iQkwyPIKy36i7y7D4iiErZ4D+SZembX6BkyTR GFRJwpUm5aCaig==; In-Reply-To: <1b3fa12138838a3fe5643a9e76a65d32a677e34d.camel@timruffing.de> (message from Tim Ruffing on Sat, 06 Jan 2024 15:32:23 +0100) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:278111 Archived-At: Stefan, any comments or ideas? I'm not very comfortable with this change, frankly, and tend to document this "feature" leaving the code alone. Note that currently various callers do different things when they get the -1 value, and it is unclear to me whether changing that will be perceived as regressions. In any case, I think the patch needs a change, if only to account for the fact that requeued_events_pending_p is called from process.c. > From: Tim Ruffing > Cc: 68272@debbugs.gnu.org > Date: Sat, 06 Jan 2024 15:32:23 +0100 > > Hey, thank you very much for the long email. I was somewhat prepared > for hesitant reply, and I totally understand that "there be dragons". > And I'm also aware that it doesn't spark confidence when someone with > (almost) zero code contributions submits a patch of this kind. I feel > the same when new people send complex patches to the C library I > maintain... And sure, it took a me quite a while to navigate this maze > of keyboard.c and to come up with this patch and make the tests pass, > but I feel rather confident now that this is the right approach. > > I agree, we can never be fully sure that this introduces regressions, > but let me still try to convince you that this approach and these > patches are carefully crafted and thought through. Here's my analysis > of the situation: > > First of all, I think the bug is real. read-event (and read-char and > read-char-exclusive, which I won't mention from now on for brevity) can > return -1, but the docs don't mention -1, and it's arguably a strange > "magic" value for a lisp function. If anything, one would expect nil, > or some special symbol. Of course, we could simply document the -1 > return value.  > > But the problem is not just that this is not elegant. It's worse > because it's also pretty unclear what the caller should do in this > case. In particular, the caller can't simply skip the -1 and try again: > The caller will see an infinite stream of -1 values, until the keyboard > macro has been resolved properly, i.e., as long as executing_kbd_macro > is non-nil. One thing the caller can simply do is to error out. But if > the caller wants an event, the only thing it can do is to touch > executing_kbd_macro and set it nil explicitly (this is what subr.el > does currently). We could also document this, but I feel something like > "this function can return -1 and if it does, set executing_kbd_macro to > nil and try again" is really a bit too unelegant (and it has more > downsides, see next paragraph). > > However, this now hints at a different approach: Could we handle this > in read-event locally and just perform the retrying for the caller? > First of all, we'd probably still do it one level below in the call > stack in order not to mess up with the timeouts (if we simply try > again, we could double the timeout), so we'd want to do it > inread_filtered_event. But I think that approach will then be *not* > localized and thus dangerous: it's not clear if setting > executing_kbd_macro to nil has unexpected side effects have. Resolving > the macro properly is ultimately the responsibility of > Fexecute_kbd_macro and pop_kbd_macro in macros.c, and we shouldn't just > declare at some other point in the code that we wish the macro handling > to be done. > > ------------- > > Let me try to convince you that these commits are more harmless than > they look at the first glance. I'll go through them one by one. I'm > also happy to add a revised version of this to the commit messages. > > 1. "Extract check for end of macro to function": > This is pure refactoring. A brief code review shows that this does not > change semantics at all. > > 2. "src/keyboard.c (requeued_events_pending_p): Revive" > > 2a. > > On Sat, 2024-01-06 at 09:42 +0200, Eli Zaretskii wrote: > >   For example, your patches modify > > requeued_events_pending_p, but that function is called in several > > places, including outside of keyboard.c -- how can we be sure we are > > not introducing regressions this way?  > > Oh, good catch. In fact, it's (without the patches here) only called > outside of keyboard.c. I was misled by the comment that said "This > isn't used yet. The hope is to make wait_reading_process_output call > it". Indeed, this comment is outdated and the (only) caller is > wait_reading_process_output. wait_reading_process_output really wants > only keyboard output, so it should really just check > Vunread_command_events and not unread_post_input_method_events and > Vunread_input_method_events; the latter two are for non-keyboard input. > I can rework this. I think the way to go is to split this into two > functions requeued_events_pending_p and > requeued_command_events_pending_p or similar, and fix the outdated > comment. > > 2b. > This changes two !NILP to CONSP to be consistent with the checks within > read_char (see 3. below). This is fine under the assumption that the > variables are always lists (as they should be). I was about to say that > I can take this back if you feel it's too dangerous, but if this > function needs to be split into two anyway, we won't need the change > from !NILP to CONSP. > > 3. "Fix -1 leaking from C to lisp in 'read-event' etc." > > 3a. > > On Sat, 2024-01-06 at 09:42 +0200, Eli Zaretskii wrote: > > And read_char is called in > > several places, including by lread.c and read_char itself -- did you > > figure out how will this change affect those? > > > > Yes, I carefully checked all the callers. (I know, it's not trivial to > review this commit, and I think any careful reviewer would need to do > the same in the end...) The only caller which acts specially on -1 is > read_key_sequence, so it makes sense to handle this case in > read_key_sequence. This is done in this commit. The other callers are > not prepared to get -1 (which is the root cause of the bug!), and > exactly what we would like to avoid. > > 3b. > > On Sat, 2024-01-06 at 09:42 +0200, Eli Zaretskii wrote: > >   Moreover, the changes in read_char modify code more than is > > necessary to handle the "at end of macro" situation, so we are > > risking > > changes that will break something else.  Here's an example: > > > > > --- a/src/keyboard.c > > > +++ b/src/keyboard.c > > > @@ -2610,7 +2610,8 @@ read_char (int commandflag, Lisp_Object map, > > >        goto reread_for_input_method; > > >      } > > >   > > > -  if (!NILP (Vexecuting_kbd_macro)) > > > +  /* If we're executing a macro, process it unless we are at its > > > end. */ > > > +  if (!NILP (Vexecuting_kbd_macro) && !at_end_of_macro_p ()) > > >      { > > >        /* We set this to Qmacro; since that's not a frame, nobody > > > will > > >   try to switch frames on us, and the selected window will > > > @@ -2624,15 +2625,6 @@ read_char (int commandflag, Lisp_Object map, > > >   selected.  */ > > >        Vlast_event_frame = internal_last_event_frame = Qmacro; > > >   > > > -      /* Exit the macro if we are at the end. > > > - Also, some things replace the macro with t > > > - to force an early exit.  */ > > > -      if (at_end_of_macro_p ()) > > > - { > > > -   XSETINT (c, -1); > > > -   goto exit; > > > - } > > > - > > > > This hunk moves the at_end_of_macro_p test to a higher level, which > > has a side effect of not executing the code before the original > > at_end_of_macro_p test -- how do we know this won't break something > > that happens to depend on that code which will now not execute in the > > at-end-of-macro case? > > The commit takes care of this, and it's not too hard to verify: If you > look at read_char, except for setting local variables, this is what it > does above the original at_end_of_macro_p test:   > * if (CONSP (Vunread_post_input_method_events)) ... > * Vlast_event_device = Qnil; > * if (CONSP (Vunread_command_events)) ... > * if (CONSP (Vunread_input_method_events)) > * Vlast_event_frame = internal_last_event_frame = Qmacro; (if in a kbd > macro)         > > The "if" blocks don't matter. That's exactly why the caller checks for > patch check for !requeued_events_pending_p () in the patch: if those > blocks were to become active, we'd *not* skip read_char. > > Consider "Vlast_event_frame = internal_last_event_frame = Qmacro". The > purpose this is to indicate that the last event came from a kbd macro. > Since we now don't return the "event" -1 any longer, it also doesn't > make sense to set this in this case. (And when we read a new event, > this will be correctly updated.) > > What remains is setting "Vlast_event_device = Qnil". The same logic > applies here. I don't think we should set to this nil at the end of a > keyboard macro, because no new event was processed, i.e., the "last > event" didn't change. That's why I choose to skip this line. > > But it's certainly more conservative to keep the two assignments to > avoid any unexpected consequences, I'm happy to add this to the caller > with an appropriate comment, if this is desired > > 4. "Remove workarounds for solved 'read-event' bug" > > On Sat, 2024-01-06 at 09:42 +0200, Eli Zaretskii wrote: > > Also note that at least in subr.el we don't only test the -1 value, > > we > > also test that a keyboard macro is being executed, and in this and > > other callers that handle -1, each application behaves in > > application-specific way when it receives -1.  It is not clear to me > > how your changes modify the behavior in those cases, and your > > explanations and the log message doesn't seem to answer this > > question. > > For example, this code in calc-prog: > > > > > --- a/lisp/calc/calc-prog.el > > > +++ b/lisp/calc/calc-prog.el > > > @@ -1230,8 +1230,6 @@ calc-kbd-skip-to-else-if > > >   ch) > > >      (while (>= count 0) > > >        (setq ch (read-char)) > > > -      (if (= ch -1) > > > -   (error "Unterminated Z[ in keyboard macro")) > > >        (if (= ch ?Z) > > >     (progn > > >       (setq ch (read-char)) > > > > now signals a specific error in the case of an unterminated keyboard > > macro: what will the behavior in that case be after the changes? > > The behavior is that it will wait for interactive user input. Say you > define a keyboard macro that performs only part of a calculation. Then > instead of signaling an error, it will simply wait for the user the > provide the remaining key strokes. > > On Sat, 2024-01-06 at 10:15 +0100, Andreas Schwab wrote: > > > > Seems like calc actually wants to know when the kbd macro ends > > prematurely, and removing the option to detect it is a regression > > To be honest, I really don't think that signaling an error here was a > crucial feature. I'm pretty sure what happened is that -1 appeared as > an unexpected return case, and then the error was added. Yes, this is a > semantic change, but I think it's an acceptable one. This doesn't > remove any real functionality, it simply avoids an error condition. > > But one thing I wasn't sure about is whether the change that -1 can't > occur anymore would warrant a NEWS item or similar. If yes, I'll add > one. > > ------------- > > On Sat, 2024-01-06 at 09:42 +0200, Eli Zaretskii wrote: > > In this case, given > > that we want a more elegant solution for a situation that we already > > handle, I tend to avoid any such changes to begin with, for the > > reasons I explained above, and instead perhaps document the special > > meaning of -1 in these cases.  And if we want to consider such > > changes > > these dangers notwithstanding, I would like to see them affecting as > > little other code as possible.  Can you suggest a safer changeset? > > As I explained above, documenting this is not trivial either. While we > can, of course, simply document that -1 can be returned, it's hard to > give meaningful advice except "this function can return -1 and if it > does and you really want to wait for input, set executing_kbd_macro to > nil and try again", which is not only unelegant, but could have further > consequences. > > I'm happy to make the changes based on your review and on other > feedback, and expand the commit message a lot, and I think this will > make the patches more conservative and safer.  > > It's still not trivial to review and verify my analysis. But this is > just how it is. The code change wasn't easy to come up with, so it's > necessarily not very easy to review. > > With the modifications I hinted at based on your review, I think what > we end up with is: > 1. commit: no semantic change > 2. commit: will be reworked to have no semantic change at all > 3. commit: only change is that -1 is not propagated to lisp, other > callers of read_char are unaffected as I explained above. (modulo > Vlast_event_device and Vlast_event_frame -- which I can also still set > if you want me to to). > 4. commit: this only removes code paths which are unreachable now, so > again no semantic chance. > > But then it will be good upfront if it's at least realistic that these > patches get in in some form or another. If I can get conceptual > approval or dismissal of this approach, I know what to do next. > > Best, > Tim > >