[ros-dev] [Fwd: Re: [ros-diffs] [greatlrd] 15879: fix bug in color fill inline asm code. Did crash cirrus drv, vmware 5 drv and if the buffer was not align. Clean up inline asm code.]

[Hartmut Birr]

-------- Original Message --------
Subject: 	Re: [ros-diffs] [greatlrd] 15879: fix bug in color fill inline
asm code. Did crash cirrus drv, vmware 5 drv and if the buffer was not
align. Clean up inline asm code.
Date: 	Sun, 12 Jun 2005 22:44:48 +0200
From: 	Hartmut Birr <hartmut.birr at gmx.de>
Reply-To: 	ReactOS Diffs List <ros-diffs at reactos.com>
To: 	ReactOS Diffs List <ros-diffs at reactos.com>
References: 	<000001c56f84$4a6757a0$6b01a8c0 at PENELOPE>



greatlrd at svn.reactos.com wrote:

>fix bug in color fill inline asm code. Did crash cirrus drv, vmware 5 drv and if the buffer was not align. Clean up inline asm code.
>
>Modified: trunk/reactos/subsys/win32k/dib/dib16bpp.c
>  
>
> ------------------------------------------------------------------------
> *Modified: trunk/reactos/subsys/win32k/dib/dib16bpp.c*
>
>--- trunk/reactos/subsys/win32k/dib/dib16bpp.c	2005-06-12 19:06:38 UTC (rev 15878)
>+++ trunk/reactos/subsys/win32k/dib/dib16bpp.c	2005-06-12 19:23:40 UTC (rev 15879)
>
>@@ -454,31 +436,34 @@
>  
>
>   ULONG delta = DestSurface->lDelta;
>   ULONG width = (DestRect->right - DestRect->left) ;
>   PULONG pos =  (PULONG) (DestSurface->pvScan0 + DestRect->top * delta + (DestRect->left<<1));
>  
>
>-  color = (color<<16)|(color&0xffff); /* If the color value is "abcd", put "abcdabcd" into color */
>  
>
>+  color = (color&0xffff);  /* If the color value is "abcd", put "abcdabcd" into color */
>+  color += (color<<16);
>  
>
>   
>   for (DestY = DestRect->top; DestY< DestRect->bottom; DestY++)
>  
>
>-  {
>-    int d0, d1, d2;
>  
>
>+  {   
>  
>
>   __asm__ __volatile__ (
>     "  cld\n"
>  
>
>+    "  mov  %0,%%eax\n"
>+    "  mov  %1,%%ebx\n" 
>  
>
>     "  test $0x03, %%edi\n" /* Align to fullword boundary */
>     "  jz   .FL1\n"
>     "  stosw\n"
>  
>
>-    "  dec  %4\n"
>  
>
>+    "  dec  %%ebx\n"
>  
>
>     "  jz   .FL2\n"
>     ".FL1:\n"
>  
>
>-    "  mov  %4,%%ecx\n"     /* Setup count of fullwords to fill */
>  
>
>+    "  mov  %%ebx,%%ecx\n"     /* Setup count of fullwords to fill */
>  
>
>     "  shr  $1,%%ecx\n"
>     "  rep stosl\n"         /* The actual fill */
>  
>
>-    "  test $0x01, %4\n"    /* One left to do at the right side? */
>  
>
>+    "  test $0x01, %%ebx\n"    /* One left to do at the right side? */
>  
>
>     "  jz   .FL2\n"
>     "  stosw\n"
>     ".FL2:\n"
>  
>
>-    : "=&A" (d0), "=&r" (d1), "=&D" (d2)
>-    : "0"(color), "1"(width), "2"(pos)
>-    : "%ecx");
>  
>
>+    : 
>+    : "r" (color), "r" (width), "D" (pos)
>+    : "%eax", "%ecx","%ebx");
>  
>
>      pos =(PULONG)((ULONG_PTR)pos + delta);	 
>   }
>  
>
>+
>  
>
> #else /* _M_IX86 */
> 
> 	for (DestY = DestRect->top; DestY< DestRect->bottom; DestY++)
>  
>
What was wrong with the inline code from rev 15869? Previous my changes
(rev 15858), the width value was only load in the first loop. This has
crashed the vbe driver on real hardware. I've no problems with the
current and the last revision, and with normal and optimized builds. The
only difference is, that your changes needs two registers more on
optimized builds and gcc uses the stack to store some values. There
exist one problem in your changes, gcc doesn't know that edi is changed.
On other optimisation as the current one, it is possible that gcc uses
edi to recalculate the pos value. With optimized build, I mean the
optimisation, if DBG is 0.

- Hartmut