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

[Siarhei Siamashka]

On Thursday 09 December 2010 14:36:15 Mike McCormack wrote:
> Hi All,
> 
> I have run into a problem with pixman's CPU detection code under QEMU ARM.
> 
> When trying to run an executable linked with pixman under QEMU, I get a
> crash like this:
> 
> 21553 open("/proc/self/auxv",O_RDONLY) = 3
> ...
> 21553 read(3,0x7ffdb0,8) = 8
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> Exit reason and status: signal 11
> 
> /proc/self/auxv is from the host x86 kernel, not an ARM kernel, as pixman
> expects. Parsing /proc/self/auxv will only work on Linux with proc
> mounted, and not under QEMU.

Thanks for reminding about this issue.

Yes, it's a known problem related to userspace qemu. Moreover, qemu also was
incorrectly emulating some NEON instructions some time ago (with the patches
available in qemu mailing list at that time) and has problems with threads,
resulting in not being able to pass standard pixman tests in qemu in normal
conditions.

I mentioned this problem earlier [1] along with an experimental patch adding
some extra qemu related checks to cpu features detection code. But getting no
feedback made me think that this userspace qemu interoperability issue was not
causing any real problems for anyone in practice. 

Where did you encounter this problem and how critical is it for you?
 
> The attached patch modified pixman/pixman-cpu.c to use a more direct method
> of detecting the CPU capabilities (i.e. trying to run a NEON instruction)
> 
> This prevents pixman from crashing under QEMU, and detects NEON correctly
> on an ARMv7 target.

Aside from the whole choice of SIGILL catching as the CPU features detection 
method, there are some minor issues in your patch:

>  #else /* linux ELF */
>  
> -#include <stdlib.h>
> -#include <unistd.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
> -#include <sys/mman.h>
> -#include <fcntl.h>
> -#include <string.h>
> -#include <elf.h>
> +#include <signal.h>
> +#include <setjmp.h>

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

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

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



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.

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.

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.



1. http://lists.freedesktop.org/archives/pixman/2010-September/000520.html
2. http://lists.freedesktop.org/archives/pixman/2010-March/000123.html
3. http://lists.freedesktop.org/archives/pixman/2010-March/000128.html
4. http://cgit.freedesktop.org/pixman/commit/?id=68d8d832
5. http://fixunix.com/sgi/111686-signal-handler-question.html
6. http://www.opengroup.org/onlinepubs/009695399/functions/sigaction.html

-- 
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/20101210/9e181e10/attachment.pgp>