| Re: [AD] x color conversion again |
[ Thread Index |
Date Index
| More lists.liballeg.org/allegro-developers Archives
]
On Thu, 2004-07-15 at 17:48 +0200, Elias Pschernig wrote:
> On Tue, 2004-07-13 at 00:13 +0200, Elias Pschernig wrote:
> > Hm, almost forgot, playing around with the signals version I noticed
> > that it doesn't work at all with the color conversion patch. I suspect,
> > it is missing some XLOCK somewhere or something.
> >
>
> Ok, finally we could track this down. The asm converters use this:
>
> #define LOCAL1 -4(%esp)
> #define LOCAL2 -8(%esp)
> #define LOCAL3 -12(%esp)
> #define LOCAL4 -16(%esp)
>
> Now, a signal occurs inside the asm converter. It will overwrite all the
> locals, since it only restores ESP itself, but not the memory it is
> pointing to.
> On return, we get a crash.
>
> Now, the solution is, Bob or Eric or someone who knows the asm code
> should rewrite it to not access locals outside of ESP, or we don't use
> the asm color converters with the signals version.
>
With the help of Bob, I made a patch for the asm code now. I'm a bit
unsure about committing it though. Especially with the MYLOCALS, I may
be overlooking something, because of that "reserve room for ONE push"
comment. I looked everywhere MYLOCAL1 is used, but couldn't find that
ONE push - so I added another comment stating I was confused by it, and
ignored it.
But in any case, I don't get any crashes anymore with that patch (only
tested 8,15,16,24->32 conversion). Oh, and Bob reminded me that Angelo
also is an x86 asm expert, so there should be plenty of people around to
look at it :)
--
Elias Pschernig
Index: src/misc/icolconv.s
===================================================================
RCS file: /cvsroot/alleg/allegro/src/misc/icolconv.s,v
retrieving revision 1.25
diff -u -p -r1.25 icolconv.s
--- src/misc/icolconv.s 7 Dec 2002 21:13:51 -0000 1.25
+++ src/misc/icolconv.s 19 Jul 2004 23:37:07 -0000
@@ -37,11 +37,12 @@
.byte 0xc0 + 8*dst + src /* mod field */
/* local variables */
-#define LOCAL1 -4(%esp)
-#define LOCAL2 -8(%esp)
-#define LOCAL3 -12(%esp)
-#define LOCAL4 -16(%esp)
-
+#define LOCAL1 12(%esp)
+#define LOCAL2 8(%esp)
+#define LOCAL3 4(%esp)
+#define LOCAL4 0(%esp)
+#define RESERVE_LOCALS subl $16, %esp
+#define CLEANUP_LOCALS addl $16, %esp
/* helper macros */
#define INIT_CONVERSION_1(mask_red, mask_green, mask_blue) \
@@ -125,6 +126,8 @@ FUNC (_colorconv_blit_8_to_16)
pushl %esi
pushl %edi
+ RESERVE_LOCALS
+
/* init register values */
movl ARG1, %esi /* esi = src_rect */
@@ -234,6 +237,8 @@ FUNC (_colorconv_blit_8_to_16)
movl %edx, LOCAL1
jnz next_line_8_to_16
+ CLEANUP_LOCALS
+
emms
popl %edi
popl %esi
@@ -258,6 +263,8 @@ FUNC (_colorconv_blit_8_to_32)
pushl %esi
pushl %edi
+ RESERVE_LOCALS
+
/* init register values */
movl ARG1, %esi /* esi = src_rect */
@@ -367,6 +374,7 @@ FUNC (_colorconv_blit_8_to_32)
movl %edx, LOCAL1
jnz next_line_8_to_32
+ CLEANUP_LOCALS
emms
popl %edi
popl %esi
@@ -1396,25 +1404,32 @@ FUNC (_colorconv_blit_32_to_24)
/* optimized for Intel Pentium */
/********************************************************************************************/
+/* reserve storage for ONE 32-bit push on the stack */
+/* I have no idea what the above means, so if someone is reading this, there may very well
+ * be something wrong now. Don't be surprised, and send a patch :)
+ */
+#define MYLOCAL1 8(%esp)
+#define MYLOCAL2 4(%esp)
+#define MYLOCAL3 0(%esp)
+#define RESERVE_MYLOCALS subl $16, %esp
+#define CLEANUP_MYLOCALS addl $16, %esp
+
/* create the (pseudo - we need %ebp) stack frame */
#define CREATE_STACK_FRAME \
pushl %ebp ; \
movl %esp, %ebp ; \
pushl %ebx ; \
pushl %esi ; \
- pushl %edi
+ pushl %edi ; \
+ RESERVE_MYLOCALS
#define DESTROY_STACK_FRAME \
+ CLEANUP_MYLOCALS ; \
popl %edi ; \
popl %esi ; \
popl %ebx ; \
popl %ebp
-/* reserve storage for ONE 32-bit push on the stack */
-#define MYLOCAL1 -8(%esp)
-#define MYLOCAL2 -12(%esp)
-#define MYLOCAL3 -16(%esp)
-
/* initialize the registers */
#define SIZE_1
#define SIZE_2 addl %ebx, %ebx