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: Sat, 18 Feb 2023 18:46:47 +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="7353"; 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 Sat Feb 18 19:47:44 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 1pTSFL-0001mj-OG for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 18 Feb 2023 19:47:43 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pTSEz-0008PU-1g; Sat, 18 Feb 2023 13:47:21 -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 1pTSEg-0008NU-Ng for bug-gnu-emacs@gnu.org; Sat, 18 Feb 2023 13:47:07 -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 1pTSEg-00077y-El for bug-gnu-emacs@gnu.org; Sat, 18 Feb 2023 13:47:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pTSEf-0006lQ-V2 for bug-gnu-emacs@gnu.org; Sat, 18 Feb 2023 13:47: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: Sat, 18 Feb 2023 18:47:01 +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.167674601825990 (code B ref 59213); Sat, 18 Feb 2023 18:47:01 +0000 Original-Received: (at 59213) by debbugs.gnu.org; 18 Feb 2023 18:46:58 +0000 Original-Received: from localhost ([127.0.0.1]:44957 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pTSEc-0006l6-6C for submit@debbugs.gnu.org; Sat, 18 Feb 2023 13:46:58 -0500 Original-Received: from mx3.muc.de ([193.149.48.5]:13289) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pTSEZ-0006kl-W4 for 59213@debbugs.gnu.org; Sat, 18 Feb 2023 13:46:56 -0500 Original-Received: (qmail 14657 invoked by uid 3782); 18 Feb 2023 19:46:49 +0100 Original-Received: from acm.muc.de (p4fe151f6.dip0.t-ipconnect.de [79.225.81.246]) (using STARTTLS) by colin.muc.de (tmda-ofmipd) with ESMTP; Sat, 18 Feb 2023 19:46:48 +0100 Original-Received: (qmail 1907 invoked by uid 1000); 18 Feb 2023 18:46:47 -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:255985 Archived-At: 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).