* Patch: debug-instrumented predicate @ 2021-10-04 15:20 Arthur Miller 2021-10-04 16:24 ` Stefan Monnier 0 siblings, 1 reply; 7+ messages in thread From: Arthur Miller @ 2021-10-04 15:20 UTC (permalink / raw) To: emacs-devel [-- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: Patch: debug-instrumented predicate 2021-10-04 15:20 Patch: debug-instrumented predicate Arthur Miller @ 2021-10-04 16:24 ` Stefan Monnier 2021-10-04 19:25 ` Arthur Miller 0 siblings, 1 reply; 7+ messages in thread From: Stefan Monnier @ 2021-10-04 16:24 UTC (permalink / raw) To: Arthur Miller; +Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch: debug-instrumented predicate 2021-10-04 16:24 ` Stefan Monnier @ 2021-10-04 19:25 ` Arthur Miller 2021-10-04 20:00 ` Stefan Monnier 0 siblings, 1 reply; 7+ messages in thread From: Arthur Miller @ 2021-10-04 19:25 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch: debug-instrumented predicate 2021-10-04 19:25 ` Arthur Miller @ 2021-10-04 20:00 ` Stefan Monnier 2021-10-04 21:58 ` Arthur Miller 0 siblings, 1 reply; 7+ messages in thread From: Stefan Monnier @ 2021-10-04 20:00 UTC (permalink / raw) To: Arthur Miller; +Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch: debug-instrumented predicate 2021-10-04 20:00 ` Stefan Monnier @ 2021-10-04 21:58 ` Arthur Miller 2021-10-04 22:14 ` Stefan Monnier 0 siblings, 1 reply; 7+ messages in thread From: Arthur Miller @ 2021-10-04 21:58 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel 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 :). ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch: debug-instrumented predicate 2021-10-04 21:58 ` Arthur Miller @ 2021-10-04 22:14 ` Stefan Monnier 2021-10-04 22:56 ` Arthur Miller 0 siblings, 1 reply; 7+ messages in thread From: Stefan Monnier @ 2021-10-04 22:14 UTC (permalink / raw) To: Arthur Miller; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch: debug-instrumented predicate 2021-10-04 22:14 ` Stefan Monnier @ 2021-10-04 22:56 ` Arthur Miller 0 siblings, 0 replies; 7+ messages in thread From: Arthur Miller @ 2021-10-04 22:56 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-04 22:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-04 15:20 Patch: debug-instrumented predicate Arthur Miller 2021-10-04 16:24 ` Stefan Monnier 2021-10-04 19:25 ` Arthur Miller 2021-10-04 20:00 ` Stefan Monnier 2021-10-04 21:58 ` Arthur Miller 2021-10-04 22:14 ` Stefan Monnier 2021-10-04 22:56 ` Arthur Miller
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).