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

Previous Next

Full log

View this message in rfc822 format

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

Previous Next

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