Patch: debug-instrumented predicate

Patch: debug-instrumented predicate

[Arthur Miller]

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


For tooling purpose I would like to have a reliable check to see if a symbol is
instrumented for debugger or not.

I have seen that internal code in debug.el uses advice
'debug--implement-debug-on-entry' for instrumented symbols, but I am not sure if
it can be used as a stable check long-term. Suggested patch uses it, but if it
is considered internal and subject of change, please add a way to have a stable
predicate.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Debug-Instrumented-Predicate.patch --]
[-- Type: text/x-patch, Size: 874 bytes --]

From 16574a7f6f9e2797fe68e06508189daafbb8c005 Mon Sep 17 00:00:00 2001
From: Arthur Miller <arthur.miller@live.com>
Date: Mon, 4 Oct 2021 15:48:13 +0200
Subject: [PATCH 2/2] Debug Instrumented Predicate

* lisp/emacs-lisp/debug.el (debug-instrumented-p): New function.
---
 lisp/emacs-lisp/debug.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index 0592db85df..8d7e150362 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -683,6 +683,10 @@ debug-on-entry
               '((depth . -100)))
   function)
 
+(defun debug-instrumented-p (function)
+  "Whether FUNCTION has been instrumented for debug."
+  (advice-member-p #'debug--implement-debug-on-entry function))
+
 (defun debug--function-list ()
   "List of functions currently set for debug on entry."
   (let ((funs '()))
-- 
2.33.0

Re: Patch: debug-instrumented predicate

[Stefan Monnier]

> I have seen that internal code in debug.el uses advice
> 'debug--implement-debug-on-entry' for instrumented symbols, but I am not sure if
> it can be used as a stable check long-term. Suggested patch uses it, but if it
> is considered internal and subject of change, please add a way to have a stable
> predicate.

Can you give us a concrete use case?


        Stefan

Re: Patch: debug-instrumented predicate

[Arthur Miller]

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

>> I have seen that internal code in debug.el uses advice
>> 'debug--implement-debug-on-entry' for instrumented symbols, but I am not sure if
>> it can be used as a stable check long-term. Suggested patch uses it, but if it
>> is considered internal and subject of change, please add a way to have a stable
>> predicate.
>
> Can you give us a concrete use case?

Concrete use-case would be to offer a user some kind of gui to instrument or remove
instrumentation for debug/edebug/profile/trace.

Helpful.el is the one package I know off that offer a button for debeg/edebug
and trace in style of enable/disable.

I have re-implemented helpful in terms of built-in help infrastructure and am
using those functions I posted as patches when toggling on/off above mentioned
instrumentation.

I would prefer to use a stable API instead of something based on internal
implementation that can change without notice.

I don't think it is unimeganble that someone else might develop some other tool
that might work with profiling, debugging etc and might need to know if
instrumentation for a function is on or off.

Re: Patch: debug-instrumented predicate

[Stefan Monnier]

> Concrete use-case would be to offer a user some kind of gui to instrument or remove
> instrumentation for debug/edebug/profile/trace.

W.r.t *removal* of debug/profile/trace, this could be done by offering
a generic removal GUI for advice.

It wouldn't cover the case where the user wants to add such
instrumentation, tho.

And currently, that wouldn't cover Edebug because Edebug doesn't install its
instrumented code with `advice-add`.


        Stefan

Re: Patch: debug-instrumented predicate

[Arthur Miller]

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

>> Concrete use-case would be to offer a user some kind of gui to instrument or remove
>> instrumentation for debug/edebug/profile/trace.
>
> W.r.t *removal* of debug/profile/trace, this could be done by offering
> a generic removal GUI for advice.

Njah, advice already has add/remove which can be connected to a gui button or
what not. However any generic interface always need a specialisation for a case
at hand anyway, at least a different label on a button. But we are drifting
away.

Edebug, debug, elp and trace already have means to add and remove instrumentation.
I ask/suggest for an API to discover if instrumentation is installed or not.

What they don't offer, except for trace, is an API to check if instrumentation
is on or off. Debug is trivial, see the attached patch; trace has
trase-is-traced, elp has private elp--instrumented-p which I suggested to make public.
The only one that truly is missing a mean to test for on/off state is edebug.


> It wouldn't cover the case where the user wants to add such
> instrumentation, tho.

No, it wouldn't, but it does not have to; see comment above.

> And currently, that wouldn't cover Edebug because Edebug doesn't install its
> instrumented code with `advice-add`.

No edebug does it by other means; it instruments function with wrapped code. I
have posted example of how helpful.el does check, and I have also attached a
patch that uses different method to check for on/off state based on built-in
function as posted with patch.

I don't know which one is more efficient, but I don't like neither of those two,
I don't think any of suggested solutions is efficient. Also they both rely on
internal state that can change whenever.

Since we already talk edebug, I think there is a bug. I meant to report it
when I have done patch to fix it, but I haven't had time with that yet:

When a function is instrumented for edebug, edebug adds some properties into
symbols property list. However when instrumentation is removed, edebug leaves
those properties to scrap behind; that inclusive the position in buffer where it
was active. IMO it is a bug; it shouldn't leave scrap behind. It would also be
trivial to check if a funciton is instrumented or not, if those properties were
removed when instrumentation is removed.

Unless I misunderstand the purpose why properties are left behind after
instrumentation is removed :).

Re: Patch: debug-instrumented predicate

[Stefan Monnier]

Arthur Miller [2021-10-04 23:58:26] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> Concrete use-case would be to offer a user some kind of gui to instrument or remove
>>> instrumentation for debug/edebug/profile/trace.
>>
>> W.r.t *removal* of debug/profile/trace, this could be done by offering
>> a generic removal GUI for advice.
>
> Njah, advice already has add/remove which can be connected to a gui button or
> what not. However any generic interface always need a specialisation for a case
> at hand anyway, at least a different label on a button. But we are drifting
> away.

I don't think it's drifting: I'm suggesting that we try to add support
for it in a way that supports them all at the same time.

> I ask/suggest for an API to discover if instrumentation is installed or not.

And I'm suggesting that make `advice-member-p` should be the way to do that.
Currently this requires reliance on some internal knowledge of the
advice's name, but we could fix it by making this name public&stable.

> The only one that truly is missing a mean to test for on/off state is edebug.

Indeed.  I haven't looked at whether it could be made to use
`advice-add` so that `advice-member-p` could be used as well.
There's a good chance that it's not really an option (I mean
technically, I'm pretty sure it could be done, but I suspect it will
come with enough downsides that it's not worth it).

Until it can be made to use `advice-member-p` we should indeed export
a public way to test whether a function is instrumented by Edebug.

> I don't know which one is more efficient, but I don't like neither of those two,
> I don't think any of suggested solutions is efficient. Also they both rely on
> internal state that can change whenever.

As long as these functions are defined in `edebug.el` it's OK if they
rely on internal state.

> When a function is instrumented for edebug, edebug adds some properties into
> symbols property list. However when instrumentation is removed, edebug leaves
> those properties to scrap behind; that inclusive the position in buffer where it
> was active. IMO it is a bug; it shouldn't leave scrap behind. It would also be
> trivial to check if a funciton is instrumented or not, if those properties were
> removed when instrumentation is removed.
>
> Unless I misunderstand the purpose why properties are left behind after
> instrumentation is removed :).

I suspect they're not left behind for any good reason, and
the best way to find out is to remove them and see if someone screams.


        Stefan

Re: Patch: debug-instrumented predicate

[Arthur Miller]

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

> Arthur Miller [2021-10-04 23:58:26] wrote:
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>> Concrete use-case would be to offer a user some kind of gui to instrument or remove
>>>> instrumentation for debug/edebug/profile/trace.
>>>
>>> W.r.t *removal* of debug/profile/trace, this could be done by offering
>>> a generic removal GUI for advice.
>>
>> Njah, advice already has add/remove which can be connected to a gui button or
>> what not. However any generic interface always need a specialisation for a case
>> at hand anyway, at least a different label on a button. But we are drifting
>> away.
>
> I don't think it's drifting: I'm suggesting that we try to add support
> for it in a way that supports them all at the same time.
>
>> I ask/suggest for an API to discover if instrumentation is installed or not.
>
> And I'm suggesting that make `advice-member-p` should be the way to do that.
> Currently this requires reliance on some internal knowledge of the
> advice's name, but we could fix it by making this name public&stable.
Allright, sorry than :).

Yes, sure, that would work of course. Personally I would still prefer if you
choose to use a function to expose as API rather than the advice name. Even if
it is just a tiny wrapper around advice-member-p, such as trace-is-traced or
elp--instrumented-p. I personally think it would be more convenient. And also,
probably anyone interested to use this functionality is goingt tomake a wrapper
just like trace and elp author(s) did :).

>> The only one that truly is missing a mean to test for on/off state is edebug.
>
> Indeed.  I haven't looked at whether it could be made to use
> `advice-add` so that `advice-member-p` could be used as well.
> There's a good chance that it's not really an option (I mean
> technically, I'm pretty sure it could be done, but I suspect it will
> come with enough downsides that it's not worth it).

Edebugg could add a tiny advice that really does nothing, but just serves
as a flag to signal that a function is instrumented. It adds a function
call extra as overhead, but I guess, we are debugging anyway, so it shouldn't
matter?

Otherwise they could just put something into symbol's plist and remove it. That
eliminates the cost of a function call.

> Until it can be made to use `advice-member-p` we should indeed export
> a public way to test whether a function is instrumented by Edebug.
>
>> I don't know which one is more efficient, but I don't like neither of those two,
>> I don't think any of suggested solutions is efficient. Also they both rely on
>> internal state that can change whenever.
>
> As long as these functions are defined in `edebug.el` it's OK if they
> rely on internal state.

Well, yes I guess; that is why I sent in those three patches, I don't want to
have it in my own (external) package.

I also would like to have *-instrumented-p as API, I think it is
self-documenting what it means; but that might be just me.

>> When a function is instrumented for edebug, edebug adds some properties into
>> symbols property list. However when instrumentation is removed, edebug leaves
>> those properties to scrap behind; that inclusive the position in buffer where it
>> was active. IMO it is a bug; it shouldn't leave scrap behind. It would also be
>> trivial to check if a funciton is instrumented or not, if those properties were
>> removed when instrumentation is removed.
>>
>> Unless I misunderstand the purpose why properties are left behind after
>> instrumentation is removed :).
>
> I suspect they're not left behind for any good reason, and
> the best way to find out is to remove them and see if someone screams.

Ok, will you remove it or do you want a patch? Same for the discussion above;
are you doing those changes yourself or is there something I can do?