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

[Mike McCormack]

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.

> This else branch is taken when _MCS_VER is not defined. But assuming that
> 'sigaction' is available on the target system based on this is not quite
> correct (yes, the original code is also wrong here).

OK, that can be fixed.

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

>> +	asm volatile (
>> +		".arch armv6\n"
>> +		"\tuqadd8 r1, r1, r2\n"
>> +		 : : :"r1", "r2");
>
> And this breaks pixman on armv4t systems. Similar to the older issue reported
> in [2], with some explanations in [3] and a fix in [4].
>
> Basically, 'readelf -a' should not report 'Tag_CPU_arch' to be set to anything
> higher than armv4 for the pixman library.

Will check it out.  Defining the instruction as a hex value would avoid the need
to set the CPU architecture.

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

> b) How can we be even sure that the instruction is implemented by the CPU,
> after trying to use it and not getting SIGILL? It might be just emulated in
> the kernel, similar to how FPU emulation is commonly done. And we may actually
> take a performance hit instead of performance improvement.

If the instructions are emulated by the kernel, then using NEON is not wrong.

Perhaps there are two questions to ask... is NEON available and is it faster than
non-NEON instructions?

> c) Preferably we would like to be able to get a bit more information about
> the CPU. Such as whether PLD instruction is actually useful (not implemented as
> NOP), whether unaligned memory accesses are supported, hardware prefetch
> availability/absence, cache line size, etc. Even though this information is not
> available now, we might ask kernel developers to provide it via aux vector
> later. But this is totally out of question for the SIGILL method.

Pixman already uses the SIGILL mechanism elsewhere in the same file.

Even if this information were added to /proc/self/auxv, it would still only be
present on newer kernels, and only on Linux.

I will work on my patch to see if I can cover some of the issues you raised.

thanks,

Mike