When calling defuns is a Bad Thing.

When calling defuns is a Bad Thing.

[Alan Mackenzie]

Hello, Emacs.

Calling defuns (as opposed to primitives) from edebug is a bad idea,
unless those defuns are also defined in edebug.el.

The reason is that edebug.el exists to debug these defuns, and if the
edebug engine uses a defun which happens to be instrumented, Emacs will
(probably) lock up.

For the same reason, edebug should probably avoid using "complicated"
macros, such as the pcase family and cl-... because it is difficult to be
sure that their expansions will not introduce non-primitives into the
code.

Currently, edebug calls (at least) one-window-p and walk-windows, which
it shouldn't.  Possibly, it should define edebug- versions of them.
There are also quite a few uses of cl-... which is somewhat worrying.

At the very least, edebug should NOT replace its own edebug-pop-to-buffer
with pop-to-buffer, as has been suggested in a comment.

Comments?

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: When calling defuns is a Bad Thing.

[Stefan Monnier]

> Calling defuns (as opposed to primitives) from edebug is a bad idea,
> unless those defuns are also defined in edebug.el.

Just as a datapoint, I remember using Edebug to debug itself, and
it was very useful to be able to do it (although it's clearly
problematic in the general case).

So, I partly agree, but I also think maybe we should try and work to
make it easier to Edebug any code, even the one used by Edebug.


        Stefan

Re: When calling defuns is a Bad Thing.

[Alan Mackenzie]

Hello, Stefan.

On Sun, Oct 21, 2018 at 14:07:18 -0400, Stefan Monnier wrote:
> > Calling defuns (as opposed to primitives) from edebug is a bad idea,
> > unless those defuns are also defined in edebug.el.

> Just as a datapoint, I remember using Edebug to debug itself, and
> it was very useful to be able to do it (although it's clearly
> problematic in the general case).

> So, I partly agree, but I also think maybe we should try and work to
> make it easier to Edebug any code, even the one used by Edebug.

I hacked up some analysis code (something which you surely have as a
nicely parameterised macro ;-) and found 50 "external" defuns called by
edebug.el, most of them, surely, in the non-engine parts.  Indeed, one
of them is sit-for, which I can vaguely remember edebugging in the past,
so yes, things aren't as bad as I initially thought.

There are, by the way, twelve "external" macros called, ranging all the
way from cl-defstruct to pcase-dolist to cl-letf*.  Most, if not all, of
these are surely "harmless", in that either they're used in non-critical
parts of edebug.el, or they don't generate calls to defuns in their
expansions.

So, I now agree with your initial assessment, there's probably nothing
much to worry about here, but I intend to explore further, and see if I
can crash/hang Emacs by using edebug.

One thing that could be done is to put a `no-edebug' property on symbols
whose function mustn't be instrumented (such as edebug-slow-after), but
that seems somewhat excessive, unless we find a real problem.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: When calling defuns is a Bad Thing.

[Stefan Monnier]

> So, I now agree with your initial assessment, there's probably nothing
> much to worry about here, but I intend to explore further, and see if I
> can crash/hang Emacs by using edebug.

IIRC normally the way things work is that the instrumentation code
checks `edebug-active` to see if we're "running normal code" or
"running Edebug's own code", so that when you instrument code that's
used by Edebug it behaves as instrumented when "called from outside" but
not when "called from Edebug".

> One thing that could be done is to put a `no-edebug' property on symbols
> whose function mustn't be instrumented (such as edebug-slow-after), but
> that seems somewhat excessive, unless we find a real problem.

Indeed.  I just tried edebugging edebug-slow-after and I got a "variable
binding depth exceeds ..." error.  Not completely sure why it breaks in
this particular way, but at least it doesn't inf-loop.

I think we could improve the code so more of Edebug can be edebugged
(e.g. I get the impression that Edebugging edebug--update-coverage will
break whereas it should be possible to edebug it), but it's probably not
worth spending the time until someone actually bumps into the need.


        Stefan