Re: [AD] x color conversion again

["Eric Botcazou" <ebotcazou@xxxxxxxxxx>]

> > 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.

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. :-)

> 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:

#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.

> 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.

--
Eric Botcazou