bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

While I was edebugging in todo-mode.el (bug#70937) Emacs got into a
state where Edebug kicked in even after I removed the instrumentation
from the code.  AFAICT from further debugging, the issue has to do with
the way todo-mode.el uses set-transient-map.  To provide a reproducer
that does not involve todo-mode.el I have extracted and simplified the
triggering function from that file and included it in the attached file.

0. emacs -Q -l test-edebug-override-map.el
1. M-x srb-test
2. At the prompt type one of the keys listed between the curly braces,
   which removes that group and prompts again; keeping typing till there
   is no further prompt, then a message displays the typed keys in the
   order they were typed in.
3. Now visit test-edebug-override-map.el and with point in the function
   srb-test--next type `C-u C-M-x' to instrument it for Edebug.
4. Type M-s srb-test again and when Edebug stops execution, type `f'
   twice to step through the entire function so Edebug releases control.
5. Now with point again in srb-test--next type `C-M-x' to remove the
   instrumentation.  You can confirm this by repeating steps 1-2 without
   Edebug kicking.
6. Now type any of the keys that were listed in the prompt on invoking
   srb-test (these keys are the strings in the value of
   srb-test--key-groups).
=> This makes Edebug kick in with execution stopped at one of the two
   occurrences of the sexp `(setq keys-so-far (concat keys-so-far " "
   (funcall this-key)))' in srb-test--next.  The same thing happens with
   sequences containg one of these keys, e.g. `M-x k'.

I did find a way to prevent Edebug from kicking in: uncomment the lines
in the test file containing "exitfn"; this explicitly deactivates the
transient map when the function returns, but it also makes the function
a noop, so it's no fix for the problem.

After step 4., when I type `C-h b' to show all current key bindings, the
*Help* buffer contains a listing like this:

 Overriding Bindings:

Key             Binding

c .. d		#<interpreted-function AA2>
h		#<interpreted-function BAB>
i		#<interpreted-function 8CC>
k		#<interpreted-function 9AC>
n		#<interpreted-function A7F>
p		#<interpreted-function 917>
r		#<interpreted-function B53>
t		#<interpreted-function BD6>
y		#<interpreted-function 9F1>

The codes AA2, etc. are links, and clicking on these pops up a *Help
Source* buffer containing elisp code like this:

#[nil
  ((edebug-enter 'edebug-anon5 (list)
		 #'(lambda nil :closure-dont-trim-context
		     (edebug-after (edebug-before 0) 7
				   (setq keys-so-far
					 (edebug-after
					  (edebug-before 1) 6
					  (concat
					   (edebug-after 0 2
							 keys-so-far)
					   " "
					   (edebug-after
					    (edebug-before 3) 5
					    (funcall
					     (edebug-after 0 4
							   this-key)))))))

This is evidently from the instrumented function.  What I haven't been
able to determine is why this remains even after the instrumentation has
been removed; it seems that Edebug has appropriated
overriding-terminal-local-map.  The only way I have found to release it
is `M-x unload-library RET edebug'.


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.41, cairo version 1.18.0) of 2024-05-08 built on strobelfs
Repository revision: e020f4e9ce5d98438033fea098d943c311b0fa3d
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101013
System Description: Linux From Scratch r12.1-53

Configured using:
 'configure -C 'CFLAGS=-Og -g3' PKG_CONFIG_PATH=/opt/qt5/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG
RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER
WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

Stephen Berman via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> 0. emacs -Q -l test-edebug-override-map.el

Where can one find "test-edebug-override-map.el"?


TIA,

Michael.

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

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

On Tue, 14 May 2024 21:59:07 +0200 Michael Heerdegen <michael_heerdegen@web.de> wrote:

> Stephen Berman via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> 0. emacs -Q -l test-edebug-override-map.el
>
> Where can one find "test-edebug-override-map.el"?

Oh, no, I forgot to attach it, sorry!  Now it's attached here.

Steve Berman


[-- Attachment #2: test-edebug-override-map.el --]
[-- Type: application/emacs-lisp, Size: 2519 bytes --]

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

Stephen Berman via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> This is evidently from the instrumented function.  What I haven't been
> able to determine is why this remains even after the instrumentation has
> been removed; it seems that Edebug has appropriated
> overriding-terminal-local-map.  The only way I have found to release it
> is `M-x unload-library RET edebug'.

Thanks.  I can reproduce the issue.  And this is a very valuable recipe,
because this may be the problem we are hunting for quite a while.

I agree with your analysis - the transient map the edebugged code adds
is never removed, its bound commands are instrumented code.  Just M-:
overriding-terminal-local-map RET to verify.

AFAIU, this is caused only by this one binding:

  (overriding-terminal-local-map nil)

in `edebug--recursive-edit'.  This seems to make the cleanup code of the
added transient map in `pre-command-hook' think that the transient map
is gone, we are done and removes itself from the hook.  Then the added
transient map stays forever, unless you clear
`overriding-terminal-local-map' manually.

But what can we do - does anyone have a good idea?  Could the cleanup
code that removes the transient map maybe check whether the variable
`overriding-terminal-local-map' it sees is really the global one?


Thanks,

Michael.

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
of text editors" <bug-gnu-emacs@gnu.org> writes:

> But what can we do - does anyone have a good idea?  Could the cleanup
> code that removes the transient map maybe check whether the variable
> `overriding-terminal-local-map' it sees is really the global one?

Here is a recipe that doesn't involve Edebug (please use a separate
emacs session to try) but does the same thing:

#+begin_src emacs-lisp
(progn
  (set-transient-map (let ((m (make-sparse-keymap)))
                       (define-key m [?^]
                         (lambda () (interactive) (message "Hmm...")))
                       m))
  (let ((overriding-terminal-local-map nil))
    (recursive-edit)))
#+end_src

After evaluating this, whenever one returns to top-level one is stuck
with the transient map (just hit ^ to check).

Michael.

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

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

Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
of text editors" <bug-gnu-emacs@gnu.org> writes:

> #+begin_src emacs-lisp
> (progn
>   (set-transient-map (let ((m (make-sparse-keymap)))
>                        (define-key m [?^]
>                          (lambda () (interactive) (message "Hmm...")))
>                        m))
>   (let ((overriding-terminal-local-map nil))
>     (recursive-edit)))
> #+end_src

A possible approach that seems to fix the reported bug:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Fix-Bug-70868-Edebug-appropriates-overriding-ter.patch --]
[-- Type: text/x-diff, Size: 2433 bytes --]

From 7f5db96e7d40fc3f71c82d7a004a21bc7515c81d Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Wed, 15 May 2024 17:13:35 +0200
Subject: [PATCH] WIP: Fix Bug#70868 Edebug appropriates
 overriding-terminal-local-map

* lisp/subr.el (set-transient-map): Try to make the cleanup code more
reliable in case `overriding-terminal-local-map' gets shadowed: When the
added map is not present there, try to detect whether shadowing is
currently happening.  Delay the cleanup in this case.
---
 lisp/subr.el | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 80c7ffe32bc..2f8809d706e 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -6573,12 +6573,25 @@ set-transient-map
          (clearfun (make-symbol "clear-transient-map"))
          (exitfun
           (lambda ()
-            (internal-pop-keymap map 'overriding-terminal-local-map)
-            (remove-hook 'pre-command-hook clearfun)
-            ;; Clear the prompt after exiting.
-            (when message (message ""))
-            (when set-transient-map-timer (cancel-timer set-transient-map-timer))
-            (when on-exit (funcall on-exit)))))
+            (cond
+             ((memq map overriding-terminal-local-map)
+              (internal-pop-keymap map 'overriding-terminal-local-map)
+              (remove-hook 'pre-command-hook clearfun)
+              ;; Clear the prompt after exiting.
+              (when message (message ""))
+              (when set-transient-map-timer (cancel-timer set-transient-map-timer))
+              (when on-exit (funcall on-exit)))
+             (;; FIXME: We rather want something like
+              ;; (variable-references-default-toplevel-value-p
+              ;;  'overriding-terminal-local-map)
+              (eq overriding-terminal-local-map
+                  (default-toplevel-value 'overriding-terminal-local-map))
+              ;; We are probably done:
+              (remove-hook 'pre-command-hook clearfun))
+             ;; If none of these conditions was fulfilled,
+             ;; overriding-terminal-local-map is currently shadowed - keep
+             ;; clearfun in 'pre-command-hook
+             ))))
     ;; Don't use letrec, because equal (in add/remove-hook) could get trapped
     ;; in a cycle. (bug#46326)
     (fset clearfun
--
2.39.2


[-- Attachment #3: Type: text/plain, Size: 83 bytes --]


Comments, corrections, enhancements and better ideas all very welcome.


Michael.

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

On Wed, 15 May 2024 17:24:24 +0200 Michael Heerdegen <michael_heerdegen@web.de> wrote:

> Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
> of text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> #+begin_src emacs-lisp
>> (progn
>>   (set-transient-map (let ((m (make-sparse-keymap)))
>>                        (define-key m [?^]
>>                          (lambda () (interactive) (message "Hmm...")))
>>                        m))
>>   (let ((overriding-terminal-local-map nil))
>>     (recursive-edit)))
>> #+end_src
>
> A possible approach that seems to fix the reported bug:

I confirm your patch prevents Edebug from "appropriating"
overriding-terminal-local-map both in my test case and also in
todo-mode.el, where I first bumped into the problem.  And your analysis
of why Edebug triggered the problem seems convincing to me, too.
Thanks!

Steve Berman

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

On Wed, 15 May 2024 20:51:08 +0200 Stephen Berman <stephen.berman@gmx.net> wrote:

> On Wed, 15 May 2024 17:24:24 +0200 Michael Heerdegen
> <michael_heerdegen@web.de> wrote:
>
>> Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
>> of text editors" <bug-gnu-emacs@gnu.org> writes:
>>
>>> #+begin_src emacs-lisp
>>> (progn
>>>   (set-transient-map (let ((m (make-sparse-keymap)))
>>>                        (define-key m [?^]
>>>                          (lambda () (interactive) (message "Hmm...")))
>>>                        m))
>>>   (let ((overriding-terminal-local-map nil))
>>>     (recursive-edit)))
>>> #+end_src
>>
>> A possible approach that seems to fix the reported bug:
>
> I confirm your patch prevents Edebug from "appropriating"
> overriding-terminal-local-map both in my test case and also in
> todo-mode.el, where I first bumped into the problem.  And your analysis
> of why Edebug triggered the problem seems convincing to me, too.
> Thanks!

Do you (or does anyone else) have any reason not to install this patch?

Steve Berman

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

Stephen Berman <stephen.berman@gmx.net> writes:

> Do you (or does anyone else) have any reason not to install this
> patch?

I wanted to see if anybody has a better idea and test the patch a bit
in that time to be sure it doesn't break anything.

Michael.

bug#70938: 30.0.50; Edebug appropriates overriding-terminal-local-map

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

On Mon, 27 May 2024 16:37:32 +0200 Michael Heerdegen <michael_heerdegen@web.de> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> Do you (or does anyone else) have any reason not to install this
>> patch?
>
> I wanted to see if anybody has a better idea and test the patch a bit
> in that time to be sure it doesn't break anything.

Ok, fine by me.

Steve Berman