bug#59213: Emacs 29: Edebug fails to instrument a parameter whose name begins with _

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan.

On Mon, Nov 14, 2022 at 07:56:34 -0500, Stefan Monnier wrote:
> > More precisely, with this defun:

> >     (defun add (a b c)
> >       (+ a b))

> > , instrument it for edebug.  Call M-: (add 1 2 6).

> > The source code with active edebug now looks like:

> >     (defun add (a b c)
> >     =>(+ a b))

> > ..  `e a` now returns 1.  `e b` returns 2.  `e c` gives the error message:

> >     Error: Symbol's value as variable is void: c

> > ..  I repeat, this is a bug.  It should have returned 6.

> [ Well, GDB does the same and claims it's not a bug, instead it says the
>   variable has been optimized away or something to that effect.  ]

> Agreed.  Edebug should be careful to prevent unused vars from being
> optimized away.  I'll try and come up with a good patch for that,

I've been looking at this the past few days (actually, many days), and
now understand what's happening.

With an `add' instrumented for edebug, and evaluating `add', this causes
edebug to create the form beginning "(function ...".  Ffunction in eval.c
delegates the creation of a closure to cconv-make-interpreted-closure.
That function analyses `add', decides that c is not used, and thus
creates a lexical environment containing bindings only for a and b.

This last is the error.  When instrumenting for edebug, EVERY lexical
variable is potentially going to be read, so
cconv-make-interpreted-closure should not remove any elements from the
lexical environment.

The included patch fixes this: edebug binds the (new) variable
cconv-dont-trim-unused-variables to non-nil around the generated calls to
edebug-enter.  cconv-make-interpreted-closure tests this variable, and
when non-nil it copies the lexical environment without change.

Also, there's a consequential change in testcover.el, where it analyses
the forms it is instrumenting, and needs to handle the new code around
edebug-enter.

This works.

What do you think?



diff --git a/lisp/emacs-lisp/cconv.el b/lisp/emacs-lisp/cconv.el
index 570c9e66060..d61ff221ecb 100644
--- a/lisp/emacs-lisp/cconv.el
+++ b/lisp/emacs-lisp/cconv.el
@@ -113,6 +113,10 @@ cconv--interactive-form-funs
 (defvar cconv--dynbound-variables nil
   "List of variables known to be dynamically bound.")
 
+(defvar cconv-dont-trim-unused-variables nil
+  "When bound to non-nil, don't remove unused variables from the environment.
+This is intended for use by edebug and similar.")
+
 ;;;###autoload
 (defun cconv-closure-convert (form &optional dynbound-vars)
   "Main entry point for closure conversion.
@@ -834,10 +838,13 @@ cconv-analyze-form
 (define-obsolete-function-alias 'cconv-analyse-form #'cconv-analyze-form "25.1")
 
 (defun cconv-fv (form lexvars dynvars)
-  "Return the list of free variables in FORM.
-LEXVARS is the list of statically scoped vars in the context
-and DYNVARS is the list of dynamically scoped vars in the context.
-Returns a pair (LEXV . DYNV) of those vars actually used by FORM."
+  "Return the free variables used in FORM.
+FORM is usually a function #\\='(lambda ...), but may be any valid
+form.  LEXVARS is a list of symbols, each of which is lexically
+bound in FORM's context.  DYNVARS is a list of symbols, each of
+which is dynamically bound in FORM's context.
+Returns a cons (LEXV . DYNV), the car and cdr being lists of the
+lexically and dynamically bound symbols actually used by FORM."
   (let* ((fun
           ;; Wrap FORM into a function because the analysis code we
           ;; have only computes freevars for functions.
@@ -875,15 +882,24 @@ cconv-fv
         (cons fvs dyns)))))
 
 (defun cconv-make-interpreted-closure (fun env)
+  "Make a closure for the interpreter.
+This function is evaluated both at compile time and run time.
+FUN, the closure's function, must be a lambda form.
+ENV, the closure's environment, is a mixture of lexical bindings of the form
+(SYMBOL . VALUE) and symbols which indicate dynamic bindings of those
+symbols."
   (cl-assert (eq (car-safe fun) 'lambda))
   (let ((lexvars (delq nil (mapcar #'car-safe env))))
     (if (null lexvars)
         ;; The lexical environment is empty, so there's no need to
         ;; look for free variables.
+        ;; Attempting to replace ,(cdr fun) by a macroexpanded version
+        ;; causes bootstrap to fail.
         `(closure ,env . ,(cdr fun))
       ;; We could try and cache the result of the macroexpansion and
       ;; `cconv-fv' analysis.  Not sure it's worth the trouble.
-      (let* ((form `#',fun)
+      (let* (newenv
+             (form `#',fun)
              (expanded-form
               (let ((lexical-binding t) ;; Tell macros which dialect is in use.
 	            ;; Make the macro aware of any defvar declarations in scope.
@@ -896,11 +912,14 @@ cconv-make-interpreted-closure
               (pcase expanded-form
                 (`#'(lambda . ,cdr) cdr)
                 (_ (cdr fun))))
-         
-             (dynvars (delq nil (mapcar (lambda (b) (if (symbolp b) b)) env)))
-             (fvs (cconv-fv expanded-form lexvars dynvars))
-             (newenv (nconc (mapcar (lambda (fv) (assq fv env)) (car fvs))
-                            (cdr fvs))))
+
+             (dynvars (delq nil (mapcar (lambda (b) (if (symbolp b) b)) env))))
+        (if cconv-dont-trim-unused-variables
+            (setq newenv (copy-alist env))
+          (let ((fvs (cconv-fv expanded-form lexvars dynvars)))
+            (setq newenv
+                  (nconc (mapcar (lambda (fv) (assq fv env)) (car fvs))
+                         (cdr fvs)))))
         ;; Never return a nil env, since nil means to use the dynbind
         ;; dialect of ELisp.
         `(closure ,(or newenv '(t)) . ,expanded-fun-cdr)))))
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 2f7d03e9d79..735a358cdba 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1217,16 +1217,16 @@ edebug-make-enter-wrapper
     (setq edebug-old-def-name nil))
   (setq edebug-def-name
 	(or edebug-def-name edebug-old-def-name (gensym "edebug-anon")))
-  `(edebug-enter
-    (quote ,edebug-def-name)
-    ,(if edebug-inside-func
-	 `(list
-	   ;; Doesn't work with more than one def-body!!
-	   ;; But the list will just be reversed.
-	   ,@(nreverse edebug-def-args))
-       'nil)
-    (function (lambda () ,@forms))
-    ))
+  `(let ((cconv-dont-trim-unused-variables t))
+     (edebug-enter
+      (quote ,edebug-def-name)
+      ,(if edebug-inside-func
+	   `(list
+	     ;; Doesn't work with more than one def-body!!
+	     ;; But the list will just be reversed.
+	     ,@(nreverse edebug-def-args))
+         'nil)
+      (function (lambda () ,@forms)))))
 
 
 (defvar edebug-form-begin-marker) ; the mark for def being instrumented
diff --git a/lisp/emacs-lisp/testcover.el b/lisp/emacs-lisp/testcover.el
index ed31b90ca32..1212905f08a 100644
--- a/lisp/emacs-lisp/testcover.el
+++ b/lisp/emacs-lisp/testcover.el
@@ -442,6 +442,11 @@ testcover-analyze-coverage
      (let ((testcover-vector (get sym 'edebug-coverage)))
        (testcover-analyze-coverage-progn body)))
 
+    (`(let ((cconv-dont-trim-unused-variables t))
+        (edebug-enter ',sym ,_ (function (lambda nil . ,body))))
+     (let ((testcover-vector (get sym 'edebug-coverage)))
+       (testcover-analyze-coverage-progn body)))
+
     (`(edebug-after ,(and before-form
                           (or `(edebug-before ,before-id) before-id))
                     ,after-id ,wrapped-form)


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).