bug#25860: 25.1; Double macro execution

bug#25860: 25.1; Double macro execution

[Marek Twardochlib]

I define a macro that simply insert "a" in the butter and called it
insert-a.
This is the macro code after C-x C-k C-e:

------------------------------------------------------------------------------
Command: last-kbd-macro
Key: none

Macro:

a            ;; self-insert-command
RET            ;; newline
------------------------------------------------------------------------------


Then I define another macro that simply call the previous macro
insert-a.

------------------------------------------------------------------------------
Command: last-kbd-macro
Key: none

Macro:

<<insert-a>>        ;; insert-a
a            ;; self-insert-command
RET            ;; newline
------------------------------------------------------------------------------

When I call the last macro the "a" will be inserted twice. Expected
behaviour is "a" inserted one time. The code of last macro shows already
the problem: there is a call to insert-a AND the body of macro insert-a.

I use macros on my daily job and this issue is fatal for me. I would be
happy if you could fix the bug.

Kind regards
Marek Twardochlib




In GNU Emacs 25.1.1 (x86_64-w64-mingw32)
 of 2016-11-15 built on LAPHROAIG
Windowing system distributor 'Microsoft Corp.', version 6.1.7601
Configured using:
 'configure --without-dbus --without-compress-install 'CFLAGS=-O2
 -static -g3''

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS

Important settings:
  value of $LANG: DEU
  locale-coding-system: cp1252

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
Mark set [2 times]
New macro counter value: 1 (1)
(Type e to repeat macro) [3 times]
Mark set [3 times]
Defining kbd macro...
Keyboard macro defined
Formatting keyboard macro...done
Defining kbd macro...
Keyboard macro defined
Formatting keyboard macro...done

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message dired format-spec rfc822 mml
mml-sec password-cache epg epg-config gnus-util mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util help-fns mail-prsvr mail-utils cl-seq
cl-extra help-mode easymenu edmacro cl-loaddefs pcase cl-lib kmacro
time-date mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel dos-w32 ls-lisp disp-table w32-win w32-vars
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list newcomment elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese charscript case-table epa-hook jka-cmpr-hook help
simple abbrev minibuffer cl-preloaded nadvice loaddefs button faces
cus-face macroexp files text-properties overlay sha1 md5 base64 format
env code-pages mule custom widget hashtable-print-readable backquote
w32notify w32 multi-tty make-network-process emacs)

Memory information:
((conses 16 91644 8248)
 (symbols 56 19844 0)
 (miscs 48 55 108)
 (strings 32 16448 5004)
 (string-bytes 1 453320)
 (vectors 16 12026)
 (vector-slots 8 427102 5471)
 (floats 8 160 246)
 (intervals 56 323 31)
 (buffers 976 18))

bug#25860: 25.1; Double macro execution

[Andreas Schwab]

On Feb 24 2017, Marek Twardochlib <wasserwerk.studio@googlemail.com> wrote:

> Then I define another macro that simply call the previous macro
> insert-a.

How did you do that?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#25860: 25.1; Double macro execution

[Tino Calancha]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Feb 24 2017, Marek Twardochlib <wasserwerk.studio@googlemail.com> wrote:
>
>> Then I define another macro that simply call the previous macro
>> insert-a.
>
> How did you do that?
DUNNO how to solve the problem, but i guess th OP means the following:

(defun insert-a-fn (&optional arg)
  (interactive "p")
  (insert "a\n"))

F3 a RET F4
C-x C-k n insert-a RET

(symbol-function 'insert-a)
=>
(lambda (&optional arg)
  "Keyboard macro."
  (interactive "p")
  (kmacro-exec-ring-item (quote ([97 return] 0 "%d")) arg))

I) ;;; Using `insert-a-fn' we insert "a" _once_:
F3 M-x insert-a-fn RET F4
;; Keyboard Macro Editor.  Press C-c C-c to finish; press C-x k RET to cancel.
;; Original keys: <<insert-a-fn>>

Command: last-kbd-macro
Key: none

Macro:

<<insert-a-fn>>		;; insert-a-fn


II) ;;; Using macro `insert-a' we insert "a" _twice_; so you need to remove
    ;;; <<insert-a>> line by hand:
F3 M-x insert-a RET F4
C-x C-k e RET
;; Keyboard Macro Editor.  Press C-c C-c to finish; press C-x k RET to cancel.
;; Original keys: <<insert-a>> a RET

Command: last-kbd-macro
Key: none

Macro:

<<insert-a>>		;; insert-a
a			;; self-insert-command
RET			;; newline

bug#25860: 25.1; Double macro execution

[Tino Calancha]

Tino Calancha <tino.calancha@gmail.com> writes:

> II) ;;; Using macro `insert-a' we insert "a" _twice_; so you need to remove
>     ;;; <<insert-a>> line by hand:
> F3 M-x insert-a RET F4
> C-x C-k e RET
> ;; Keyboard Macro Editor.  Press C-c C-c to finish; press C-x k RET to cancel.
> ;; Original keys: <<insert-a>> a RET
>
> Command: last-kbd-macro
> Key: none
>
> Macro:
>
> <<insert-a>>		;; insert-a
> a			;; self-insert-command
> RET			;; newline

How about introduce a function doing such clean up for us?
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 9c3b20f573b3f448ba98fa4f1f5c950bd2e9f39a Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Sat, 25 Feb 2017 19:05:43 +0900
Subject: [PATCH] Prevent from unintentional duplicate kbd calls

lisp/kmacro.el (kmacro-avoid-double-kbd-calls): New defun.
(kmacro-end-macro): Use it (Bug#25860).
---
 lisp/kmacro.el | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lisp/kmacro.el b/lisp/kmacro.el
index 838a492b6c..baa1c4bc7a 100644
--- a/lisp/kmacro.el
+++ b/lisp/kmacro.el
@@ -592,6 +592,16 @@ kmacro-start-macro
       (if (and defining-kbd-macro append)
 	  (setq defining-kbd-macro 'append)))))
 
+(declare-function edmacro-finish-edit "edmacro")
+
+;; Prevent from unintentional duplicate kbd calls (Bug#25860).
+(defun kmacro--avoid-double-kbd-calls ()
+  (edit-kbd-macro [f4] t)
+  (while (re-search-forward "<<\\([^>]+\\)>>" nil t)
+    (let ((sym (intern (match-string 1))))
+      (when (get sym 'kmacro)
+        (delete-region (match-beginning 0) (match-end 0)))))
+  (edmacro-finish-edit))
 
 ;;;###autoload
 (defun kmacro-end-macro (arg)
@@ -609,6 +619,7 @@ kmacro-end-macro
    ;; Just ignore it when executing the macro.
   (unless executing-kbd-macro
     (end-kbd-macro arg #'kmacro-loop-setup-function)
+    (kmacro--avoid-double-kbd-calls)
     (when (and last-kbd-macro (= (length last-kbd-macro) 0))
       (setq last-kbd-macro nil)
       (message "Ignore empty macro")
-- 
2.11.0

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50 (build 8, x86_64-pc-linux-gnu, GTK+ Version 3.22.7)
 of 2017-02-25
Repository revision: ecbd5f9ac6eb2d31241657bbb3e3f9b860391054

bug#25860: 25.1; Double macro execution

[npostavs]

Tino Calancha <tino.calancha@gmail.com> writes:

> How about introduce a function doing such clean up for us?

It would be preferable to avoid making a mess in the first place.

> +;; Prevent from unintentional duplicate kbd calls (Bug#25860).

Aren't we in danger of removing *intentional* duplicate calls?

bug#25860: 25.1; Double macro execution

[Tino Calancha]

y

On Sat, 25 Feb 2017, npostavs@users.sourceforge.net wrote:

> Tino Calancha <tino.calancha@gmail.com> writes:
>
>> How about introduce a function doing such clean up for us?
>
> It would be preferable to avoid making a mess in the first place.
>
>> +;; Prevent from unintentional duplicate kbd calls (Bug#25860).
>
> Aren't we in danger of removing *intentional* duplicate calls?
That's an interesting question.  I need to think about it.

bug#25860: 25.1; Double macro execution

[Marek Twardochlib]

Hi,

thanks you for a very quickly response!

2017-02-24 15:10 GMT+01:00, Andreas Schwab <schwab@linux-m68k.org>:
> On Feb 24 2017, Marek Twardochlib <wasserwerk.studio@googlemail.com> wrote:
>
>> Then I define another macro that simply call the previous macro
>> insert-a.
>
> How did you do that?

C-x ( M-x insert-a RET C-x )

The first macro, insert-a, was defined with:
C-x ( a RET C-x )
and then named with:
C-x C-k n insert-a RET

The wrong behavior (double execution) will happen as well when I bind
macro defintion to a key and later use this key sequence in the second
macro.

This macro mistake occur only in Emacs version 25.1.1. In 24.1 and
earlier everything works properly.

Kind regards
Marek Twardochlib

bug#25860: 25.1; Double macro execution

[Tino Calancha]

Marek Twardochlib <wasserwerk.studio@googlemail.com> writes:

>> How did you do that?
>
> C-x ( M-x insert-a RET C-x )
>
> The first macro, insert-a, was defined with:
> C-x ( a RET C-x )
> and then named with:
> C-x C-k n insert-a RET
>
> The wrong behavior (double execution) will happen as well when I bind
> macro defintion to a key and later use this key sequence in the second
> macro.
>
> This macro mistake occur only in Emacs version 25.1.1. In 24.1 and
> earlier everything works properly.
Thank you for the additional information.
You are right.  This is a regression introduced in Emacs 25.1.

I] Not observed with Emacs-24.5.
II] I reproduce the issue with Emacs-25.1.
*)  I see the issue with Emacs-25.1 even after loading kmacro.el
    from Emacs-24.5.  That seems to point to changes in the
    C source code; maybe some changes in src/macro.c?

I am having some difficulties to do a bisect to track down
changes in macro.c.  Maybe someone can do it much faster and find
the commit causing this issue.
    

  

bug#25860: 25.1; Double macro execution

[npostavs]

tags 25860 confirmed
quit

Tino Calancha <tino.calancha@gmail.com> writes:
> Thank you for the additional information.
> You are right.  This is a regression introduced in Emacs 25.1.
>
> I] Not observed with Emacs-24.5.
> II] I reproduce the issue with Emacs-25.1.
> *)  I see the issue with Emacs-25.1 even after loading kmacro.el
>     from Emacs-24.5.  That seems to point to changes in the
>     C source code; maybe some changes in src/macro.c?
>
> I am having some difficulties to do a bisect to track down
> changes in macro.c.  Maybe someone can do it much faster and find
> the commit causing this issue.

I traced the execution in gdb, the code removed in the following diff
seems to be the culprit:

--- i/src/keyboard.c
+++ w/src/keyboard.c
@@ -3029,14 +3029,6 @@ read_char (int commandflag, Lisp_Object map,
       Vunread_post_input_method_events
 	= nconc2 (XCDR (tem), Vunread_post_input_method_events);
     }
-  /* When we consume events from the various unread-*-events lists, we
-     bypass the code that records input, so record these events now if
-     they were not recorded already.  */
-  if (!recorded)
-    {
-      record_char (c);
-      recorded = true;
-    }
 
  reread_first:

This was added in [1: 30a6b1f814].  Obviously just removing it will
reintroduce the bug it fixed, I'm not sure what the proper fix should
be.

1: 2015-08-10 21:15:03 +0300 30a6b1f81412044aa7dda5573b0142a0a03c4fd3
  Fix recording of events pushed onto unread-command-events



Slightly simpler reproduction: evaluate

    (defun insert-a ()
      (interactive)
      (execute-kbd-macro "a" 1))
    (define-key global-map [f12] 'insert-a)

then f3 f12 f4 C-x e

bug#25860: 25.1; Double macro execution

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: Marek Twardochlib <wasserwerk.studio@googlemail.com>,  25860@debbugs.gnu.org,  eggert@cs.ucla.edu,  jwiegley@gmail.com,  Andreas Schwab <schwab@linux-m68k.org>,  eliz@gnu.org
> Date: Sun, 26 Feb 2017 02:12:33 -0500
> 
> --- i/src/keyboard.c
> +++ w/src/keyboard.c
> @@ -3029,14 +3029,6 @@ read_char (int commandflag, Lisp_Object map,
>        Vunread_post_input_method_events
>  	= nconc2 (XCDR (tem), Vunread_post_input_method_events);
>      }
> -  /* When we consume events from the various unread-*-events lists, we
> -     bypass the code that records input, so record these events now if
> -     they were not recorded already.  */
> -  if (!recorded)
> -    {
> -      record_char (c);
> -      recorded = true;
> -    }
>  
>   reread_first:
> 
> This was added in [1: 30a6b1f814].  Obviously just removing it will
> reintroduce the bug it fixed, I'm not sure what the proper fix should
> be.

Like below?

I admit that I don't see myself as an expert on all the complexity
that goes on in keyboard.c and friends.  With this change,
kmacro-tests.el still passes, but I'm not sure that's good enough
(e.g., can a macro define another macro when it runs? if so, this
change will get in the way).  Comments and test cases welcome.

Thanks.

diff --git a/src/keyboard.c b/src/keyboard.c
index 0245dfa..ec4dee0 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -3245,7 +3245,7 @@ record_char (Lisp_Object c)
 	    }
 	}
     }
-  else
+  else if (NILP (Vexecuting_kbd_macro))
     store_kbd_macro_char (c);
 
   /* recent_keys should not include events from keyboard macros.  */

bug#25860: 25.1; Double macro execution

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

> I admit that I don't see myself as an expert on all the complexity
> that goes on in keyboard.c and friends.  With this change,
> kmacro-tests.el still passes, but I'm not sure that's good enough
> (e.g., can a macro define another macro when it runs? if so, this
> change will get in the way).

AFAICT, you can't define a macro from a macro:

    DEFUN ("start-kbd-macro", Fstart_kbd_macro, Sstart_kbd_macro, 1, 2, "P",
    ...
    {
      if (!NILP (KVAR (current_kboard, defining_kbd_macro)))
        error ("Already defining kbd macro");

Not sure if there might be some other complications though.

bug#25860: 25.1; Double macro execution

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 25860@debbugs.gnu.org,  eggert@cs.ucla.edu,  tino.calancha@gmail.com,  jwiegley@gmail.com,  wasserwerk.studio@googlemail.com,  schwab@linux-m68k.org
> Date: Sun, 26 Feb 2017 11:02:32 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I admit that I don't see myself as an expert on all the complexity
> > that goes on in keyboard.c and friends.  With this change,
> > kmacro-tests.el still passes, but I'm not sure that's good enough
> > (e.g., can a macro define another macro when it runs? if so, this
> > change will get in the way).
> 
> AFAICT, you can't define a macro from a macro:
> 
>     DEFUN ("start-kbd-macro", Fstart_kbd_macro, Sstart_kbd_macro, 1, 2, "P",
>     ...
>     {
>       if (!NILP (KVAR (current_kboard, defining_kbd_macro)))
>         error ("Already defining kbd macro");

Yes, but that's not the scenario I had in mind.  What I had in mind is
a macro that, when run, will call start-kbd-macro etc.  It's clear
that such a macro cannot be easily defined using F3 and F4, but
perhaps some ingenious method with using some Lisp data in the
macro...

Anyway, if no objections come up in a few days, I will push this.

Thanks for the feedback.

bug#25860: 25.1; Double macro execution

[Noam Postavsky]

On Mon, Feb 27, 2017 at 10:57 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>
> Yes, but that's not the scenario I had in mind.  What I had in mind is
> a macro that, when run, will call start-kbd-macro etc.  It's clear
> that such a macro cannot be easily defined using F3 and F4, but
> perhaps some ingenious method with using some Lisp data in the
> macro...

C-x ( is bound to kmacro-start-macro which refuses to define a macro
while a macro is executing:

(defun kmacro-start-macro (arg)
...
  (if (or defining-kbd-macro executing-kbd-macro)
      (message "Already defining keyboard macro.")
...)

I tried making a macro to call M-x start-kbd-macro directly:

<<start-kbd-macro>>    ;; start-kbd-macro
zzz            ;; self-insert-command * 3
<<end-kbd-macro>>    ;; end-kbd-macro

Executing that macro seems to define an empty macro though (i.e., the
"zzz" only gets inserted while running the outer defining macro, but
hitting C-x e after is a nop (apart from the (Type e to repeat macro)
message appearing)). Probably start-kbd-macro should also refuse to
run if a macro is executing as it fails to do anything useful in this
case.

bug#25860: 25.1; Double macro execution

[Eli Zaretskii]

> Date: Mon, 27 Feb 2017 17:57:59 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 25860@debbugs.gnu.org, eggert@cs.ucla.edu, tino.calancha@gmail.com,
> 	jwiegley@gmail.com, wasserwerk.studio@googlemail.com, schwab@linux-m68k.org
> 
> Anyway, if no objections come up in a few days, I will push this.

Done.

bug#25860: 25.1; Double macro execution

[Marek Twardochlib]

Hello,

in the current, fresh Emacs 25.3.1 the bug still exist. It would be so
very nice when someone could fix that. I use macro calls from maco
very often and this bug prevent this.

Kind regards
Marek Twardochlib



2017-03-03 15:21 GMT+01:00 Eli Zaretskii <eliz@gnu.org>:
>> Date: Mon, 27 Feb 2017 17:57:59 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: 25860@debbugs.gnu.org, eggert@cs.ucla.edu, tino.calancha@gmail.com,
>>       jwiegley@gmail.com, wasserwerk.studio@googlemail.com, schwab@linux-m68k.org
>>
>> Anyway, if no objections come up in a few days, I will push this.
>
> Done.

bug#25860: 25.1; Double macro execution

[Tino Calancha]

On Fri, 29 Sep 2017, Marek Twardochlib wrote:

> Hello,
>
> in the current, fresh Emacs 25.3.1 the bug still exist. It would be so
> very nice when someone could fix that. I use macro calls from maco
> very often and this bug prevent this.
Hi,

Emacs 25.3.1 was an emergency release to fix a security vulnerability.
The fix for this bug was included in the master branch at that time; that 
means you will see the fix once Emacs 26.1 be released.  I think you
don't need to wait very long to see that release: i guess people are 
planning to release it soon.

Regards,
Tino