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

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

[Alan Mackenzie]

Hello, Emacs.

In Emacs 29 (not started with -Q, but...),

I instrumented for edebug a function which looked like:

    (defun c-trim-found-types (beg end _old-len) ....)

, the compilation being with lexical-binding: t.

During the edebug session, I attempted

    e _old-len RET.

Instead of giving me the value of _old-len (which was 3) it gave the
error message

    Error: Symbol's value as variable is void: _old-len

..  This is a bug.

Just because a function doesn't use a particular argument (here
_old-len) doesn't mean the person debugging it isn't interested in its
value.  In this particular case, it was extremely interesting, because
beg and end were unequal, and _old-len was 3.

In the end, I found out the info with the d command (backtrace), but I
shouldn't have to.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Michael Heerdegen]

Alan Mackenzie <acm@muc.de> writes:

> During the edebug session, I attempted
>
>     e _old-len RET.
>
> Instead of giving me the value of _old-len (which was 3) it gave the
> error message
>
>     Error: Symbol's value as variable is void: _old-len

Here is a test case:

(defun test (a b _c)
  (+ a b))

Instrument, etc., and e _c RET gives you the void variable error.

I don't think this behavior is intended.

When I change

(defun edebug-eval (expr)
  (backtrace-eval expr 0 'edebug-after))

to

(defun edebug-eval (expr)
  (backtrace-eval expr 1 'edebug-after))
;;                     ^

a binding of _c is being printed.  Let's CC Stefan who is the real
expert and ask whether this change would make sense.  I'm not sure when
and in which frames a binding is or should be visible.  Note: when _c is
used in the function body a binding _is_ visible also in the 0th frame.

Michael.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> In Emacs 29 (not started with -Q, but...),
>
> I instrumented for edebug a function which looked like:
>
>     (defun c-trim-found-types (beg end _old-len) ....)
>
> , the compilation being with lexical-binding: t.
>
> During the edebug session, I attempted
>
>     e _old-len RET.

The behavior depends on where you are in the *Backtrace* buffer, because
each line in the backtrace can be in a different lexical scope.
So please clarify on which line you were when you did the above.

> Instead of giving me the value of _old-len (which was 3) it gave the
> error message
>
>     Error: Symbol's value as variable is void: _old-len
>
> ..  This is a bug.

Could be.  Or could be that you were trying to use `_old-len` in
a lexical context where there is no such variable.

> Just because a function doesn't use a particular argument (here

I think the leading underscore is purely incidental and you'd get the
same behavior with `beg` and `end`.


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Sun, Nov 13, 2022 at 22:53:12 -0500, Stefan Monnier wrote:
> > In Emacs 29 (not started with -Q, but...),

> > I instrumented for edebug a function which looked like:

> >     (defun c-trim-found-types (beg end _old-len) ....)

> > , the compilation being with lexical-binding: t.

> > During the edebug session, I attempted

> >     e _old-len RET.

> The behavior depends on where you are in the *Backtrace* buffer, because
> each line in the backtrace can be in a different lexical scope.
> So please clarify on which line you were when you did the above.

I wasn't in the backtrace.  I was stepping through the code in edebug.

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.

> > Instead of giving me the value of _old-len (which was 3) it gave the
> > error message

> >     Error: Symbol's value as variable is void: _old-len

> > ..  This is a bug.

> Could be.  Or could be that you were trying to use `_old-len` in
> a lexical context where there is no such variable.

_c is in the same lexical context as a and b, surely?

> > Just because a function doesn't use a particular argument (here

> I think the leading underscore is purely incidental and you'd get the
> same behavior with `beg` and `end`.

No, beg and end returned their values.  I admit I'm speculating about the
leading underscore, but I still say there's a bug, somewhere here.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> The behavior depends on where you are in the *Backtrace* buffer, because
>> each line in the backtrace can be in a different lexical scope.
>> So please clarify on which line you were when you did the above.
>
> I wasn't in the backtrace.  I was stepping through the code in edebug.

Oh, duh!  I was misaligned, sorry.  Let me reboot myself.  I'll be back shortly.


        Stefan

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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,


        Stefan

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

[Eli Zaretskii]

> Cc: 59213@debbugs.gnu.org
> Date: Sun, 13 Nov 2022 22:53:12 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > In Emacs 29 (not started with -Q, but...),
> >
> > I instrumented for edebug a function which looked like:
> >
> >     (defun c-trim-found-types (beg end _old-len) ....)
> >
> > , the compilation being with lexical-binding: t.
> >
> > During the edebug session, I attempted
> >
> >     e _old-len RET.
> 
> The behavior depends on where you are in the *Backtrace* buffer, because
> each line in the backtrace can be in a different lexical scope.
> So please clarify on which line you were when you did the above.
> 
> > Instead of giving me the value of _old-len (which was 3) it gave the
> > error message
> >
> >     Error: Symbol's value as variable is void: _old-len
> >
> > ..  This is a bug.
> 
> Could be.  Or could be that you were trying to use `_old-len` in
> a lexical context where there is no such variable.

Could you please document this in the "Edebug Eval" node of the ELisp
reference manual?  I don't think the text there has any hints about
this subtle aspect.  It should also be mentioned in "Backtraces" and
in "Debugger Commands" (the latter says something vague about this,
but it's too vague, IMO).

TIA

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

[Alan Mackenzie]

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).

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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.

Yup.

> 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?

I think that's good enough for `emacs-29`, yes.
I don't like the use of a dynbound variable to control this, but it's
not clear how to do better.  One thing that occurred to me right now is
that we could mark the Edebug closures themselves, e.g. by replacing

    (function (lambda () ,@forms))

with

    (function (lambda () :closure-dont-trim-context ,@forms))

and then have `cconv-make-interpreted-closure` look for this
tell-tale sign.

A few minor comments about your patch below.

> +(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.")

This variable name sounds like it controls the closure conversion code
(e.g. `cconv-convert`).  I don't have a better suggestion, tho :-(

> @@ -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)))))

Hmm... any reason why we can't just replace

    (if (null lexvars)

with

    (if (or cconv-dont-trim-unused-variables (null lexvars))

and be done with it?


        Stefan

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

[Eli Zaretskii]

> Cc: 59213@debbugs.gnu.org
> Date: Fri, 10 Feb 2023 17:05:32 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I think that's good enough for `emacs-29`, yes.

Why do we have to fix this in Emacs 29?  The problem doesn't sound
very important to me: variables that are ignored should not be a
subject of scrutiny in Edebug too frequently, and I cannot even
imagine a use case where it would be important for debugging.

So I'd prefer to fix this on master instead.

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

[Alan Mackenzie]

Hello, Stefan.

On Fri, Feb 10, 2023 at 17:05:32 -0500, Stefan Monnier wrote:

[ .... ]

> > What do you think?

> I think that's good enough for `emacs-29`, yes.

I've committed the fix (with modifications) to master, as requested by
Eli, and I'm closing the bug with this post.

> I don't like the use of a dynbound variable to control this, but it's
> not clear how to do better.  One thing that occurred to me right now is
> that we could mark the Edebug closures themselves, e.g. by replacing

>     (function (lambda () ,@forms))

> with

>     (function (lambda () :closure-dont-trim-context ,@forms))

> and then have `cconv-make-interpreted-closure` look for this
> tell-tale sign.

Interesting.  Isn't there a good chance this would foul up programs which
analyse the structure of a closure?  (A bit like
testcover-analyze-coverage analyses the calling structure of
edebug-enter).

> A few minor comments about your patch below.

[ .... ]

> Hmm... any reason why we can't just replace

>     (if (null lexvars)

> with

>     (if (or cconv-dont-trim-unused-variables (null lexvars))

> and be done with it?

No, no reason at all.  I think I was concerned to preserve the macro
expansion, but seeing as how that wouldn't even be in the uninstrumented
form, this is clearly not a valid concern.  So I modified the patch to do
that before committing it.

Thanks!

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I think that's good enough for `emacs-29`, yes.
>
> Why do we have to fix this in Emacs 29?  The problem doesn't sound
> very important to me: variables that are ignored should not be a
> subject of scrutiny in Edebug too frequently, and I cannot even
> imagine a use case where it would be important for debugging.
>
> So I'd prefer to fix this on master instead.

The reason I was thinking of `emacs-29` was that it's a regression in
Emacs-29.  But I agree that it's not super important, so it can go to
`master` (we may want to try and remember to backport it for
Emacs-29.2, tho).


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: acm@muc.de,  59213@debbugs.gnu.org
> Date: Sun, 12 Feb 2023 22:26:55 -0500
> 
> >> I think that's good enough for `emacs-29`, yes.
> >
> > Why do we have to fix this in Emacs 29?  The problem doesn't sound
> > very important to me: variables that are ignored should not be a
> > subject of scrutiny in Edebug too frequently, and I cannot even
> > imagine a use case where it would be important for debugging.
> >
> > So I'd prefer to fix this on master instead.
> 
> The reason I was thinking of `emacs-29` was that it's a regression in
> Emacs-29.  But I agree that it's not super important, so it can go to
> `master` (we may want to try and remember to backport it for
> Emacs-29.2, tho).

Right.  Emacs 29.1 will have enough exciting new regressions anyway.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>     (function (lambda () ,@forms))
>> with
>>     (function (lambda () :closure-dont-trim-context ,@forms))
>> and then have `cconv-make-interpreted-closure` look for this
>> tell-tale sign.
> Interesting.  Isn't there a good chance this would foul up programs which
> analyse the structure of a closure?

It's possible, indeed.  Such programs should be quite rare, tho.
I'd expect packages which analyse such code would either:
A) be Edebug-specific and would have to handle any values of `forms`, in
   which case having `:closure-dont-trim-context` prepended to it would
   not affect them.
B) be non-Edebug specific in which case they should be looking for
   a specific pattern somewhere inside `forms` and they should presumably
   not find the above transformation worse than the other transformations
   applied by Edebug in general.

> (A bit like testcover-analyze-coverage analyses the calling structure
> of edebug-enter).

Indeed, it's only such code I have found so far.  I know there are other
tools out there that use Edebug's instrumentation in creative ways, but
I can't remember where they are.

And, AFAICT in the case of `testcover.el`, the above suggestion would
"just work" (fall into case (A)) whereas your approach required a tweak.

I'll try it on `master`.


        Stefan

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Alan,

>  (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."

BTW, what did you mean by "This function is evaluated both at compile
time and run time"?

Also, I append the current state of the patch I plan to install on
`master`.


        Stefan


diff --git a/lisp/emacs-lisp/cconv.el b/lisp/emacs-lisp/cconv.el
index b8121aeba55..d055026cb1a 100644
--- a/lisp/emacs-lisp/cconv.el
+++ b/lisp/emacs-lisp/cconv.el
@@ -113,10 +113,6 @@ 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.
@@ -886,11 +882,16 @@ cconv-make-interpreted-closure
 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
+\(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 (or cconv-dont-trim-unused-variables (null lexvars))
+    (if (or (null lexvars)
+            ;; Functions of the form (lambda (..) :closure-dont-trim-context ..)
+            ;; should keep their whole context untrimmed (bug#59213).
+            (and (eq :closure-dont-trim-context (nth 2 fun))
+                 ;; Check the function doesn't just return the magic keyword.
+                 (nthcdr 3 fun)))
         ;; The lexical environment is empty, or needs to be preserved,
         ;; so there's no need to look for free variables.
         ;; Attempting to replace ,(cdr fun) by a macroexpanded version
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 735a358cdba..4b625dd076e 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1217,16 +1217,17 @@ 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")))
-  `(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)))))
+  `(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)
+    ;; Make sure `forms' is not nil so we don't accidentally return
+    ;; the magic keyword.
+    #'(lambda () :closure-dont-trim-context ,@(or forms '(nil)))))
 
 
 (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 1212905f08a..ed31b90ca32 100644
--- a/lisp/emacs-lisp/testcover.el
+++ b/lisp/emacs-lisp/testcover.el
@@ -442,11 +442,6 @@ 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)
diff --git a/test/lisp/emacs-lisp/cconv-tests.el b/test/lisp/emacs-lisp/cconv-tests.el
index 83013cf46a9..349ffeb7e47 100644
--- a/test/lisp/emacs-lisp/cconv-tests.el
+++ b/test/lisp/emacs-lisp/cconv-tests.el
@@ -364,5 +364,18 @@ cconv-tests--intern-all
                            (call-interactively f))
                      '((t 51696) (nil 51695) (t 51697)))))))
 
+(ert-deftest cconv-safe-for-space ()
+  (let* ((magic-string "This-is-a-magic-string")
+         (safe-p (lambda (x) (not (string-match magic-string (format "%S" x))))))
+    (should (funcall safe-p (lambda (x) (+ x 1))))
+    (should (funcall safe-p (eval '(lambda (x) (+ x 1))
+                                  `((y . ,magic-string)))))
+    (should (funcall safe-p (eval '(lambda (x) :closure-dont-trim-context)
+                                  `((y . ,magic-string)))))
+    (should-not (funcall safe-p
+                         (eval '(lambda (x) :closure-dont-trim-context (+ x 1))
+                               `((y . ,magic-string)))))))
+
+
 (provide 'cconv-tests)
 ;;; cconv-tests.el ends here

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>  (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."
>
> BTW, what did you mean by "This function is evaluated both at compile
> time and run time"?

I still have no idea what you mean by that.  I just a put a FIXME for
that in the mean time.

> Also, I append the current state of the patch I plan to install on
> `master`.

Pushed.


        Stefan

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

[Alan Mackenzie]

Hello, Stefan,

Sorry you needed to ping me to get an answer to this.

On Tue, Feb 14, 2023 at 17:19:12 -0500, Stefan Monnier wrote:
> Hi Alan,

> >  (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."

> BTW, what did you mean by "This function is evaluated both at compile
> time and run time"?

It was my state of gradually diminishing confusion (which started at a
high level) as I worked through the bug.  It took me some while before it
occurred to me that the creation of closures was happening at run time,
not compile time.  Of course, it's got to, because the lexical
environment only exists at run time.  But I would have got through all
this faster if there had been a comment saying
cconv-make-interpreted-closure is called at run time.  So I put such a
comment in for the next highly confused hacker.

As for the "at compile time", I saw this happening whilst I was running
things in gdb.  I'm not actually sure about this anymore, since these
calls to c-m-i-closure might well have been run time for bits of the
compiler, not compile time.  So maybe the words "both compile time and"
should be removed.  What do you say?

> Also, I append the current state of the patch I plan to install on
> `master`.

Thanks.  I'm not sure what advantages this approach has, but it certainly
gets rid of the ugly cconv-dont-trim-unused-variables.  I'm sure it will
work, too, which is the main thing.

>         Stefan


> diff --git a/lisp/emacs-lisp/cconv.el b/lisp/emacs-lisp/cconv.el
> index b8121aeba55..d055026cb1a 100644
> --- a/lisp/emacs-lisp/cconv.el
> +++ b/lisp/emacs-lisp/cconv.el
> @@ -113,10 +113,6 @@ 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.
> @@ -886,11 +882,16 @@ cconv-make-interpreted-closure
>  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
> +\(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 (or cconv-dont-trim-unused-variables (null lexvars))
> +    (if (or (null lexvars)
> +            ;; Functions of the form (lambda (..) :closure-dont-trim-context ..)
> +            ;; should keep their whole context untrimmed (bug#59213).
> +            (and (eq :closure-dont-trim-context (nth 2 fun))
> +                 ;; Check the function doesn't just return the magic keyword.
> +                 (nthcdr 3 fun)))
>          ;; The lexical environment is empty, or needs to be preserved,
>          ;; so there's no need to look for free variables.
>          ;; Attempting to replace ,(cdr fun) by a macroexpanded version
> diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
> index 735a358cdba..4b625dd076e 100644
> --- a/lisp/emacs-lisp/edebug.el
> +++ b/lisp/emacs-lisp/edebug.el
> @@ -1217,16 +1217,17 @@ 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")))
> -  `(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)))))
> +  `(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)
> +    ;; Make sure `forms' is not nil so we don't accidentally return
> +    ;; the magic keyword.
> +    #'(lambda () :closure-dont-trim-context ,@(or forms '(nil)))))
 
 
>  (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 1212905f08a..ed31b90ca32 100644
> --- a/lisp/emacs-lisp/testcover.el
> +++ b/lisp/emacs-lisp/testcover.el
> @@ -442,11 +442,6 @@ 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)
> diff --git a/test/lisp/emacs-lisp/cconv-tests.el b/test/lisp/emacs-lisp/cconv-tests.el
> index 83013cf46a9..349ffeb7e47 100644
> --- a/test/lisp/emacs-lisp/cconv-tests.el
> +++ b/test/lisp/emacs-lisp/cconv-tests.el
> @@ -364,5 +364,18 @@ cconv-tests--intern-all
>                             (call-interactively f))
>                       '((t 51696) (nil 51695) (t 51697)))))))
 
> +(ert-deftest cconv-safe-for-space ()
> +  (let* ((magic-string "This-is-a-magic-string")
> +         (safe-p (lambda (x) (not (string-match magic-string (format "%S" x))))))
> +    (should (funcall safe-p (lambda (x) (+ x 1))))
> +    (should (funcall safe-p (eval '(lambda (x) (+ x 1))
> +                                  `((y . ,magic-string)))))
> +    (should (funcall safe-p (eval '(lambda (x) :closure-dont-trim-context)
> +                                  `((y . ,magic-string)))))
> +    (should-not (funcall safe-p
> +                         (eval '(lambda (x) :closure-dont-trim-context (+ x 1))
> +                               `((y . ,magic-string)))))))
> +
> +
>  (provide 'cconv-tests)
>  ;;; cconv-tests.el ends here

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> BTW, what did you mean by "This function is evaluated both at compile
>> time and run time"?
>
> It was my state of gradually diminishing confusion (which started at a
> high level) as I worked through the bug.  It took me some while before it
> occurred to me that the creation of closures was happening at run time,
> not compile time.  Of course, it's got to, because the lexical
> environment only exists at run time.  But I would have got through all
> this faster if there had been a comment saying
> cconv-make-interpreted-closure is called at run time.  So I put such a
> comment in for the next highly confused hacker.

Thanks.  I found that part more confusing, so I reworked it in
a way that I hope doesn't make it too much worse.

> As for the "at compile time", I saw this happening whilst I was running
> things in gdb.  I'm not actually sure about this anymore, since these
> calls to c-m-i-closure might well have been run time for bits of the
> compiler, not compile time.

That'd be my guess, yes.  Tho it can also be due to execution of (not
yet byte-compiled) macros.

> Thanks.  I'm not sure what advantages this approach has, but it certainly
> gets rid of the ugly cconv-dont-trim-unused-variables.

Getting rid of the dynamically scoped `cconv-dont-trim-unused-variables`
is the whole point.

In theory it's also "more correct" since it marks the actual lambdas for
which we care to preserve the full environment, regardless of *when*
we'll manipulate it.  E.g. if we ever get to byte-compile the
Edebug-instrumented code, the compiler will get to see this marker so it
can do the right thing, whereas it wouldn't see the
`cconv-dont-trim-unused-variables` because that war was only active when
the code was actually executed [ Of course, this is moot in practice
since the byte-compiler currently doesn't know what to do with this
marker.  ]


        Stefan