[Pixman] [PATCH] Check for NEON using a signal handler to catch SIGILL

[Siarhei Siamashka]

On Monday 13 December 2010 10:01:32 Mike McCormack wrote:
> On 12/10/2010 09:56 PM, Siarhei Siamashka wrote:
> > Where did you encounter this problem and how critical is it for you?
> 
> We're using a build system based on QEMU (scratchbox/scratchbox2).  After
> building debian packages, installing them in the emulated environment
> causes a crash.

OK, I see. I guess making sure that pixman works in QEMU would be good indeed.

Does this build system run pixman test suite as one of the steps? Or is pixman
used in some other way, like rendering some images as part of the build
process?

> >> +        asm volatile (
> >> +		".fpu neon\n"
> >> +		"\tvqadd.u8 d0, d1, d0\n"
> >> +	);
> > 
> > This code is corrupting the value in d0 register, which is not nice. It
> > would be better to use some non-destructive NEON instruction here
> > (something like "vorr.u8 d0, d0, d0" may be a better choice).
> 
> Sure, or add "d0" to the clobber list...

It's kind of nitpicking, but gcc would be allowed to be strict and reject
this code in some cases just because not all ARM processors have VFP registers.
As a related example, adding "mm0" register to the clobber list on x86 results
in a compilation error unless -mmmx option is used. 

> > Now regarding the use of SIGILL method itself, I have come concerns about
> > it:
> > 
> > a) Portability. Especially considering that pixman is a library and can't
> > control the behavior of the application that is using it. My
> > understanding of the discussion at [5] is that not all POSIX
> > implementations provide per-thread signal handlers for synchronous
> > signals (such as SIGILL). Additionally [6] mentions that "In particular,
> > sigaction() and signal() should not be used in the same process to
> > control the same signal.". A comment from somebody who is more familiar
> > with POSIX would be welcome here.
> 
> Seems more portable than reading Linux specific /proc/self/auxv.
> You've got yourself a safer failure case with this /proc test, but it will
> choose the wrong answer on more platforms...

Technically, both of these methods are not fully portable, so being "more"
portable is a joke. The CPU features detection is a horrible mess on ARM in
general, with multiple alternatives available, each having their own
disadvantages. SIGILL has a potential chance to fail in a really nasty
way on some hypothetical non-linux systems, and may even fail on linux
as pointed by Soeren in another post.

BTW, have you tested your patch on some ARM hardware without NEON or ARMv6
instructions support?

I myself just don't like the SIGILL method and would prefer some other
solution if possible. Are there any other ideas?

-- 
Best regards,
Siarhei Siamashka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20101215/602230d8/attachment-0001.pgp>