bug#40992: 27.0.90; Evaluating a function while using edebug breaks

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Phillip Lord]

Edebug fails while tracing a function, if that function is evaluated
during the edebug session. The error reported is:

Wrong type argument: listp, #<marker at 4045 in hanoi.el>
edebug--display-1: Wrong type argument: listp, #<marker at 4045 in hanoi.el>

This is a regression as it works in Emacs-26, but fails in Emacs-27.

Reproduction:

In clean emacs -q

M-x hanoi
jump to hanoi function
C-u C-M-x
M-x hanoi

After stepping through into hanoi with point still in

C-M-x

and step once more.


I use this workflow quite a lot when debugging. I may start to debug a
function, decided I have seen enough, to un-instrument so I don't see it
debugged again.

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Alan Mackenzie]

Hello, Phillip and Lars.

On Fri, May 01, 2020 at 09:26:44 +0100, Phillip Lord wrote:


> Edebug fails while tracing a function, if that function is evaluated
> during the edebug session. The error reported is:

> Wrong type argument: listp, #<marker at 4045 in hanoi.el>
> edebug--display-1: Wrong type argument: listp, #<marker at 4045 in hanoi.el>

> This is a regression as it works in Emacs-26, but fails in Emacs-27.

Yes.  A bit of bisection shows that the commit which introduced this bug
is:
commit e8b3a15cb6ff187ce08afcb43bd9a0b7907268ca
Author: Lars Ingebrigtsen <larsi@gnus.org>
Date:   Sun Oct 20 12:07:42 2019 +0200

    Mark breakpoints in edebug with highlights

    * lisp/emacs-lisp/edebug.el (edebug--overlay-breakpoints)
    (edebug--overlay-breakpoints-remove): New functions (bug#23468).
    (edebug-modify-breakpoint): Use them to highlight breakpoints.
    (edebug--display-1): Restore breakpoint highlights.
    (edebug--recursive-edit): Remove highlights on exit.

Lars, have you got any comment on this?

> Reproduction:

> In clean emacs -q

> M-x hanoi
> jump to hanoi function
> C-u C-M-x
> M-x hanoi

> After stepping through into hanoi with point still in

> C-M-x

> and step once more.


> I use this workflow quite a lot when debugging. I may start to debug a
> function, decided I have seen enough, to un-instrument so I don't see it
> debugged again.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Alan Mackenzie]

Hello, Phillip and Lars.

On Fri, May 01, 2020 at 15:56:30 +0000, Alan Mackenzie wrote:
> On Fri, May 01, 2020 at 09:26:44 +0100, Phillip Lord wrote:


> > Edebug fails while tracing a function, if that function is evaluated
> > during the edebug session. The error reported is:

> > Wrong type argument: listp, #<marker at 4045 in hanoi.el>
> > edebug--display-1: Wrong type argument: listp, #<marker at 4045 in hanoi.el>

> > This is a regression as it works in Emacs-26, but fails in Emacs-27.

> Yes.  A bit of bisection shows that the commit which introduced this bug
> is:
> commit e8b3a15cb6ff187ce08afcb43bd9a0b7907268ca
> Author: Lars Ingebrigtsen <larsi@gnus.org>
> Date:   Sun Oct 20 12:07:42 2019 +0200

>     Mark breakpoints in edebug with highlights

>     * lisp/emacs-lisp/edebug.el (edebug--overlay-breakpoints)
>     (edebug--overlay-breakpoints-remove): New functions (bug#23468).
>     (edebug-modify-breakpoint): Use them to highlight breakpoints.
>     (edebug--display-1): Restore breakpoint highlights.
>     (edebug--recursive-edit): Remove highlights on exit.

> Lars, have you got any comment on this?

> > Reproduction:

> > In clean emacs -q

> > M-x hanoi
> > jump to hanoi function
> > C-u C-M-x
> > M-x hanoi

> > After stepping through into hanoi with point still in

> > C-M-x

> > and step once more.

The cause is now clear.  When you instrument a function, the symbol is
given a property 'edebug, whose value is a list of edebug information.
It is this list which edebug--overlay-breakpoints is trying to access
when it throws the error.

When you recompile the function without instrumentation, edebug sets the
'edebug property to the marker #<marker at 4045 in hanoi.el>.  This
non-list value of the property acts elsewhere as a signal that the
function isn't instrumented.

This is a fundamental problem, that the edebug information is attached
to the symbol `hanoi' rather than to the (instrumented) function which
is no longer the symbol-function of `hanoi'.  Sadly, there is no way of
attaching a property to a function, only to a symbol (or a buffer
position).

There doesn't appear to be a good way of solving this bug.  A workable
workaround would be to check that the 'edebug property value is a list
in edebug--overlay-breakpoints, and just to remove the breakpoint
highlights when it's not a list.  When it's not a list, it's a marker
pointing to the start of the function, from which the end of the
function can be found, to serve as the END argument to
edebug--overlay-breakpoints-remove.

Or something like that.  :-(

> > I use this workflow quite a lot when debugging. I may start to debug a
> > function, decided I have seen enough, to un-instrument so I don't see it
> > debugged again.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Eli Zaretskii]

> Date: Fri, 1 May 2020 17:33:06 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: 40992@debbugs.gnu.org
> 
> There doesn't appear to be a good way of solving this bug.  A workable
> workaround would be to check that the 'edebug property value is a list
> in edebug--overlay-breakpoints, and just to remove the breakpoint
> highlights when it's not a list.  When it's not a list, it's a marker
> pointing to the start of the function, from which the end of the
> function can be found, to serve as the END argument to
> edebug--overlay-breakpoints-remove.
> 
> Or something like that.  :-(

How about if we revert the offending commit on emacs-27?  The feature
it adds doesn't sound important enough when compared to the breakage
it causes.

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> There doesn't appear to be a good way of solving this bug.  A workable
> workaround would be to check that the 'edebug property value is a list
> in edebug--overlay-breakpoints, and just to remove the breakpoint
> highlights when it's not a list.  When it's not a list, it's a marker
> pointing to the start of the function, from which the end of the
> function can be found, to serve as the END argument to
> edebug--overlay-breakpoints-remove.
>
> Or something like that.  :-(

I think that sounds like a good solution on master, but the patch that
introduced this should probably be reverted on emacs-27 -- it wasn't a
bug fix, but a new feature, so reverting it should be safe, I think.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alan Mackenzie <acm@muc.de> writes:
>
>> There doesn't appear to be a good way of solving this bug.  A workable
>> workaround would be to check that the 'edebug property value is a list
>> in edebug--overlay-breakpoints, and just to remove the breakpoint
>> highlights when it's not a list.  When it's not a list, it's a marker
>> pointing to the start of the function, from which the end of the
>> function can be found, to serve as the END argument to
>> edebug--overlay-breakpoints-remove.
>>
>> Or something like that.  :-(
>
> I think that sounds like a good solution on master, but the patch that
> introduced this should probably be reverted on emacs-27 -- it wasn't a
> bug fix, but a new feature, so reverting it should be safe, I think.

I don't understand; the fix looks trivial to me (leaving out
indentation), and only touches a new function.  Surely this is okay for
emacs-27?

--- i/lisp/emacs-lisp/edebug.el
+++ w/lisp/emacs-lisp/edebug.el
@@ -3236,8 +3236,9 @@ 'edebug-breakpoint
   "\x3c\x7e\xff\xff\xff\xff\x7e\x3c")
 
 (defun edebug--overlay-breakpoints (function)
-  (let* ((data (get function 'edebug))
-         (start (nth 0 data))
+  (let ((data (get function 'edebug)))
+    (when (listp data)
+      (let* ((start (nth 0 data))
          (breakpoints (nth 1 data))
          (offsets (nth 2 data)))
     ;; First remove all old breakpoint overlays.
@@ -3264,6 +3265,7 @@ edebug--overlay-breakpoints
                          (propertize
                           "x" 'display
                           `(left-fringe edebug-breakpoint ,face)))))))))
+    ))
 
 (defun edebug--overlay-breakpoints-remove (start end)
   (dolist (overlay (overlays-in start end))

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Fri, 01 May 2020 23:05:32 -0400
> Cc: Alan Mackenzie <acm@muc.de>, 40992@debbugs.gnu.org,
>  Phillip Lord <phillip.lord@russet.org.uk>
> 
> > I think that sounds like a good solution on master, but the patch that
> > introduced this should probably be reverted on emacs-27 -- it wasn't a
> > bug fix, but a new feature, so reverting it should be safe, I think.
> 
> I don't understand; the fix looks trivial to me (leaving out
> indentation), and only touches a new function.  Surely this is okay for
> emacs-27?

If this fixes the problem, it's okay for emacs-27.  But then why did
Alan say there was no easy solution?

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Alan Mackenzie]

Hello, Noam, Eli, Lars, and Phillip.

On Fri, May 01, 2020 at 23:05:32 -0400, Noam Postavsky wrote:
> Lars Ingebrigtsen <larsi@gnus.org> writes:

> > Alan Mackenzie <acm@muc.de> writes:

> >> There doesn't appear to be a good way of solving this bug.  A workable
> >> workaround would be to check that the 'edebug property value is a list
> >> in edebug--overlay-breakpoints, and just to remove the breakpoint
> >> highlights when it's not a list.  When it's not a list, it's a marker
> >> pointing to the start of the function, from which the end of the
> >> function can be found, to serve as the END argument to
> >> edebug--overlay-breakpoints-remove.

> >> Or something like that.  :-(

> > I think that sounds like a good solution on master, but the patch that
> > introduced this should probably be reverted on emacs-27 -- it wasn't a
> > bug fix, but a new feature, so reverting it should be safe, I think.

> I don't understand; the fix looks trivial to me (leaving out
> indentation), and only touches a new function.  Surely this is okay for
> emacs-27?

> --- i/lisp/emacs-lisp/edebug.el
> +++ w/lisp/emacs-lisp/edebug.el
> @@ -3236,8 +3236,9 @@ 'edebug-breakpoint
>    "\x3c\x7e\xff\xff\xff\xff\x7e\x3c")
 
>  (defun edebug--overlay-breakpoints (function)
> -  (let* ((data (get function 'edebug))
> -         (start (nth 0 data))
> +  (let ((data (get function 'edebug)))
> +    (when (listp data)
> +      (let* ((start (nth 0 data))
>           (breakpoints (nth 1 data))
>           (offsets (nth 2 data)))
>      ;; First remove all old breakpoint overlays.
> @@ -3264,6 +3265,7 @@ edebug--overlay-breakpoints
>                           (propertize
>                            "x" 'display
>                            `(left-fringe edebug-breakpoint ,face)))))))))
> +    ))
 
>  (defun edebug--overlay-breakpoints-remove (start end)
>    (dolist (overlay (overlays-in start end))

That function might not remove any existing breakpoint highlights (which
normally would get removed by the call to
edebug--overlay-breakpoints-remove, which takes the value of the 'edebug
property (indirectly) as an argument.

I've tried vaguely to create a situation where the highlights get left
in place, but haven't managed it yet.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Alan Mackenzie]

Hello, Eli.

On Sat, May 02, 2020 at 10:10:04 +0300, Eli Zaretskii wrote:
> > From: Noam Postavsky <npostavs@gmail.com>
> > Date: Fri, 01 May 2020 23:05:32 -0400
> > Cc: Alan Mackenzie <acm@muc.de>, 40992@debbugs.gnu.org,
> >  Phillip Lord <phillip.lord@russet.org.uk>

> > > I think that sounds like a good solution on master, but the patch that
> > > introduced this should probably be reverted on emacs-27 -- it wasn't a
> > > bug fix, but a new feature, so reverting it should be safe, I think.

> > I don't understand; the fix looks trivial to me (leaving out
> > indentation), and only touches a new function.  Surely this is okay for
> > emacs-27?

> If this fixes the problem, it's okay for emacs-27.  But then why did
> Alan say there was no easy solution?

I think I said "good" rather than "easy".  This 'edebug property is
essentially associated with a function, but is in a symbol's property
list.

Noam's fix, I think, will either leave stale highlights on what used to
be breakpoints, or won't leave the highlights active whilst the
debugging is still in progress.  It might be the least bad fix, though.

At the time of debugging, there are _two_ functions associated with
`hanoi', the one just compiled with C-M-x, and the instrumented one
still being debugged (which will soon find its way to the garbage
collector).

There is no way to attach a property list to a function (and it's too
late to add one for Emacs 27 ;-), so prop 'edebug gets on the symbol's
plist, and confusion results.  Hence this bug.

A good solution would fix this confusion.  I can't think of a way to do
this which doesn't involve deep surgery on Emacs's internals.

I think the same mechanism is behind the annoying bug (not yet reported
by me) where you do M-x compile-defun on a previously instrumented
function, then try to instrument it again by typing I
(`edebug-instrument-callee') whilst in Edebug.  You get the spurious
error message "Already instrumented".

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Eli Zaretskii]

> Date: Sat, 2 May 2020 13:34:47 +0000
> Cc: Noam Postavsky <npostavs@gmail.com>, larsi@gnus.org,
>   40992@debbugs.gnu.org, phillip.lord@russet.org.uk
> From: Alan Mackenzie <acm@muc.de>
> 
> Noam's fix, I think, will either leave stale highlights on what used to
> be breakpoints, or won't leave the highlights active whilst the
> debugging is still in progress.  It might be the least bad fix, though.

If Noam's fix is incomplete, I still think reverting the feature on
emacs-27 is a better solution.  We should continue working on this on
master until we find a better solution.

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Date: Fri, 01 May 2020 23:05:32 -0400
>> Cc: Alan Mackenzie <acm@muc.de>, 40992@debbugs.gnu.org,
>>  Phillip Lord <phillip.lord@russet.org.uk>
>> 
>> > I think that sounds like a good solution on master, but the patch that
>> > introduced this should probably be reverted on emacs-27 -- it wasn't a
>> > bug fix, but a new feature, so reverting it should be safe, I think.
>> 
>> I don't understand; the fix looks trivial to me (leaving out
>> indentation), and only touches a new function.  Surely this is okay for
>> emacs-27?
>
> If this fixes the problem, it's okay for emacs-27.  But then why did
> Alan say there was no easy solution?

I guess it's because the edebug should really still be using the overlay
information for the function it is still stepping through.

When I do this, it's because I do not want the function instrumented any
more; I'd probably be just as happy if edebug dropped out at this point
(i.e. stopped stepping through and continued). That I can step through a
function definition that is no longer instrumented is not really a
feature for me.

Not having it error, clearly, is a feature!

Thanks all for looking at this.

Phil

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Alan Mackenzie]

Hello, everybody.

On Sat, May 02, 2020 at 16:57:26 +0300, Eli Zaretskii wrote:
> > Date: Sat, 2 May 2020 13:34:47 +0000
> > Cc: Noam Postavsky <npostavs@gmail.com>, larsi@gnus.org,
> >   40992@debbugs.gnu.org, phillip.lord@russet.org.uk
> > From: Alan Mackenzie <acm@muc.de>

> > Noam's fix, I think, will either leave stale highlights on what used to
> > be breakpoints, or won't leave the highlights active whilst the
> > debugging is still in progress.  It might be the least bad fix, though.

> If Noam's fix is incomplete, I still think reverting the feature on
> emacs-27 is a better solution.  We should continue working on this on
> master until we find a better solution.

OK.  I've reverted the feature in the emacs-27 branch (with a directive
not to merge to master).  This should fix the bug in emacs-27.

I've had two ideas about fixing this bug in master whilst leaving the
breakpoint highlight feature in place:

(i): (Please bear in mind it was ~04:30 when I thought this up. ;-)  To
each type of function (subr, byte-compile, lambda form, closure form) we
add an extra field for a @dfn{function property list}, possibly
optional.  For dumped functions, the contents will be the index into a
vector holding lots of function property lists.  For functions in r/w
storage, we have a property list directly.  We will have functions like
put-function-property, get-function-property,
remove-function-properties.  Having got this infrastructure, edebug will
now store the 'edebug property on this function-property-list, and the
problems we've had with the symbol-plist just evaporate.

(ii) We devise a new (symbol-) property called something like
'ghost-edebug.  When an instrumented function gets recompiled by C-M-x,
etc., the previous contents of 'edebug get moved to 'ghost-edebug, and
'edebug gets a marker, as it currently does.  Whilst manipulating the
breakpoint highlights, edebug will use 'ghost-edebug when 'edebug holds
a marker.  The 'ghost-edebug property will be removed when the `hanoi'
function is next instrumented.  Maybe this will work.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Alan Mackenzie]

Bug fixed in the emacs-27 branch by reverting commit

commit e8b3a15cb6ff187ce08afcb43bd9a0b7907268ca
Author: Lars Ingebrigtsen <larsi@gnus.org>
Date:   Sun Oct 20 12:07:42 2019 +0200

    Mark breakpoints in edebug with highlights

    * lisp/emacs-lisp/edebug.el (edebug--overlay-breakpoints)
    (edebug--overlay-breakpoints-remove): New functions (bug#23468).
    (edebug-modify-breakpoint): Use them to highlight breakpoints.
    (edebug--display-1): Restore breakpoint highlights.
    (edebug--recursive-edit): Remove highlights on exit.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Pouar Dragon]

There's still some code in edebug.el that calls
edebug--overlay-breakpoints-remove and edebug--overlay-breakpoints
-- 
Pouar Dragon
My GPG Keys: https://keybase.io/pouar

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Alan Mackenzie]

Hello, Pouar.

On Sun, May 03, 2020 at 20:27:16 -0500, Pouar Dragon wrote:
> There's still some code in edebug.el that calls
> edebug--overlay-breakpoints-remove and edebug--overlay-breakpoints
> -- 
> Pouar Dragon
> My GPG Keys: https://keybase.io/pouar

Yes, you're right.  I'd neglected to compile the file under emacs -Q, so
didn't spot the errors.  It should be fixed, now.

Sorry about the annoyance this must have caused, and thanks for
reporting it.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#40992: 27.0.90; Evaluating a function while using edebug breaks

[Alan Mackenzie]

Hello, Phillip.

On Sat, May 02, 2020 at 22:54:52 +0100, Phillip Lord wrote:
> Eli Zaretskii <eliz@gnu.org> writes:

> >> From: Noam Postavsky <npostavs@gmail.com>
> >> Date: Fri, 01 May 2020 23:05:32 -0400
> >> Cc: Alan Mackenzie <acm@muc.de>, 40992@debbugs.gnu.org,
> >>  Phillip Lord <phillip.lord@russet.org.uk>

> > If this fixes the problem, it's okay for emacs-27.  But then why did
> > Alan say there was no easy solution?

> I guess it's because the edebug should really still be using the overlay
> information for the function it is still stepping through.

> When I do this, it's because I do not want the function instrumented any
> more; I'd probably be just as happy if edebug dropped out at this point
> (i.e. stopped stepping through and continued). That I can step through a
> function definition that is no longer instrumented is not really a
> feature for me.

> Not having it error, clearly, is a feature!

> Thanks all for looking at this.

I've just committed a fix to master, whereby the information in property
`edebug' is saved in property `ghost-edebug' when the function is
re-evaluated.  `ghost-edebug' is used by most of edebug in place of 
`edebug' when the latter contains a bare marker.

Modulo any bugs in this patch, I think it fixes bug #40992 completely.

> Phil

-- 
Alan Mackenzie (Nuremberg, Germany).