From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
To: "Mattias Engdegård" <mattiase@acm.org>
Cc: Michael Heerdegen <michael_heerdegen@web.de>,
Paul Pogonyshev <pogonyshev@gmail.com>,
51982@debbugs.gnu.org
Subject: bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas
Date: Tue, 30 Nov 2021 09:12:20 -0500 [thread overview]
Message-ID: <jwvfsrdkaf8.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <ED0329E9-B3EF-4F2A-AD7A-329B9A382D9E@acm.org> ("Mattias Engdegård"'s message of "Mon, 22 Nov 2021 18:35:18 +0100")
Sorry 'bout the delay, and thanks Mattias for finding the bug and the fix.
> @@ -428,10 +428,26 @@ cconv-convert
> ;; One of the lambda-lifted vars is shadowed, so add
> ;; a reference to the outside binding and arrange to use
> ;; that reference.
> - (let ((closedsym (make-symbol (format "closed-%s" var))))
> - (setq new-env (cconv--remap-llv new-env var closedsym))
> - (setq new-extend (cons closedsym (remq var new-extend)))
> - (push `(,closedsym ,var) binders-new)))
> + (let* ((mapping (cdr (assq var env)))
> + (var-def
> + (pcase-exhaustive mapping
> + (`(internal-get-closed-var . ,_)
> + ;; The variable is captured.
> + mapping)
> + (`(car-safe (internal-get-closed-var . ,_))
> + ;; The variable is mutably captured; skip
> + ;; the indirection step because the variable is
> + ;; passed "by rerefence" to the λ-lifted function.
> + (cadr mapping))
> + ((or '() `(car-safe ,(pred symbolp)))
> + ;; The variable is not captured. Add a
> + ;; reference to the outside binding and arrange
> + ;; to use that reference.
> + var))))
> + (let ((closedsym (make-symbol (format "closed-%s" var))))
> + (setq new-env (cconv--remap-llv new-env var closedsym))
> + (setq new-extend (cons closedsym (remq var new-extend)))
> + (push `(,closedsym ,var-def) binders-new))))
>
> ;; We push the element after redefined free variables are
> ;; processed. This is important to avoid the bug when free
> @@ -449,14 +465,28 @@ cconv-convert
> ;; before we know that the var will be in `new-extend' (bug#24171).
> (dolist (binder binders-new)
> (when (memq (car-safe binder) new-extend)
> - ;; One of the lambda-lifted vars is shadowed, so add
> - ;; a reference to the outside binding and arrange to use
> - ;; that reference.
> + ;; One of the lambda-lifted vars is shadowed.
> (let* ((var (car-safe binder))
> - (closedsym (make-symbol (format "closed-%s" var))))
> - (setq new-env (cconv--remap-llv new-env var closedsym))
> - (setq new-extend (cons closedsym (remq var new-extend)))
> - (push `(,closedsym ,var) binders-new)))))
> + (mapping (cdr (assq var env)))
> + (var-def
> + (pcase-exhaustive mapping
> + (`(internal-get-closed-var . ,_)
> + ;; The variable is captured.
> + mapping)
> + (`(car-safe (internal-get-closed-var . ,_))
> + ;; The variable is mutably captured; skip
> + ;; the indirection step because the variable is
> + ;; passed "by rerefence" to the λ-lifted function.
> + (cadr mapping))
> + ((or '() `(car-safe ,(pred symbolp)))
> + ;; The variable is not captured. Add a
> + ;; reference to the outside binding and
> + ;; arrange to use that reference.
> + var))))
> + (let ((closedsym (make-symbol (format "closed-%s" var))))
> + (setq new-env (cconv--remap-llv new-env var closedsym))
> + (setq new-extend (cons closedsym (remq var new-extend)))
> + (push `(,closedsym ,var-def) binders-new))))))
Can we avoid this duplication by moving that code to a separate function?
> + (let ((f (lambda (x)
> + (let ((g (lambda () x))
> + (h (lambda () (setq x x))))
> + (let ((x 'b))
> + (list x (funcall g) (funcall h)))))))
> + (funcall (funcall f 'b)))
> + (let ((f (lambda (x)
> + (let ((g (lambda () x))
> + (h (lambda () (setq x x))))
> + (let* ((x 'b))
> + (list x (funcall g) (funcall h)))))))
> + (funcall (funcall f 'b)))
These two tests are identical aren't they? Also, can we change the
(setq x x) into something like (setq x (list x x)) and avoid using the
same `b` value for both `x` vars, so as to catch more potential errors?
> @@ -428,10 +428,27 @@ cconv-convert
> ;; One of the lambda-lifted vars is shadowed, so add
> ;; a reference to the outside binding and arrange to use
> ;; that reference.
> - (let ((closedsym (make-symbol (format "closed-%s" var))))
> - (setq new-env (cconv--remap-llv new-env var closedsym))
> - (setq new-extend (cons closedsym (remq var new-extend)))
> - (push `(,closedsym ,var) binders-new)))
> + (let* ((mapping (cdr (assq var env)))
> + (remap-to
> + (pcase-exhaustive mapping
> + (`(internal-get-closed-var . ,_)
> + ;; The variable is captured; remap.
> + mapping)
> + (`(car-safe (internal-get-closed-var . ,_))
> + ;; The variable is mutably captured; remap, but skip
> + ;; the indirection step because the variable is
> + ;; passed "by rerefence" to the λ-lifted function.
> + (cadr mapping))
> + ((or '() `(car-safe ,(pred symbolp)))
> + ;; The variable is not captured. Add a
> + ;; reference to the outside binding and arrange
> + ;; to use that reference.
> + (let ((closedsym
> + (make-symbol (format "closed-%s" var))))
> + (push `(,closedsym ,var) binders-new)
> + closedsym)))))
> + (setq new-env (cconv--remap-llv new-env var remap-to))
> + (setq new-extend (cons remap-to (remq var new-extend)))))
>
> ;; We push the element after redefined free variables are
> ;; processed. This is important to avoid the bug when free
Looks good (better than patch A).
You say "On the other hand, patch B does abuse the cconv data structures
a little (but it works!)" so the code should say something about
this abuse. A least I failed to see where the abuse lies.
> @@ -449,14 +466,29 @@ cconv-convert
> ;; before we know that the var will be in `new-extend' (bug#24171).
> (dolist (binder binders-new)
> (when (memq (car-safe binder) new-extend)
> - ;; One of the lambda-lifted vars is shadowed, so add
> - ;; a reference to the outside binding and arrange to use
> - ;; that reference.
> + ;; One of the lambda-lifted vars is shadowed.
> (let* ((var (car-safe binder))
> - (closedsym (make-symbol (format "closed-%s" var))))
> - (setq new-env (cconv--remap-llv new-env var closedsym))
> - (setq new-extend (cons closedsym (remq var new-extend)))
> - (push `(,closedsym ,var) binders-new)))))
> + (mapping (cdr (assq var env)))
> + (remap-to
> + (pcase-exhaustive mapping
> + (`(internal-get-closed-var . ,_)
> + ;; The variable is captured; remap.
> + mapping)
> + (`(car-safe (internal-get-closed-var . ,_))
> + ;; The variable is mutably captured; remap, but skip
> + ;; the indirection step because the variable is
> + ;; passed "by rerefence" to the λ-lifted function.
> + (cadr mapping))
> + ((or '() `(car-safe ,(pred symbolp)))
> + ;; The variable is not captured. Add a
> + ;; reference to the outside binding and arrange
> + ;; to use that reference.
> + (let ((closedsym
> + (make-symbol (format "closed-%s" var))))
> + (push `(,closedsym ,var) binders-new)
> + closedsym)))))
> + (setq new-env (cconv--remap-llv new-env var remap-to))
> + (setq new-extend (cons remap-to (remq var new-extend)))))))
Same comment as before about the code duplication.
Stefan
next prev parent reply other threads:[~2021-11-30 14:12 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-19 20:31 bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas Paul Pogonyshev
2021-11-20 4:44 ` Michael Heerdegen
2021-11-20 8:45 ` Mattias Engdegård
2021-11-20 10:51 ` Michael Heerdegen
2021-11-20 16:54 ` Paul Pogonyshev
2021-11-20 17:04 ` Mattias Engdegård
2021-11-20 17:22 ` Paul Pogonyshev
2021-11-20 18:34 ` Mattias Engdegård
2021-11-20 20:53 ` Paul Pogonyshev
2021-11-21 7:59 ` Michael Heerdegen
2021-11-21 9:59 ` Mattias Engdegård
2021-11-22 10:29 ` Michael Heerdegen
2021-11-22 13:56 ` Mattias Engdegård
2021-11-22 17:35 ` Mattias Engdegård
2021-11-30 14:12 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors [this message]
2021-11-30 17:01 ` Mattias Engdegård
2021-11-30 22:41 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-12-01 16:04 ` Mattias Engdegård
2021-12-01 18:34 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-12-01 22:32 ` Mattias Engdegård
2021-12-02 9:13 ` Mattias Engdegård
2022-09-09 17:59 ` Lars Ingebrigtsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jwvfsrdkaf8.fsf-monnier+emacs@gnu.org \
--to=bug-gnu-emacs@gnu.org \
--cc=51982@debbugs.gnu.org \
--cc=mattiase@acm.org \
--cc=michael_heerdegen@web.de \
--cc=monnier@iro.umontreal.ca \
--cc=pogonyshev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).