Quite sneaky bug in SRFI-9

Quite sneaky bug in SRFI-9

[Andreas Rottmann]

[-- Attachment #1: Type: text/plain, Size: 1951 bytes --]

Hi!   After being puzzled for some time, I found the root cause 
for an issue with my code was actually a quite serious bug in 
SRFI-9's `define-inlinable' (here we go again -- but this time 
it's an actual bug, I promise ;-)):  The current implementation of 
`define-inlinable' duplicates actual parameters passed to the 
macros it generates, for example (note the `(find-next-solution! s 
10000)' expression):  scheme@(guile-user)> (tree-il->scheme 
(macroexpand '(solution? (find-next-solution! s 10000))))
$6 = (if ((@@ (srfi srfi-9) struct?) (find-next-solution! s 10000))
         ((@@ (srfi srfi-9) eq?) 
              ((@@ (srfi srfi-9) struct-vtable) (find-next-solution! s 10000))
              (@@ (dorodango solver) solution))
          #f)


Attached is a patch to fix this (along with a test case), but I guess
it's not as good as it could be:

- It doesn't preserve the current error-at-syntax-time behavior when an
  argument count mismatch occurs (this makes the first two "constructor"
  testcases in srfi-9.test fail, and obsolete).  I don't know if this is
  considered important.

- It's certainly not as efficient as the current, broken implementation.
  Ideally, the expansion would be a `let' block instead of the
  `call-with-values' invocation.  I'd guess the former might be more
  amendable for optimization, but perhaps I'm wrong here.  I didn't
  implement the solution using `let', as I believe it cannot be done in
  a straightforward way (i.e. by exploiting the pattern matcher only)
  with Guile's current syntax-case implementation.

Has anyone done benchmarks of the benefits of the current (broken)
SRFI-9 accessors and predicates vs. regular procedures on real code?
Given the argument duplication issue, it might be the case that
`define-inlinable' actually slowed things down :-).

Perhaps removing `define-inlinable' entirely would be an acceptable
alternative fix for this issue?

Anyway, here's the patch:



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: srfi-9-fix2.diff --]
[-- Type: text/x-diff, Size: 2561 bytes --]

From: Andreas Rottmann <a.rottmann@gmx.at>
Subject: Fix argument duplication `define-inlinable' in SRFI-9

* modules/srfi/srfi-9.scm (define-inlinable): Don't duplicate argument
  expressions in the expansion of the macros generated.
* test-suite/tests/srfi-9.test (constructor): Adapted.
  (side-effecting arguments): New test for the argument duplication
  issue.

---
 module/srfi/srfi-9.scm       |    5 +++--
 test-suite/tests/srfi-9.test |   22 +++++++++-------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/module/srfi/srfi-9.scm b/module/srfi/srfi-9.scm
index fad570b..34dbb53 100644
--- a/module/srfi/srfi-9.scm
+++ b/module/srfi/srfi-9.scm
@@ -87,8 +87,9 @@
              (define-syntax name
                (lambda (x)
                  (syntax-case x ()
-                   ((_ formals ...)
-                    #'(begin body ...))
+                   ((_ . arguments)
+                    #'(call-with-values (lambda () (values . arguments))
+                        (lambda (formals ...) body ...)))
                    (_
                     (identifier? x)
                     #'proc-name))))))))))
diff --git a/test-suite/tests/srfi-9.test b/test-suite/tests/srfi-9.test
index f8006c4..6766482 100644
--- a/test-suite/tests/srfi-9.test
+++ b/test-suite/tests/srfi-9.test
@@ -38,21 +38,10 @@
 
 (with-test-prefix "constructor"
 
-  ;; Constructors are defined using `define-integrable', meaning that direct
-  ;; calls as in `(make-foo)' lead to a compile-time psyntax error, hence the
-  ;; distinction below.
-
-  (pass-if-exception "foo 0 args (inline)" exception:syntax-pattern-unmatched
-     (compile '(make-foo) #:env (current-module)))
-  (pass-if-exception "foo 2 args (inline)" exception:syntax-pattern-unmatched
-     (compile '(make-foo 1 2) #:env (current-module)))
-
   (pass-if-exception "foo 0 args" exception:wrong-num-args
-     (let ((make-foo make-foo))
-       (make-foo)))
+    (make-foo))
   (pass-if-exception "foo 2 args" exception:wrong-num-args
-     (let ((make-foo make-foo))
-       (make-foo 1 2))))
+    (make-foo 1 2)))
 
 (with-test-prefix "predicate"
 
@@ -94,6 +83,13 @@
   (pass-if-exception "set-y! on bar" exception:wrong-type-arg
      (set-y! b 99)))
 
+(with-test-prefix "side-effecting arguments"
+
+    (pass-if "predicate"
+      (let ((x 0))
+        (and (foo? (begin (set! x (+ x 1)) f))
+             (= x 1)))))
+
 (with-test-prefix "non-toplevel"
     
   (define-record-type :frotz (make-frotz a b) frotz?
-- 
tg: (df12979..) t/srfi-9-fix2 (depends on: stable-2.0)

[-- Attachment #3: Type: text/plain, Size: 63 bytes --]


Regards, Rotty
-- 
Andreas Rottmann -- <http://rotty.yi.org/>

Re: Quite sneaky bug in SRFI-9

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

Hi Andreas,

Andreas Rottmann <a.rottmann@gmx.at> writes:

> Hi!   After being puzzled for some time, I found the root cause for an
> issue with my code was actually a quite serious bug in SRFI-9's
> define-inlinable' (here we go again -- but this time it's an actual
> bug, I promise ;-)):  The current implementation of `define-inlinable'
> duplicates actual parameters passed to the macros it generates, for
> example (note the `(find-next-solution! s 10000)' expression):
> scheme@(guile-user)> (tree-il->scheme (macroexpand '(solution?
> (find-next-solution! s 10000))))
> $6 = (if ((@@ (srfi srfi-9) struct?) (find-next-solution! s 10000))
>         ((@@ (srfi srfi-9) eq?)            ((@@ (srfi srfi-9)
> struct-vtable) (find-next-solution! s 10000))
>              (@@ (dorodango solver) solution))
>          #f)
>

Ouch, good catch!

> - It's certainly not as efficient as the current, broken implementation.
>  Ideally, the expansion would be a `let' block instead of the
>  `call-with-values' invocation.

Yes, that would be nice.  This patch seems to do the trick:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1365 bytes --]

diff --git a/module/srfi/srfi-9.scm b/module/srfi/srfi-9.scm
index fad570b..54bc8a8 100644
--- a/module/srfi/srfi-9.scm
+++ b/module/srfi/srfi-9.scm
@@ -64,6 +64,12 @@
 
 (cond-expand-provide (current-module) '(srfi-9))
 
+(define-syntax letify
+  (syntax-rules ()
+    ((_ (var ...) (val ...) body ...)
+     (let ((var val) ...)
+       body ...))))
+
 (define-syntax define-inlinable
   ;; Define a macro and a procedure such that direct calls are inlined, via
   ;; the macro expansion, whereas references in non-call contexts refer to
@@ -80,15 +86,17 @@
     (syntax-case x ()
       ((_ (name formals ...) body ...)
        (identifier? #'name)
-       (with-syntax ((proc-name (make-procedure-name #'name)))
+       (with-syntax ((proc-name  (make-procedure-name #'name))
+                     ((args ...) (generate-temporaries #'(formals ...))))
          #`(begin
              (define (proc-name formals ...)
                body ...)
              (define-syntax name
                (lambda (x)
                  (syntax-case x ()
-                   ((_ formals ...)
-                    #'(begin body ...))
+                   ((_ args ...)
+                    #'(letify (formals ...) (args ...)
+                              (begin body ...)))
                    (_
                     (identifier? x)
                     #'proc-name))))))))))

[-- Attachment #3: Type: text/plain, Size: 794 bytes --]


I can’t think of any way to do it more elegantly.

> Has anyone done benchmarks of the benefits of the current (broken)
> SRFI-9 accessors and predicates vs. regular procedures on real code?
> Given the argument duplication issue, it might be the case that
> `define-inlinable' actually slowed things down :-).

I benchmarked it indirectly via ‘vlist-cons’ et al. in ‘vlists.bm’.

The bonus is that there are special opcodes for ‘struct-vtable’,
‘make-struct’, and ‘struct-ref’.  So when you inline constructors and
accessors, you also get to use directly those opcodes, which makes quite
a difference.

I would like to keep this property.  Not doing this gives an incentive
to using lists everywhere since ‘car’, ‘pair?’, etc. are open-coded.

Thanks!

Ludo’.

Re: Quite sneaky bug in SRFI-9

[Andy Wingo]

On Fri 11 Mar 2011 14:55, ludo@gnu.org (Ludovic Courtès) writes:

>>  Ideally, the expansion would be a `let' block instead of the
>>  `call-with-values' invocation.
>
> Yes, that would be nice.  This patch seems to do the trick:

Looks good to me.  Want to apply?

Andy
-- 
http://wingolog.org/

Re: Quite sneaky bug in SRFI-9

[Andy Wingo]

Hi,

On Fri 11 Mar 2011 14:55, ludo@gnu.org (Ludovic Courtès) writes:

>               (define-syntax name
>                 (lambda (x)
>                   (syntax-case x ()
> -                   ((_ formals ...)
> -                    #'(begin body ...))
> +                   ((_ args ...)
> +                    #'(letify (formals ...) (args ...)
> +                              (begin body ...)))

Alternately:

  ((_ args ...)
   #'((lambda (formals ...)
        body ...)
      args ...))

Guile will turn that into a let.

Andy
-- 
http://wingolog.org/

Re: Quite sneaky bug in SRFI-9

[Ludovic Courtès]

Hi!

Andy Wingo <wingo@pobox.com> writes:

> On Fri 11 Mar 2011 14:55, ludo@gnu.org (Ludovic Courtès) writes:
>
>>               (define-syntax name
>>                 (lambda (x)
>>                   (syntax-case x ()
>> -                   ((_ formals ...)
>> -                    #'(begin body ...))
>> +                   ((_ args ...)
>> +                    #'(letify (formals ...) (args ...)
>> +                              (begin body ...)))
>
> Alternately:
>
>   ((_ args ...)
>    #'((lambda (formals ...)
>         body ...)
>       args ...))
>
> Guile will turn that into a let.

Nice!

I pushed this and Andreas’ test case, thank you!

Ludo’.