bug#51026: 29.0.50; Edebug leaves data in symbols plist after instrumentation

bug#51026: 29.0.50; Edebug leaves data in symbols plist after instrumentation

[Arthur Miller]

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

1. Instrument any defun/macro for edebug with C-u M-x RET eval-defun
2. Remove instrumentation with M-x edebug-remove-instrumentation RET
3. Check the chosed symbols plist: M-x (symbol-plist 'whatever-you-used)

Edebug data will be present in plist after instrumentation is removed.

Attached patch is suggestion to fix 'edebug-remove-instrumentation' to
remove unnecessary data after instrumentation.

Is edebug-remove-instrumentation only function that can remove edebug
instrumentation?

I am not though sure, if some other place needs to be patched, I am
thinking mostly of edebug-read-and-maybe-wrap-form. 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Clean-edebug-props-on-instrumentation-removal.patch --]
[-- Type: text/x-patch, Size: 1577 bytes --]

From 057a4278bf0b8c8a8985850b505f0a1addd8156f Mon Sep 17 00:00:00 2001
From: Arthur Miller <arthur.miller@live.com>
Date: Tue, 5 Oct 2021 09:17:20 +0200
Subject: [PATCH] Clean edebug props on instrumentation removal

* lisp/emacs-lisp/edebug.el (edebug--strip-plist): New function.
(edebug-remove-instrumentation): Added call to 'edebug--strip-plist'.
---
 lisp/emacs-lisp/edebug.el | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index a38c8bd5ca..da8a3f1eb8 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -4529,6 +4529,13 @@ edebug--unwrap*-symbol-function
         (was-macro               `(macro . ,unwrapped))
         (t                       unwrapped))))))
 
+(defun edebug--strip-plist (symbol)
+  "Remove edebug related properties from SYMBOL's plist."
+  (dolist (prop '(edebug edebug-behavior edebug-coverage
+                         edebug-form-spec edebug-freq-count
+                         ghost-edebug))
+    (cl-remprop symbol prop)))
+
 (defun edebug-remove-instrumentation (functions)
   "Remove Edebug instrumentation from FUNCTIONS.
 Interactively, the user is prompted for the function to remove
@@ -4560,6 +4567,7 @@ edebug-remove-instrumentation
   (dolist (symbol functions)
     (when-let ((unwrapped
                 (edebug--unwrap*-symbol-function symbol)))
+      (edebug--strip-plist symbol)
       (defalias symbol unwrapped)))
   (message "Removed edebug instrumentation from %s"
            (mapconcat #'symbol-name functions ", ")))
-- 
2.33.0


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





In GNU Emacs 29.0.50 (build 2, x86_64-pc-linux-gnu, cairo version 1.17.4)
 of 2021-10-04 built on pascal
Repository revision: c6be44d9b3ec09195f6279e9a503175f8fd60e14
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: Arch Linux

Configured using:
 'configure --without-modules --with-cairo --with-compress-install
 --with-x-toolkit=no --with-gnutls --without-gconf --without-xwidgets
 --without-toolkit-scroll-bars --without-xaw3d --without-gsettings
 --with-mailutils --with-native-compilation 'CFLAGS=-O2 -march=native
 -mtune=native''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM HARFBUZZ JPEG JSON LCMS2
LIBOTF LIBSYSTEMD LIBXML2 M17N_FLT NATIVE_COMP NOTIFY INOTIFY OLDXMENU
PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF X11 XDBE XIM XPM ZLIB

Important settings:
  value of $LANG: sv_SE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: ELisp/l

Minor modes in effect:
  text-scale-mode: t
  show-paren-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  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
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source eieio eieio-core eieio-loaddefs
password-cache json map mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils time-date compile text-property-search
comint ansi-color ring helper edmacro kmacro help-fns radix-tree edebug
comp comp-cstr warnings rx cl-seq cl-macs cl-extra debug backtrace
help-mode find-func trace elp face-remap vc-git diff-mode easy-mmode
vc-dispatcher cl-loaddefs cl-lib seq gv subr-x byte-opt bytecomp
byte-compile cconv paren iso-transl tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer 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 composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting font-render-setting cairo x multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 123973 8004)
 (symbols 48 10003 0)
 (strings 32 29672 1979)
 (string-bytes 1 1004452)
 (vectors 16 21210)
 (vector-slots 8 371233 17772)
 (floats 8 50 42)
 (intervals 56 638 1)
 (buffers 992 15))

bug#51026: 29.0.50; Edebug leaves data in symbols plist after instrumentation

[Lars Ingebrigtsen]

Arthur Miller <arthur.miller@live.com> writes:

> +(defun edebug--strip-plist (symbol)
> +  "Remove edebug related properties from SYMBOL's plist."
> +  (dolist (prop '(edebug edebug-behavior edebug-coverage
> +                         edebug-form-spec edebug-freq-count
> +                         ghost-edebug))
> +    (cl-remprop symbol prop)))

This will break edebug -- edebug-form-spec is set when loading
edebug.el, it's not something that happens when you instrument a
function.

I don't know about the other symbols.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51026: 29.0.50; Edebug leaves data in symbols plist after instrumentation

[Arthur Miller]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Arthur Miller <arthur.miller@live.com> writes:
>
>> +(defun edebug--strip-plist (symbol)
>> +  "Remove edebug related properties from SYMBOL's plist."
>> +  (dolist (prop '(edebug edebug-behavior edebug-coverage
>> +                         edebug-form-spec edebug-freq-count
>> +                         ghost-edebug))
>> +    (cl-remprop symbol prop)))
>
> This will break edebug -- edebug-form-spec is set when loading
> edebug.el, it's not something that happens when you instrument a
> function.

Ahh that pcase in the niddle of nowhere; what does it do there? Why is it not in
some "init" function? :-)

Yes, it is true what you say, but only for a handful of symbols, not even all of
those handled in that pcase. I think it is emitted only for let, let*, setq and
quote, but it is not important. See attached patch, if it is acceptable. It will
check if a symbol is one of those specially treated and do nothing for those.

I think that is what you mean? I don't see any other place that adds properties
and I don't see any edebug properties in "random" symbols unless instrumented.

> I don't know about the other symbols.

That seems to be the only one added automatically.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Clean-edebug-props-on-instrumentation-removal.patch --]
[-- Type: text/x-patch, Size: 1740 bytes --]

From 508dea329d8ef389b6b07d6339faf753d746ff16 Mon Sep 17 00:00:00 2001
From: Arthur Miller <arthur.miller@live.com>
Date: Tue, 5 Oct 2021 13:07:07 +0200
Subject: [PATCH] Clean edebug props on instrumentation removal

* lisp/emacs-lisp/edebug.el (edebug--strip-plist): New function.
(edebug-remove-instrumentation): Added call to 'edebug--strip-plist'.
---
 lisp/emacs-lisp/edebug.el | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index a38c8bd5ca..97869a2bb9 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -4529,6 +4529,15 @@ edebug--unwrap*-symbol-function
         (was-macro               `(macro . ,unwrapped))
         (t                       unwrapped))))))
 
+(defun edebug--strip-plist (symbol)
+  "Remove edebug related properties from SYMBOL's plist."
+  (unless (memq symbol '(quote defvar defconst defun defmacro function
+                               let let* setq cond condition-case \` \, \,@))
+    (dolist (prop '(edebug edebug-behavior edebug-coverage
+                           edebug-form-spec edebug-freq-count
+                           ghost-edebug))
+      (cl-remprop symbol prop))))
+
 (defun edebug-remove-instrumentation (functions)
   "Remove Edebug instrumentation from FUNCTIONS.
 Interactively, the user is prompted for the function to remove
@@ -4560,6 +4569,7 @@ edebug-remove-instrumentation
   (dolist (symbol functions)
     (when-let ((unwrapped
                 (edebug--unwrap*-symbol-function symbol)))
+      (edebug--strip-plist symbol)
       (defalias symbol unwrapped)))
   (message "Removed edebug instrumentation from %s"
            (mapconcat #'symbol-name functions ", ")))
-- 
2.33.0

bug#51026: 29.0.50; Edebug leaves data in symbols plist after instrumentation

[Lars Ingebrigtsen]

Arthur Miller <arthur.miller@live.com> writes:

> Yes, it is true what you say, but only for a handful of symbols, not
> even all of those handled in that pcase. I think it is emitted only
> for let, let*, setq and quote, but it is not important.

No, `edebug-form-spec' is added to symbols by the (declare (debug ...))
form, and should never be removed from any symbols.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51026: 29.0.50; Edebug leaves data in symbols plist after instrumentation

[Arthur Miller]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Arthur Miller <arthur.miller@live.com> writes:
>
>> Yes, it is true what you say, but only for a handful of symbols, not
>> even all of those handled in that pcase. I think it is emitted only
>> for let, let*, setq and quote, but it is not important.
>
> No, `edebug-form-spec' is added to symbols by the (declare (debug ...))
> form, and should never be removed from any symbols.

Yes, but as I have tested, the previously mentioned pcase-dolist, line 2075 in
edebug.el is what get's the action going when edebug is loaded.

'declare' just marks stuff, it does not really do anything on it's own. 

For some speical forms that property seems to be added via gv.el (let,
quote, etc ?), and for some (defun and defmacro), it is tested and done in
pcase-dolist itself, since they are no longer special forms but macros.

I have tested to remove that property and exec that pcase-dolist, and the
property was back as expected.

I don't know, I can be wrong too; there is a bit of interaction going on between
all this function vars and alists and what "real" function gets executed when
edebug starts, so sorry if I am getting it a bit wrong.

Anyway, since there can be other special forms and stuff added over time, the
static list won't do. The obvious is just to leave that property alone and remove
the rest of leftover stuff. It can be quite a lot of data in some cases, in
'edebug-coverage' vector, as I have seen while tested. Since it will be
referenced in symbol's  plist, it won't get garbage collected either.

Also this patch does just the half the job. It removes leftovers only if
'edebug-remove-instrumentation' is called. If eval-defun is called with C-u
prefix, but with both 'edebug-all-defs' and 'edebug-all-forms' set to nil, it
will "uninstrument" original function, but it does not seem to call
'edebug-remove-instrumentation', so all the leftovers will be left. I haven't
looked at that part since I am using 'edebug-remove-instrumentation' in my own
code.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Clean-edebug-props-on-instrumentation-removal.patch --]
[-- Type: text/x-patch, Size: 1537 bytes --]

From 94016b063faf3f69cf03d7ffa3eac1a15bad34c8 Mon Sep 17 00:00:00 2001
From: Arthur Miller <arthur.miller@live.com>
Date: Wed, 6 Oct 2021 14:16:21 +0200
Subject: [PATCH] Clean edebug props on instrumentation removal

* lisp/emacs-lisp/edebug.el (edebug--strip-plist): New function.
(edebug-remove-instrumentation): Added call to 'edebug--strip-plist'.
---
 lisp/emacs-lisp/edebug.el | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index a38c8bd5ca..2489680bcb 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -4529,6 +4529,12 @@ edebug--unwrap*-symbol-function
         (was-macro               `(macro . ,unwrapped))
         (t                       unwrapped))))))
 
+(defun edebug--strip-plist (symbol)
+  "Remove edebug related properties from plist for SYMBOL."
+  (dolist (prop '(edebug edebug-behavior edebug-coverage
+                         edebug-freq-count ghost-edebug))
+      (cl-remprop symbol prop)))
+
 (defun edebug-remove-instrumentation (functions)
   "Remove Edebug instrumentation from FUNCTIONS.
 Interactively, the user is prompted for the function to remove
@@ -4560,6 +4566,7 @@ edebug-remove-instrumentation
   (dolist (symbol functions)
     (when-let ((unwrapped
                 (edebug--unwrap*-symbol-function symbol)))
+      (edebug--strip-plist symbol)
       (defalias symbol unwrapped)))
   (message "Removed edebug instrumentation from %s"
            (mapconcat #'symbol-name functions ", ")))
-- 
2.33.0

bug#51026: 29.0.50; Edebug leaves data in symbols plist after instrumentation

[Lars Ingebrigtsen]

Arthur Miller <arthur.miller@live.com> writes:

> * lisp/emacs-lisp/edebug.el (edebug--strip-plist): New function.
> (edebug-remove-instrumentation): Added call to 'edebug--strip-plist'.

Thanks; pushed to Emacs 29.

bug#51026: 29.0.50; Edebug leaves data in symbols plist after instrumentation

[Arthur Miller]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Arthur Miller <arthur.miller@live.com> writes:
>
>> * lisp/emacs-lisp/edebug.el (edebug--strip-plist): New function.
>> (edebug-remove-instrumentation): Added call to 'edebug--strip-plist'.
>
> Thanks; pushed to Emacs 29.

Are you sure about that one? You said in the conversaion at the time that those
(or some) symbols should not be stripped, i.e. that the idea there wasn't
correct. Please check twice, I don't want to introduce bugs into Emacs :).

bug#51026: 29.0.50; Edebug leaves data in symbols plist after instrumentation

[Lars Ingebrigtsen]

Arthur Miller <arthur.miller@live.com> writes:

> Are you sure about that one? You said in the conversaion at the time
> that those (or some) symbols should not be stripped, i.e. that the
> idea there wasn't correct. Please check twice, I don't want to
> introduce bugs into Emacs :).

I had a look at the symbol-plist elements that we remove here, and it
looked OK to me.  (But I might be mistaken, of course.)