[Patch, ARM] Enable libsanitizer

[Patch, ARM] Enable libsanitizer

[Christophe Lyon]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

Hi,
This small patch enables libsanitizer on ARM.
It has been tested successfully on cortex-a9 hardware (via the GCC testsuite).

I have chosen to bundle -funwind-table with -fsanitize=* so that a
useful backtrace can be printed to the user in case of error,
otherwise the reporting is limited to one line belonging to
libsanitizer.so.

Note that the testsuite currently fails when executing under qemu:
- support of /proc/self/maps does not conform to the kernel format.
  One extra space is missing from some lines, which confuses libsanitizer.
  Patch proposed to upstream qemu:
  http://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03051.html

- qemu reserves some memory space by default, conflicting with
libsanitizer needs.
  Workaround: invoke qemu with -R 0

- libsanitizer detects if its output is a tty, and when GCC testsuite
is executed under qemu, libsanitizer concludes that it is actually
running under a tty, and adds beautyfying characters which confuse
dejanu.


OK?

Christophe.

2013-03-27  Christophe Lyon <christophe.lyon@linaro.org>

    gcc/
    * config/arm/arm.c (arm_asan_shadow_offset): New function.
    (TARGET_ASAN_SHADOW_OFFSET): Define.
    * config/arm/linux-eabi.h (ASAN_CC1_SPEC): Define.
    (LINUX_OR_ANDROID_CC): Add ASAN_CC1_SPEC.

    libsanitizer/
    * configure.tgt: Add ARM pattern.

[-- Attachment #2: sanitizer.patch.txt --]
[-- Type: text/plain, Size: 2015 bytes --]

=== modified file 'gcc/config/arm/arm.c'
--- gcc/config/arm/arm.c	2013-02-28 10:26:41 +0000
+++ gcc/config/arm/arm.c	2013-03-04 08:39:02 +0000
@@ -280,6 +280,8 @@
 
 static void arm_canonicalize_comparison (int *code, rtx *op0, rtx *op1,
 					 bool op0_preserve_value);
+static unsigned HOST_WIDE_INT arm_asan_shadow_offset (void);
+
 \f
 /* Table of machine attributes.  */
 static const struct attribute_spec arm_attribute_table[] =
@@ -649,6 +651,9 @@
 #define TARGET_CANONICALIZE_COMPARISON \
   arm_canonicalize_comparison
 
+#undef TARGET_ASAN_SHADOW_OFFSET
+#define TARGET_ASAN_SHADOW_OFFSET arm_asan_shadow_offset
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 /* Obstack for minipool constant handling.  */
@@ -27393,4 +27398,12 @@
 
 }
 
+/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
+
+static unsigned HOST_WIDE_INT
+arm_asan_shadow_offset (void)
+{
+  return (unsigned HOST_WIDE_INT) 1 << 29;
+}
+
 #include "gt-arm.h"

=== modified file 'gcc/config/arm/linux-eabi.h'
--- gcc/config/arm/linux-eabi.h	2013-01-10 20:38:27 +0000
+++ gcc/config/arm/linux-eabi.h	2013-03-26 09:59:11 +0000
@@ -84,10 +84,14 @@
   LINUX_OR_ANDROID_LD (LINUX_TARGET_LINK_SPEC,				\
 		       LINUX_TARGET_LINK_SPEC " " ANDROID_LINK_SPEC)
 
+#undef  ASAN_CC1_SPEC
+#define ASAN_CC1_SPEC "%{fsanitize=*:-funwind-tables}"
+
 #undef  CC1_SPEC
 #define CC1_SPEC							\
-  LINUX_OR_ANDROID_CC (GNU_USER_TARGET_CC1_SPEC,			\
-		       GNU_USER_TARGET_CC1_SPEC " " ANDROID_CC1_SPEC)
+  LINUX_OR_ANDROID_CC (GNU_USER_TARGET_CC1_SPEC " " ASAN_CC1_SPEC,	\
+		       GNU_USER_TARGET_CC1_SPEC " " ASAN_CC1_SPEC " "	\
+		       ANDROID_CC1_SPEC)
 
 #define CC1PLUS_SPEC \
   LINUX_OR_ANDROID_CC ("", ANDROID_CC1PLUS_SPEC)

=== modified file 'libsanitizer/configure.tgt'
--- libsanitizer/configure.tgt	2013-02-11 23:13:37 +0000
+++ libsanitizer/configure.tgt	2013-03-04 08:39:02 +0000
@@ -29,6 +29,8 @@
 	;;
   sparc*-*-linux*)
 	;;
+  arm*-*-linux*)
+	;;
   x86_64-*-darwin[1]* | i?86-*-darwin[1]*)
 	TSAN_SUPPORTED=no
 	;;

Re: [Patch, ARM] Enable libsanitizer

[Konstantin Serebryany]

+eugenis@google.com

Hi Christophe,

On Thu, Mar 28, 2013 at 2:09 AM, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> Hi,
> This small patch enables libsanitizer on ARM.
> It has been tested successfully on cortex-a9 hardware (via the GCC testsuite).
>
> I have chosen to bundle -funwind-table with -fsanitize=* so that a
> useful backtrace can be printed to the user in case of error,
> otherwise the reporting is limited to one line belonging to
> libsanitizer.so.
>
> Note that the testsuite currently fails when executing under qemu:
> - support of /proc/self/maps does not conform to the kernel format.
>   One extra space is missing from some lines, which confuses libsanitizer.
>   Patch proposed to upstream qemu:
>   http://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03051.html

As we discussed in
https://code.google.com/p/address-sanitizer/issues/detail?id=160
this may be fixed in libsanitizer, although i'd still prefer the qemu fix.

>
> - qemu reserves some memory space by default, conflicting with
> libsanitizer needs.
>   Workaround: invoke qemu with -R 0

Good to know!

>
> - libsanitizer detects if its output is a tty, and when GCC testsuite
> is executed under qemu, libsanitizer concludes that it is actually
> running under a tty, and adds beautyfying characters which confuse
> dejanu.

Is this again a quemu problem?
Or should we do some more checks before emitting color codes?

A comment about this patch and a question to Evgeniy:
on Android/ARM we use zero shadow offset.
(code.google.com/p/address-sanitizer/wiki/ZeroBasedShadow)
Can we do it on other ARM targets too?

--kcc

>
>
> OK?
>
> Christophe.
>
> 2013-03-27  Christophe Lyon <christophe.lyon@linaro.org>
>
>     gcc/
>     * config/arm/arm.c (arm_asan_shadow_offset): New function.
>     (TARGET_ASAN_SHADOW_OFFSET): Define.
>     * config/arm/linux-eabi.h (ASAN_CC1_SPEC): Define.
>     (LINUX_OR_ANDROID_CC): Add ASAN_CC1_SPEC.
>
>     libsanitizer/
>     * configure.tgt: Add ARM pattern.

Re: [Patch, ARM] Enable libsanitizer

[Evgeniy Stepanov]

On Thu, Mar 28, 2013 at 11:36 AM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> +eugenis@google.com
>
> Hi Christophe,
>
> On Thu, Mar 28, 2013 at 2:09 AM, Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
>> Hi,
>> This small patch enables libsanitizer on ARM.
>> It has been tested successfully on cortex-a9 hardware (via the GCC testsuite).
>>
>> I have chosen to bundle -funwind-table with -fsanitize=* so that a
>> useful backtrace can be printed to the user in case of error,
>> otherwise the reporting is limited to one line belonging to
>> libsanitizer.so.
>>
>> Note that the testsuite currently fails when executing under qemu:
>> - support of /proc/self/maps does not conform to the kernel format.
>>   One extra space is missing from some lines, which confuses libsanitizer.
>>   Patch proposed to upstream qemu:
>>   http://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03051.html
>
> As we discussed in
> https://code.google.com/p/address-sanitizer/issues/detail?id=160
> this may be fixed in libsanitizer, although i'd still prefer the qemu fix.
>
>>
>> - qemu reserves some memory space by default, conflicting with
>> libsanitizer needs.
>>   Workaround: invoke qemu with -R 0
>
> Good to know!
>
>>
>> - libsanitizer detects if its output is a tty, and when GCC testsuite
>> is executed under qemu, libsanitizer concludes that it is actually
>> running under a tty, and adds beautyfying characters which confuse
>> dejanu.
>
> Is this again a quemu problem?
> Or should we do some more checks before emitting color codes?
>
> A comment about this patch and a question to Evgeniy:
> on Android/ARM we use zero shadow offset.
> (code.google.com/p/address-sanitizer/wiki/ZeroBasedShadow)
> Can we do it on other ARM targets too?

We do it because newer versions of Android use PIE binaries, and,
combined with other specifics of address space on Linux/ARM, there is
no space for ASan shadow anywhere else. And it's faster.

Zero-based shadow requires PIE. Non-zero-based requires non-PIE on
Android. Is it the same with QEMU? If so, we should switch to
zero-based for uniformity and performance.

>
> --kcc
>
>>
>>
>> OK?
>>
>> Christophe.
>>
>> 2013-03-27  Christophe Lyon <christophe.lyon@linaro.org>
>>
>>     gcc/
>>     * config/arm/arm.c (arm_asan_shadow_offset): New function.
>>     (TARGET_ASAN_SHADOW_OFFSET): Define.
>>     * config/arm/linux-eabi.h (ASAN_CC1_SPEC): Define.
>>     (LINUX_OR_ANDROID_CC): Add ASAN_CC1_SPEC.
>>
>>     libsanitizer/
>>     * configure.tgt: Add ARM pattern.

Re: [Patch, ARM] Enable libsanitizer

[Jakub Jelinek]

On Thu, Mar 28, 2013 at 12:00:23PM +0400, Evgeniy Stepanov wrote:
> We do it because newer versions of Android use PIE binaries, and,
> combined with other specifics of address space on Linux/ARM, there is
> no space for ASan shadow anywhere else. And it's faster.
> 
> Zero-based shadow requires PIE. Non-zero-based requires non-PIE on
> Android. Is it the same with QEMU? If so, we should switch to
> zero-based for uniformity and performance.

I don't think most of the arm-linux-gnueabi binaries are PIEs, so using
zero shadow offset would be wrong on Linux.  If 1 << 29 works (e.g. prelink
library area on linux-arm is 0x41000000 .. 0x50000000, so
shadow of 0x20000000 .. 0x3fffffff is fine for that), IMHO we should use it.

	Jakub

Re: [Patch, ARM] Enable libsanitizer

[Konstantin Serebryany]

On Thu, Mar 28, 2013 at 12:07 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 28, 2013 at 12:00:23PM +0400, Evgeniy Stepanov wrote:
>> We do it because newer versions of Android use PIE binaries, and,
>> combined with other specifics of address space on Linux/ARM, there is
>> no space for ASan shadow anywhere else. And it's faster.
>>
>> Zero-based shadow requires PIE. Non-zero-based requires non-PIE on
>> Android. Is it the same with QEMU? If so, we should switch to
>> zero-based for uniformity and performance.
>
> I don't think most of the arm-linux-gnueabi binaries are PIEs, so using
> zero shadow offset would be wrong on Linux.  If 1 << 29 works (e.g. prelink
> library area on linux-arm is 0x41000000 .. 0x50000000, so
> shadow of 0x20000000 .. 0x3fffffff is fine for that), IMHO we should use it.

Do we need two separate offsets for Linux/ARM and Android/ARM?
That's what we have in clang today.

--kcc

>
>         Jakub

Re: [Patch, ARM] Enable libsanitizer

[Christophe Lyon]

>> Note that the testsuite currently fails when executing under qemu:
>> - support of /proc/self/maps does not conform to the kernel format.
>>   One extra space is missing from some lines, which confuses libsanitizer.
>>   Patch proposed to upstream qemu:
>>   http://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03051.html
>
> As we discussed in
> https://code.google.com/p/address-sanitizer/issues/detail?id=160
> this may be fixed in libsanitizer, although i'd still prefer the qemu fix.
>
I'll ping the qemu list.


>>
>> - libsanitizer detects if its output is a tty, and when GCC testsuite
>> is executed under qemu, libsanitizer concludes that it is actually
>> running under a tty, and adds beautyfying characters which confuse
>> dejanu.
>
> Is this again a quemu problem?
I still don't know. I tried to investigate some time ago; I thought it
could be a problem when qemu interprets a ~isatty syscall, but IIRC
this syscall isn't used. So I don't know who finally asnwers to the
isatty() query.

Christophe.

Re: [Patch, ARM] Enable libsanitizer

[Christophe Lyon]

~/src/qemu/qemu-git/arm-linux-user/qemu-arm -cpu cortex-a9 -R 0 -L
/home/lyon/src/GCC/builds/gcc-fsf-asan-arm-none-linux-gnueabihf/sysroot
 ./heap-overflow-1.arm

On 28 March 2013 15:33, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>>> - libsanitizer detects if its output is a tty, and when GCC testsuite
>>> is executed under qemu, libsanitizer concludes that it is actually
>>> running under a tty, and adds beautyfying characters which confuse
>>> dejanu.
>>
>> Is this again a quemu problem?
> I still don't know. I tried to investigate some time ago; I thought it
> could be a problem when qemu interprets a ~isatty syscall, but IIRC
> this syscall isn't used. So I don't know who finally asnwers to the
> isatty() query.
>

After a bit more debugging, the libsanitizer query isatty(2) is
handled by glibc as a call to __tcgetattr(2,...), which is turned into
ioctl(2, TCGETS,...).

Qemu issues the same query to the host, and I have observed that when
called by expect, qemu's fd 2 points to /dev/pts/XXX which explains
why it thinks it is a tty.

So far I have haven't look at what actually happens when executed on a
board, but why would expect behave differently?

Christophe.

Re: [Patch, ARM] Enable libsanitizer

[Christophe Lyon]

On 4 April 2013 14:19, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> ~/src/qemu/qemu-git/arm-linux-user/qemu-arm -cpu cortex-a9 -R 0 -L
> /home/lyon/src/GCC/builds/gcc-fsf-asan-arm-none-linux-gnueabihf/sysroot
>  ./heap-overflow-1.arm
>
> On 28 March 2013 15:33, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>>>> - libsanitizer detects if its output is a tty, and when GCC testsuite
>>>> is executed under qemu, libsanitizer concludes that it is actually
>>>> running under a tty, and adds beautyfying characters which confuse
>>>> dejanu.
>>>
>>> Is this again a quemu problem?
>> I still don't know. I tried to investigate some time ago; I thought it
>> could be a problem when qemu interprets a ~isatty syscall, but IIRC
>> this syscall isn't used. So I don't know who finally asnwers to the
>> isatty() query.
>>
>
> After a bit more debugging, the libsanitizer query isatty(2) is
> handled by glibc as a call to __tcgetattr(2,...), which is turned into
> ioctl(2, TCGETS,...).
>
> Qemu issues the same query to the host, and I have observed that when
> called by expect, qemu's fd 2 points to /dev/pts/XXX which explains
> why it thinks it is a tty.
>
> So far I have haven't look at what actually happens when executed on a
> board, but why would expect behave differently?
>

After debugging on ARM hardware, I noticed that in this latter case,
runtest uses "unix" as target, and uses pipes to communicate with the
test program, while it creates ptys when using a simulator.
By adding the "readonly" flag in sim_spawn
(/usr/share/dejagnu/config/sim.exp) I managed to have these tests pass
under qemu, but I don't know if this is safe or if there is a better
way of achieving the same effect.

However, doing this makes other tests fail "harder": the tests
involving clone() fail when run under qemu, but when the "readonly"
flag is set, qemu sometimes fail in timeout rather than exiting with
an error code. Threads in general are not well supported by qemu
(there are other random failures in GCC testsuite related to this).

So.... maybe it would be better to skip asan tests when running under
qemu: is the GCC testsuite aware of being run under qemu?

Thanks for any suggestion.

Christophe.

Re: [Patch, ARM] Enable libsanitizer

[Christophe Lyon]

On 18 April 2013 11:30, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> On 4 April 2013 14:19, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>> ~/src/qemu/qemu-git/arm-linux-user/qemu-arm -cpu cortex-a9 -R 0 -L
>> /home/lyon/src/GCC/builds/gcc-fsf-asan-arm-none-linux-gnueabihf/sysroot
>>  ./heap-overflow-1.arm
>>
>> On 28 March 2013 15:33, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>>>>> - libsanitizer detects if its output is a tty, and when GCC testsuite
>>>>> is executed under qemu, libsanitizer concludes that it is actually
>>>>> running under a tty, and adds beautyfying characters which confuse
>>>>> dejanu.
>>>>
>>>> Is this again a quemu problem?
>>> I still don't know. I tried to investigate some time ago; I thought it
>>> could be a problem when qemu interprets a ~isatty syscall, but IIRC
>>> this syscall isn't used. So I don't know who finally asnwers to the
>>> isatty() query.
>>>
>>
>> After a bit more debugging, the libsanitizer query isatty(2) is
>> handled by glibc as a call to __tcgetattr(2,...), which is turned into
>> ioctl(2, TCGETS,...).
>>
>> Qemu issues the same query to the host, and I have observed that when
>> called by expect, qemu's fd 2 points to /dev/pts/XXX which explains
>> why it thinks it is a tty.
>>
>> So far I have haven't look at what actually happens when executed on a
>> board, but why would expect behave differently?
>>
>
> After debugging on ARM hardware, I noticed that in this latter case,
> runtest uses "unix" as target, and uses pipes to communicate with the
> test program, while it creates ptys when using a simulator.
> By adding the "readonly" flag in sim_spawn
> (/usr/share/dejagnu/config/sim.exp) I managed to have these tests pass
> under qemu, but I don't know if this is safe or if there is a better
> way of achieving the same effect.
>
> However, doing this makes other tests fail "harder": the tests
> involving clone() fail when run under qemu, but when the "readonly"
> flag is set, qemu sometimes fail in timeout rather than exiting with
> an error code. Threads in general are not well supported by qemu
> (there are other random failures in GCC testsuite related to this).
>
> So.... maybe it would be better to skip asan tests when running under
> qemu: is the GCC testsuite aware of being run under qemu?
>
> Thanks for any suggestion.
>

Replying to myself:
I have added a new check_effective_target_hw function in the
testsuite, to enable to skip tests when running under a simulator.
My concern is that in this particular case, it's a problem with a
particular simulator (qemu-user), not simulators in general (ie it's
not a speed problem).

So, should/could I add another variable that people would set when using qemu?

For the record, we have to skip clone-test-1.c and
rlimit-mmap-test-1.c because qemu isn't currently able to run them
(whatever the target).

Regarding the isatty(2) problem discussed earlier, I can workaround it
by modifying the regexps in dg-output clauses such as:
-/* { dg-output "AddressSanitizer can not provide additional
info.*(\n|\r\n|\r)" } */
+/* { dg-output ".*AddressSanitizer can not provide additional
info.*(\n|\r\n|\r)" } */

What do people think about this proposal?

Christophe.

Re: [Patch, ARM] Enable libsanitizer

[Konstantin Serebryany]

On Fri, Apr 19, 2013 at 7:59 AM, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> On 18 April 2013 11:30, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>> On 4 April 2013 14:19, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>>> ~/src/qemu/qemu-git/arm-linux-user/qemu-arm -cpu cortex-a9 -R 0 -L
>>> /home/lyon/src/GCC/builds/gcc-fsf-asan-arm-none-linux-gnueabihf/sysroot
>>>  ./heap-overflow-1.arm
>>>
>>> On 28 March 2013 15:33, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>>>>>> - libsanitizer detects if its output is a tty, and when GCC testsuite
>>>>>> is executed under qemu, libsanitizer concludes that it is actually
>>>>>> running under a tty, and adds beautyfying characters which confuse
>>>>>> dejanu.
>>>>>
>>>>> Is this again a quemu problem?
>>>> I still don't know. I tried to investigate some time ago; I thought it
>>>> could be a problem when qemu interprets a ~isatty syscall, but IIRC
>>>> this syscall isn't used. So I don't know who finally asnwers to the
>>>> isatty() query.
>>>>
>>>
>>> After a bit more debugging, the libsanitizer query isatty(2) is
>>> handled by glibc as a call to __tcgetattr(2,...), which is turned into
>>> ioctl(2, TCGETS,...).
>>>
>>> Qemu issues the same query to the host, and I have observed that when
>>> called by expect, qemu's fd 2 points to /dev/pts/XXX which explains
>>> why it thinks it is a tty.
>>>
>>> So far I have haven't look at what actually happens when executed on a
>>> board, but why would expect behave differently?
>>>
>>
>> After debugging on ARM hardware, I noticed that in this latter case,
>> runtest uses "unix" as target, and uses pipes to communicate with the
>> test program, while it creates ptys when using a simulator.
>> By adding the "readonly" flag in sim_spawn
>> (/usr/share/dejagnu/config/sim.exp) I managed to have these tests pass
>> under qemu, but I don't know if this is safe or if there is a better
>> way of achieving the same effect.
>>
>> However, doing this makes other tests fail "harder": the tests
>> involving clone() fail when run under qemu, but when the "readonly"
>> flag is set, qemu sometimes fail in timeout rather than exiting with
>> an error code. Threads in general are not well supported by qemu
>> (there are other random failures in GCC testsuite related to this).
>>
>> So.... maybe it would be better to skip asan tests when running under
>> qemu: is the GCC testsuite aware of being run under qemu?
>>
>> Thanks for any suggestion.
>>
>
> Replying to myself:
> I have added a new check_effective_target_hw function in the
> testsuite, to enable to skip tests when running under a simulator.
> My concern is that in this particular case, it's a problem with a
> particular simulator (qemu-user), not simulators in general (ie it's
> not a speed problem).
>
> So, should/could I add another variable that people would set when using qemu?
>
> For the record, we have to skip clone-test-1.c and
> rlimit-mmap-test-1.c because qemu isn't currently able to run them
> (whatever the target).
>
> Regarding the isatty(2) problem discussed earlier, I can workaround it

For the issue with colors: Evgeniy, you probably want to implement
something like ASAN_OPTIONS=color=0
to fight https://code.google.com/p/address-sanitizer/issues/detail?id=174
This will also help with quemu.

> by modifying the regexps in dg-output clauses such as:
> -/* { dg-output "AddressSanitizer can not provide additional
> info.*(\n|\r\n|\r)" } */
> +/* { dg-output ".*AddressSanitizer can not provide additional
> info.*(\n|\r\n|\r)" } */
>
> What do people think about this proposal?
>
> Christophe.