* bug#10756: [2.0.5+] Miscompilation with peval: local shadows module-ref
@ 2012-02-07 23:04 Ludovic Courtès
2012-02-09 13:17 ` Andy Wingo
0 siblings, 1 reply; 5+ messages in thread
From: Ludovic Courtès @ 2012-02-07 23:04 UTC (permalink / raw)
To: 10756
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’.
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#10756: [2.0.5+] Miscompilation with peval: local shadows module-ref
2012-02-07 23:04 bug#10756: [2.0.5+] Miscompilation with peval: local shadows module-ref Ludovic Courtès
@ 2012-02-09 13:17 ` Andy Wingo
2012-03-08 7:11 ` Mark H Weaver
2012-03-08 7:13 ` Mark H Weaver
0 siblings, 2 replies; 5+ messages in thread
From: Andy Wingo @ 2012-02-09 13:17 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: 10756
On Wed 08 Feb 2012 00:04, ludo@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/
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#10756: [2.0.5+] Miscompilation with peval: local shadows module-ref
2012-02-09 13:17 ` Andy Wingo
@ 2012-03-08 7:11 ` Mark H Weaver
2012-07-06 18:20 ` Andy Wingo
2012-03-08 7:13 ` Mark H Weaver
1 sibling, 1 reply; 5+ messages in thread
From: Mark H Weaver @ 2012-03-08 7:11 UTC (permalink / raw)
To: Andy Wingo; +Cc: Ludovic Courtès, 10756
Andy Wingo <wingo@pobox.com> writes:
> On Wed 08 Feb 2012 00:04, ludo@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#10756: [2.0.5+] Miscompilation with peval: local shadows module-ref
2012-02-09 13:17 ` Andy Wingo
2012-03-08 7:11 ` Mark H Weaver
@ 2012-03-08 7:13 ` Mark H Weaver
1 sibling, 0 replies; 5+ messages in thread
From: Mark H Weaver @ 2012-03-08 7:13 UTC (permalink / raw)
To: Andy Wingo; +Cc: Ludovic Courtès, 10756
[-- Attachment #1: Type: text/plain, Size: 42 bytes --]
And here's the actual patch.
Mark
[-- Attachment #2: [PATCH] Fix @ and @@ to not capture lexicals; new @@ @@ form for R6RS libraries --]
[-- Type: text/x-patch, Size: 8595 bytes --]
From 13668b618c4345d51a5667056d1d515c21a874a9 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#10756: [2.0.5+] Miscompilation with peval: local shadows module-ref
2012-03-08 7:11 ` Mark H Weaver
@ 2012-07-06 18:20 ` Andy Wingo
0 siblings, 0 replies; 5+ messages in thread
From: Andy Wingo @ 2012-07-06 18:20 UTC (permalink / raw)
To: Mark H Weaver; +Cc: Ludovic Courtès, 10756-done
On Thu 08 Mar 2012 08:11, Mark H Weaver <mhw@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/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-07-06 18:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-07 23:04 bug#10756: [2.0.5+] Miscompilation with peval: local shadows module-ref Ludovic Courtès
2012-02-09 13:17 ` Andy Wingo
2012-03-08 7:11 ` Mark H Weaver
2012-07-06 18:20 ` Andy Wingo
2012-03-08 7:13 ` Mark H Weaver
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).