From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#59213: Emacs 29: Edebug fails to instrument a parameter whose name begins with _ Date: Fri, 10 Feb 2023 17:05:32 -0500 Message-ID: References: Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="19734"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 59213@debbugs.gnu.org To: Alan Mackenzie Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Feb 10 23:06:15 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 1pQbX4-0004us-Lk for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 10 Feb 2023 23:06:15 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pQbWu-0004uz-S9; Fri, 10 Feb 2023 17:06: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 1pQbWs-0004up-Jb for bug-gnu-emacs@gnu.org; Fri, 10 Feb 2023 17:06:03 -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 1pQbWs-0006HM-9a for bug-gnu-emacs@gnu.org; Fri, 10 Feb 2023 17:06:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pQbWs-00017y-5N for bug-gnu-emacs@gnu.org; Fri, 10 Feb 2023 17:06:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 10 Feb 2023 22:06: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.16760667464308 (code B ref 59213); Fri, 10 Feb 2023 22:06:02 +0000 Original-Received: (at 59213) by debbugs.gnu.org; 10 Feb 2023 22:05:46 +0000 Original-Received: from localhost ([127.0.0.1]:38242 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pQbWb-00017P-H0 for submit@debbugs.gnu.org; Fri, 10 Feb 2023 17:05:45 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:47636) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pQbWZ-000179-1Q for 59213@debbugs.gnu.org; Fri, 10 Feb 2023 17:05:44 -0500 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id B7F3D4414EE; Fri, 10 Feb 2023 17:05:36 -0500 (EST) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id B6F954414D4; Fri, 10 Feb 2023 17:05:34 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1676066734; bh=kNFrHUiaXzzddNTmiXGHeIQ4DgrhNPXREmUmCCSUxS4=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=hBBkaqqvkH2whDjsT2xbHBRFVkuapEX5oxTmThjBe9XimqBPu0mXZwnGxBMX8bDnv mR3XKPj83qbBNIqwuG4Yb+FH206LR/v/cB1pdEYLKTyuv5PhBHHMOguLFSeldLmsIT rnx3sTvcvtB8kdWIHirITiPMGAo+RT2U9usmKzQjnmjqPPXDy8yYPZaYnExV0qN9o9 +hCcAaIG7ZM5LQKFUfeH8LAcqQgfx6YM2ENtaXNZuPiERoXiqxkElNbcX9AVVulkV+ aS8BMXzkA4Y+yhSGng5fmtD8W/QZDh/esSDeEt9bPv/TCETcEm4gsc73q7zgj3oTUX GZ2v7XpRS8azg== Original-Received: from pastel (unknown [104.247.245.112]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 5DDE1120334; Fri, 10 Feb 2023 17:05:34 -0500 (EST) In-Reply-To: (Alan Mackenzie's message of "Fri, 10 Feb 2023 18:51:37 +0000") 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:255305 Archived-At: > 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