bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Alan Mackenzie]

Hello again, Allen, hello, Gemini.

On Wed, Oct 31, 2018 at 20:53:25 -0700, Allen Li wrote:
> On Wed, Oct 31, 2018 at 8:06 AM Alan Mackenzie <acm@muc.de> wrote:

> > Yes.  There is no handling for closures in 26.1's edebug.

> > Handling for closures was added to the Emacs master branch on 2018-06-19
> > by Gemini Lasswell, as an incidental enhancement when introducing better
> > backtrace handling.  Your scenario above works fine in master.

> > This fix could probably be backported to the emacs-26 branch, but how
> > important is it?  What were you actually trying to do when you uncovered
> > this bug?

> I don't remember.  I have encountered this error multiple times before
> and given up debugging something.  Since one generally runs into bugs
> when doing work (that is not hacking on Emacs itself), it's helpful to
> be able to debug with minimal obstruction, otherwise it becomes an
> unjustifiable distraction from the main task.

OK, it sounds like it's a major problem.  I've extracted the pertinent
part of Gemini's patch and it appears to fix your test case in Emacs
26.1.

@Gemini: The patch below is a (small) portion of
e09120d68694272ea5efbe13b16936b4382389d8 from 2018-06-19, the commit that
introduced Backtrace Mode.  Can you see anything against applying this
commit to the emacs-26 branch to fix this bug?

> I'll try to get a master build working again, but it's always a balance 
> between
> instability, getting new bugfixes, and getting actual work done (which
> unfortunately
> doesn't include hacking on Emacs for me).

Could you try this patch out, please, and let me know whether or not it
fixes the bug properly.  Thanks!



--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -206,8 +207,7 @@ edebug-unwrap-results
   "Non-nil if Edebug should unwrap results of expressions.
 That is, Edebug will try to remove its own instrumentation from the result.
 This is useful when debugging macros where the results of expressions
-are instrumented expressions.  But don't do this when results might be
-circular or an infinite loop will result."
+are instrumented expressions."
   :type 'boolean
   :group 'edebug)
 
@@ -1265,25 +1265,59 @@ edebug-make-after-form
 (defun edebug-unwrap (sexp)
   "Return the unwrapped SEXP or return it as is if it is not wrapped.
 The SEXP might be the result of wrapping a body, which is a list of
-expressions; a `progn' form will be returned enclosing these forms."
-  (if (consp sexp)
-      (cond
-       ((eq 'edebug-after (car sexp))
-       (nth 3 sexp))
-       ((eq 'edebug-enter (car sexp))
-        (macroexp-progn (nthcdr 2 (nth 1 (nth 3 sexp)))))
-       (t sexp);; otherwise it is not wrapped, so just return it.
-       )
-    sexp))
+expressions; a `progn' form will be returned enclosing these forms.
+Does not unwrap inside vectors, records, structures, or hash tables."
+  (pcase sexp
+    (`(edebug-after ,_before-form ,_after-index ,form)
+     form)
+    (`(lambda ,args (edebug-enter ',_sym ,_arglist
+                                  (function (lambda nil . ,body))))
+     `(lambda ,args ,@body))
+    (`(closure ,env ,args (edebug-enter ',_sym ,_arglist
+                                        (function (lambda nil . ,body))))
+     `(closure ,env ,args ,@body))
+    (`(edebug-enter ',_sym ,_args (function (lambda nil . ,body)))
+     (macroexp-progn body))
+    (_ sexp)))
 
 (defun edebug-unwrap* (sexp)
   "Return the SEXP recursively unwrapped."
+  (let ((ht (make-hash-table :test 'eq)))
+    (edebug--unwrap1 sexp ht)))
+
+(defun edebug--unwrap1 (sexp hash-table)
+  "Unwrap SEXP using HASH-TABLE of things already unwrapped.
+HASH-TABLE contains the results of unwrapping cons cells within
+SEXP, which are reused to avoid infinite loops when SEXP is or
+contains a circular object."
   (let ((new-sexp (edebug-unwrap sexp)))
     (while (not (eq sexp new-sexp))
       (setq sexp new-sexp
            new-sexp (edebug-unwrap sexp)))
     (if (consp new-sexp)
-       (mapcar #'edebug-unwrap* new-sexp)
+       (let ((result (gethash new-sexp hash-table nil)))
+         (unless result
+           (let ((remainder new-sexp)
+                 current)
+             (setq result (cons nil nil)
+                   current result)
+             (while
+                 (progn
+                   (puthash remainder current hash-table)
+                   (setf (car current)
+                         (edebug--unwrap1 (car remainder) hash-table))
+                   (setq remainder (cdr remainder))
+                   (cond
+                    ((atom remainder)
+                     (setf (cdr current)
+                           (edebug--unwrap1 remainder hash-table))
+                     nil)
+                    ((gethash remainder hash-table nil)
+                     (setf (cdr current) (gethash remainder hash-table nil))
+                     nil)
+                    (t (setq current
+                             (setf (cdr current) (cons nil nil)))))))))
+         result)
       new-sexp)))
  
  


-- 
Alan Mackenzie (Nuremberg, Germany).