Opened 3 months ago

Closed 5 weeks ago

#1703 closed defect (fixed)

Arguments being passed to procedure incorrectly

Reported by: Jakob L. Kreuze Owned by: sjamaan
Priority: major Milestone: 5.3
Component: compiler Version: 5.2.0
Keywords: Cc:
Estimated difficulty: hard

Description

Here is the offending part of a program I have been writing.

(define (reduce proc list init)
  (define (reduce-iter list result)
    (if (null? list)
        result
        (reduce-iter (cdr list) (proc result (car list)))))
  (reduce-iter list init))

(define-record-type <vec3>
  (make-vec3 x y z)
  vec3?
  (x vec3-x)
  (y vec3-y)
  (z vec3-z))

;; Macro for destructuring a vec3.
(define-syntax vec3-bind
  (syntax-rules ()
    ((vec3-bind ((names vec) ...)
       body)
     (let-values ((names (values (vec3-x vec)
                                 (vec3-y vec)
                                 (vec3-z vec)))
                  ...)
       body))))

(define (vec3- . vecs)
  "Return the difference of VECS, as in vector space subtraction."
  (define (sub u v)
    (vec3-bind (((x1 y1 z1) u) ((x2 y2 z2) v))
      (make-vec3 (- x1 x2) (- y1 y2) (- z1 z2))))
  (if (zero? (length vecs))
      (make-vec3 0 0 0)
      (reduce sub (cdr vecs) (car vecs))))

(vec3- (make-vec3 1.00 1.00 1.00)
       (make-vec3 1.00 1.00 1.00)
       (make-vec3 1.00 1.00 1.00))

This runs without error on Version 4.13.0 (rev 68eeaaef), but on Version 5.2.0 (rev 317468e4), I get the following runtime error.

Error: (vec3-x) bad argument type - not a structure of the required type
#<procedure (sub u v)>
<vec3>

	Call history:

	r7rs-raytracer.scm:35: make-vec3	  
	r7rs-raytracer.scm:36: make-vec3	  
	r7rs-raytracer.scm:37: make-vec3	  
	r7rs-raytracer.scm:35: vec3-	  
	r7rs-raytracer.scm:33: reduce	  
	r7rs-raytracer.scm:6: reduce-iter	  
	r7rs-raytracer.scm:5: proc	  
	r7rs-raytracer.scm:29: ##sys#call-with-values	  
	r7rs-raytracer.scm:29: vec3-x	  	<--

I could not build the master branch, so I was unable to see if this has been resolved.

I suspect the arguments are being passed in the wrong order to 'reduce', somehow. This error only occurs if the number of arguments to vec3- is greater than 2.

Change History (7)

comment:1 Changed 3 months ago by sjamaan

Owner: set to sjamaan
Status: newaccepted

This looks like a problem with the car/cdr rewrite of rest args

comment:2 Changed 3 months ago by felix winkelmann

Here a slightly simplified variant:

(define (reduce proc list init)
  (define (reduce-iter list result)
    (if (null? list)
        result
        (reduce-iter (cdr list) (proc result (car list)))))
  (reduce-iter list init))

(define (vec3- . vecs)
  (define (sub u v)
    (vector (- (vector-ref u 0) (vector-ref v 0))
            (- (vector-ref u 1) (vector-ref v 1))
            (- (vector-ref u 2) (vector-ref v 2))))
  (reduce sub (cdr vecs) (car vecs)))

(vec3- #(1.00 1.00 1.00)
       #(1.00 1.00 1.00)
       #(1.00 1.00 1.00))

which results in

Error: (vector-ref) bad argument type: #<procedure (sub u v)>

	Call history:

	x.scm:15: vec3-	  
	x.scm:13: reduce	  
	x.scm:6: reduce-iter	  
	x.scm:5: proc	  	<--

csc -debug 7 looks ok to me. Making sub trivial seems to make it run to completion, but again, the debug output doesn't show any noteworthy difference.

comment:3 Changed 3 months ago by megane

Smaller yet:

(set! reduce (lambda (_l ini) (+ ini 1)))

;; (print ((lambda xs (reduce xs (car xs))) 1 2 3)) ;; prints 2

(set! fold- (lambda xs (reduce xs (car xs))))
(print (fold- 1 2 3))

;; Error: (+) bad argument type - not a number: (1 2 3)
;;
;;         Call history:
;;
;;         rest-arg.scm:19: fold-
;;         rest-arg.scm:18: reduce         <--

comment:4 Changed 7 weeks ago by sjamaan

Milestone: someday5.3

comment:5 Changed 7 weeks ago by sjamaan

This seems to be a bad interaction between argvector re-use and the rest-arg optimization:

static void C_ccall f_147(C_word c,C_word *av){
C_word tmp;
C_word t0=av[0];
C_word t1=av[1];
C_word t2;
C_word *a;
C_check_for_interrupt;
if(C_unlikely(!C_demand(C_calculate_demand((c-2)*C_SIZEOF_PAIR +0,c,3)))){
C_save_and_reclaim((void*)f_147,c,av);}
a=C_alloc((c-2)*C_SIZEOF_PAIR+0);
t2=C_build_rest(&a,c,2,av);
C_word t3;
C_trace(C_text("test.scm:5: reduce"));
{C_proc tp=(C_proc)C_fast_retrieve_proc(*((C_word*)lf[0]+1));
C_word *av2;
if(c >= 4) {
  av2=av;   // Argvector re-use, av = av2
} else {
  av2=C_alloc(4);
}
av2[0]=*((C_word*)lf[0]+1);
av2[1]=t1;
av2[2]=t2;
av2[3]=C_get_rest_arg(c,2,av,2,t0); // <- Picks av[2], which just got set in the line above!
tp(4,av2);}}

I need to figure out how to best deal with this...

comment:6 Changed 7 weeks ago by sjamaan

In CHICKEN 5.1.0, the function looks like this:

static void C_ccall f_147(C_word c,C_word *av){
C_word tmp;
C_word t0=av[0];
C_word t1=av[1];
C_word t2;
C_word *a;
C_check_for_interrupt;
if(C_unlikely(!C_demand(C_calculate_demand((c-2)*C_SIZEOF_PAIR +0,c,3)))){
C_save_and_reclaim((void*)f_147,c,av);}
a=C_alloc((c-2)*C_SIZEOF_PAIR+0);
t2=C_build_rest(&a,c,2,av);
C_word t3;
C_word t4;
t3=C_i_car(t2); // Access got moved up, a new temporary is assigned.
C_trace(C_text("test.scm:5: reduce"));
{C_proc tp=(C_proc)C_fast_retrieve_proc(*((C_word*)lf[0]+1));
C_word *av2;
if(c >= 4) {
  av2=av;
} else {
  av2=C_alloc(4);
}
av2[0]=*((C_word*)lf[0]+1);
av2[1]=t1;
av2[2]=t2;
av2[3]=t3;
tp(4,av2);}}

comment:7 Changed 5 weeks ago by felix winkelmann

Resolution: fixed
Status: acceptedclosed

This should be fixed with commit da39e738821488c545d17ca2d89db0ad6f65769f.

Note: See TracTickets for help on using tickets.