GNU bug report logs - #10756
[2.0.5+] Miscompilation with peval: local shadows module-ref

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 10756 in the body.
You can then email your comments to 10756 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: bug-guile <at> gnu.org
Subject: [2.0.5+] Miscompilation with peval: local shadows module-ref
Date: Wed, 08 Feb 2012 00:04:10 +0100

Hi!

Consider this:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> ,optimize (define (foo) (define bar (@ (chbouib) bar)) bar)
$11 = (define foo
  (lambda ()
    (let ((bar-1510 (if #f #f)))
      (letrec*
        ()
        (begin (set! bar-1510 bar-1510) bar-1510)))))
--8<---------------cut here---------------end--------------->8---

Here, the ‘bar’ local is always set to *undefined*, wrongfully.

Ludo’.

Message #8 received at 10756 <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: 10756 <at> debbugs.gnu.org
Subject: Re: bug#10756: [2.0.5+] Miscompilation with peval: local shadows
	module-ref
Date: Thu, 09 Feb 2012 14:17:22 +0100

On Wed 08 Feb 2012 00:04, ludo <at> gnu.org (Ludovic Courtès) writes:

> scheme@(guile-user)> ,optimize (define (foo) (define bar (@ (chbouib) bar)) bar)
> $11 = (define foo
>   (lambda ()
>     (let ((bar-1510 (if #f #f)))
>       (letrec*
>         ()
>         (begin (set! bar-1510 bar-1510) bar-1510)))))
>
> Here, the ‘bar’ local is always set to *undefined*, wrongfully.

It's actually an expander bug:

scheme@(guile-user)> ,expand (define (foo) (define bar (@ (chbouib) bar)) bar)
$2 = (define foo
  (lambda () (letrec* ((bar-92 bar-92)) bar-92)))

Andy
-- 
http://wingolog.org/

Message #11 received at 10756 <at> debbugs.gnu.org (full text, mbox):

From: Mark H Weaver <mhw <at> netris.org>
To: Andy Wingo <wingo <at> pobox.com>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 10756 <at> debbugs.gnu.org
Subject: Re: bug#10756: [2.0.5+] Miscompilation with peval: local shadows
	module-ref
Date: Thu, 08 Mar 2012 02:11:01 -0500

Andy Wingo <wingo <at> pobox.com> writes:

> On Wed 08 Feb 2012 00:04, ludo <at> gnu.org (Ludovic Courtès) writes:
>
>> scheme@(guile-user)> ,optimize (define (foo) (define bar (@ (chbouib) bar)) bar)
>> $11 = (define foo
>>   (lambda ()
>>     (let ((bar-1510 (if #f #f)))
>>       (letrec*
>>         ()
>>         (begin (set! bar-1510 bar-1510) bar-1510)))))
>>
>> Here, the ‘bar’ local is always set to *undefined*, wrongfully.
>
> It's actually an expander bug:
>
> scheme@(guile-user)> ,expand (define (foo) (define bar (@ (chbouib) bar)) bar)
> $2 = (define foo
>   (lambda () (letrec* ((bar-92 bar-92)) bar-92)))
>
> Andy

I've attached a patch that fixes this bug.  Fixing '@' was literally a
one word fix (w -> top-wrap), and the same would have been true of '@@'
if not for the way it had been extended to support R6RS library forms.
Unlike '@' which uses syntax->datum on the 'id' to strip the wrap, '@@'
kept syntax objects fully intact and only changed their module.

I think it was a mistake to overload '@@' to do these two different
jobs, so I changed the R6RS-support syntax to (@@ @@ (mod ...) body) and
left its behavior as-is, and then made (@@ (mod ...) id) act the same
way as '@': use 'syntax->datum' on the 'id' and return top-wrap.

I think it's okay to change the internal R6RS-support syntax in
stable-2.0, because it's undocumented and only exists as a temporary
intermediate form during macro expansion.  What do you think?

Also: since 'boot-9.go' was not automatically recompiled by changing
'r6rs-libraries.scm', I added explicit dependencies to
module/Makefile.am.  However, I'm almost wholly ignorant of automake, so
please double-check what I did there.

    Thanks,
      Mark

Message #14 received at 10756 <at> debbugs.gnu.org (full text, mbox):

From: Mark H Weaver <mhw <at> netris.org>
To: Andy Wingo <wingo <at> pobox.com>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 10756 <at> debbugs.gnu.org
Subject: Re: bug#10756: [2.0.5+] Miscompilation with peval: local shadows
	module-ref
Date: Thu, 08 Mar 2012 02:13:37 -0500

[Message part 1 (text/plain, inline)]

And here's the actual patch.

     Mark

[0001-Fix-and-to-not-capture-lexicals-new-form-for-R6RS-li.patch (text/x-patch, inline)]

From 13668b618c4345d51a5667056d1d515c21a874a9 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw <at> netris.org>
Date: Thu, 8 Mar 2012 01:24:25 -0500
Subject: [PATCH] Fix @ and @@ to not capture lexicals; new @@ @@ form for
 R6RS libraries
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* module/ice-9/psyntax.scm (@): Return top-wrap instead of the wrap
  applied to the '@' form, so that the symbol will be interpreted as a
  top-level identifier and never refer to any lexical variable.

  (@@): Change the syntax used to support R6RS 'library' forms to:
  (@@ @@ (mod ...) body).  Change the behavior of the documented
  (@@ (mod ...) id) form to be the same as that of @, except for the use
  of 'private' instead of 'public' in the psyntax mod: use syntax->datum
  on the identifier, and return top-wrap instead of the wrap applied to
  the '@@' form.

  This fixes <http://bugs.gnu.org/10756> reported by Ludovic Courtès.

* module/ice-9/psyntax-pp.scm: Regenerate.

* module/ice-9/r6rs-libraries.scm (library): Use '@@ @@' syntax instead
  of the older '@@' syntax.

* test-suite/tests/syncase.test (changes to expansion environment): Use
  '@@ @@' syntax.

* module/Makefile.am: Add explicit dependencies for boot-9.go on the
  files that it includes: quasisyntax.scm and r6rs-libraries.scm.
---
 module/Makefile.am              |    2 ++
 module/ice-9/psyntax-pp.scm     |   36 ++++++++++++++++++++++++++----------
 module/ice-9/psyntax.scm        |   19 ++++++++++++++++---
 module/ice-9/r6rs-libraries.scm |    2 +-
 test-suite/tests/syncase.test   |   16 ++++++++--------
 5 files changed, 53 insertions(+), 22 deletions(-)

diff --git a/module/Makefile.am b/module/Makefile.am
index 9c9d8ed..a97f2ef 100644
--- a/module/Makefile.am
+++ b/module/Makefile.am
@@ -32,6 +32,8 @@ nobase_ccache_DATA += ice-9/eval.go
 EXTRA_DIST += ice-9/eval.scm
 ETAGS_ARGS += ice-9/eval.scm
 
+ice-9/boot-9.go: ice-9/boot-9.scm ice-9/quasisyntax.scm ice-9/r6rs-libraries.scm
+
 # We can compile these in any order, but it's fastest if we compile
 # psyntax and boot-9 first, then the compiler itself, then the rest of
 # the code.
diff --git a/module/ice-9/psyntax-pp.scm b/module/ice-9/psyntax-pp.scm
index 7475983..68d1bf6 100644
--- a/module/ice-9/psyntax-pp.scm
+++ b/module/ice-9/psyntax-pp.scm
@@ -1950,7 +1950,7 @@
                    (values
                      (syntax->datum id)
                      r
-                     w
+                     '((top))
                      #f
                      (syntax->datum
                        (cons '#(syntax-object public ((top)) (hygiene guile)) mod))))
@@ -1982,16 +1982,32 @@
                             (loop (+ i 1)))))))
                    (else x)))))
         (let* ((tmp-1 e) (tmp ($sc-dispatch tmp-1 '(_ each-any any))))
-          (if (and tmp (apply (lambda (mod exp) (and-map id? mod)) tmp))
-            (apply (lambda (mod exp)
-                     (let ((mod (syntax->datum
-                                  (cons '#(syntax-object private ((top)) (hygiene guile)) mod))))
-                       (values (remodulate exp mod) r w (source-annotation exp) mod)))
+          (if (and tmp
+                   (apply (lambda (mod id) (and (and-map id? mod) (id? id))) tmp))
+            (apply (lambda (mod id)
+                     (values
+                       (syntax->datum id)
+                       r
+                       '((top))
+                       #f
+                       (syntax->datum
+                         (cons '#(syntax-object private ((top)) (hygiene guile)) mod))))
                    tmp)
-            (syntax-violation
-              #f
-              "source expression failed to match any pattern"
-              tmp-1))))))
+            (let ((tmp ($sc-dispatch
+                         tmp-1
+                         '(_ #(free-id #(syntax-object @@ ((top)) (hygiene guile)))
+                             each-any
+                             any))))
+              (if (and tmp (apply (lambda (mod exp) (and-map id? mod)) tmp))
+                (apply (lambda (mod exp)
+                         (let ((mod (syntax->datum
+                                      (cons '#(syntax-object private ((top)) (hygiene guile)) mod))))
+                           (values (remodulate exp mod) r w (source-annotation exp) mod)))
+                       tmp)
+                (syntax-violation
+                  #f
+                  "source expression failed to match any pattern"
+                  tmp-1))))))))
   (global-extend
     'core
     'if
diff --git a/module/ice-9/psyntax.scm b/module/ice-9/psyntax.scm
index 6015eff..b8ab8b8 100644
--- a/module/ice-9/psyntax.scm
+++ b/module/ice-9/psyntax.scm
@@ -2239,7 +2239,9 @@
                      (syntax-case e ()
                        ((_ (mod ...) id)
                         (and (and-map id? #'(mod ...)) (id? #'id))
-                        (values (syntax->datum #'id) r w #f
+                        ;; Strip the wrap from the identifier and return top-wrap
+                        ;; so that the identifier will not be captured by lexicals.
+                        (values (syntax->datum #'id) r top-wrap #f
                                 (syntax->datum
                                  #'(public mod ...)))))))
 
@@ -2262,9 +2264,20 @@
                                       ((fx= i n) v)
                                     (vector-set! v i (remodulate (vector-ref x i) mod)))))
                                (else x))))
-                     (syntax-case e ()
-                       ((_ (mod ...) exp)
+                     (syntax-case e (@@)
+                       ((_ (mod ...) id)
+                        (and (and-map id? #'(mod ...)) (id? #'id))
+                        ;; Strip the wrap from the identifier and return top-wrap
+                        ;; so that the identifier will not be captured by lexicals.
+                        (values (syntax->datum #'id) r top-wrap #f
+                                (syntax->datum
+                                 #'(private mod ...))))
+                       ((_ @@ (mod ...) exp)
                         (and-map id? #'(mod ...))
+                        ;; This is a special syntax used to support R6RS library forms.
+                        ;; Unlike the syntax above, the last item is not restricted to
+                        ;; be a single identifier, and the syntax objects are kept
+                        ;; intact, with only their module changed.
                         (let ((mod (syntax->datum #'(private mod ...))))
                           (values (remodulate #'exp mod)
                                   r w (source-annotation #'exp)
diff --git a/module/ice-9/r6rs-libraries.scm b/module/ice-9/r6rs-libraries.scm
index bf1127e..f71b90b 100644
--- a/module/ice-9/r6rs-libraries.scm
+++ b/module/ice-9/r6rs-libraries.scm
@@ -197,7 +197,7 @@
                  (export e ...)
                  (re-export r ...)
                  (export! x ...)
-                 (@@ (name name* ...) body)
+                 (@@ @@ (name name* ...) body)
                  ...))))))))
     
 (define-syntax import
diff --git a/test-suite/tests/syncase.test b/test-suite/tests/syncase.test
index 6183df8..0e81f65 100644
--- a/test-suite/tests/syncase.test
+++ b/test-suite/tests/syncase.test
@@ -115,16 +115,16 @@
          'foo)))
 
 (with-test-prefix "changes to expansion environment"
-  (pass-if "expander detects changes to current-module with @@"
+  (pass-if "expander detects changes to current-module with @@ @@"
     (compile '(begin
                 (define-module (new-module))
-                (@@ (new-module)
-                    (define-syntax new-module-macro
-                      (lambda (stx)
-                        (syntax-case stx () 
-                          ((_ arg) (syntax arg))))))
-                (@@ (new-module)
-                    (new-module-macro #t)))
+                (@@ @@ (new-module)
+                       (define-syntax new-module-macro
+                         (lambda (stx)
+                           (syntax-case stx () 
+                             ((_ arg) (syntax arg))))))
+                (@@ @@ (new-module)
+                       (new-module-macro #t)))
              #:env (current-module))))
 
 (define-module (test-suite test-syncase-2)
-- 
1.7.5.4

Message #19 received at 10756-done <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: Mark H Weaver <mhw <at> netris.org>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 10756-done <at> debbugs.gnu.org
Subject: Re: bug#10756: [2.0.5+] Miscompilation with peval: local shadows
	module-ref
Date: Fri, 06 Jul 2012 20:20:13 +0200

On Thu 08 Mar 2012 08:11, Mark H Weaver <mhw <at> netris.org> writes:

>> scheme@(guile-user)> ,expand (define (foo) (define bar (@ (chbouib) bar)) bar)
>> $2 = (define foo
>>   (lambda () (letrec* ((bar-92 bar-92)) bar-92)))
>>
>> Andy
>
> I've attached a patch that fixes this bug.  Fixing '@' was literally a
> one word fix (w -> top-wrap), and the same would have been true of '@@'
> if not for the way it had been extended to support R6RS library forms.
> Unlike '@' which uses syntax->datum on the 'id' to strip the wrap, '@@'
> kept syntax objects fully intact and only changed their module.
>
> I think it was a mistake to overload '@@' to do these two different
> jobs, so I changed the R6RS-support syntax to (@@ @@ (mod ...) body) and
> left its behavior as-is, and then made (@@ (mod ...) id) act the same
> way as '@': use 'syntax->datum' on the 'id' and return top-wrap.
>
> I think it's okay to change the internal R6RS-support syntax in
> stable-2.0, because it's undocumented and only exists as a temporary
> intermediate form during macro expansion.  What do you think?
>
> Also: since 'boot-9.go' was not automatically recompiled by changing
> 'r6rs-libraries.scm', I added explicit dependencies to
> module/Makefile.am.  However, I'm almost wholly ignorant of automake, so
> please double-check what I did there.

Looks great to me, pushed.  It's strictly incompatible, but hey.  Sorry
for taking so long!  The only thing I would note is that it seems to me
that this "R6RS-support" is useful in a general sense.  Just an
impression though, I've never had occasion to use it outside the R6RS
libs.

Regards, and thanks very much,

Andy
-- 
http://wingolog.org/

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.