stack overflow equal? values

stack overflow equal? values

[Marco Maggi]

Dunno if this was reported before:

guile> (equal? (values 1 2) (values 1 2))
ERROR: Stack overflow

Guile 1.8.1


--
Marco Maggi




_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-guile

Re: stack overflow equal? values

[Kevin Ryde]

"Marco Maggi" <marco.maggi-ipsu@poste.it> writes:
>
> guile> (equal? (values 1 2) (values 1 2))
> ERROR: Stack overflow

Thanks, looks like a bug introduced by recursing into structs in
equal?.  I guess scm_i_struct_equalp should check and ignore a field
type "s" ...


_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-guile

Re: stack overflow equal? values

[Ludovic Courtès]

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

Hi Kevin,

Kevin Ryde <user42@zip.com.au> writes:

> "Marco Maggi" <marco.maggi-ipsu@poste.it> writes:
>>
>> guile> (equal? (values 1 2) (values 1 2))
>> ERROR: Stack overflow
>
> Thanks, looks like a bug introduced by recursing into structs in
> equal?.  I guess scm_i_struct_equalp should check and ignore a field
> type "s" ...

Indeed, good guess!

I propose the following simple fix.  Ok to apply?

Actually, `scm_i_struct_equalp ()' should also compare the "tail
elements" (when there are tail elements), but their semantics are a
little fuzzy to me.  In particular, I don't understand why the size of
the tail array can be specified in both `make-vtable-vtable' and
`make-struct': What does that mean?  Which one should really be taken
into account?  It seems that the code is a bit unclear on this too:

  guile> (define v (make-vtable-vtable "pr" 0))
  guile> (define s (make-struct v 123))
  guile> (struct-ref s 10)
  Segmentation fault

(Looks like the API is so complex that few people actually bothered to
use it to its full extents.  ;-))

Thanks,
Ludovic.



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

--- orig/libguile/struct.c
+++ mod/libguile/struct.c
@@ -564,8 +564,11 @@
       field1 = scm_struct_ref (s1, s_field_num);
       field2 = scm_struct_ref (s2, s_field_num);
 
-      if (scm_is_false (scm_equal_p (field1, field2)))
-	return SCM_BOOL_F;
+      /* Self-referencing fields (type `s') must be skipped to avoid infinite
+	 recursion.  */
+      if (!(scm_is_eq (field1, s1) && (scm_is_eq (field2, s2))))
+	if (scm_is_false (scm_equal_p (field1, field2)))
+	  return SCM_BOOL_F;
     }
 
   return SCM_BOOL_T;


--- orig/test-suite/tests/structs.test
+++ mod/test-suite/tests/structs.test
@@ -82,12 +82,18 @@
        (set-owner! ball "Bill")
        (string=? (owner ball) "Bill")))
 
-  (pass-if "equal?"
+  (pass-if "equal? (simple structs)"
+     (let* ((vtable (make-vtable-vtable "pr" 0))
+            (s1     (make-struct vtable 0 "hello"))
+            (s2     (make-struct vtable 0 "hello")))
+       (equal? s1 s2)))
+
+  (pass-if "equal? (more complex structs)"
      (let ((first (make-ball red (string-copy "Bob")))
-	   (second (make-ball red (string-copy "Bob"))))
+           (second (make-ball red (string-copy "Bob"))))
        (equal? first second)))
 
   (pass-if "not-equal?"
-     (not (or (equal? (make-ball red "Bob") (make-ball green "Bill"))
+     (not (or (equal? (make-ball red "Bob") (make-ball green "Bob"))
 	      (equal? (make-ball red "Bob") (make-ball red "Bill"))))))
 

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

_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-guile

Re: stack overflow equal? values

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> I propose the following simple fix.  Ok to apply?

Yep, looks good.

After I posted I wondered if the values struct is an actual "s" or if
there's some strange extra I couldn't spot.  Testing eq avoids
worrying about that.

Dunno why values are a struct and not a smob cell.

> Actually, `scm_i_struct_equalp ()' should also compare the "tail
> elements" (when there are tail elements),

Yes.

> but their semantics are a
> little fuzzy to me.  In particular, I don't understand why the size of
> the tail array can be specified in both `make-vtable-vtable' and
> `make-struct': What does that mean?  Which one should really be taken
> into account?

Dunno :).

> It seems that the code is a bit unclear on this too:
>
>   guile> (define v (make-vtable-vtable "pr" 0))
>   guile> (define s (make-struct v 123))
>   guile> (struct-ref s 10)
>   Segmentation fault

A segv is a bug, obviously, whichever way it's actually meant to be.

> (Looks like the API is so complex that few people actually bothered to
> use it to its full extents.  ;-))

The records level is friendlier I guess.  For a long time I couldn't
understand what "vtable" meant, I still don't think I quite do.  Maybe
the docs should be tweaked, to help show what structs are typically
meant to be.

> --- orig/test-suite/tests/structs.test
> +++ mod/test-suite/tests/structs.test
> @@ -82,12 +82,18 @@
>         (set-owner! ball "Bill")
>         (string=? (owner ball) "Bill")))
>  
> -  (pass-if "equal?"

You can make a with-test-prefix group if you like.  An exercise of an
actual values too will be good, maybe in eval.test.


_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-guile

Re: stack overflow equal? values

[Ludovic Courtès]

Hi,

Kevin Ryde <user42@zip.com.au> writes:

> Yep, looks good.

Applied to both branches (I also converted the change logs in 1.8 to
UTF-8 :-)).

> After I posted I wondered if the values struct is an actual "s" or if
> there's some strange extra I couldn't spot.  Testing eq avoids
> worrying about that.

Yes.  The `s' field is actually part of the REQUIRED_VTABLE_FIELDS that
gets automatically added in `make-vtable-vtable'.

> Dunno why values are a struct and not a smob cell.

Maybe because structs looked cool at that time.  ;-)

>> but their semantics are a
>> little fuzzy to me.  In particular, I don't understand why the size of
>> the tail array can be specified in both `make-vtable-vtable' and
>> `make-struct': What does that mean?  Which one should really be taken
>> into account?
>
> Dunno :).

Then maybe we should "do something" about it in 1.9, and fix remaining
bugs.

> The records level is friendlier I guess.  For a long time I couldn't
> understand what "vtable" meant, I still don't think I quite do.  Maybe
> the docs should be tweaked, to help show what structs are typically
> meant to be.

Yes.  Actually, I used mostly exclusively either SRFI-9 records or GOOPS
classes.  Then there's also Guile's built-in records as you said.

Structs are nice in that they can be easily accessed both from C and
Scheme, so they can probably be used in lieu of SMOBs in some cases.

Still, that leaves us with 3 (or 4) record/struct APIs, and we may also
want to implement R6RS records at some point...

Thanks,
Ludovic.


_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-guile