Re: [Chicken-hackers] [PATCH] Detect self-reference in "letrec"
On Sun, Sep 15, 2019 at 05:12:00PM +0200, felix.winkelm...@bevuta.com wrote: > I moved the variable into c.-e. and also changed the error message > to be slightly more useful. Thanks, pushed! Cheers, Peter signature.asc Description: PGP signature ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Detect self-reference in "letrec"
> On Wed, Aug 28, 2019 at 01:41:22PM +0200, felix.winkelm...@bevuta.com wrote: > > An attempt to fix #1645. See commit message for more details. > > Hi Felix, > > I've taken a look at it and the logic itself looks good, but I find the > usage of fluid-let on a global variable unsettling. Why not pass the > list in "walk" to itself and to "resolve-variable"? > > If that's too unwieldy, I would at least put the variable inside > "canonicalize-expression" to clarify that it's only used there. > I moved the variable into c.-e. and also changed the error message to be slightly more useful. felix From acfcb678de00be0f531c5ce9bab6b346ac70d6c7 Mon Sep 17 00:00:00 2001 From: felix Date: Wed, 28 Aug 2019 13:37:16 +0200 Subject: [PATCH] Catch self-referencing variables in `letrec' form "(letrec ((x x)) ...)" results in and endless recursion in the optimizer, since a letrec-bound variable ends up being replacable by itself. The intermediate form generated by expanding letrec in the compiler binds "x" first to (##core#undefined) which the compiler uses to assume the variable has no value, leading to (in this context) somewhat questionable conclusion. Since this is a rather tricky part of the analysis framework of the compiler, and due to the fact that catching the situation earlier (during canonicalization) gives a more useful error message, "letrec" expands now into a use of the internal form "##core#with-forbidden-refs", which marks references to variables temporarily as invalid, until the code walk encounters a lambda form. "letrec" expands into a complex "let" form and is then walked again, during which the information about the original letrec is lost, so we need an intermediate form that also takes care of variable renaming. "##core#with-forbidden-refs" is dropped as soon as it is walked. A test from syntax-tests.scm that actually used such a self-reference has been dropped. No changes have been made to the interpreter. I think it's ok for the compiler to be more picky and give better error detection. Signed-off-by: felix --- core.scm | 45 +++--- tests/syntax-tests.scm | 4 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/core.scm b/core.scm index 5c49a683..9bb08b42 100644 --- a/core.scm +++ b/core.scm @@ -149,6 +149,7 @@ ; (##core#the ) ; (##core#typecase ( ) ... [(else )]) ; (##core#debug-event { }) +; (##core#with-forbidden-refs ( ...) ) ; ( {}) ; - Core language: @@ -512,7 +513,8 @@ ;;; Expand macros and canonicalize expressions: (define (canonicalize-expression exp) - (let ((compiler-syntax '())) + (let ((compiler-syntax '()) +(forbidden-refs '())) (define (find-id id se) ; ignores macro bindings (cond ((null? se) #f) @@ -559,11 +561,9 @@ x) ) (define (resolve-variable x0 e dest ldest h) - (when (memq x0 unlikely-variables) (warning (sprintf "reference to variable `~s' possibly unintended" x0) )) - (let ((x (lookup x0))) (d `(RESOLVE-VARIABLE: ,x0 ,x ,(map (lambda (x) (car x)) (##sys#current-environment (cond ((not (symbol? x)) x0) ; syntax? @@ -592,6 +592,13 @@ t) e dest ldest h #f #f ((not (memq x e)) (##sys#alias-global-hook x #f h)) ; only if global +((assq x forbidden-refs) => + (lambda (a) + (let ((ln (cdr a))) + (quit-compiling + "~acyclical reference in LETREC binding for variable `~a'" + (if ln (sprintf "(~a) - " ln) "") + (get-real-name x) (else x (define (emit-import-lib name il) @@ -766,12 +773,26 @@ (list (car b) '(##core#undefined))) bindings) (##core#let - ,(map (lambda (t b) (list t (cadr b))) tmps bindings) + ,(map (lambda (t b) + (list t `(##core#with-forbidden-refs + ,vars ,ln ,(cadr b + tmps bindings) ,@(map (lambda (v t) `(##core#set! ,v ,t)) vars tmps) (##core#let () ,@body) ) ) e dest ldest h ln #f))) + +((##core#with-forbidden-refs) + (let* ((loc (caddr x)) +(vars (map (lambda (v) + (cons (resolve-variable v e dest + ldest h) + loc)) +(cadr x +
Re: [Chicken-hackers] [PATCH] Detect self-reference in "letrec"
On Wed, Aug 28, 2019 at 01:41:22PM +0200, felix.winkelm...@bevuta.com wrote: > An attempt to fix #1645. See commit message for more details. Hi Felix, I've taken a look at it and the logic itself looks good, but I find the usage of fluid-let on a global variable unsettling. Why not pass the list in "walk" to itself and to "resolve-variable"? If that's too unwieldy, I would at least put the variable inside "canonicalize-expression" to clarify that it's only used there. Cheers, Peter signature.asc Description: PGP signature ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
[Chicken-hackers] [PATCH] Detect self-reference in "letrec"
An attempt to fix #1645. See commit message for more details. felix From a0a8eed96e06520c996a8ee3965eae5aa80c655b Mon Sep 17 00:00:00 2001 From: felix Date: Wed, 28 Aug 2019 13:37:16 +0200 Subject: [PATCH] Catch self-referencing variables in `letrec' form "(letrec ((x x)) ...)" results in and endless recursion in the optimizer, since a letrec-bound variable ends up being replacable by itself. The intermediate form generated by expanding letrec in the compiler binds "x" first to (##core#undefined) which the compiler uses to assume the variable has no value, leading to (in this context) somewhat questionable conclusion. Since this is a rather tricky part of the analysis framework of the compiler, and due to the fact that catching the situation earlier (during canonicalization) gives a more useful error message, "letrec" expands now into a use of the internal form "##core#with-forbidden-refs", which marks references to variables temporarily as invalid, until the code walk encounters a lambda form. "letrec" expands into a complex "let" form and is then walked again, during which the information about the original letrec is lost, so we need an intermediate form that also takes care of variable renaming. "##core#with-forbidden-refs" is dropped as soon as it is walked. A test from syntax-tests.scm that actually used such a self-reference has been dropped. No changes have been made to the interpreter. I think it's ok for the compiler to be more picky and give better error detection. --- core.scm | 43 -- tests/syntax-tests.scm | 4 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/core.scm b/core.scm index bd36448b..a88f2fc7 100644 --- a/core.scm +++ b/core.scm @@ -149,6 +149,7 @@ ; (##core#the ) ; (##core#typecase ( ) ... [(else )]) ; (##core#debug-event { }) +; (##core#with-forbidden-refs ( ...) ) ; ( {}) ; - Core language: @@ -440,6 +441,7 @@ (define file-requirements #f) (define provided '()) (define linked-static-extensions '()) +(define forbidden-refs '()) (define unlikely-variables '(unquote unquote-splicing)) @@ -563,11 +565,9 @@ x) ) (define (resolve-variable x0 e dest ldest h) - (when (memq x0 unlikely-variables) (warning (sprintf "reference to variable `~s' possibly unintended" x0) )) - (let ((x (lookup x0))) (d `(RESOLVE-VARIABLE: ,x0 ,x ,(map (lambda (x) (car x)) (##sys#current-environment (cond ((not (symbol? x)) x0) ; syntax? @@ -596,6 +596,13 @@ t) e dest ldest h #f #f ((not (memq x e)) (##sys#alias-global-hook x #f h)) ; only if global +((assq x forbidden-refs) => + (lambda (a) + (let ((ln (cdr a))) + (quit-compiling + "~abinding for variable `~a' refers to itself" + (if ln (sprintf "(~a) - " ln) "") + (get-real-name x) (else x (define (emit-import-lib name il) @@ -770,12 +777,26 @@ (list (car b) '(##core#undefined))) bindings) (##core#let - ,(map (lambda (t b) (list t (cadr b))) tmps bindings) + ,(map (lambda (t b) + (list t `(##core#with-forbidden-refs + ,vars ,ln ,(cadr b + tmps bindings) ,@(map (lambda (v t) `(##core#set! ,v ,t)) vars tmps) (##core#let () ,@body) ) ) e dest ldest h ln #f))) + +((##core#with-forbidden-refs) + (let* ((loc (caddr x)) +(vars (map (lambda (v) + (cons (resolve-variable v e dest + ldest h) + loc)) +(cadr x + (fluid-let ((forbidden-refs + (append vars forbidden-refs))) + (walk (cadddr x) e dest ldest h ln #f ((##core#lambda) (let ((llist (cadr x)) @@ -794,13 +815,15 @@ (body (parameterize ((##sys#current-environment se2)) (let ((body0 (canonicalize-body/ln ln obody compiler-syntax-enabled))) - (walk -