From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.bugs Subject: bug#59213: Emacs 29: Edebug fails to instrument a parameter whose name begins with _ Date: Fri, 10 Feb 2023 18:51:37 +0000 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="5082"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 59213@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Feb 10 19:52:11 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pQYVG-0001AT-UO for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 10 Feb 2023 19:52:11 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pQYVA-0005do-LW; Fri, 10 Feb 2023 13:52:04 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pQYV8-0005dd-TQ for bug-gnu-emacs@gnu.org; Fri, 10 Feb 2023 13:52:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1pQYV8-00015Y-L5 for bug-gnu-emacs@gnu.org; Fri, 10 Feb 2023 13:52:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pQYV8-0004fg-GW for bug-gnu-emacs@gnu.org; Fri, 10 Feb 2023 13:52:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Alan Mackenzie Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 10 Feb 2023 18:52:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 59213 X-GNU-PR-Package: emacs Original-Received: via spool by 59213-submit@debbugs.gnu.org id=B59213.167605510817935 (code B ref 59213); Fri, 10 Feb 2023 18:52:02 +0000 Original-Received: (at 59213) by debbugs.gnu.org; 10 Feb 2023 18:51:48 +0000 Original-Received: from localhost ([127.0.0.1]:38142 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pQYUu-0004fC-4G for submit@debbugs.gnu.org; Fri, 10 Feb 2023 13:51:48 -0500 Original-Received: from mx3.muc.de ([193.149.48.5]:61624) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pQYUr-0004eu-EE for 59213@debbugs.gnu.org; Fri, 10 Feb 2023 13:51:47 -0500 Original-Received: (qmail 49287 invoked by uid 3782); 10 Feb 2023 19:51:38 +0100 Original-Received: from acm.muc.de (pd953a764.dip0.t-ipconnect.de [217.83.167.100]) (using STARTTLS) by colin.muc.de (tmda-ofmipd) with ESMTP; Fri, 10 Feb 2023 19:51:38 +0100 Original-Received: (qmail 29759 invoked by uid 1000); 10 Feb 2023 18:51:37 -0000 Content-Disposition: inline In-Reply-To: X-Submission-Agent: TMDA/1.3.x (Ph3nix) X-Primary-Address: acm@muc.de X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:255303 Archived-At: 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).