MIPS ASAN status? (and "volunteering")

MIPS ASAN status? (and "volunteering")

[Hans-Peter Nilsson]

There's no address-sanitizer support for MIPS (in particular for
O32) on trunk, at least not when building for
mipsisa32r2el-linux-gnu and libsanitizer/configure.tgt seems
to support that observation.  There's a set of patches "floating
around", but the last sign of work-in-progress was more than
four years ago, according to a well-known search engine.

Is there something holding it up getting it into trunk gcc?
Is it just that someone needs to go the last mile?

I can do that.  I can even go two miles!  Maybe even a merge
from compiler-rt and MIPS port hacking (to be merged post
gcc-8-branch to trunk, I presume).

I'm a little worried that the "patches floating around" have
unclear copyright status, so I haven't looked at them yet.  I'd
rather not re-do MIPS ASAN on the gcc-side from scratch, but if
it comes to that, so be it.

brgds, H-P

Re: MIPS ASAN status? (and "volunteering")

[Jean Lee]

2018-03-02 7:53 GMT+08:00 Hans-Peter Nilsson <hans-peter.nilsson@axis.com>:

> There's no address-sanitizer support for MIPS (in particular for
> O32) on trunk, at least not when building for
> mipsisa32r2el-linux-gnu and libsanitizer/configure.tgt seems
> to support that observation.  There's a set of patches "floating
> around", but the last sign of work-in-progress was more than
> four years ago, according to a well-known search engine.
>
> Is there something holding it up getting it into trunk gcc?
> Is it just that someone needs to go the last mile?
>
> I can do that.  I can even go two miles!  Maybe even a merge
> from compiler-rt and MIPS port hacking (to be merged post
> gcc-8-branch to trunk, I presume).
>
> I'm a little worried that the "patches floating around" have
> unclear copyright status, so I haven't looked at them yet.  I'd
> rather not re-do MIPS ASAN on the gcc-side from scratch, but if
> it comes to that, so be it.
>
> brgds, H-P
>

It is great to go the last mile. I had done the port  to
mipsel-linux-uclibc gcc for GCC 4.9/5.0/6.0.
Maybe I can give some help for you. Let's do the last mile.
To do this, I have some questions.
Should we port to the upstream LLVM first and port them back to GCC?
Maybe some GCC-only files should be targeted directly like gcc/config/ and
even configure.tgt.

Regards,
Jean

Re: MIPS ASAN status? (and "volunteering")

[Hans-Peter Nilsson]

> From: Jean Lee <xiaoyur347@gmail.com>
> Date: Fri, 2 Mar 2018 13:29:39 +0800

> 2018-03-02 7:53 GMT+08:00 Hans-Peter Nilsson <hans-peter.nilsson@axis.com>:
> 
> > There's no address-sanitizer support for MIPS (in particular for
> > O32) on trunk, at least not when building for
> > mipsisa32r2el-linux-gnu and libsanitizer/configure.tgt seems
> > to support that observation.  There's a set of patches "floating
> > around", but the last sign of work-in-progress was more than
> > four years ago, according to a well-known search engine.
> >
> > Is there something holding it up getting it into trunk gcc?
> > Is it just that someone needs to go the last mile?
> >
> > I can do that.  I can even go two miles!  Maybe even a merge
> > from compiler-rt and MIPS port hacking (to be merged post
> > gcc-8-branch to trunk, I presume).
> >
> > I'm a little worried that the "patches floating around" have
> > unclear copyright status, so I haven't looked at them yet.  I'd
> > rather not re-do MIPS ASAN on the gcc-side from scratch, but if
> > it comes to that, so be it.
> >
> > brgds, H-P
> >
> 
> It is great to go the last mile. I had done the port  to
> mipsel-linux-uclibc gcc for GCC 4.9/5.0/6.0.
> Maybe I can give some help for you.

That would be great, thanks in advance!

First a few troublesome questions:

Are you the sole copyright owner of the patches to gcc that you
know of?  (Not including compiler-rt patches, those are for the
compiler-rt people to worry about; I don't know their process.)

If so, have you copyright assignment paper work in progress
done or in progress with the FSF for gcc?  (From what I can
tell, if so, it's not completed.)  If not, it'd be great if you
can get that started, it takes quite a while.

I believe that's necessary for gcc-specific parts, but I don't
really decide that.  But, if those patches are small enough to
not requiring paperwork they're probably also uninteresting
enough that I add them as I go by.

> Let's do the last mile.
> To do this, I have some questions.
> Should we port to the upstream LLVM first and port them back to GCC?

I have no interest in LLVM at present, and can't wait, so:
no, not on my behalf.

But the question makes me wonder: are you implying that ASAN
support for MIPS (O32 little-endian) isn't in upstream
Clang/LLVM *too* or do I misunderstand you?  I'm not sure how to
tell myself (save for building it from scratch and trying it,
which I haven't done).

*searches the web again...*

It's not mentioned on
<http://clang.llvm.org/docs/AddressSanitizer.html>, but that
page seems out-of-date (known working platforms not mentioned).
It's mentioned on
<https://github.com/google/sanitizers/wiki/AddressSanitizer> so
maybe I concluded that since MIPS O32 support is not yet in GCC,
it must have been developed using LLVM.  I thought I saw reviews
for 3+ year accepted patches, but looking closer that must have
been mips64 support.

Thanks again for your support!

brgds, H-P

Re: MIPS ASAN status? (and "volunteering")

[Jean Lee]

2018-03-03 21:14 GMT+08:00 Hans-Peter Nilsson <hans-peter.nilsson@axis.com>:

> > From: Jean Lee <xiaoyur347@gmail.com>
> > Date: Fri, 2 Mar 2018 13:29:39 +0800
>
> > 2018-03-02 7:53 GMT+08:00 Hans-Peter Nilsson <
> hans-peter.nilsson@axis.com>:
> >
> > > There's no address-sanitizer support for MIPS (in particular for
> > > O32) on trunk, at least not when building for
> > > mipsisa32r2el-linux-gnu and libsanitizer/configure.tgt seems
> > > to support that observation.  There's a set of patches "floating
> > > around", but the last sign of work-in-progress was more than
> > > four years ago, according to a well-known search engine.
> > >
> > > Is there something holding it up getting it into trunk gcc?
> > > Is it just that someone needs to go the last mile?
> > >
> > > I can do that.  I can even go two miles!  Maybe even a merge
> > > from compiler-rt and MIPS port hacking (to be merged post
> > > gcc-8-branch to trunk, I presume).
> > >
> > > I'm a little worried that the "patches floating around" have
> > > unclear copyright status, so I haven't looked at them yet.  I'd
> > > rather not re-do MIPS ASAN on the gcc-side from scratch, but if
> > > it comes to that, so be it.
> > >
> > > brgds, H-P
> > >
> >
> > It is great to go the last mile. I had done the port  to
> > mipsel-linux-uclibc gcc for GCC 4.9/5.0/6.0.
> > Maybe I can give some help for you.
>
> That would be great, thanks in advance!
>
> First a few troublesome questions:
>
> Are you the sole copyright owner of the patches to gcc that you
> know of?  (Not including compiler-rt patches, those are for the
> compiler-rt people to worry about; I don't know their process.)
>
> If so, have you copyright assignment paper work in progress
> done or in progress with the FSF for gcc?  (From what I can
> tell, if so, it's not completed.)  If not, it'd be great if you
> can get that started, it takes quite a while.
>
> I believe that's necessary for gcc-specific parts, but I don't
> really decide that.  But, if those patches are small enough to
> not requiring paperwork they're probably also uninteresting
> enough that I add them as I go by.
>

My port of MIPS uclibc libsanitizer of gcc-6.x is now put in github.
https://github.com/xiaoyur347/sanitizer/
Much work was done to fix uclibc not MIPS.
If you just need MIPS, maybe two patches are needed.

1.
https://github.com/xiaoyur347/sanitizer/commit/016a8187c47f58de9b059eb77aa00e066aae309c
for gcc/config
2. libsanitizer/configure.tgt
+ mipsel-*-linux*)
    ;;
3.  Other works are for stack unwind, if the stack unwind doesn't call
malloc() at all, it will be easy.


> > Let's do the last mile.
> > To do this, I have some questions.
> > Should we port to the upstream LLVM first and port them back to GCC?
>
> I have no interest in LLVM at present, and can't wait, so:
> no, not on my behalf.
>
> But the question makes me wonder: are you implying that ASAN
> support for MIPS (O32 little-endian) isn't in upstream
> Clang/LLVM *too* or do I misunderstand you?  I'm not sure how to
> tell myself (save for building it from scratch and trying it,
> which I haven't done).
>
> *searches the web again...*
>
> It's not mentioned on
> <http://clang.llvm.org/docs/AddressSanitizer.html>, but that
> page seems out-of-date (known working platforms not mentioned).
> It's mentioned on
> <https://github.com/google/sanitizers/wiki/AddressSanitizer> so
> maybe I concluded that since MIPS O32 support is not yet in GCC,
> it must have been developed using LLVM.  I thought I saw reviews
> for 3+ year accepted patches, but looking closer that must have
> been mips64 support.
>
> Thanks again for your support!
>
> brgds, H-P
>

Re: MIPS ASAN status? (and "volunteering")

[Hans-Peter Nilsson]

TL;DR: see last sentence.

> From: Jean Lee <xiaoyur347@gmail.com>
> Date: Mon, 5 Mar 2018 19:56:59 +0800

> 2018-03-03 21:14 GMT+08:00 Hans-Peter Nilsson <hans-peter.nilsson@axis.com>:
> 
> > > From: Jean Lee <xiaoyur347@gmail.com>
> > > Date: Fri, 2 Mar 2018 13:29:39 +0800
> > > It is great to go the last mile. I had done the port  to
> > > mipsel-linux-uclibc gcc for GCC 4.9/5.0/6.0.
> > > Maybe I can give some help for you.
> >
> > That would be great, thanks in advance!
> >
> > First a few troublesome questions:
> >
> > Are you the sole copyright owner of the patches to gcc that you
> > know of?  (Not including compiler-rt patches, those are for the
> > compiler-rt people to worry about; I don't know their process.)
> >
> > If so, have you copyright assignment paper work in progress
> > done or in progress with the FSF for gcc?  (From what I can
> > tell, if so, it's not completed.)  If not, it'd be great if you
> > can get that started, it takes quite a while.
> >
> > I believe that's necessary for gcc-specific parts, but I don't
> > really decide that.  But, if those patches are small enough to
> > not requiring paperwork they're probably also uninteresting
> > enough that I add them as I go by.
> >
> 
> My port of MIPS uclibc libsanitizer of gcc-6.x is now put in github.

Thanks but sorry, before license and copyright issues are
handled, I'm very reluctant to look at anything I don't know is
upstream or trivial.

> https://github.com/xiaoyur347/sanitizer/
> Much work was done to fix uclibc not MIPS.

Plain bugs fixed or something ASAN-specific?  I looked in the
upstream uclibc-ng repo and mailing-list for the last three
years (a trickle compared to the flood of the gcc lists) but
didn't see a match.

> If you just need MIPS, maybe two patches are needed.
> 
> 1.
> https://github.com/xiaoyur347/sanitizer/commit/016a8187c47f58de9b059eb77aa00e066aae309c
> for gcc/config

See above regarding looking at patches, but I guess you mean
that the patch is trivial, so then I presume it was more or less
the same as this, which is basically a copy-paste from looking
at rs6000 and xtensa.  I checked a typical /proc/self/maps and
guess that 1<<29 probably fits for MIPS o32 too:

------------------
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 00cece2..1d4b087 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -22307,6 +22309,14 @@ mips_constant_alignment (const_tree exp, HOST_WIDE_INT align)
   return align;
 }
 
+/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
+
+static unsigned HOST_WIDE_INT
+mips_asan_shadow_offset (void)
+{
+  return HOST_WIDE_INT_1U << 29;
+}
+
 /* Implement TARGET_STARTING_FRAME_OFFSET.  See mips_compute_frame_info
    for details about the frame layout.  */
 
@@ -22618,6 +22628,9 @@ mips_starting_frame_offset (void)
 #undef TARGET_CONSTANT_ALIGNMENT
 #define TARGET_CONSTANT_ALIGNMENT mips_constant_alignment
 
+#undef TARGET_ASAN_SHADOW_OFFSET
+#define TARGET_ASAN_SHADOW_OFFSET mips_asan_shadow_offset
+
 #undef TARGET_STARTING_FRAME_OFFSET
 #define TARGET_STARTING_FRAME_OFFSET mips_starting_frame_offset
 
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index 84ae675..5802587 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -2289,7 +2289,8 @@ enum reg_class
 
 #define STACK_GROWS_DOWNWARD 1
 
-#define FRAME_GROWS_DOWNWARD flag_stack_protect
+#define FRAME_GROWS_DOWNWARD (flag_stack_protect != 0			\
+			      || (flag_sanitize & SANITIZE_ADDRESS) != 0)
 
 /* Size of the area allocated in the frame to save the GP.  */
------------------

> 2. libsanitizer/configure.tgt
> + mipsel-*-linux*)
>     ;;

Right, that part is as trivial as it gets.  I have something similar:

------------------
diff --git a/libsanitizer/configure.tgt b/libsanitizer/configure.tgt
index 573e3b4..6993da9 100644
--- a/libsanitizer/configure.tgt
+++ b/libsanitizer/configure.tgt
@@ -1,5 +1,5 @@
 # -*- shell-script -*-
-#   Copyright (C) 2012-2017 Free Software Foundation, Inc.
+#   Copyright (C) 2012-2018 Free Software Foundation, Inc.
 
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -45,6 +45,11 @@ case "${target}" in
 	;;
   arm*-*-linux*)
 	;;
+  mips*64*-*-linux*)
+	UNSUPPORTED=1
+	;;
+  mips*-*-linux*)
+	;;
   aarch64*-*-linux*)
 	if test x$ac_cv_sizeof_void_p = x8; then
 		TSAN_SUPPORTED=yes
------------------

...as I can't check any mips64-target and don't want to give the
impression that it'd work, or quite likely causing a failing
build.

With just those changes I got a compilation error that puzzled
me, where perhaps you have a clue: do you have a vmlinux for
mipsel-linux-gnu, and can do "pahole -c 32 -C stat vmlinux"?

I get "size: 144" for both Linux 3.4 and 3.10, and this is not
something I expect to be different for any kernel less than
(say) 10 years old; it'd be a major ABI breakage.  Still, the
one-line commit of upstream compiler-rt r301307 set this to 160
for some reason, and it hasn't changed since then.  At first I
thought this was an o32 vs N32 thing, but I doubt that, with the
many conditionals on o32 in that library.  (I'll discuss this
with them as soon as I got everything else working.)

Anyway, with that fixed, I have just a trivial fix for a
gcc-warning regarding structure initialization.  To wit, the
above plus the following gets me through the build for a
mipsisa32r2el-linux-gnu toolchain (that's basically the
"mipsel-linux-gnu" you have, but also setting default_mips_arch
to mips32r2 as opposed to "mips1").  I'm using glibc-2.19
together with that, which might seem old, but it gets me a
ASAN-enabled toolchain for arm-linux-gnueabihf which works for
trivial examples, just to compare.

To wit, I use what I pasted above plus:

------------------
diff --git a/libsanitizer/sanitizer_common/sanitizer_atomic_clang_other.h b/libsanitizer/sanitizer_common/sanitizer_atomic_clang_other.h
index a0605bb..59e9f64 100644
--- a/libsanitizer/sanitizer_common/sanitizer_atomic_clang_other.h
+++ b/libsanitizer/sanitizer_common/sanitizer_atomic_clang_other.h
@@ -35,7 +35,7 @@ static void __spin_unlock(volatile int *lock) { __sync_lock_release(lock); }
 static struct {
   int lock;
   char pad[32 - sizeof(int)];
-} __attribute__((aligned(32))) lock = {0};
+} __attribute__((aligned(32))) lock = {0, {0}};
 
 template <class T>
 T __mips_sync_fetch_and_add(volatile T *ptr, T val) {
diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
index 4d11d07..132e5e9 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -86,7 +86,7 @@ namespace __sanitizer {
 #elif defined(__mips__)
   const unsigned struct_kernel_stat_sz =
                  SANITIZER_ANDROID ? FIRST_32_SECOND_64(104, 128) :
-                                     FIRST_32_SECOND_64(160, 216);
+                                     FIRST_32_SECOND_64(144, 216);
   const unsigned struct_kernel_stat64_sz = 104;
 #elif defined(__s390__) && !defined(__s390x__)
   const unsigned struct_kernel_stat_sz = 64;
------------------

> 3.  Other works are for stack unwind, if the stack unwind doesn't call
> malloc() at all, it will be easy.

Heh.  I'll settle for a gcc that passes important ASAN-specific
test-suite parts (like g++.dg/asan) and works the first few
examples from web pages I can find.

Right now, I'm facing SEGV for trivial hello-world-type programs
(both with and without flaws expected to be caught by ASAN), so
I guess there's more to it.  I'll dig a little using gdb and if
that doesn't get me anywhere, I think I'll try 1) using a newer
snapshot (I'm using gcc-8-20180225) and 2) merging upstream
compiler-rt using the methods in libsanitizer/HOWTO_MERGE,
locally; I see non-trivial changes upstream.

If you (or anyone else) has a clue on the kernel-stat size
160 vs. 144 issue, or can point out "usual suspects" when always
getting SEGV with -fsanitize=address, I'm very interested.

brgds, H-P

Re: MIPS ASAN status? (and "volunteering")

[Jean Lee]

> See above regarding looking at patches, but I guess you mean
> that the patch is trivial, so then I presume it was more or less
> the same as this, which is basically a copy-paste from looking
> at rs6000 and xtensa.  I checked a typical /proc/self/maps and
> guess that 1<<29 probably fits for MIPS o32 too:

No, Not more or less the same as a copy-paste from rs6000.
mips_asan_shadow_offset should use 0x0aaa0000 as defined in
libsanitizer/sanitizer_common/asan_mapping.h
static const u64 kMIPS32_ShadowOffset32 = 0x0aaa0000;
Maybe it is the main reason of your crash.

>-                                     FIRST_32_SECOND_64(160, 216);
>+                                     FIRST_32_SECOND_64(144, 216);
>If you (or anyone else) has a clue on the kernel-stat size
>160 vs. 144 issue, or can point out "usual suspects" when always
>getting SEGV with -fsanitize=address, I'm very interested.
struct_kernel_stat_sz was once 144 for MIPS but it changes to 160 after
this commit
https://llvm.org/viewvc/llvm-project?view=revision&revision=301307
or
https://github.com/llvm-mirror/compiler-rt/commit/e80e955171a57aa3be9a9d03404a7e9652b8a78d
Maybe you should ask llvm for help.

Regards.

Re: MIPS ASAN status? (and "volunteering")

[Hans-Peter Nilsson]

H.J.: please see last.

> From: Jean Lee <xiaoyur347@gmail.com>
> Date: Sat, 10 Mar 2018 20:22:45 +0800

> > See above regarding looking at patches, but I guess you mean
> > that the patch is trivial, so then I presume it was more or less
> > the same as this, which is basically a copy-paste from looking
> > at rs6000 and xtensa.  I checked a typical /proc/self/maps and
> > guess that 1<<29 probably fits for MIPS o32 too:
> 
> No, Not more or less the same as a copy-paste from rs6000.
> mips_asan_shadow_offset should use 0x0aaa0000 as defined in
> libsanitizer/sanitizer_common/asan_mapping.h
> static const u64 kMIPS32_ShadowOffset32 = 0x0aaa0000;

Thanks!
I really should have inspected the library to verify a match for
my guess for TARGET_ASAN_SHADOW_OFFSET!  That's an odd number though.

> Maybe it is the main reason of your crash.

Yes, it does!  Off to testing now.

> >-                                     FIRST_32_SECOND_64(160, 216);
> >+                                     FIRST_32_SECOND_64(144, 216);
> >If you (or anyone else) has a clue on the kernel-stat size
> >160 vs. 144 issue, or can point out "usual suspects" when always
> >getting SEGV with -fsanitize=address, I'm very interested.
> struct_kernel_stat_sz was once 144 for MIPS but it changes to 160 after
> this commit
> https://llvm.org/viewvc/llvm-project?view=revision&revision=301307

Right, that's what I meant by "the one-line commit of upstream
compiler-rt r301307 set this to 160 for some reason".

> or
> https://github.com/llvm-mirror/compiler-rt/commit/e80e955171a57aa3be9a9d03404a7e9652b8a78d
> Maybe you should ask llvm for help.

Actually, I've got that part figured out.  The compiler-rt
commit of r301307 has the commit-log "[Compiler-rt][MIPS] Fix
assert introduce with commit rl301171."  Now, the rl301171 is
not a typo:ed plain svn revision number, but LLVM-lingo for a
svn commit to LLVM proper.  This commit changed LLVM to compile
with -D_FILE_OFFSET_BITS=64 (thanks to my colleague Rabin
Vincent for this observation).

This should ring a bell: _FILE_OFFSET_BITS affects *user* struct
stat (on 32-bit platforms) but definitely not *kernel* struct
stat.  Indeed, when looking, the bug is apparently that
libsanitizer/sanitizer_common/sanitizer_platform_limits_linux.cc
has:

#if defined(__x86_64__) ||  defined(__mips__)
#include <sys/stat.h>
#else
#define ino_t __kernel_ino_t
#define mode_t __kernel_mode_t
#define nlink_t __kernel_nlink_t
#define uid_t __kernel_uid_t
#define gid_t __kernel_gid_t
#define off_t __kernel_off_t
#define time_t __kernel_time_t
// This header seems to contain the definitions of _kernel_ stat* structs.
#include <asm/stat.h>
#undef ino_t
...
<compiletime-asserting certain values for sizeof(struct stat)>

...and that this wasn't adjusted when -D_FILE_OFFSET_BITS=64 was
added.  For x86_64 this is unimportant, as "struct stat" does
not change size if sizeof(long) == 8 (or rather, struct stat64
doesn't exist).  IMHO, that "#include <sys/stat.h>" should not
be there, for any platform.  I guess it's supposed to be a
workaround for <asm/stat.h> not being compilable at one time for
x86_64 and then the "|| defined(__mips__)" was added later
without deeper analysis, but something else really should be
done and <asm/stat.h> be used everywhere without ifdeffery.
I'll see if I get time to address this or just pile on.
Thankfully sanitizer_platform_limits_linux.cc doesn't do much
besides those sizeof-asserts.

brgds, H-P