Re: [AD] x color conversion again

[Elias Pschernig <elias@xxxxxxxxxx>]

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