Re: [AD] rotate.c hang fix

[Sven Sandberg <svens@xxxxxxxxxx>]

Eric Botcazou wrote:
> I can't reproduce on x86 (compiled with GCC), but the x86 has a notoriously
> weird FPU (i.e non IEEE-754 compliant) which may hide the bug.

Yes, this may explain it, because spr_dx/spr_dy are computed using 
floating numbers. Or, the bug may simply be so rarely exposed that 
reproducing it is very hard.

[snip]
> There are three other similar non-symmetrical conditions just below, so I 
> think the attached patch is more complete.
>
> Sven, what do think about this?

Although I don't know any example where the bug occurs, I think I 
understand what the problem might be, and in this case I think your 
patch fixed it. Nice!

I attach another patch that does two things to rotate.c:

 (1) It improves "safety" one step more: by moving the
`if (l_bmp_x_rounded > r_bmp_x_rounded)' tests inside the loops, it 
guarantees that the loops never do more iterations than the width of the 
scanline. Although your patch already fixes the real problem, this makes 
the number of iterations independent of the precision of numbers, for 
sure. And it is easy to see that the new code does the same thing as the 
old.

 (2) It replaces `while(){}' by `do{}while()'. This is clearly valid 
because the condition has already been tested. Although it's ridiculous 
as an optimization, I think it also serves as a clarification.

Can I commit it?

--
Sven

Index: src/rotate.c
===================================================================
RCS file: /cvsroot/alleg/allegro/src/rotate.c,v
retrieving revision 1.15
diff -c -r1.15 rotate.c
*** src/rotate.c	1 Oct 2003 10:24:06 -0000	1.15
--- src/rotate.c	4 Oct 2003 19:10:09 -0000
***************
*** 517,529 ****
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
! 		  while ((unsigned)(l_spr_x_rounded >> 16) >=
! 			 (unsigned)spr->w) {
  		     l_spr_x_rounded += spr_dx;
  		     l_bmp_x_rounded += 65536;
! 		  }
! 		  if (l_bmp_x_rounded > r_bmp_x_rounded)
! 		     goto skip_draw;
  	       }
  	    }
  	    right_edge_test = l_spr_x_rounded +
--- 517,530 ----
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
! 		  do {
  		     l_spr_x_rounded += spr_dx;
  		     l_bmp_x_rounded += 65536;
! 		     if (l_bmp_x_rounded > r_bmp_x_rounded)
! 			goto skip_draw;
! 		  } while ((unsigned)(l_spr_x_rounded >> 16) >=
! 			   (unsigned)spr->w);
! 
  	       }
  	    }
  	    right_edge_test = l_spr_x_rounded +
***************
*** 533,545 ****
  	       if (((right_edge_test < 0) && (spr_dx <= 0)) ||
  		   ((right_edge_test > 0) && (spr_dx >= 0))) {
  		  /* This can happen. */
! 		  while ((unsigned)(right_edge_test >> 16) >=
! 			 (unsigned)spr->w) {
  		     r_bmp_x_rounded -= 65536;
  		     right_edge_test -= spr_dx;
! 		  }
! 		  if (l_bmp_x_rounded > r_bmp_x_rounded)
! 		     goto skip_draw;
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
--- 534,546 ----
  	       if (((right_edge_test < 0) && (spr_dx <= 0)) ||
  		   ((right_edge_test > 0) && (spr_dx >= 0))) {
  		  /* This can happen. */
! 		  do {
  		     r_bmp_x_rounded -= 65536;
  		     right_edge_test -= spr_dx;
! 		     if (l_bmp_x_rounded > r_bmp_x_rounded)
! 			goto skip_draw;
! 		  } while ((unsigned)(right_edge_test >> 16) >=
! 			   (unsigned)spr->w);
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
***************
*** 554,566 ****
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
! 		  while (((unsigned)l_spr_y_rounded >> 16) >=
! 			 (unsigned)spr->h) {
  		     l_spr_y_rounded += spr_dy;
  		     l_bmp_x_rounded += 65536;
! 		  }
! 		  if (l_bmp_x_rounded > r_bmp_x_rounded)
! 		     goto skip_draw;
  	       }
  	    }
  	    right_edge_test = l_spr_y_rounded +
--- 555,567 ----
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
! 		  do {
  		     l_spr_y_rounded += spr_dy;
  		     l_bmp_x_rounded += 65536;
! 		     if (l_bmp_x_rounded > r_bmp_x_rounded)
! 			goto skip_draw;
! 		  } while (((unsigned)l_spr_y_rounded >> 16) >=
! 			   (unsigned)spr->h);
  	       }
  	    }
  	    right_edge_test = l_spr_y_rounded +
***************
*** 570,582 ****
  	       if (((right_edge_test < 0) && (spr_dy <= 0)) ||
  		   ((right_edge_test > 0) && (spr_dy >= 0))) {
  		  /* This can happen. */
! 		  while ((unsigned)(right_edge_test >> 16) >=
! 			 (unsigned)spr->h) {
  		     r_bmp_x_rounded -= 65536;
  		     right_edge_test -= spr_dy;
! 		  }
! 		  if (l_bmp_x_rounded > r_bmp_x_rounded)
! 		     goto skip_draw;
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
--- 571,583 ----
  	       if (((right_edge_test < 0) && (spr_dy <= 0)) ||
  		   ((right_edge_test > 0) && (spr_dy >= 0))) {
  		  /* This can happen. */
! 		  do {
  		     r_bmp_x_rounded -= 65536;
  		     right_edge_test -= spr_dy;
! 		     if (l_bmp_x_rounded > r_bmp_x_rounded)
! 			goto skip_draw;
! 		  } while ((unsigned)(right_edge_test >> 16) >=
! 			   (unsigned)spr->h);
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */