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

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

[Tim Ruffing]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

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





[-- Attachment #2: 0001-Extract-check-for-end-of-macro-to-function.patch --]
[-- Type: text/x-patch, Size: 2056 bytes --]

From f3bd0b346e7aa435d778e56b9a7c6a123315df18 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:26:26 +0100
Subject: [PATCH 1/4] Extract check for end of macro to function

* src/macros.h (at_end_of_macro_p):
* src/macros.c (at_end_of_macro_p):
New function.
* src/keyboard.c (read_char): Use the new function.
---
 src/keyboard.c |  3 +--
 src/macros.c   | 12 ++++++++++++
 src/macros.h   |  5 +++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index e1d738dd6ef..d47363bc39f 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2627,8 +2627,7 @@ read_char (int commandflag, Lisp_Object map,
       /* Exit the macro if we are at the end.
 	 Also, some things replace the macro with t
 	 to force an early exit.  */
-      if (EQ (Vexecuting_kbd_macro, Qt)
-	  || executing_kbd_macro_index >= XFIXNAT (Flength (Vexecuting_kbd_macro)))
+      if (at_end_of_macro_p ())
 	{
 	  XSETINT (c, -1);
 	  goto exit;
diff --git a/src/macros.c b/src/macros.c
index 5f71bcbd361..62129be1629 100644
--- a/src/macros.c
+++ b/src/macros.c
@@ -353,6 +353,18 @@ init_macros (void)
   executing_kbd_macro = Qnil;
 }
 
+/* Whether the execution of a macro has reached its end.
+   It makes only sense to call this when while executing a macro.  */
+
+bool
+at_end_of_macro_p (void)
+{
+  eassume (!NILP (Vexecuting_kbd_macro));
+  /* Some things replace the macro with t to force an early exit.  */
+  return EQ (Vexecuting_kbd_macro, Qt)
+    || executing_kbd_macro_index >= XFIXNAT (Flength (Vexecuting_kbd_macro));
+}
+
 void
 syms_of_macros (void)
 {
diff --git a/src/macros.h b/src/macros.h
index 51599a29bcd..7870aa4de1d 100644
--- a/src/macros.h
+++ b/src/macros.h
@@ -47,4 +47,9 @@ #define EMACS_MACROS_H
 
 extern void store_kbd_macro_char (Lisp_Object);
 
+/* Whether the execution of a macro has reached its end.
+   It makes only sense to call this when while executing a macro.  */
+
+extern bool at_end_of_macro_p (void);
+
 #endif /* EMACS_MACROS_H */
-- 
2.43.0


[-- Attachment #3: 0002-src-keyboard.c-requeued_events_pending_p-Revive.patch --]
[-- Type: text/x-patch, Size: 2034 bytes --]

From 23bcea43d9c56e829bf24877653d59a051e6cd19 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:29:34 +0100
Subject: [PATCH 2/4] * src/keyboard.c (requeued_events_pending_p): Revive

* src/keyboard.c (requeued_events_pending_p):
Fix 'requeued_events_pending_p' function by adding missing
'Vunread_*_event' variables. When 'requeued_events_pending_p'
was added (commit b1878f450868365f27af0c71900237056a44f900),
'Vunread_command_events' was the only variable of this kind.
* (Finput_pending_p): Actually use the changed function.
---
 src/keyboard.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index d47363bc39f..041b616d9aa 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11556,7 +11556,7 @@ clear_input_pending (void)
 }
 
 /* Return true if there are pending requeued events.
-   This isn't used yet.  The hope is to make wait_reading_process_output
+   In the future, the hope is to make wait_reading_process_output
    call it, and return if it runs Lisp code that unreads something.
    The problem is, kbd_buffer_get_event needs to be fixed to know what
    to do in that case.  It isn't trivial.  */
@@ -11564,7 +11564,9 @@ clear_input_pending (void)
 bool
 requeued_events_pending_p (void)
 {
-  return (CONSP (Vunread_command_events));
+  return (CONSP (Vunread_command_events)
+	  || CONSP (Vunread_post_input_method_events)
+	  || CONSP (Vunread_input_method_events));
 }
 
 DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
@@ -11575,9 +11577,7 @@ DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
 If CHECK-TIMERS is non-nil, timers that are ready to run will do so.  */)
   (Lisp_Object check_timers)
 {
-  if (CONSP (Vunread_command_events)
-      || !NILP (Vunread_post_input_method_events)
-      || !NILP (Vunread_input_method_events))
+  if (requeued_events_pending_p ())
     return (Qt);
 
   /* Process non-user-visible events (Bug#10195).  */
-- 
2.43.0


[-- Attachment #4: 0003-Fix-1-leaking-from-C-to-lisp-in-read-event-etc.patch --]
[-- Type: text/x-patch, Size: 3096 bytes --]

From 27db0625e5348d50bc59674c0a8c8d45ed4db874 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:32:09 +0100
Subject: [PATCH 3/4] Fix -1 leaking from C to lisp in 'read-event' etc.

This fixes a bug that could make 'read-event', 'read-char', and
'read-char-exclusive' erroneously return -1, an internal magic return
value of 'read_char' leaked from C to lisp.
* src/keyboard.c (read_char, read_key_sequence): Move handling
of the end of a keyboard macro from 'read_char' to its caller
'read_key_sequence', which is the only caller that can
meaningfully deal with this case.
---
 src/keyboard.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index 041b616d9aa..9b5e020238c 100644
--- 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;
-	}
-
       c = Faref (Vexecuting_kbd_macro, make_int (executing_kbd_macro_index));
       if (STRINGP (Vexecuting_kbd_macro)
 	  && (XFIXNAT (c) & 0x80) && (XFIXNAT (c) <= 0xff))
@@ -10684,8 +10676,19 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	    }
 	  used_mouse_menu = used_mouse_menu_history[t];
 	}
-
-      /* 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;
+	}
+      /* Otherwise, we should actually read a character.  */
       else
 	{
 	  {
@@ -10777,18 +10780,6 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	      return -1;
 	    }
 
-	  /* read_char returns -1 at the end of a macro.
-	     Emacs 18 handles this by returning immediately with a
-	     zero, so that's what we'll do.  */
-	  if (FIXNUMP (key) && XFIXNUM (key) == -1)
-	    {
-	      t = 0;
-	      /* The Microsoft C compiler can't handle the goto that
-		 would go here.  */
-	      dummyflag = true;
-	      break;
-	    }
-
 	  /* If the current buffer has been changed from under us, the
 	     keymap may have changed, so replay the sequence.  */
 	  if (BUFFERP (key))
-- 
2.43.0


[-- Attachment #5: 0004-Remove-workarounds-for-solved-read-event-bug.patch --]
[-- Type: text/x-patch, Size: 3001 bytes --]

From e73e08927792303013a8a9935656365f9f2299b6 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@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"))
       (if (= ch ?Z)
 	  (progn
 	    (setq ch (read-char))
@@ -1300,8 +1298,6 @@ calc-kbd-loop
 	(message "Reading loop body..."))
     (while (>= count 0)
       (setq ch (read-event))
-      (if (eq ch -1)
-	  (error "Unterminated Z%c in keyboard macro" open))
       (if (eq ch ?Z)
 	  (progn
 	    (setq ch (read-event)
@@ -1428,8 +1424,6 @@ calc-kbd-push
 	       (message "Reading body..."))
 	   (while (>= count 0)
 	     (setq ch (read-char))
-	     (if (= ch -1)
-		 (error "Unterminated Z` in keyboard macro"))
 	     (if (= ch ?Z)
 		 (progn
 		   (setq ch (read-char)
diff --git a/lisp/net/dbus.el b/lisp/net/dbus.el
index 77b334e704e..46f85daba24 100644
--- a/lisp/net/dbus.el
+++ b/lisp/net/dbus.el
@@ -371,11 +371,7 @@ dbus-call-method
 	 (apply
           #'dbus-message-internal dbus-message-type-method-call
           bus service path interface method #'dbus-call-method-handler args))
-        (result (unless executing-kbd-macro (cons :pending nil))))
-
-    ;; While executing a keyboard macro, we run into an infinite loop,
-    ;; receiving the event -1.  So we don't try to get the result.
-    ;; (Bug#62018)
+        (result (cons :pending nil)))
 
     ;; Wait until `dbus-call-method-handler' has put the result into
     ;; `dbus-return-values-table'.  If no timeout is given, use the
diff --git a/lisp/subr.el b/lisp/subr.el
index df28989b399..78e7ffbc979 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3539,11 +3539,6 @@ read-char-choice-with-read-key
 		 (help-form-show)))
 	   ((memq char chars)
 	    (setq done t))
-	   ((and executing-kbd-macro (= char -1))
-	    ;; read-event returns -1 if we are in a kbd macro and
-	    ;; there are no more events in the macro.  Attempt to
-	    ;; get an event interactively.
-	    (setq executing-kbd-macro nil))
 	   ((not inhibit-keyboard-quit)
 	    (cond
 	     ((and (null esc-flag) (eq char ?\e))
-- 
2.43.0

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

[Eli Zaretskii]

> From: Tim Ruffing <crypto@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.

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

[Tim Ruffing]

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

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

[Eli Zaretskii]

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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

[Tim Ruffing]

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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

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

[Tim Ruffing]

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

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

[Tim Ruffing]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

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

[-- Attachment #2: 0001-Extract-check-for-end-of-macro-to-function.patch --]
[-- Type: text/x-patch, Size: 2056 bytes --]

From 27de73af150c458669de29e8662a281baeb0a603 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:26:26 +0100
Subject: [PATCH 1/5] Extract check for end of macro to function

* src/macros.h (at_end_of_macro_p):
* src/macros.c (at_end_of_macro_p):
New function.
* src/keyboard.c (read_char): Use the new function.
---
 src/keyboard.c |  3 +--
 src/macros.c   | 12 ++++++++++++
 src/macros.h   |  5 +++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index eb0de98bad1..b6fc568cde5 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2637,8 +2637,7 @@ read_char (int commandflag, Lisp_Object map,
       /* Exit the macro if we are at the end.
 	 Also, some things replace the macro with t
 	 to force an early exit.  */
-      if (EQ (Vexecuting_kbd_macro, Qt)
-	  || executing_kbd_macro_index >= XFIXNAT (Flength (Vexecuting_kbd_macro)))
+      if (at_end_of_macro_p ())
 	{
 	  XSETINT (c, -1);
 	  goto exit;
diff --git a/src/macros.c b/src/macros.c
index 5f71bcbd361..62129be1629 100644
--- a/src/macros.c
+++ b/src/macros.c
@@ -353,6 +353,18 @@ init_macros (void)
   executing_kbd_macro = Qnil;
 }
 
+/* Whether the execution of a macro has reached its end.
+   It makes only sense to call this when while executing a macro.  */
+
+bool
+at_end_of_macro_p (void)
+{
+  eassume (!NILP (Vexecuting_kbd_macro));
+  /* Some things replace the macro with t to force an early exit.  */
+  return EQ (Vexecuting_kbd_macro, Qt)
+    || executing_kbd_macro_index >= XFIXNAT (Flength (Vexecuting_kbd_macro));
+}
+
 void
 syms_of_macros (void)
 {
diff --git a/src/macros.h b/src/macros.h
index 51599a29bcd..7870aa4de1d 100644
--- a/src/macros.h
+++ b/src/macros.h
@@ -47,4 +47,9 @@ #define EMACS_MACROS_H
 
 extern void store_kbd_macro_char (Lisp_Object);
 
+/* Whether the execution of a macro has reached its end.
+   It makes only sense to call this when while executing a macro.  */
+
+extern bool at_end_of_macro_p (void);
+
 #endif /* EMACS_MACROS_H */
-- 
2.44.0


[-- Attachment #3: 0002-src-keyboard.c-requeued_events_pending_p-Improve-nam.patch --]
[-- Type: text/x-patch, Size: 3421 bytes --]

From 848ede9b578833414e764d9f9dd85b7677a4b110 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:29:34 +0100
Subject: [PATCH 2/5] * src/keyboard.c (requeued_events_pending_p): Improve
 name and fix comment

* src/keyboard.c, src/keyboard.h (requeued_events_pending_p): Rename to
'requeued_command_events_pending_p' to clarify that the function covers
only command events. Fix wrong comment that claimed that the function
was unused.
* src/process.c (wait_reading_process_output): Update caller to use the
new name.
---
 src/keyboard.c | 8 ++------
 src/keyboard.h | 2 +-
 src/process.c  | 8 ++++----
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index b6fc568cde5..e5efde4ef53 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11565,14 +11565,10 @@ clear_input_pending (void)
   input_pending = false;
 }
 
-/* Return true if there are pending requeued events.
-   This isn't used yet.  The hope is to make wait_reading_process_output
-   call it, and return if it runs Lisp code that unreads something.
-   The problem is, kbd_buffer_get_event needs to be fixed to know what
-   to do in that case.  It isn't trivial.  */
+/* Return true if there are pending requeued command events.  */
 
 bool
-requeued_events_pending_p (void)
+requeued_command_events_pending_p (void)
 {
   return (CONSP (Vunread_command_events));
 }
diff --git a/src/keyboard.h b/src/keyboard.h
index 68e68bc2ae3..600aaf11517 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -483,7 +483,7 @@ #define EVENT_HEAD_KIND(event_head) \
 extern int gobble_input (void);
 extern bool input_polling_used (void);
 extern void clear_input_pending (void);
-extern bool requeued_events_pending_p (void);
+extern bool requeued_command_events_pending_p (void);
 extern void bind_polling_period (int);
 extern int make_ctrl_char (int) ATTRIBUTE_CONST;
 extern void stuff_buffered_input (Lisp_Object);
diff --git a/src/process.c b/src/process.c
index 48a2c0c8e53..6b8b483cdf7 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5439,7 +5439,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
 	  /* If there is unread keyboard input, also return.  */
 	  if (read_kbd != 0
-	      && requeued_events_pending_p ())
+	      && requeued_command_events_pending_p ())
 	    break;
 
           /* This is so a breakpoint can be put here.  */
@@ -5849,7 +5849,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
       /* If there is unread keyboard input, also return.  */
       if (read_kbd != 0
-	  && requeued_events_pending_p ())
+	  && requeued_command_events_pending_p ())
 	break;
 
       /* If we are not checking for keyboard input now,
@@ -8036,7 +8036,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
 	  /* If there is unread keyboard input, also return.  */
 	  if (read_kbd != 0
-	      && requeued_events_pending_p ())
+	      && requeued_command_events_pending_p ())
 	    break;
 
 	  if (timespec_valid_p (timer_delay))
@@ -8109,7 +8109,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
       /* If there is unread keyboard input, also return.  */
       if (read_kbd
-	  && requeued_events_pending_p ())
+	  && requeued_command_events_pending_p ())
 	break;
 
       /* If wait_for_cell. check for keyboard input
-- 
2.44.0


[-- Attachment #4: 0003-src-keyboard.c-requeued_events_pending_p-New-functio.patch --]
[-- Type: text/x-patch, Size: 2432 bytes --]

From 17593b2c1e7313b156ecc7077aaa019290b8a096 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:29:34 +0100
Subject: [PATCH 3/5] * src/keyboard.c (requeued_events_pending_p): New
 function

* src/keyboard.c, src/keyboard.h (requeued_events_pending_p): Add
function 'requeued_events_pending_p' (whose name was made available in
the previous commit). As opposed to the previous function with the same
name, the new function covers both command and other events.
* src/keyboard.c (Finput_pending_p): Use the new function.
---
 src/keyboard.c | 15 ++++++++++++---
 src/keyboard.h |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index e5efde4ef53..c898988dc5f 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11573,6 +11573,17 @@ requeued_command_events_pending_p (void)
   return (CONSP (Vunread_command_events));
 }
 
+/* Return true if there are any pending requeued events (command events
+   or events to be processed by input methods).  */
+
+bool
+requeued_events_pending_p (void)
+{
+  return (requeued_command_events_pending_p ()
+	  || !NILP (Vunread_post_input_method_events)
+	  || !NILP (Vunread_input_method_events));
+}
+
 DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
        doc: /* Return t if command input is currently available with no wait.
 Actually, the value is nil only if we can be sure that no input is available;
@@ -11581,9 +11592,7 @@ DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
 If CHECK-TIMERS is non-nil, timers that are ready to run will do so.  */)
   (Lisp_Object check_timers)
 {
-  if (CONSP (Vunread_command_events)
-      || !NILP (Vunread_post_input_method_events)
-      || !NILP (Vunread_input_method_events))
+  if (requeued_events_pending_p ())
     return (Qt);
 
   /* Process non-user-visible events (Bug#10195).  */
diff --git a/src/keyboard.h b/src/keyboard.h
index 600aaf11517..2ce003fd444 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -484,6 +484,7 @@ #define EVENT_HEAD_KIND(event_head) \
 extern bool input_polling_used (void);
 extern void clear_input_pending (void);
 extern bool requeued_command_events_pending_p (void);
+extern bool requeued_events_pending_p (void);
 extern void bind_polling_period (int);
 extern int make_ctrl_char (int) ATTRIBUTE_CONST;
 extern void stuff_buffered_input (Lisp_Object);
-- 
2.44.0


[-- Attachment #5: 0004-Continue-reading-in-read-event-etc.-at-the-end-of-a-.patch --]
[-- Type: text/x-patch, Size: 5065 bytes --]

From 6be0f5f6c4253cefd350b23c79e469058d6c589c Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:32:09 +0100
Subject: [PATCH 4/5] Continue reading in 'read-event' etc. at the end of a
 keyboard macro

This fixes a bug that could make 'read-event', 'read-char', and
'read-char-exclusive' erroneously return -1, an internal magic return
value of 'read_char' leaked from C to lisp. Instead of returning -1, the
aforementioned lisp functions now transparently continue reading
available input (e.g., from the keyboard) when reaching the end of a
keyboard macro.
* src/keyboard.c (read_char, read_key_sequence): Move handling
of the end of a keyboard macro from 'read_char' to its caller
'read_key_sequence', which is the only caller that can
meaningfully deal with this case.
* src/macros.c (Fexecute_kbd_macro): Document how the end of keyboard
macro is processed.
---
 src/keyboard.c | 39 +++++++++++++++------------------------
 src/macros.c   | 23 +++++++++++++++++++++++
 2 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index c898988dc5f..e34d1b5491d 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2620,7 +2620,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
@@ -2634,15 +2635,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;
-	}
-
       c = Faref (Vexecuting_kbd_macro, make_int (executing_kbd_macro_index));
       if (STRINGP (Vexecuting_kbd_macro)
 	  && (XFIXNAT (c) & 0x80) && (XFIXNAT (c) <= 0xff))
@@ -10694,8 +10686,19 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	    }
 	  used_mouse_menu = used_mouse_menu_history[t];
 	}
-
-      /* 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;
+	}
+      /* Otherwise, we should actually read a character.  */
       else
 	{
 	  {
@@ -10787,18 +10790,6 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	      return -1;
 	    }
 
-	  /* read_char returns -1 at the end of a macro.
-	     Emacs 18 handles this by returning immediately with a
-	     zero, so that's what we'll do.  */
-	  if (FIXNUMP (key) && XFIXNUM (key) == -1)
-	    {
-	      t = 0;
-	      /* The Microsoft C compiler can't handle the goto that
-		 would go here.  */
-	      dummyflag = true;
-	      break;
-	    }
-
 	  /* If the current buffer has been changed from under us, the
 	     keymap may have changed, so replay the sequence.  */
 	  if (BUFFERP (key))
diff --git a/src/macros.c b/src/macros.c
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).
+
+     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).
+
+     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.  */
   do
     {
       Vexecuting_kbd_macro = final;
-- 
2.44.0


[-- Attachment #6: 0005-Remove-workarounds-for-solved-read-event-bug.patch --]
[-- Type: text/x-patch, Size: 3001 bytes --]

From 96684bd0dd5b2ed701e3b440abd210dd9c6fdfd6 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:32:09 +0100
Subject: [PATCH 5/5] 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"))
       (if (= ch ?Z)
 	  (progn
 	    (setq ch (read-char))
@@ -1300,8 +1298,6 @@ calc-kbd-loop
 	(message "Reading loop body..."))
     (while (>= count 0)
       (setq ch (read-event))
-      (if (eq ch -1)
-	  (error "Unterminated Z%c in keyboard macro" open))
       (if (eq ch ?Z)
 	  (progn
 	    (setq ch (read-event)
@@ -1428,8 +1424,6 @@ calc-kbd-push
 	       (message "Reading body..."))
 	   (while (>= count 0)
 	     (setq ch (read-char))
-	     (if (= ch -1)
-		 (error "Unterminated Z` in keyboard macro"))
 	     (if (= ch ?Z)
 		 (progn
 		   (setq ch (read-char)
diff --git a/lisp/net/dbus.el b/lisp/net/dbus.el
index 77b334e704e..46f85daba24 100644
--- a/lisp/net/dbus.el
+++ b/lisp/net/dbus.el
@@ -371,11 +371,7 @@ dbus-call-method
 	 (apply
           #'dbus-message-internal dbus-message-type-method-call
           bus service path interface method #'dbus-call-method-handler args))
-        (result (unless executing-kbd-macro (cons :pending nil))))
-
-    ;; While executing a keyboard macro, we run into an infinite loop,
-    ;; receiving the event -1.  So we don't try to get the result.
-    ;; (Bug#62018)
+        (result (cons :pending nil)))
 
     ;; Wait until `dbus-call-method-handler' has put the result into
     ;; `dbus-return-values-table'.  If no timeout is given, use the
diff --git a/lisp/subr.el b/lisp/subr.el
index d58f8ba3b27..ce933e3bfdc 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3554,11 +3554,6 @@ read-char-choice-with-read-key
 		 (help-form-show)))
 	   ((memq char chars)
 	    (setq done t))
-	   ((and executing-kbd-macro (= char -1))
-	    ;; read-event returns -1 if we are in a kbd macro and
-	    ;; there are no more events in the macro.  Attempt to
-	    ;; get an event interactively.
-	    (setq executing-kbd-macro nil))
 	   ((not inhibit-keyboard-quit)
 	    (cond
 	     ((and (null esc-flag) (eq char ?\e))
-- 
2.44.0

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

[Eli Zaretskii]

> From: Tim Ruffing <crypto@timruffing.de>
> Cc: Eli Zaretskii <eliz@gnu.org>, 68272@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?

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  68272@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).

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: crypto@timruffing.de,  68272@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?

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: crypto@timruffing.de,  68272@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.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: crypto@timruffing.de,  68272@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.

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

[Tim Ruffing]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

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

[-- Attachment #2: 0001-Extract-check-for-end-of-macro-to-function.patch --]
[-- Type: text/x-patch, Size: 2034 bytes --]

From 22d890c88e721f15a591789d233bc943c9095593 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:26:26 +0100
Subject: [PATCH 1/7] Extract check for end of macro to function

* src/macros.h (at_end_of_macro_p):
* src/macros.c (at_end_of_macro_p):
New function.
* src/keyboard.c (read_char): Use the new function.
---
 src/keyboard.c |  3 +--
 src/macros.c   | 12 ++++++++++++
 src/macros.h   |  5 +++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index eb0de98bad1..b6fc568cde5 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2637,8 +2637,7 @@ read_char (int commandflag, Lisp_Object map,
       /* Exit the macro if we are at the end.
 	 Also, some things replace the macro with t
 	 to force an early exit.  */
-      if (EQ (Vexecuting_kbd_macro, Qt)
-	  || executing_kbd_macro_index >= XFIXNAT (Flength (Vexecuting_kbd_macro)))
+      if (at_end_of_macro_p ())
 	{
 	  XSETINT (c, -1);
 	  goto exit;
diff --git a/src/macros.c b/src/macros.c
index 5f71bcbd361..faec9dc646d 100644
--- a/src/macros.c
+++ b/src/macros.c
@@ -353,6 +353,18 @@ init_macros (void)
   executing_kbd_macro = Qnil;
 }
 
+/* Whether the execution of a macro has reached its end.
+   This should be called only while executing a macro.  */
+
+bool
+at_end_of_macro_p (void)
+{
+  eassume (!NILP (Vexecuting_kbd_macro));
+  /* Some things replace the macro with t to force an early exit.  */
+  return EQ (Vexecuting_kbd_macro, Qt)
+    || executing_kbd_macro_index >= XFIXNAT (Flength (Vexecuting_kbd_macro));
+}
+
 void
 syms_of_macros (void)
 {
diff --git a/src/macros.h b/src/macros.h
index 51599a29bcd..cb6ac8aa206 100644
--- a/src/macros.h
+++ b/src/macros.h
@@ -47,4 +47,9 @@ #define EMACS_MACROS_H
 
 extern void store_kbd_macro_char (Lisp_Object);
 
+/* Whether the execution of a macro has reached its end.
+   This should be called only while executing a macro.  */
+
+extern bool at_end_of_macro_p (void);
+
 #endif /* EMACS_MACROS_H */
-- 
2.44.0


[-- Attachment #3: 0002-src-keyboard.c-requeued_events_pending_p-Improve-nam.patch --]
[-- Type: text/x-patch, Size: 3421 bytes --]

From 929cbc6c6c4cd4a0d71ef37fc47ca7e9ee5b116b Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:29:34 +0100
Subject: [PATCH 2/7] * src/keyboard.c (requeued_events_pending_p): Improve
 name and fix comment

* src/keyboard.c, src/keyboard.h (requeued_events_pending_p): Rename to
'requeued_command_events_pending_p' to clarify that the function covers
only command events. Fix wrong comment that claimed that the function
was unused.
* src/process.c (wait_reading_process_output): Update caller to use the
new name.
---
 src/keyboard.c | 8 ++------
 src/keyboard.h | 2 +-
 src/process.c  | 8 ++++----
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index b6fc568cde5..e5efde4ef53 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11565,14 +11565,10 @@ clear_input_pending (void)
   input_pending = false;
 }
 
-/* Return true if there are pending requeued events.
-   This isn't used yet.  The hope is to make wait_reading_process_output
-   call it, and return if it runs Lisp code that unreads something.
-   The problem is, kbd_buffer_get_event needs to be fixed to know what
-   to do in that case.  It isn't trivial.  */
+/* Return true if there are pending requeued command events.  */
 
 bool
-requeued_events_pending_p (void)
+requeued_command_events_pending_p (void)
 {
   return (CONSP (Vunread_command_events));
 }
diff --git a/src/keyboard.h b/src/keyboard.h
index 68e68bc2ae3..600aaf11517 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -483,7 +483,7 @@ #define EVENT_HEAD_KIND(event_head) \
 extern int gobble_input (void);
 extern bool input_polling_used (void);
 extern void clear_input_pending (void);
-extern bool requeued_events_pending_p (void);
+extern bool requeued_command_events_pending_p (void);
 extern void bind_polling_period (int);
 extern int make_ctrl_char (int) ATTRIBUTE_CONST;
 extern void stuff_buffered_input (Lisp_Object);
diff --git a/src/process.c b/src/process.c
index 48a2c0c8e53..6b8b483cdf7 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5439,7 +5439,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
 	  /* If there is unread keyboard input, also return.  */
 	  if (read_kbd != 0
-	      && requeued_events_pending_p ())
+	      && requeued_command_events_pending_p ())
 	    break;
 
           /* This is so a breakpoint can be put here.  */
@@ -5849,7 +5849,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
       /* If there is unread keyboard input, also return.  */
       if (read_kbd != 0
-	  && requeued_events_pending_p ())
+	  && requeued_command_events_pending_p ())
 	break;
 
       /* If we are not checking for keyboard input now,
@@ -8036,7 +8036,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
 	  /* If there is unread keyboard input, also return.  */
 	  if (read_kbd != 0
-	      && requeued_events_pending_p ())
+	      && requeued_command_events_pending_p ())
 	    break;
 
 	  if (timespec_valid_p (timer_delay))
@@ -8109,7 +8109,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
       /* If there is unread keyboard input, also return.  */
       if (read_kbd
-	  && requeued_events_pending_p ())
+	  && requeued_command_events_pending_p ())
 	break;
 
       /* If wait_for_cell. check for keyboard input
-- 
2.44.0


[-- Attachment #4: 0003-src-keyboard.c-requeued_events_pending_p-New-functio.patch --]
[-- Type: text/x-patch, Size: 2467 bytes --]

From 13a1547153c139897fd1e5652654eb6bad360390 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:29:34 +0100
Subject: [PATCH 3/7] * src/keyboard.c (requeued_events_pending_p): New
 function

* src/keyboard.c, src/keyboard.h (requeued_events_pending_p): Add
function 'requeued_events_pending_p' (whose name was made available in
the previous commit). As opposed to the previous function with the same
name, the new function covers both command and other events.
* src/keyboard.c (Finput_pending_p): Use the new function.
---
 src/keyboard.c | 16 +++++++++++++---
 src/keyboard.h |  1 +
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index e5efde4ef53..bd8d3aa7ecf 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11573,6 +11573,18 @@ requeued_command_events_pending_p (void)
   return (CONSP (Vunread_command_events));
 }
 
+/* Return true if there are any pending requeued events (command events
+   or events to be processed by other levels of the input processing
+   stages).  */
+
+bool
+requeued_events_pending_p (void)
+{
+  return (requeued_command_events_pending_p ()
+	  || !NILP (Vunread_post_input_method_events)
+	  || !NILP (Vunread_input_method_events));
+}
+
 DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
        doc: /* Return t if command input is currently available with no wait.
 Actually, the value is nil only if we can be sure that no input is available;
@@ -11581,9 +11593,7 @@ DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
 If CHECK-TIMERS is non-nil, timers that are ready to run will do so.  */)
   (Lisp_Object check_timers)
 {
-  if (CONSP (Vunread_command_events)
-      || !NILP (Vunread_post_input_method_events)
-      || !NILP (Vunread_input_method_events))
+  if (requeued_events_pending_p ())
     return (Qt);
 
   /* Process non-user-visible events (Bug#10195).  */
diff --git a/src/keyboard.h b/src/keyboard.h
index 600aaf11517..2ce003fd444 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -484,6 +484,7 @@ #define EVENT_HEAD_KIND(event_head) \
 extern bool input_polling_used (void);
 extern void clear_input_pending (void);
 extern bool requeued_command_events_pending_p (void);
+extern bool requeued_events_pending_p (void);
 extern void bind_polling_period (int);
 extern int make_ctrl_char (int) ATTRIBUTE_CONST;
 extern void stuff_buffered_input (Lisp_Object);
-- 
2.44.0


[-- Attachment #5: 0004-Continue-reading-in-read-event-etc.-at-the-end-of-a-.patch --]
[-- Type: text/x-patch, Size: 7069 bytes --]

From f8dfb4056bac10e1aa5b63407a7af5f78ad61147 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:32:09 +0100
Subject: [PATCH 4/7] Continue reading in 'read-event' etc. at the end of a
 keyboard macro

This fixes a bug that could make 'read-event', 'read-char', and
'read-char-exclusive' erroneously return -1, an internal magic return
value of 'read_char' leaked from C to lisp. Instead of returning -1, the
aforementioned lisp functions now transparently continue reading
available input (e.g., from the keyboard) when reaching the end of a
keyboard macro.

* src/keyboard.c (read_char, read_key_sequence): Move handling
of the end of a keyboard macro from 'read_char' to its caller
'read_key_sequence', which is the only caller that can
meaningfully deal with this case.
* src/macros.c (Fexecute_kbd_macro): Document how the end of keyboard
macro is processed.
* etc/NEWS: Announce this change.
---
 etc/NEWS       | 12 ++++++++++++
 src/keyboard.c | 39 +++++++++++++++------------------------
 src/macros.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 24 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 2aa669be344..bb57c7640d6 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2131,6 +2131,18 @@ Like the variable with the same name, it adds menus from the list that
 is the value of the property to context menus shown when clicking on the
 text which as this property.
 
+---
+** Detecting the end of an iteration of a keyboard macro
+'read-event', 'read-char', and 'read-char-exclusive' no longer return -1
+when called at the end of an iteration of a the execution of a keyboard
+macro.  Instead, they will transparently continue reading available input
+(e.g., from the keyboard).  If you need to detect the end of a macro
+iteration, check the following condition before calling one of the
+aforementioned functions:
+
+    (and (arrayp executing-kbd-macro)
+         (>= executing-kbd-macro-index (length executing-kbd-macro))))
+
 \f
 * Changes in Emacs 30.1 on Non-Free Operating Systems
 
diff --git a/src/keyboard.c b/src/keyboard.c
index bd8d3aa7ecf..cadb376430e 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2620,7 +2620,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
@@ -2634,15 +2635,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;
-	}
-
       c = Faref (Vexecuting_kbd_macro, make_int (executing_kbd_macro_index));
       if (STRINGP (Vexecuting_kbd_macro)
 	  && (XFIXNAT (c) & 0x80) && (XFIXNAT (c) <= 0xff))
@@ -10694,8 +10686,19 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	    }
 	  used_mouse_menu = used_mouse_menu_history[t];
 	}
-
-      /* 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;
+	}
+      /* Otherwise, we should actually read a character.  */
       else
 	{
 	  {
@@ -10787,18 +10790,6 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	      return -1;
 	    }
 
-	  /* read_char returns -1 at the end of a macro.
-	     Emacs 18 handles this by returning immediately with a
-	     zero, so that's what we'll do.  */
-	  if (FIXNUMP (key) && XFIXNUM (key) == -1)
-	    {
-	      t = 0;
-	      /* The Microsoft C compiler can't handle the goto that
-		 would go here.  */
-	      dummyflag = true;
-	      break;
-	    }
-
 	  /* If the current buffer has been changed from under us, the
 	     keymap may have changed, so replay the sequence.  */
 	  if (BUFFERP (key))
diff --git a/src/macros.c b/src/macros.c
index faec9dc646d..230195d9488 100644
--- a/src/macros.c
+++ b/src/macros.c
@@ -314,6 +314,48 @@ 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 possibly multiple
+     iterations of the macro.
+
+     The state variables that control the execution of a single
+     iteration are Vexecuting_kbd_macro and executing_kbd_macro_index,
+     which can be accessed from lisp. The purpose of the variables
+     executing_kbd_macro and executing_kbd_macro_iteration is to
+     remember the most recently started macro and its iteration count.
+     This makes it possible to produce a meaningful message in case of
+     errors during the execution of the macro.
+
+     In a single iteration, individual characters from the macro are
+     read by read_char, which takes care of incrementing
+     executing_kbd_macro_index after each character.
+
+     The end of a macro iteration is handled as follows:
+      - read_key_sequence asks at_end_of_macro_p whether the end of the
+        iteration 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).
+
+     Macro executions form a stack.  After the last iteration of the
+     execution of one stack item, or in case of an error during one of
+     the iterations, pop_kbd_macro (invoked via unwind-protect) will
+     restore Vexecuting_kbd_macro and executing_kbd_macro_index, and
+     run 'kbd-macro-termination-hook'.
+
+     If read_char happens to be called at the end of a macro interation,
+     but before read_key_sequence could handle the end (e.g., when lisp
+     code calls 'read-event', 'read-char', or 'read-char-exclusive'),
+     read_char will simply continue reading other available input
+     (Bug#68272).  Vexecuting_kbd_macro and executing_kbd_macro remain
+     untouched until the end of the iteration is handled.
+
+     This is similar (in observable behavior) to a posibly simpler
+     implementation of keyboard macros in which this function 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.  */
   do
     {
       Vexecuting_kbd_macro = final;
-- 
2.44.0


[-- Attachment #6: 0005-Remove-workarounds-for-solved-read-event-bug.patch --]
[-- Type: text/x-patch, Size: 2012 bytes --]

From 54e01b030b299049ff710f617082447828882b0c Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:32:09 +0100
Subject: [PATCH 5/7] Remove workarounds for solved 'read-event' bug

* lisp/subr.el (read-char-choice-with-read-key):
* lisp/net/dbus.el (dbus-call-method):
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/net/dbus.el | 6 +-----
 lisp/subr.el     | 5 -----
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/lisp/net/dbus.el b/lisp/net/dbus.el
index 77b334e704e..46f85daba24 100644
--- a/lisp/net/dbus.el
+++ b/lisp/net/dbus.el
@@ -371,11 +371,7 @@ dbus-call-method
 	 (apply
           #'dbus-message-internal dbus-message-type-method-call
           bus service path interface method #'dbus-call-method-handler args))
-        (result (unless executing-kbd-macro (cons :pending nil))))
-
-    ;; While executing a keyboard macro, we run into an infinite loop,
-    ;; receiving the event -1.  So we don't try to get the result.
-    ;; (Bug#62018)
+        (result (cons :pending nil)))
 
     ;; Wait until `dbus-call-method-handler' has put the result into
     ;; `dbus-return-values-table'.  If no timeout is given, use the
diff --git a/lisp/subr.el b/lisp/subr.el
index d58f8ba3b27..ce933e3bfdc 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3554,11 +3554,6 @@ read-char-choice-with-read-key
 		 (help-form-show)))
 	   ((memq char chars)
 	    (setq done t))
-	   ((and executing-kbd-macro (= char -1))
-	    ;; read-event returns -1 if we are in a kbd macro and
-	    ;; there are no more events in the macro.  Attempt to
-	    ;; get an event interactively.
-	    (setq executing-kbd-macro nil))
 	   ((not inhibit-keyboard-quit)
 	    (cond
 	     ((and (null esc-flag) (eq char ?\e))
-- 
2.44.0


[-- Attachment #7: 0006-lisp-calc-calc-prog.el-Switch-to-new-method-of-detec.patch --]
[-- Type: text/x-patch, Size: 2136 bytes --]

From caa30fe9aa3cf75114d9db05cd0aed538444e957 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Sat, 9 Mar 2024 12:29:39 +0100
Subject: [PATCH 6/7] * lisp/calc/calc-prog.el: Switch to new method of
 detecting end of kbd macro

'read-char' will no longer return -1 as of
ac82baea1c41ec974ad49f2861ae6c06bda2b4ed. This switches to a cleaner
method of detecting whether the end of a keyboard macro has been
reached.

* lisp/calc/calc-prog.el (calc--at-end-of-kmacro-p): New function.
(calc-kbd-skip-to-else-if): Use the function.

Co-authored-by: Stefan Monnier <monnier@iro.umontreal.ca>
---
 lisp/calc/calc-prog.el | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

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


[-- Attachment #8: 0007-src-keyboard.c-read_key_sequence-Remove-MSVC-compati.patch --]
[-- Type: text/x-patch, Size: 1914 bytes --]

From a5cefe9ab516d2b062f3a0c68113e55f43e3085b Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Sat, 9 Mar 2024 12:15:22 +0100
Subject: [PATCH 7/7] * src/keyboard.c (read_key_sequence): Remove MSVC
 compatibility hack

---
 src/keyboard.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index cadb376430e..1ba74a59537 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -10442,9 +10442,6 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
   Lisp_Object original_uppercase UNINIT;
   int original_uppercase_position = -1;
 
-  /* Gets around Microsoft compiler limitations.  */
-  bool dummyflag = false;
-
 #ifdef HAVE_TEXT_CONVERSION
   bool disabled_conversion;
 
@@ -10693,10 +10690,7 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	  && !requeued_events_pending_p ())
 	{
 	  t = 0;
-	  /* The Microsoft C compiler can't handle the goto that
-	     would go here.  */
-	  dummyflag = true;
-	  break;
+	  goto done;
 	}
       /* Otherwise, we should actually read a character.  */
       else
@@ -11291,10 +11285,7 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	  && help_char_p (EVENT_HEAD (key)) && t > 1)
 	    {
 	      read_key_sequence_cmd = Vprefix_help_command;
-	      /* The Microsoft C compiler can't handle the goto that
-		 would go here.  */
-	      dummyflag = true;
-	      break;
+	      goto done;
 	    }
 
       /* If KEY is not defined in any of the keymaps,
@@ -11343,8 +11334,9 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	    }
 	}
     }
-  if (!dummyflag)
-    read_key_sequence_cmd = current_binding;
+  read_key_sequence_cmd = current_binding;
+
+  done:
   read_key_sequence_remapped
     /* Remap command through active keymaps.
        Do the remapping here, before the unbind_to so it uses the keymaps
-- 
2.44.0

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  68272@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.

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@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.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thank you, pushed, closing,


        Stefan

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

[Andreas Schwab]

On Jan 05 2024, Tim Ruffing wrote:

> From e73e08927792303013a8a9935656365f9f2299b6 Mon Sep 17 00:00:00 2001
> From: Tim Ruffing <crypto@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@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."