RE: [AD] x color conversion again

["Robert Ohannessian" <ROhannessian@xxxxxxxxxx>]

%esp only needs to be aligned to 4-byte boundaries.

> -----Original Message-----
> From: alleg-developers-admin@xxxxxxxxxx [mailto:alleg-
> developers-admin@xxxxxxxxxx] On Behalf Of Elias Pschernig
> Sent: Saturday, July 24, 2004 3:48 AM
> To: alleg-developers
> Subject: Re: [AD] x color conversion again
> 
> On Sat, 2004-07-24 at 09:43 +0200, Eric Botcazou wrote:
> > > > 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.
> >
> > Elias, I'm terribly sorry, I missed your message from 07/15 about
this
> stack
> > pointer frobbing.  I skim through [AD] these days, so please CC me
> > explicitly when you expect an answer from me.  Thanks in advance.
> >
> 
> Well, no problem. I expected you to be busy otherwise, and I wasn't
> quite sure if you, Angelo or Bob is the most of an x86 asm expert :)
> 
> > Yes, the stack pointer handling is terminally broken here.  In my
own
> > defense, I borrowed it from Isaac's code and was young when I wrote
it.
> :-)
> >
> 
> Heh. Does this mean, there might be other code like that? Like, the
asm
> blitters and so on. But since there's no more crashes in the signals
> version, I guess not.
> 
> > > 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.
> >
> > The "push" is used here for example:
> >
> > #define CONV_TRUE_TO_8_NO_MMX(name, bytes_ppixel)
> >    _align_
> >    next_line_##name:
> >       movl MYLOCAL1, %ecx
> >       pushl %edx
> >
> >       _align_
> >       next_block_##name:
> >          xorl %edx, %edx
> >          movb (%esi), %al          /* read 1 pixel */
> >          movb 1(%esi), %bl
> >          movb 2(%esi), %dl
> >          shrb $4, %al
> >          addl $bytes_ppixel, %esi
> >          shll $4, %edx
> >          andb $0xf0, %bl
> >          orb  %bl, %al             /* combine to get 4.4.4 */
> >          incl %edi
> >          movb %al, %dl
> >          movb (%ebp, %edx), %dl    /* look it up */
> >          movb %dl, -1(%edi)        /* write 1 pixel */
> >          decl %ecx
> >          jnz next_block_##name
> >
> >       popl %edx
> >       addl MYLOCAL2, %esi
> >       addl MYLOCAL3, %edi
> >
> > This worked because the MYLOCAL? variables are not accessed from
within
> the
> > push/pop pair.  So I think your patch should theoritically work
because
> of
> > the same property.
> >
> > However, I can see a possible problem:
RESERVE_MYLOCALS/CLEANUP_MYLOCALS
> > builds/destroys the frame by decrementing/incrementing 16 from the
stack
> > pointer (I presume the 16 comes from Bob, in order to preserve
16-byte
> > alignment for %esp?).  I'm almost sure that some OSes/ABIs *require*
> %esp to
> > be 16-byte aligned on x86, so I think the code should be fixed in
order
> not
> > to use push/pop.  This probably means something like:
> >
> 
> Well, the 16 was my idea, since the original code said:
> 
> #define LOCAL1   -4(%esp)
> #define LOCAL2   -8(%esp)
> #define LOCAL3   -12(%esp)
> #define LOCAL4   -16(%esp)
> 
> So I assumed adding 16 would be the way to fix it. I must admit, I'm
not
> sure what you mean with 16-byte alignement. Does it mean, I can use
> something like -4(%esp) if and only if %esp is 16-byte aligned?
> 
> > #define MYLOCAL1    12(%esp)
> > #define MYLOCAL2    8(%esp)
> > #define MYLOCAL3    4(%esp)
> > #define SPILL_SLOT    0(%esp)
> >
> > #define RESERVE_MYLOCALS subl $16, %esp
> > #define CLEANUP_MYLOCALS addl $16, %esp
> >
> > #define CONV_TRUE_TO_8_NO_MMX(name, bytes_ppixel)
> >    _align_
> >    next_line_##name:
> >       movl MYLOCAL1, %ecx
> >       movl %edx, SPILL_SLOT
> >       [...]
> >       movl SPILL_SLOT, %edx
> >       addl MYLOCAL2, %esi
> >       addl MYLOCAL3, %edi
> >
> > With the nice side-effect that the MYLOCAL? variables can now be
> accessed
> > from anywhere in the code.
> >
> 
> SPILL_SPOT would be sort of a temp variable?
> 
> > > 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 :)
> >
> > I think I can still test the 24-bit code, I'll do it as soon as I'm
back
> > from vacation.
> >
> 
> Ah, great.
> 
> --
> Elias Pschernig
> 
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by BEA Weblogic Workshop
> FREE Java Enterprise J2EE developer tools!
> Get your free copy of BEA WebLogic Workshop 8.1 today.
> http://ads.osdn.com/?ad_id=4721&alloc_id=10040&op=click
> --
> https://lists.sourceforge.net/lists/listinfo/alleg-developers