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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 38195 in the body.
You can then email your comments to 38195 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: bug-gnu-emacs <at> gnu.org
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced
 functions
Date: Wed, 13 Nov 2019 14:55:58 +0100

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

Message #8 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Thu, 14 Nov 2019 06:20:40 +0100

Michael Heerdegen <michael_heerdegen <at> 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

Message #11 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Thu, 14 Nov 2019 17:51:58 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Michael Heerdegen <michael_heerdegen <at> 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.

Message #14 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Thu, 14 Nov 2019 17:55:28 +0100

[Message part 1 (text/plain, inline)]

Lars Ingebrigtsen <larsi <at> 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?

[0001-WIP-Try-to-fix-edebug-remove-instrumentation-for-adv.patch (text/x-diff, inline)]

From c1c2cd4faa0adc87f9334ec92ace8bcf5ae8333e Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen <at> 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

[Message part 3 (text/plain, inline)]

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.

Message #17 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50;
 `edebug-remove-instrumentation' doesn't work for adviced functions
Date: Thu, 14 Nov 2019 14:08:24 -0500

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

Message #20 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Thu, 14 Nov 2019 21:27:18 +0100

Stefan Monnier <monnier <at> 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.

Message #23 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Thu, 14 Nov 2019 22:15:17 +0100

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.

Message #26 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50;
 `edebug-remove-instrumentation' doesn't work for adviced functions
Date: Thu, 14 Nov 2019 16:33:52 -0500

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

Message #29 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Thu, 14 Nov 2019 23:39:29 +0100

Michael Heerdegen <michael_heerdegen <at> 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.

Message #32 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Fri, 15 Nov 2019 08:57:31 +0100

Michael Heerdegen <michael_heerdegen <at> 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

Message #35 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Fri, 15 Nov 2019 09:03:36 +0100

Michael Heerdegen <michael_heerdegen <at> 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

Message #38 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Fri, 15 Nov 2019 13:11:53 +0100

Lars Ingebrigtsen <larsi <at> 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.

Message #41 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Fri, 15 Nov 2019 13:15:44 +0100

Michael Heerdegen <michael_heerdegen <at> 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

Message #44 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Fri, 15 Nov 2019 13:34:19 +0100

Lars Ingebrigtsen <larsi <at> 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.

Message #47 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Fri, 15 Nov 2019 13:39:32 +0100

Lars Ingebrigtsen <larsi <at> 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.

Message #50 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Fri, 15 Nov 2019 14:54:04 +0100

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.

Message #53 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Fri, 15 Nov 2019 12:30:54 -0500

> 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

Message #56 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Sat, 16 Nov 2019 05:28:03 +0100

Michael Heerdegen <michael_heerdegen <at> 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

Message #59 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Sat, 16 Nov 2019 13:25:16 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

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

Perfect, thanks.

Michael.

Message #62 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Sun, 17 Nov 2019 13:35:49 +0100

Stefan Monnier <monnier <at> 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.

Message #65 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Sun, 17 Nov 2019 13:55:16 +0100

[Message part 1 (text/plain, inline)]

Michael Heerdegen <michael_heerdegen <at> 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.

[0001-WIP-Try-to-fix-edebug-remove-instrumentation-for-adv.patch (text/x-diff, inline)]

From 29380dcd30523e057d8fd45106f1d077e4212198 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen <at> 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

[Message part 3 (text/plain, inline)]

Thanks,

Michael.

Message #68 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Sun, 17 Nov 2019 11:04:34 -0500

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

Message #71 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Thu, 21 Nov 2019 12:49:26 +0100

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> 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:

[0001-Fix-edebug-instrumentation-removing-from-advised-fun.patch (text/x-diff, inline)]

From aab2cd47da230993e374d378c434989c98ce68ed Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen <at> 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

[Message part 3 (text/plain, inline)]

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.

Message #74 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Sat, 23 Nov 2019 14:32:12 +0100

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

> New patch:

I've installed that patch now.

Michael.

Message #77 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Tue, 26 Nov 2019 22:01:32 +0100

[Message part 1 (text/plain, inline)]

Michael Heerdegen <michael_heerdegen <at> 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)?

[0001-WIP-edebug-see-38195-ask-whether-to-reload-files-to-.patch (text/x-diff, inline)]

From 681370954b2f5168e6e0793a9a7ded76db671682 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen <at> 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

[Message part 3 (text/plain, inline)]

Regards,

Michael.

Message #80 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Wed, 27 Nov 2019 13:17:51 +0100

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

> Michael Heerdegen <michael_heerdegen <at> 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

Message #83 received at 38195 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 38195 <at> debbugs.gnu.org
Subject: Re: bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't
 work for adviced functions
Date: Sun, 20 Sep 2020 12:54:26 +0200

Lars Ingebrigtsen <larsi <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.