GNU bug report logs - #68272
[PATCH] Fix -1 leaking from C to lisp in 'read-event' etc.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 68272 in the body.
You can then email your comments to 68272 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tim Ruffing <crypto <at> timruffing.de>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc.
Date: Fri, 05 Jan 2024 22:19:10 +0100

[Message part 1 (text/plain, inline)]

'read_char' in src/keyboard.c returns -1 to indicate the end of a
keyboard macro. This case is supposed to be propagated via 
'read_key_sequence' and 'command_loop_2' to 'Fexecute_kbd_macro'.

But 'read_key_sequence' is not the only caller of 'read_char'. It is
also called by 'read-event' and similar lisp functions, and in that
case, the magic C return value -1 is wrongly propagated to the lisp
caller. 

There are at least workarounds for this bug in at least three lisp
modules in the code base, in subr.el, in calc and most recently added
in dbus.el (bug #62018), see the attached patches. These patches are
supposed to resolve the underlying issue, and then remove the
workarounds.

[0001-Extract-check-for-end-of-macro-to-function.patch (text/x-patch, attachment)]

[0002-src-keyboard.c-requeued_events_pending_p-Revive.patch (text/x-patch, attachment)]

[0003-Fix-1-leaking-from-C-to-lisp-in-read-event-etc.patch (text/x-patch, attachment)]

[0004-Remove-workarounds-for-solved-read-event-bug.patch (text/x-patch, attachment)]

Message #8 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tim Ruffing <crypto <at> timruffing.de>
Cc: 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event'
 etc.
Date: Sat, 06 Jan 2024 09:42:39 +0200

> From: Tim Ruffing <crypto <at> timruffing.de>
> Date: Fri, 05 Jan 2024 22:19:10 +0100
> 
> 'read_char' in src/keyboard.c returns -1 to indicate the end of a
> keyboard macro. This case is supposed to be propagated via 
> 'read_key_sequence' and 'command_loop_2' to 'Fexecute_kbd_macro'.
> 
> But 'read_key_sequence' is not the only caller of 'read_char'. It is
> also called by 'read-event' and similar lisp functions, and in that
> case, the magic C return value -1 is wrongly propagated to the lisp
> caller. 
> 
> There are at least workarounds for this bug in at least three lisp
> modules in the code base, in subr.el, in calc and most recently added
> in dbus.el (bug #62018), see the attached patches. These patches are
> supposed to resolve the underlying issue, and then remove the
> workarounds.

"There be dragons."  AFAIU, this is basically a cleanup: a non-elegant
solution already exists, but we'd want to do it more elegantly.  In
such cases, and in a maze such as keyboard.c's processing of input and
related code, the danger is to introduce regressions because some code
somewhere expects something to happen, and the changes disrupt that.
With that in mind, couldn't we solve this in a more localized manner,
such that we are sure the changes could not affect unrelated code
paths and use cases?  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?  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?

Given your description above, that read_char is called by read-event
and other Lisp functions, I would expect a fix to be localized to
read_char and those functions calling read_char (to limit special
handling of -1 to those functions), but that is not what I see in the
patch.  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?

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 questions I ask above are not theoretical -- we have been bitten
before, more than once or twice, by making seemingly innocuous changes
like this in keyboard.c, only to discover later, sometimes much later,
that some important use case became broken due to the change.  My take
from those incidents was that read_char and related code in keyboard.c
is a complex maze of code that should be touched only if we have a
very good reason, and then in a way that makes changes as localized as
possible to the very fragments we want to change.  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?

Adding Stefan to the discussion, in case he has comments.

Thanks.

Message #11 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Tim Ruffing <crypto <at> timruffing.de>
Cc: 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Sat, 06 Jan 2024 10:15:26 +0100

On Jan 05 2024, Tim Ruffing wrote:

> From e73e08927792303013a8a9935656365f9f2299b6 Mon Sep 17 00:00:00 2001
> From: Tim Ruffing <crypto <at> timruffing.de>
> Date: Wed, 27 Dec 2023 14:32:09 +0100
> Subject: [PATCH 4/4] Remove workarounds for solved 'read-event' bug
>
> * lisp/subr.el (read-char-choice-with-read-key):
> * lisp/net/dbus.el (dbus-call-method):
> * lisp/calc/calc-prog.el:
> Remove workarounds for the bug fixed in the previous commit
> ac82baea1c41ec974ad49f2861ae6c06bda2b4ed, where 'read-event',
> 'read-char' and 'read-char-exclusively' could return wrongly -1.
> In the case of lisp/dbus.el, this reverts commit
> 7177393826c73c87ffe9b428f0e5edae244d7a98.
> ---
>  lisp/calc/calc-prog.el | 6 ------
>  lisp/net/dbus.el       | 6 +-----
>  lisp/subr.el           | 5 -----
>  3 files changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/lisp/calc/calc-prog.el b/lisp/calc/calc-prog.el
> index 03210995eb3..177c179433d 100644
> --- 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"))

Seems like calc actually wants to know when the kbd macro ends
prematurely, and removing the option to detect it is a regression.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Message #14 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Tim Ruffing <crypto <at> timruffing.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
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

Message #17 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tim Ruffing <crypto <at> timruffing.de>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Sat, 13 Jan 2024 11:39:29 +0200

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 <crypto <at> timruffing.de>
> Cc: 68272 <at> 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
> 
> 

Message #20 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Tim Ruffing <crypto <at> timruffing.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Sat, 13 Jan 2024 12:40:31 -0500

Hi Tim,

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

[ Actually, such carefully written code and (even more so) the
  convention-abiding commit messages coming from someone who's not
  a regular contributor spiked my ears.  It made me think
  that this is either a carefully crafted trojan, or it's a really
  welcome new kid on the block :-)  ]

> First of all, I think the bug is real.

No doubt.  Bug#62018 brought to my attention to nasty situation, and I'm
really glad you went through the trouble to come up with a cleanup.

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

Yes, I think we should document in a comment somewhere how the end of
kmacro is handled from a "global" perspective: what `read_char`
does when it's the case, where/when it's supposed to be detected and how
this "signal" is propagated from there to the corresponding call to
`execute-kbd-macro`, how that relates to setting `executing-kbd-macro`
back to nil, ...

> One thing the caller can simply do is to error out.

My first intuition is that we should do that more often.
That's what `calc.el` does.

In `read-char-choice-with-read-key`, OTOH we apparently decided to
consider that not as an error but to "transparently" continue execution
past the end of the kmacro by reading from the keyboard (which
is the behavior that your patch implements).

In the case of `dbus.el` my head hurts trying to understand what the code
really does, really should do, and how your patch changes its behavior.
First, the `dbus.el` code is problematic in any case because it relies
on "pre-reading" input events and pushing them back on
`unread-command-events`, which is sadly not 100% "a no-op".  We should
provide better primitives to handle such parallel input streams
without having to "read" all events and them push them back.

IIUC, with your patch, we have the following scenario:
- Say we're inside a kmacro with N events left to execute.
- Dbus reads those N events and stashes them them onto `unread-command-events`.
- Dbus finally can read the actual dbus event and does its thing.
- Good!
- But now `at_end_of_macro_p` will return true, even though we've yet to
  execute the last N events.  We'll presumably still execute them
  (since they're in `unread-command-events`) but that won't be
  considered as coming from a kmacro any more.

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

My head also hurts trying to understand what are the consequences of
doing that.

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

Agreed.  I think the problem is that we haven't clearly decided what
`execute-kbd-macro` is supposed to do.  The problem is that the way it's
expected to work implies a form of "nesting" such that at the end of
processing those events we return from the function, but sometimes
that's not what happens.  E.g. a kmacro can end in the middle of
a recursive edit.

Your patch brings is closer to a non-nesting semantics, a bit as if
`execute-kbd-macro` pushed all those events into the incoming input
queue and returned immediately.

Maybe that's what `execute-kbd-macro` should do, really: push all the
events into `unread-command-events` and return.  But then, how would we
do the boolean tests for `executing-kbd-macro` which we perform at
various places?  [ I guess we could label/wrap the events such that they
get executed in a special way (which let-binds `executing-kbd-macro` to
t?  ]

Seeing how `calc.el` used the -1 to signal an error (i.e. forcing
kmacros to contain "complete sequences"), maybe a half-way
behavior between the current one and your new one would be for
`read_char` to return -1 (or rather a more explicit `end-of-kbd-macro`)
event *once* and then on the next call to go on and read from
the keyboard.

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

"-1 is not propagated to lisp" doesn't say what happens in its stead.
What this does really is transparently continue reading input from the
keyboard when reaching the end of a kmacro.

I tend to agree that it's closer to the ideal behavior (even though
I still don't know what that is :-).


        Stefan

Message #23 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Tim Ruffing <crypto <at> timruffing.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Fri, 02 Feb 2024 19:04:45 +0100

On Sat, 2024-01-13 at 12:40 -0500, Stefan Monnier wrote:

> I tend to agree that it's closer to the ideal behavior (even though
> I still don't know what that is :-).

I think that's the crucial question for now. I'm happy to follow up
with an updated patch, but we should first see in which direction we
want to head.


> I think the problem is that we haven't clearly decided what
> `execute-kbd-macro` is supposed to do. The problem is that the way
> it's
> expected to work implies a form of "nesting" such that at the end of
> processing those events we return from the function, but sometimes
> that's not what happens. E.g. a kmacro can end in the middle of
> a recursive edit.
> 
> Your patch brings is closer to a non-nesting semantics, a bit as if
> `execute-kbd-macro` pushed all those events into the incoming input
> queue and returned immediately.
> 
> Maybe that's what `execute-kbd-macro` should do, really: push all the
> events into `unread-command-events` and return.

Just to make sure we're on same page: You mean it should (maybe) push
the events into `unread-command-events` assuming LIFO semantics? 

That is, in simple cases when there's no lisp code that reads events
explicitly (`read-char` etc) and you have a kmacro A that executes
another kmacro B, the it's still the case that we get the normal
nesting semantics.

In other words, consider the following kmacros:

A: read char x, read char y (which does nothing but trigger B), read
char z
B: read char b

Then executing A will be as if the user typed "x b z" explicitly.

I think your proposal makes sense. It's simple, it does "what you'd
expect" in simple cases, and it seems compatible to the current
behavior, up to the -1 bug. (And don't think there's a natural
expectation for cases where we hit -1.) 

> But then, how would we
> do the boolean tests for `executing-kbd-macro` which we perform at
> various places? [ I guess we could label/wrap the events such that
> they
> get executed in a special way (which let-binds `executing-kbd-macro`
> to
> t? ]

Yeah, this sounds reasonable (at least at first glance :)). 

> Seeing how `calc.el` used the -1 to signal an error (i.e. forcing
> kmacros to contain "complete sequences"), maybe a half-way
> behavior between the current one and your new one would be for
> `read_char` to return -1 (or rather a more explicit `end-of-kbd-
> macro`)
> event *once* and then on the next call to go on and read from
> the keyboard.

Hm, indeed. But to be honest, I'm not convinced that lisp code should
be able to distinguish this at all when trying to get an event. (If you
*really* want to detect this, you could still check `executing-kbd-
macro` after reading an event and see if it has changed from non-nil to
nil.) Moreover, it would make the C code more complicated, because
we'll probably have to add another state variable that captures if
we've returned -1 (or a symbol) already or not. 

Another compromise, which I find nicer, is to introduce a new variant
of `read-char` (or add a flag) that suppresses the -1 entirely. We'd
probably still need the state variable in the C code, but at least this
won't change anything for existing callers. We could even keep the -
1...

Well okay, assuming we agree that the non-nesting semantics is in
general what we want to have, what should we (or I) do now?

   1. Work on an improved version of my patch, which brings it closer
      to the ideal semantics, but doesn't touch too much of the code?
   2. Write a new patch that implements the "full" non-nesting
      semantics suggest above?
   3. Rewrite my patch to do something in the middle? (Returning -1
      once, or have a boolean flag that fixes the -1.)
   4. Do nothing because all of it is too dangerous.

If anything, I think option 3 with a flag beats 4, because 3 won't be
too dangerous then.

But for the rest, I'm really unsure (and anyway, I don't think I'm the
one to make the call). 2 sounds nice, but do you think there will be
even more dragons with 2 than with 1? It will be an even larger change
for sure...

----

More inline replies:


> IIUC, with your patch, we have the following scenario:
> - Say we're inside a kmacro with N events left to execute.
> - Dbus reads those N events and stashes them them onto `unread-
> command-events`.
> - Dbus finally can read the actual dbus event and does its thing.
> - Good!
> - But now `at_end_of_macro_p` will return true, even though we've yet
> to
> execute the last N events. We'll presumably still execute them
> (since they're in `unread-command-events`) but that won't be
> considered as coming from a kmacro any more.

This matches my understanding. My thinking is that this is not a big
deal in this specific case. The dbus code currently relies on the idea
of reading events and putting them back to `unread-command-events`.
While the patch affects the behavior, it doesn't change anything about
the fact that this is an ugly hack anyway. So I'm not really convinced
that we should care about this change of behavior too much, as long as
we don't break anything. 


> 
> > And I'm also aware that it doesn't spark confidence when someone
> > with
> > (almost) zero code contributions submits a patch of this kind.
> 
> [ Actually, such carefully written code and (even more so) the
>   convention-abiding commit messages coming from someone who's not
>   a regular contributor spiked my ears.  It made me think
>   that this is either a carefully crafted trojan, or it's a really
>   welcome new kid on the block :-)  ]
> 

Hehe, thanks for the kind words. I'll submit the trojan in the next
patch then. ;) On a more serious note, I took the time to dig deeply
here, but I don't think I'll have the bandwidth regularly (as you see
from my response time...) 

By the way, the CONTRIBUTE file is pretty long, but it's also pretty
good and clear. 

> Yes, I think we should document in a comment somewhere how the end of
> kmacro is handled from a "global" perspective: what `read_char`
> does when it's the case, where/when it's supposed to be detected and
> how
> this "signal" is propagated from there to the corresponding call to
> `execute-kbd-macro`, how that relates to setting `executing-kbd-
> macro`
> back to nil, ...
> 

Agreed.

> "-1 is not propagated to lisp" doesn't say what happens in its stead.
> What this does really is transparently continue reading input from
> the
> keyboard when reaching the end of a kmacro.
> 

Agreed, I can change it. But let's first decide what we would like to
do in general.

Best,
Tim

Message #26 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Tim Ruffing <crypto <at> timruffing.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Tue, 06 Feb 2024 16:04:48 -0500

> Just to make sure we're on same page: You mean it should (maybe) push
> the events into `unread-command-events` assuming LIFO semantics? 

Yes.

>> Seeing how `calc.el` used the -1 to signal an error (i.e. forcing
>> kmacros to contain "complete sequences"), maybe a half-way behavior
>> between the current one and your new one would be for `read_char` to
>> return -1 (or rather a more explicit `end-of-kbd-macro`) event
>> *once* and then on the next call to go on and read from the keyboard.
>
> Hm, indeed. But to be honest, I'm not convinced that lisp code should
> be able to distinguish this at all when trying to get an event. (If you
> *really* want to detect this, you could still check `executing-kbd-
> macro` after reading an event and see if it has changed from non-nil to
> nil.) Moreover, it would make the C code more complicated, because
> we'll probably have to add another state variable that captures if
> we've returned -1 (or a symbol) already or not.

FWIW, I tend to agree.

> Another compromise, which I find nicer, is to introduce a new variant
> of `read-char` (or add a flag) that suppresses the -1 entirely.

I'd rather not go there, if we can avoid it.

>    1. Work on an improved version of my patch, which brings it closer
>       to the ideal semantics, but doesn't touch too much of the code?

That's what I'd vote for.

>    2. Write a new patch that implements the "full" non-nesting
>       semantics suggest above?

In the long run it might be worth trying it out, but I suspect this
/will/ bump into surprising corner cases, so I'd keep it as a subsequent
step which could be made optional (I suspect it can be implemented fully
in ELisp).

>> IIUC, with your patch, we have the following scenario:
>> - Say we're inside a kmacro with N events left to execute.
>> - Dbus reads those N events and stashes them them onto `unread-
>> command-events`.
>> - Dbus finally can read the actual dbus event and does its thing.
>> - Good!
>> - But now `at_end_of_macro_p` will return true, even though we've yet
>> to
>> execute the last N events. We'll presumably still execute them
>> (since they're in `unread-command-events`) but that won't be
>> considered as coming from a kmacro any more.
> This matches my understanding. My thinking is that this is not a big
> deal in this specific case. The dbus code currently relies on the idea
> of reading events and putting them back to `unread-command-events`.
> While the patch affects the behavior, it doesn't change anything about
> the fact that this is an ugly hack anyway.

You mean they had it coming?  I can agree to some extent, but currently
there aren't very many alternative approaches :-(

>> Yes, I think we should document in a comment somewhere how the end of
>> kmacro is handled from a "global" perspective: what `read_char` does
>> when it's the case, where/when it's supposed to be detected and how
>> this "signal" is propagated from there to the corresponding call to
>> `execute-kbd-macro`, how that relates to setting
>> `executing-kbd-macro` back to nil, ...
>
> Agreed.

Could you try writing such a description?  It doesn't have to be
complete: just write what you happen to know already.


        Stefan

Message #29 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Tim Ruffing <crypto <at> timruffing.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Fri, 01 Mar 2024 13:14:54 +0100

Hi, sorry for the delay again... I'll try to do my best to come up with
an updated patch within the following week.

Tim

Message #32 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Tim Ruffing <crypto <at> timruffing.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Mon, 04 Mar 2024 19:42:09 +0100

[Message part 1 (text/plain, inline)]

Hi, this is an updated patch set.

Changes to the previous revision:
 * Instead of changing requeued_events_pending_p, it's renamed to
   requeued_command_events_pending_p, and I've fixed the outdated
   comment that had misled me.  Then a new requeued_events_pending_p
   with different semantics is added and used in following commits.
   This addresses one of Eli's comments, and should make sure that
   existing callers of requeued_events_pending_p are not affected by
   the patch.
 * I've added a large comment to src/macro.c as an attempt to explain
   how we handle the end of a keyboard macro.
 * I've improved the commit message of (now) 6be0f5f. 

Best,
Tim

[0001-Extract-check-for-end-of-macro-to-function.patch (text/x-patch, attachment)]

[0002-src-keyboard.c-requeued_events_pending_p-Improve-nam.patch (text/x-patch, attachment)]

[0003-src-keyboard.c-requeued_events_pending_p-New-functio.patch (text/x-patch, attachment)]

[0004-Continue-reading-in-read-event-etc.-at-the-end-of-a-.patch (text/x-patch, attachment)]

[0005-Remove-workarounds-for-solved-read-event-bug.patch (text/x-patch, attachment)]

Message #35 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tim Ruffing <crypto <at> timruffing.de>
Cc: 68272 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Tue, 05 Mar 2024 15:10:39 +0200

> From: Tim Ruffing <crypto <at> timruffing.de>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 68272 <at> debbugs.gnu.org
> Date: Mon, 04 Mar 2024 19:42:09 +0100
> 
> Hi, this is an updated patch set.
> 
> Changes to the previous revision:
>  * Instead of changing requeued_events_pending_p, it's renamed to
>    requeued_command_events_pending_p, and I've fixed the outdated
>    comment that had misled me.  Then a new requeued_events_pending_p
>    with different semantics is added and used in following commits.
>    This addresses one of Eli's comments, and should make sure that
>    existing callers of requeued_events_pending_p are not affected by
>    the patch.
>  * I've added a large comment to src/macro.c as an attempt to explain
>    how we handle the end of a keyboard macro.
>  * I've improved the commit message of (now) 6be0f5f. 

Thanks, but what about the comments in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=68272#11 ?

IOW, what about callers that actually _want_ to know when the macro
ends prematurely?

Message #38 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Tim Ruffing <crypto <at> timruffing.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Tue, 05 Mar 2024 11:45:41 -0500

> Hi, this is an updated patch set.

Looks really nice, thank you.
Comments/nitpicks below.

Eli Zaretskii [2024-03-05 15:10:39] wrote:
> IOW, what about callers that actually _want_ to know when the macro
> ends prematurely?

I couldn't find any, really.  Of course, we could export
`at_end_of_macro_p` to ELisp, but I don't see any need for it.

[ Also: define "prematurely".  My impression is that the callers of
  `read-char` are generally not in a position to know what is premature
  and what isn't because it tends to depend on the users' intentions.   ]

> +/* Whether the execution of a macro has reached its end.
> +   It makes only sense to call this when while executing a macro.  */
               ^^^^                    ^^^^
                 \----------------------^

[ And same for the other copy of that coment.  ]

> +/* Return true if there are any pending requeued events (command events
> +   or events to be processed by input methods).  */

I think I'd say "other levels of the input processing stages" instead of
"input methods", so as to conceptually include any other
"unread_*_events" we may end up with.

> -      /* If not, we should actually read a character.  */
> +      /* If we're at the end of a macro, exit it by returning 0,
> +	 unless there are unread events pending.  */
> +      else if (!NILP (Vexecuting_kbd_macro)
> +	  && at_end_of_macro_p ()
> +	  && !requeued_events_pending_p ())
> +	{
> +	  t = 0;
> +	  /* The Microsoft C compiler can't handle the goto that
> +	     would go here.  */
> +	  dummyflag = true;
> +	  break;
> +	}

This "Microsoft C compiler" business dates back to 1994 (commit
bc536d847736f466727453ca6aa7c07aef6fce46).
I think it's safe to clean it up now :-)

> index 62129be1629..98290e7e276 100644
> --- a/src/macros.c
> +++ b/src/macros.c
> @@ -314,6 +314,29 @@ DEFUN ("execute-kbd-macro", Fexecute_kbd_macro, Sexecute_kbd_macro, 1, 3, 0,
>  		      Vreal_this_command));
>    record_unwind_protect (pop_kbd_macro, tem);
>  
> +  /* The following loop starts the execution of the macro.  Individual
> +     characters from the macro are read by read_char, which takes care
> +     of incrementing executing_kbd_macro_index.  The end of the
> +     macro is handled as follows:
> +     - read_key_sequence asks at_end_of_macro_p whether the end of
> +     (one iteration of the macro) has been reached.  If so, it returns
> +     the magic value 0 to command_loop_1.
> +     - command_loop_1 returns Qnil to command_loop_2.
> +     - command_loop_2 returns Qnil to this function
> +       (but only the returning is relevant, not the actual value).

Could you complete the sequence to the point where we clear
Vexecuting_kbd_macro?

> +     If read_char happens to be called at the end of the macro, but
> +     before read_key_sequence could handle the end (e.g., because lisp
> +     code calls 'read-event', 'read-char', and 'read-char-exclusive'),
> +     read_char will simply continue reading other available input
> +     (Bug#68272).

Could you clarify here what happens w.r.t the value of
Vexecuting_kbd_macro (AFAICT, we "remain `at_end_of_macro_p`").

> +     Note that this is similar (in observable behavior) to a simpler
> +     implementation of keyboard macros in which this function simply
> +     pushed all characters of the macro into the incoming event queue
> +     and returned immediately.  Maybe this is the implementation that
> +     we ideally would like to have, but switching to it will require
> +     a larger code change.  */

It might be worth mentioning that the main difference is the
availability of `executing-kbd-macro` to let ELisp code behave
differently when called via a kmacro than via "live input".
Which also kind of justifies why `read-key-sequence` wants to
detect the end: if a kmacro ends in the middle of a key sequence, then
it's triggered both my kmacro and by live input.
[ Of course, we could handle it in the command loop instead:
  check and compare the set of pending kmacro events before and after we
  call `read-key-sequence`.  ]


        Stefan

Message #41 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: crypto <at> timruffing.de, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Tue, 05 Mar 2024 18:55:18 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  68272 <at> debbugs.gnu.org
> Date: Tue, 05 Mar 2024 11:45:41 -0500
> 
> Eli Zaretskii [2024-03-05 15:10:39] wrote:
> > IOW, what about callers that actually _want_ to know when the macro
> > ends prematurely?
> 
> I couldn't find any, really.

??? calc is one, obviously.

> > +	  t = 0;
> > +	  /* The Microsoft C compiler can't handle the goto that
> > +	     would go here.  */
> > +	  dummyflag = true;
> > +	  break;
> > +	}
> 
> This "Microsoft C compiler" business dates back to 1994 (commit
> bc536d847736f466727453ca6aa7c07aef6fce46).
> I think it's safe to clean it up now :-)

The commit and its date don't matter, since we dropped support for
MSVC long ago.  We only support GCC on Windows (at least officially).

Message #44 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: crypto <at> timruffing.de, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Tue, 05 Mar 2024 12:57:56 -0500

>> > IOW, what about callers that actually _want_ to know when the macro
>> > ends prematurely?
>> I couldn't find any, really.
> ??? calc is one, obviously.

I don't think so: AFAICT the tests there were added simply
because they had to do something with this -1 return value.

Other places deal with it differently (e.g. by setting
`executing-kbd-macro` to nil), so *maybe* the specific way they deal
with it is indicative of intent, but from where I stand it looks like
they just chose that behavior arbitrarily.

I can't think of a good UI reason why they'd *want* to signal an error
when a kmacro ends in the middle of a `Z` thingy: it's an acceptable
behavior but it's not clearly superior to just continuing reading the
Z thingy from live input (I'd even tend to think that continuing is
a better choice since it lets users use kmacro that provide the first
part of a Z thingy and let the user finish it).


        Stefan

Message #47 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: crypto <at> timruffing.de, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Tue, 05 Mar 2024 20:53:44 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: crypto <at> timruffing.de,  68272 <at> debbugs.gnu.org
> Date: Tue, 05 Mar 2024 12:57:56 -0500
> 
> >> > IOW, what about callers that actually _want_ to know when the macro
> >> > ends prematurely?
> >> I couldn't find any, really.
> > ??? calc is one, obviously.
> 
> I don't think so: AFAICT the tests there were added simply
> because they had to do something with this -1 return value.

Any evidence?  Andreas thought differently, what if he is right?

Message #50 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: crypto <at> timruffing.de, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Tue, 05 Mar 2024 14:29:43 -0500

>> >> > IOW, what about callers that actually _want_ to know when the macro
>> >> > ends prematurely?
>> >> I couldn't find any, really.
>> > ??? calc is one, obviously.
>> 
>> I don't think so: AFAICT the tests there were added simply
>> because they had to do something with this -1 return value.
>
> Any evidence?  Andreas thought differently, what if he is right?

I think the mail you responded to already provided my answer to the case
the original author did it on purpose.  I have nothing to add.
Do you have a better alternative?


        Stefan

Message #53 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: crypto <at> timruffing.de, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Tue, 05 Mar 2024 21:55:49 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: crypto <at> timruffing.de,  68272 <at> debbugs.gnu.org
> Date: Tue, 05 Mar 2024 14:29:43 -0500
> 
> >> >> > IOW, what about callers that actually _want_ to know when the macro
> >> >> > ends prematurely?
> >> >> I couldn't find any, really.
> >> > ??? calc is one, obviously.
> >> 
> >> I don't think so: AFAICT the tests there were added simply
> >> because they had to do something with this -1 return value.
> >
> > Any evidence?  Andreas thought differently, what if he is right?
> 
> I think the mail you responded to already provided my answer to the case
> the original author did it on purpose.  I have nothing to add.
> Do you have a better alternative?

I'd prefer that we allowed Lisp programs to diagnose the situation
where the macro ends prematurely, and that at least the two places in
Calc keep showing the error messages they show now in those cases.
The other places, which use the special -1 value to simply keep
reading, or do something else silently -- those should use the new
facility which does the same in low-level code.

One way of doing that is to allow some hook to be called when we
currently return -1, and if that hook returns some special value,
avoid continuing to read -- this will allow Lisp programs that want it
to keep the current behavior by setting up the hook.

Message #56 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: crypto <at> timruffing.de, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Tue, 05 Mar 2024 15:18:45 -0500

> I'd prefer that we allowed Lisp programs to diagnose the situation
> where the macro ends prematurely,

If we prefer to preserve the current behavior in Calc, the patch below
should do the trick.


        Stefan


diff --git a/lisp/calc/calc-prog.el b/lisp/calc/calc-prog.el
index 03210995eb3..8dff7f1f264 100644
--- a/lisp/calc/calc-prog.el
+++ b/lisp/calc/calc-prog.el
@@ -1225,13 +1225,17 @@ calc-kbd-else-if
   (interactive)
   (calc-kbd-if))
 
+(defun calc--at-end-of-kmacro-p ()
+  (and (arrayp executing-kbd-macro)
+       (>= executing-kbd-macro-index (length executing-kbd-macro))))
+
 (defun calc-kbd-skip-to-else-if (else-okay)
   (let ((count 0)
 	ch)
     (while (>= count 0)
-      (setq ch (read-char))
-      (if (= ch -1)
+      (if (calc--at-end-of-kmacro-p)
 	  (error "Unterminated Z[ in keyboard macro"))
+      (setq ch (read-char))
       (if (= ch ?Z)
 	  (progn
 	    (setq ch (read-char))
@@ -1299,9 +1303,9 @@ calc-kbd-loop
     (or executing-kbd-macro
 	(message "Reading loop body..."))
     (while (>= count 0)
-      (setq ch (read-event))
-      (if (eq ch -1)
+      (if (calc--at-end-of-kmacro-p)
 	  (error "Unterminated Z%c in keyboard macro" open))
+      (setq ch (read-event))
       (if (eq ch ?Z)
 	  (progn
 	    (setq ch (read-event)
@@ -1427,9 +1431,9 @@ calc-kbd-push
 	   (if defining-kbd-macro
 	       (message "Reading body..."))
 	   (while (>= count 0)
-	     (setq ch (read-char))
-	     (if (= ch -1)
+	     (if (calc--at-end-of-kmacro-p)
 		 (error "Unterminated Z` in keyboard macro"))
+	     (setq ch (read-char))
 	     (if (= ch ?Z)
 		 (progn
 		   (setq ch (read-char)

Message #59 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: crypto <at> timruffing.de, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Wed, 06 Mar 2024 13:46:05 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: crypto <at> timruffing.de,  68272 <at> debbugs.gnu.org
> Date: Tue, 05 Mar 2024 15:18:45 -0500
> 
> > I'd prefer that we allowed Lisp programs to diagnose the situation
> > where the macro ends prematurely,
> 
> If we prefer to preserve the current behavior in Calc, the patch below
> should do the trick.

Fine by me, provided that we describe this technique in NEWS when we
install the changes.

Thanks.

Message #62 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Tim Ruffing <crypto <at> timruffing.de>
To: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Sat, 09 Mar 2024 13:33:53 +0100

[Message part 1 (text/plain, inline)]

Third revision.

 - Addressed Stefan's nit.
 - Extended the large comment.
 - Switched to Stefan's suggestion for calc.
 - Added a NEWS item that describes the suggestion
 - Added a commit that removes the Microsoft hack

> > It might be worth mentioning that the main difference is the
> > availability of `executing-kbd-macro` to let ELisp code behave
> > differently when called via a kmacro than via "live input".
> > Which also kind of justifies why `read-key-sequence` wants to
> > detect the end: if a kmacro ends in the middle of a key sequence,
> > then
> > it's triggered both my kmacro and by live input.
> > [ Of course, we could handle it in the command loop instead:
> >   check and compare the set of pending kmacro events before and
> > after
> > we
> >   call `read-key-sequence`.  ]

I didn't do this, because I couldn't follow exactly what you're saying.
Feel free to suggest a sentence.

Best,
Tim

[0001-Extract-check-for-end-of-macro-to-function.patch (text/x-patch, attachment)]

[0002-src-keyboard.c-requeued_events_pending_p-Improve-nam.patch (text/x-patch, attachment)]

[0003-src-keyboard.c-requeued_events_pending_p-New-functio.patch (text/x-patch, attachment)]

[0004-Continue-reading-in-read-event-etc.-at-the-end-of-a-.patch (text/x-patch, attachment)]

[0005-Remove-workarounds-for-solved-read-event-bug.patch (text/x-patch, attachment)]

[0006-lisp-calc-calc-prog.el-Switch-to-new-method-of-detec.patch (text/x-patch, attachment)]

[0007-src-keyboard.c-read_key_sequence-Remove-MSVC-compati.patch (text/x-patch, attachment)]

Message #65 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Tim Ruffing <crypto <at> timruffing.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Sat, 09 Mar 2024 13:08:44 -0500

> Third revision.
>
>  - Addressed Stefan's nit.
>  - Extended the large comment.
>  - Switched to Stefan's suggestion for calc.
>  - Added a NEWS item that describes the suggestion
>  - Added a commit that removes the Microsoft hack

LGTM.  Eli?


        Stefan

Message #68 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Michael Albinus <michael.albinus <at> gmx.de>
Cc: crypto <at> timruffing.de, 68272 <at> debbugs.gnu.org
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Sat, 09 Mar 2024 20:37:08 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  68272 <at> debbugs.gnu.org
> Date: Sat, 09 Mar 2024 13:08:44 -0500
> 
> > Third revision.
> >
> >  - Addressed Stefan's nit.
> >  - Extended the large comment.
> >  - Switched to Stefan's suggestion for calc.
> >  - Added a NEWS item that describes the suggestion
> >  - Added a commit that removes the Microsoft hack
> 
> LGTM.  Eli?

The dbus.el change should be reviewed by Michael (CC'ed).

Otherwise, I'm okay with this, fingers crossed.

Message #71 received at 68272 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: crypto <at> timruffing.de, 68272 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Sun, 10 Mar 2024 09:24:45 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

Hi,

> The dbus.el change should be reviewed by Michael (CC'ed).

There's no essential change in dbus.el. The check for executing-kbd-macro
is removed, which looks OK in the contect of the other changes.

> Otherwise, I'm okay with this, fingers crossed.

Best regards, Michael.

Message #76 received at 68272-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: crypto <at> timruffing.de, 68272-done <at> debbugs.gnu.org,
 Michael Albinus <michael.albinus <at> gmx.de>
Subject: Re: bug#68272: [PATCH] Fix -1 leaking from C to lisp in
 'read-event' etc.
Date: Sun, 10 Mar 2024 10:48:40 -0400

Thank you, pushed, closing,


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.