bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Hi,

e.g. (just a random example)

  (defun f (x) (* 2 (+ 1 x)))

  (advice-add 'f :around (lambda (orig-f x) (1+ (funcall orig-f x))))

Instrument `f'.  Then M-x edebug-remove-instrumentation gives

"Found no functions to remove instrumentation from".

BTW, another point: the user interface for edebug-remove-instrumentation
could be similar to `cancel-edebug-on-entry', i.e. it could prompt for a
function to remove instrumentation from, defaulting to all functions.

TIA,

Michael.


In GNU Emacs 27.0.50 (build 7, x86_64-pc-linux-gnu, GTK+ Version 3.24.12)
 of 2019-11-13 built on drachen
Repository revision: 3c0c95d1cca0875b7ad03247a35ba9e8c1c062ee
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux bullseye/sid

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Hi,
>
> e.g. (just a random example)
>
>   (defun f (x) (* 2 (+ 1 x)))
>
>   (advice-add 'f :around (lambda (orig-f x) (1+ (funcall orig-f x))))
>
> Instrument `f'.  Then M-x edebug-remove-instrumentation gives
>
> "Found no functions to remove instrumentation from".

This is due to edebug-unwrap* not actually unwrapping something if it
has been advised, apparently...  I'm not familiar enough with how edebug
really works to fix that.

> BTW, another point: the user interface for edebug-remove-instrumentation
> could be similar to `cancel-edebug-on-entry', i.e. it could prompt for a
> function to remove instrumentation from, defaulting to all functions.

Makes sense -- I've now done this.

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

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:

> > the user interface for edebug-remove-instrumentation could be
> > similar to `cancel-edebug-on-entry', i.e. it could prompt for a
> > function to remove instrumentation from, defaulting to all
> > functions.
>
> Makes sense -- I've now done this.

Thanks!

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> > e.g. (just a random example)
> >
> >   (defun f (x) (* 2 (+ 1 x)))
> >
> >   (advice-add 'f :around (lambda (orig-f x) (1+ (funcall orig-f x))))
> >
> > Instrument `f'.  Then M-x edebug-remove-instrumentation gives
> >
> > "Found no functions to remove instrumentation from".
>
> This is due to edebug-unwrap* not actually unwrapping something if it
> has been advised, apparently...  I'm not familiar enough with how edebug
> really works to fix that.

Stefan, does this patch make sense?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Try-to-fix-edebug-remove-instrumentation-for-adv.patch --]
[-- Type: text/x-diff, Size: 3086 bytes --]

From c1c2cd4faa0adc87f9334ec92ace8bcf5ae8333e Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Thu, 14 Nov 2019 17:47:51 +0100
Subject: [PATCH] WIP: Try to fix edebug-remove-instrumentation for advised
 funs

---
 lisp/emacs-lisp/edebug.el | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 6b55d7cff0..e94adeb64b 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -56,6 +56,7 @@
 (require 'macroexp)
 (require 'cl-lib)
 (eval-when-compile (require 'pcase))
+(eval-when-compile (require 'subr-x))

 ;;; Options

@@ -4571,6 +4572,20 @@ edebug-unload-function
   ;; Continue standard unloading.
   nil)

+(defun edebug--advised-p (symbol)
+  ;; Non-nil when SYMBOL's `symbol-function' is advised.  The non-nil
+  ;; return value is the unwrapped base function if it was wrapped,
+  ;; and the symbol t else.
+  (pcase (symbol-function symbol)
+    ((and (pred advice--p)
+          (app advice--cd*r orig-f)
+          (let unwrapped (edebug-unwrap* orig-f)))
+     (if (equal unwrapped orig-f) t unwrapped))
+    (`(macro . ,(and (pred advice--p)
+                     (app advice--cd*r orig-f)
+                     (let unwrapped (edebug-unwrap* orig-f))))
+     (if (equal unwrapped orig-f) t `(macro . ,unwrapped)))))
+
 (defun edebug-remove-instrumentation (functions)
   "Remove Edebug instrumentation from FUNCTIONS.
 Interactively, the user is prompted for the function to remove
@@ -4582,9 +4597,13 @@ edebug-remove-instrumentation
        (lambda (symbol)
          (when (and (functionp symbol)
                     (get symbol 'edebug))
-           (let ((unwrapped (edebug-unwrap* (symbol-function symbol))))
-             (unless (equal unwrapped (symbol-function symbol))
-               (push symbol functions)))))
+           (if-let ((advised (edebug--advised-p symbol)))
+               (unless (eq advised t)
+                 (push symbol functions))
+             (let ((unwrapped (edebug-unwrap*
+                               (symbol-function symbol))))
+               (unless (equal unwrapped (symbol-function symbol))
+                 (push symbol functions))))))
        obarray)
       (unless functions
         (error "Found no functions to remove instrumentation from"))
@@ -4598,8 +4617,12 @@ edebug-remove-instrumentation
           functions)))))
   ;; Remove instrumentation.
   (dolist (symbol functions)
-    (setf (symbol-function symbol)
-          (edebug-unwrap* (symbol-function symbol))))
+    (if-let ((advised (edebug--advised-p symbol)))
+        (unless (eq advised t)
+          (funcall (or (get symbol 'defalias-fset-function) #'fset)
+                   symbol advised))
+      (setf (symbol-function symbol)
+            (edebug-unwrap* (symbol-function symbol)))))
   (message "Removed edebug instrumentation from %s"
            (mapconcat #'symbol-name functions ", ")))

--
2.24.0


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


I'm not sure if I should use `indirect-function' somewhere instead of
`symbol-function', but I also don't understand (and also didn't yet try
to find out) why nadvice doesn't use `indirect-function' for its
manipulations.

Thanks,

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Stefan Monnier]

> +(defun edebug--advised-p (symbol)
> +  ;; Non-nil when SYMBOL's `symbol-function' is advised.  The non-nil
> +  ;; return value is the unwrapped base function if it was wrapped,
> +  ;; and the symbol t else.

Do you really mean "advised"?  It seems that this tests whether the code
is *instrumented* by looking past the (potential) pieces of advice.

I'll assume the above should say "instrumented", but if not, then
disregard the rest because I'm just confused.

> +  (pcase (symbol-function symbol)
> +    ((and (pred advice--p)
> +          (app advice--cd*r orig-f)
> +          (let unwrapped (edebug-unwrap* orig-f)))
> +     (if (equal unwrapped orig-f) t unwrapped))
> +    (`(macro . ,(and (pred advice--p)
> +                     (app advice--cd*r orig-f)
> +                     (let unwrapped (edebug-unwrap* orig-f))))
> +     (if (equal unwrapped orig-f) t `(macro . ,unwrapped)))))

[ That's pretty ugly.  I think I'd move the `app` and the `let` outside
  of those patterns and into the code of the corresponding branch.  ]

> -           (let ((unwrapped (edebug-unwrap* (symbol-function symbol))))
> -             (unless (equal unwrapped (symbol-function symbol))
> -               (push symbol functions)))))
> +           (if-let ((advised (edebug--advised-p symbol)))
> +               (unless (eq advised t)
> +                 (push symbol functions))
> +             (let ((unwrapped (edebug-unwrap*
> +                               (symbol-function symbol))))
> +               (unless (equal unwrapped (symbol-function symbol))
> +                 (push symbol functions))))))

How 'bout using something like

    (defun edebug--symbol-function (sym)
      (let ((def (symbol-function sym)))
        (if (eq 'macro (car-safe def))
            (setq def (cdr def)))
        (advice--cdr* def)))

and replacing (some) uses of `symbol-function` with it?

>    ;; Remove instrumentation.
>    (dolist (symbol functions)
> -    (setf (symbol-function symbol)
> -          (edebug-unwrap* (symbol-function symbol))))
> +    (if-let ((advised (edebug--advised-p symbol)))
> +        (unless (eq advised t)
> +          (funcall (or (get symbol 'defalias-fset-function) #'fset)
> +                   symbol advised))
> +      (setf (symbol-function symbol)
> +            (edebug-unwrap* (symbol-function symbol)))))

Yuck!

Can't we just use `defalias` rather than `fset` (and that should
take care of calling `defalias-fset-function` when needed)?

> I'm not sure if I should use `indirect-function' somewhere instead of
> `symbol-function',

That's a question for Edebug's ;-)
In the above I assumed that there's a good reason why it uses
`symbol-function` so I preserved it.

> but I also don't understand (and also didn't yet try to find out) why
> nadvice doesn't use `indirect-function' for its manipulations.

That's because if I have `foo` as an alias for `bar` and I advise `foo`
I don't want it to affect `bar`: if you want to affect both, then you
should advise `bar`.


        Stefan

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> > +(defun edebug--advised-p (symbol)
> > +  ;; Non-nil when SYMBOL's `symbol-function' is advised.  The non-nil
> > +  ;; return value is the unwrapped base function if it was wrapped,
> > +  ;; and the symbol t else.
>
> Do you really mean "advised"?  It seems that this tests whether the code
> is *instrumented* by looking past the (potential) pieces of advice.

It tests both - sorry, a bit ugly - for the sake of fitting into the
existing code.  As a predicate it tests whether the thing is advised,
and the return value is the unwrapped original function when it can be
unwrapped, or t if it can't.

> > +  (pcase (symbol-function symbol)
> > +    ((and (pred advice--p)
> > +          (app advice--cd*r orig-f)
> > +          (let unwrapped (edebug-unwrap* orig-f)))
> > +     (if (equal unwrapped orig-f) t unwrapped))
> > +    (`(macro . ,(and (pred advice--p)
> > +                     (app advice--cd*r orig-f)
> > +                     (let unwrapped (edebug-unwrap* orig-f))))
> > +     (if (equal unwrapped orig-f) t `(macro . ,unwrapped)))))
>
> [ That's pretty ugly.  I think I'd move the `app` and the `let` outside
>   of those patterns and into the code of the corresponding branch.  ]

Hmm, that's what I had first, and I find this better...I guess nobody
else does, so I'll do what you suggest.

> >    ;; Remove instrumentation.
> >    (dolist (symbol functions)
> > -    (setf (symbol-function symbol)
> > -          (edebug-unwrap* (symbol-function symbol))))
> > +    (if-let ((advised (edebug--advised-p symbol)))
> > +        (unless (eq advised t)
> > +          (funcall (or (get symbol 'defalias-fset-function) #'fset)
> > +                   symbol advised))
> > +      (setf (symbol-function symbol)
> > +            (edebug-unwrap* (symbol-function symbol)))))
>
> Yuck!
>
> Can't we just use `defalias` rather than `fset` (and that should
> take care of calling `defalias-fset-function` when needed)?

That's what I want to know too!  I guess we can, but I thought defalias
would change the source file association as a side effect?

> That's because if I have `foo` as an alias for `bar` and I advise `foo`
> I don't want it to affect `bar`: if you want to affect both, then you
> should advise `bar`.

I hoped it would be like that.  So after advising `foo` the
`symbol-function' of `foo` is no longer a symbol, so there is no need to
call `indirect-function' in my change.  That wasn't clear to me.


Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Hi Lars,

when testing the advised macro case (AFAIK macros can be advised) I
found that `edebug-remove-instrumentation' doesn't handle macros at all
because the `functionp' test you use fails for macros.  What is the
correct test here (are there even more cases to consider)?


Thanks,

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Stefan Monnier]

>> Can't we just use `defalias` rather than `fset` (and that should
>> take care of calling `defalias-fset-function` when needed)?
> That's what I want to know too!  I guess we can, but I thought defalias
> would change the source file association as a side effect?

I don't think it would.


        Stefan

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> > > the user interface for edebug-remove-instrumentation could be
> > > similar to `cancel-edebug-on-entry', i.e. it could prompt for a
> > > function to remove instrumentation from, defaulting to all
> > > functions.
> >
> > Makes sense -- I've now done this.
>
> Thanks!

But could you please update this sentence in "(elisp) Instrumenting":

"If you want to remove Edebug instrumentation from all functions, you can
use the ‘edebug-remove-instrumentation’ command."

And when this works reliably, we can also say that
‘edebug-remove-instrumentation’ is the preferred way to remove
instrumentation I think.

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> But could you please update this sentence in "(elisp) Instrumenting":
>
> "If you want to remove Edebug instrumentation from all functions, you can
> use the ‘edebug-remove-instrumentation’ command."

The previous paragraph says:

---
  To remove instrumentation from a definition, simply re-evaluate its
definition in a way that does not instrument.
---

So while the new command can do this for one function, the salient
feature is that it can do it for all functions, too.  The re-evaluation
thing is still easier if you just want to do it for one function, I think?

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

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> when testing the advised macro case (AFAIK macros can be advised) I
> found that `edebug-remove-instrumentation' doesn't handle macros at all
> because the `functionp' test you use fails for macros. 

Hm, I didn't think about edebugging macros, but I guess that's also
possible?  Let's see...

(defmacro foo (x)
  `(+ 1 ,x))

(foo 2)

Yes indeed.  I've now adjusted the command to also do this for macrop
symbols.

> What is the correct test here (are there even more cases to consider)?

Hm...  Are there more cases?  There's special forms, but they can't be
edebugged, I think?  I can't recall any more function-ey things...

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

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm, I didn't think about edebugging macros, but I guess that's also
> possible?  Let's see...
>
> (defmacro foo (x)
>   `(+ 1 ,x))
>
> (foo 2)
>
> Yes indeed.

You don't remember all the debug specs in macro declarations?  These are
all for making edebug work with these macros.

> I've now adjusted the command to also do this for macrop
> symbols.

Looks good, thanks.

> > What is the correct test here (are there even more cases to consider)?
>
> Hm...  Are there more cases?  There's special forms, but they can't be
> edebugged, I think?  I can't recall any more function-ey things...

Autoload functions are unlikely to be edebugged.  `defsubst's -- for
source code they are like normal functions (i.e. covered).  So let's
assume these were all cases to consider.

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> You don't remember all the debug specs in macro declarations?  These are
> all for making edebug work with these macros.

They're mostly for making edebugging functions that use those macros
possible, aren't they?

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

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> > You don't remember all the debug specs in macro declarations?  These
> > are all for making edebug work with these macros.
>
> They're mostly for making edebugging functions that use those macros
> possible, aren't they?

I think you are right: instrumenting the macro let's you edebug the
generation of the expansion.  The purpose of the edebug spec is making
edebugging the generated code possible.

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> The previous paragraph says:
>
> ---
>   To remove instrumentation from a definition, simply re-evaluate its
> definition in a way that does not instrument.
> ---
>
> So while the new command can do this for one function, the salient
> feature is that it can do it for all functions, too.  The
> re-evaluation thing is still easier if you just want to do it for one
> function, I think?

Depends if you have the buffer at hand and how confused you got while
edebugging... I think I would always prefer the handy new command.

With one exception: the new command will never give you compiled
definitions.  If you want to use compiled code for the rest of your
session after you are done you need to reload the elc.

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Hello again,

I'm preparing a new patch with less quirks.  I have a question about
advising macros now.  We don't say much about that, only this paragraph
in (info "(elisp) Advising Named Functions"):

   Special forms (*note Special Forms::) cannot be advised, however
macros can be advised, in much the same way as functions.  Of course,
this will not affect code that has already been macro-expanded, so you
need to make sure the advice is installed before the macro is expanded.

All good - but it seems that when you advice a macro, you actually
operate on the expander _function_.  Maybe we should speak that out more
clearly?  Because that implies that everything you deal with is a
function - e.g. in an :override advice you must specify a function - if
you specify a macro the thing breaks.

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Stefan Monnier]

> All good - but it seems that when you advice a macro, you actually
> operate on the expander _function_.  Maybe we should speak that out more
> clearly?  Because that implies that everything you deal with is a
> function - e.g. in an :override advice you must specify a function - if
> you specify a macro the thing breaks.

Right.  Currently, this is just implied by the fact that the third
argument of `advice-add` is described as "FUNCTION", and that the
explanations (in `add-function`) of how this arg is combined with the
original definition all use `funcall` and `apply`.

I'm not sure how to write the doc to make it more clear, tho (largely
because it's just too obvious to me that it's the only way it can work),
so if you have a suggestion, fell free to send it.


        Stefan

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Depends if you have the buffer at hand and how confused you got while
> edebugging... I think I would always prefer the handy new command.

Makes sense.  I've now reworded the manual to make it clearer what it
offers.

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

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>  I've now reworded the manual to make it clearer what it offers.

Perfect, thanks.

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I'm not sure how to write the doc to make it more clear, tho (largely
> because it's just too obvious to me that it's the only way it can
> work), so if you have a suggestion, fell free to send it.

I think - although it would add some more lines - an example would be
best to illustrate that fact.  How about something like this?


  Advising macros operates on the defining expander functions.
  Example: to prepend a `debug' call to any expansion of `my-macro'
  you could do

  (advice-add 'my-macro
              :around
              (lambda (original-expander &rest macro-args)
                `(progn
                   (debug)
                   ,(apply original-expander macro-args)))
              '((name . debug-expanded-code)))


Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Hello again,
>
> I'm preparing a new patch with less quirks.

New patch: not much different but hopefully better understandable now.
I also added a comment about why we are allowed to just strip advises.
Also did some testing - seems to work ok.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Try-to-fix-edebug-remove-instrumentation-for-adv.patch --]
[-- Type: text/x-diff, Size: 2834 bytes --]

From 29380dcd30523e057d8fd45106f1d077e4212198 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Thu, 14 Nov 2019 17:47:51 +0100
Subject: [PATCH] WIP: Try to fix edebug-remove-instrumentation for advised
 funs

---
 lisp/emacs-lisp/edebug.el | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 5d52704410..e0fcdf0f86 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -4571,6 +4571,23 @@ edebug-unload-function
   ;; Continue standard unloading.
   nil)

+(defun edebug--unwrap*-symbol-function (symbol)
+  ;; Try to unwrap SYMBOL's symbol function.  The result is suitable
+  ;; to be fbound to SYMBOL with `defalias'.  When no unwrapping could
+  ;; be done return nil.
+  (pcase (symbol-function symbol)
+    ((or (and `(macro . ,(and (pred advice--p) ad)) (let was-macro t))
+         (and (pred advice--p) ad                   (let was-macro nil)))
+     ;; `defalias' takes care of any advises so we can just strip them
+     (let* ((orig-f (advice--cd*r ad))
+            (unwrapped (edebug-unwrap* orig-f)))
+       (cond
+        ((equal unwrapped orig-f) nil)
+        (was-macro               `(macro . ,unwrapped))
+        (t                       unwrapped))))
+    (sym-fun (let ((unwrapped (edebug-unwrap* sym-fun)))
+               (if (equal sym-fun unwrapped) nil unwrapped)))))
+
 (defun edebug-remove-instrumentation (functions)
   "Remove Edebug instrumentation from FUNCTIONS.
 Interactively, the user is prompted for the function to remove
@@ -4582,10 +4599,10 @@ edebug-remove-instrumentation
        (lambda (symbol)
          (when (and (get symbol 'edebug)
                     (or (functionp symbol)
-                        (macrop symbol)))
-           (let ((unwrapped (edebug-unwrap* (symbol-function symbol))))
-             (unless (equal unwrapped (symbol-function symbol))
-               (push symbol functions)))))
+                        (macrop symbol))
+                    (edebug--unwrap*-symbol-function
+                     symbol))
+           (push symbol functions)))
        obarray)
       (unless functions
         (error "Found no functions to remove instrumentation from"))
@@ -4599,8 +4616,9 @@ edebug-remove-instrumentation
           functions)))))
   ;; Remove instrumentation.
   (dolist (symbol functions)
-    (setf (symbol-function symbol)
-          (edebug-unwrap* (symbol-function symbol))))
+    (when-let ((unwrapped
+                (edebug--unwrap*-symbol-function symbol)))
+      (defalias symbol unwrapped)))
   (message "Removed edebug instrumentation from %s"
            (mapconcat #'symbol-name functions ", ")))

--
2.24.0


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



Thanks,

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Stefan Monnier]

> +  (pcase (symbol-function symbol)
> +    ((or (and `(macro . ,(and (pred advice--p) ad)) (let was-macro t))
> +         (and (pred advice--p) ad                   (let was-macro nil)))

I don't think you need the `advice--p` here since `advice--cd*r` uses the
`*` meaning of regexps: "*zero* or more".

> +     ;; `defalias' takes care of any advises so we can just strip them

Actually, you *have* to strip them (otherwise you'd end up copying them).


        Stefan

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I don't think you need the `advice--p` here since `advice--cd*r` uses the
> `*` meaning of regexps: "*zero* or more".

Ok, removed.

> > +     ;; `defalias' takes care of any advises so we can just strip them
>
> Actually, you *have* to strip them (otherwise you'd end up copying them).

Sure, I made that comment clearer.

New patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-edebug-instrumentation-removing-from-advised-fun.patch --]
[-- Type: text/x-diff, Size: 2752 bytes --]

From aab2cd47da230993e374d378c434989c98ce68ed Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Thu, 14 Nov 2019 17:47:51 +0100
Subject: [PATCH] Fix edebug instrumentation removing from advised functions

* lisp/emacs-lisp/edebug.el (edebug-remove-instrumentation): Handle
advised functions correctly.
---
 lisp/emacs-lisp/edebug.el | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 5d52704410..d68ed966f8 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -4571,6 +4571,21 @@ edebug-unload-function
   ;; Continue standard unloading.
   nil)

+(defun edebug--unwrap*-symbol-function (symbol)
+  ;; Try to unwrap SYMBOL's `symbol-function'.  The result is suitable
+  ;; to be fbound back to SYMBOL with `defalias'.  When no unwrapping
+  ;; could be done return nil.
+  (pcase (symbol-function symbol)
+    ((or (and `(macro . ,f) (let was-macro t))
+         (and  f            (let was-macro nil)))
+     ;; `defalias' takes care of advises so we must strip them
+     (let* ((orig-f (advice--cd*r f))
+            (unwrapped (edebug-unwrap* orig-f)))
+       (cond
+        ((equal unwrapped orig-f) nil)
+        (was-macro               `(macro . ,unwrapped))
+        (t                       unwrapped))))))
+
 (defun edebug-remove-instrumentation (functions)
   "Remove Edebug instrumentation from FUNCTIONS.
 Interactively, the user is prompted for the function to remove
@@ -4582,10 +4597,10 @@ edebug-remove-instrumentation
        (lambda (symbol)
          (when (and (get symbol 'edebug)
                     (or (functionp symbol)
-                        (macrop symbol)))
-           (let ((unwrapped (edebug-unwrap* (symbol-function symbol))))
-             (unless (equal unwrapped (symbol-function symbol))
-               (push symbol functions)))))
+                        (macrop symbol))
+                    (edebug--unwrap*-symbol-function
+                     symbol))
+           (push symbol functions)))
        obarray)
       (unless functions
         (error "Found no functions to remove instrumentation from"))
@@ -4599,8 +4614,9 @@ edebug-remove-instrumentation
           functions)))))
   ;; Remove instrumentation.
   (dolist (symbol functions)
-    (setf (symbol-function symbol)
-          (edebug-unwrap* (symbol-function symbol))))
+    (when-let ((unwrapped
+                (edebug--unwrap*-symbol-function symbol)))
+      (defalias symbol unwrapped)))
   (message "Removed edebug instrumentation from %s"
            (mapconcat #'symbol-name functions ", ")))

--
2.24.0


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


Ok to install?


BTW, I also wonder if we should enhance the command
`edebug-remove-instrumentation' so that it is able to reload source
files.  It could look at the SYMOL's `symbol-file's, collect these, load
the files, and only do what it does now for the symbols that are still
wrapped.  Could be controlled via prefix argument.


Regards,

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> New patch:

I've installed that patch now.

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen]

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> BTW, I also wonder if we should enhance the command
> `edebug-remove-instrumentation' so that it is able to reload source
> files.  It could look at the SYMOL's `symbol-file's, collect these, load
> the files, and only do what it does now for the symbols that are still
> wrapped.

Here is a draft.  Any thoughts (Lars)?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-edebug-see-38195-ask-whether-to-reload-files-to-.patch --]
[-- Type: text/x-diff, Size: 4366 bytes --]

From 681370954b2f5168e6e0793a9a7ded76db671682 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Tue, 26 Nov 2019 19:23:45 +0100
Subject: [PATCH] WIP: edebug: see 38195, ask whether to reload files to
 deinstrument

---
 lisp/emacs-lisp/edebug.el | 73 ++++++++++++++++++++++++++++++---------
 1 file changed, 57 insertions(+), 16 deletions(-)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index d68ed966f8..29bb27fc0d 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -4586,36 +4586,77 @@ edebug--unwrap*-symbol-function
         (was-macro               `(macro . ,unwrapped))
         (t                       unwrapped))))))

+(defcustom edebug-reload-files 'ask
+  "Whether `edebug-remove-instrumentation' should reload files.
+
+When non-nil, `edebug-remove-instrumentation' will reload files
+where possible to get rid of instrumentation.  When the non-nil
+value is the symbol 'ask, ask for every individual file before
+loading it.
+
+When nil or when no defining file can be found remove edebug
+instrumentation by manipulating symbol functions."
+  :type '(choice (const :tag "On" t)
+                 (const :tag "Ask for every file" ask)
+                 (const :tag "Off" nil)))
+
+(defun edebug-get-instrumented-functions ()
+  (let ((functions '()))
+    (mapatoms
+     (lambda (symbol)
+       (when (and (get symbol 'edebug)
+                  (or (functionp symbol)
+                      (macrop symbol))
+                  (edebug--unwrap*-symbol-function
+                   symbol))
+         (push symbol functions)))
+     obarray)
+    functions))
+
 (defun edebug-remove-instrumentation (functions)
   "Remove Edebug instrumentation from FUNCTIONS.
 Interactively, the user is prompted for the function to remove
 instrumentation for, defaulting to all functions."
   (interactive
    (list
-    (let ((functions nil))
-      (mapatoms
-       (lambda (symbol)
-         (when (and (get symbol 'edebug)
-                    (or (functionp symbol)
-                        (macrop symbol))
-                    (edebug--unwrap*-symbol-function
-                     symbol))
-           (push symbol functions)))
-       obarray)
+    (let ((functions (edebug-get-instrumented-functions)))
       (unless functions
         (error "Found no functions to remove instrumentation from"))
       (let ((name
              (completing-read
               "Remove instrumentation from (default all functions): "
               functions)))
-        (if (and name
-                 (not (equal name "")))
+        (if (and name (not (equal name "")))
             (list (intern name))
-          functions)))))
-  ;; Remove instrumentation.
+          t)))))
+  (unless (listp functions)
+    (setq functions (edebug-get-instrumented-functions)))
+  (when edebug-reload-files
+    (let ((files '()))
+      (dolist (f functions)
+        (when-let ((file (symbol-file f 'defun)))
+          (unless (cl-some (apply-partially #'file-equal-p file) files)
+            (push file files))))
+      (let ((do-all (eq edebug-reload-files t))
+            file)
+        (while files
+          (setq file (pop files))
+          (when (or do-all
+                    (pcase (car (read-multiple-choice
+                                 (format "Load %s ?" file)
+                                 (list (list ?y "y" "Reload this file")
+                                       (list ?Y "Y" "\
+Reload this and all following files")
+                                       (list ?n "n" "Don't load this file")
+                                       (list ?N "N" "\
+Don't load this and any following files"))))
+                      (?y t)
+                      (?Y (setq do-all t)  t)
+                      (?n nil)
+                      (?N (setq files nil) nil)))
+            (load file 'noerror nil 'nosuffix))))))
   (dolist (symbol functions)
-    (when-let ((unwrapped
-                (edebug--unwrap*-symbol-function symbol)))
+    (when-let ((unwrapped (edebug--unwrap*-symbol-function symbol)))
       (defalias symbol unwrapped)))
   (message "Removed edebug instrumentation from %s"
            (mapconcat #'symbol-name functions ", ")))
--
2.24.0


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



Regards,

Michael.

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> BTW, I also wonder if we should enhance the command
>> `edebug-remove-instrumentation' so that it is able to reload source
>> files.  It could look at the SYMOL's `symbol-file's, collect these, load
>> the files, and only do what it does now for the symbols that are still
>> wrapped.
>
> Here is a draft.  Any thoughts (Lars)?

[...]

> +(defcustom edebug-reload-files 'ask
> +  "Whether `edebug-remove-instrumentation' should reload files.
> +
> +When non-nil, `edebug-remove-instrumentation' will reload files
> +where possible to get rid of instrumentation.  When the non-nil
> +value is the symbol 'ask, ask for every individual file before
> +loading it.

I don't think this is something that should be mixed up with the
edebug-remove-instrumentation command.  Reloading files can have other
side effects, and those may get in the way.  The user just wants the
edebugging to go away so that they can continue to use Emacs, not change
other bits.

But adding a new command like edebug-reload-instrumented-files would be
OK.

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

bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I don't think this is something that should be mixed up with the
> edebug-remove-instrumentation command.  Reloading files can have other
> side effects, and those may get in the way.  The user just wants the
> edebugging to go away so that they can continue to use Emacs, not change
> other bits.

So my feeling is that piggy-backing this somewhat unrelated
functionality on top of the edebug machinery doesn't really make
conceptual sense, and I'm closing this bug report.

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