[PATCH, RFC] Enable libsanitizer on powerpc{,64}

[PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

Attached below is an initial port of ASAN to powerpc*-linux.
With the patch below along with Jakub's ASAN testsuite patch:

  http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01365.html

ASAN not only builds, but seems to be working.  The lone ASAN
test case does fail, but it seems to be related to us using
_Unwind_Backtrace() and we end up with two extra frames at the
bottom of our stack frame, so we don't match the expected
results.

==47550== ERROR: AddressSanitizer stack-buffer-overflow on address 0x0fffe65540a4 at pc 0xfff769d0d70 bp 0xfffe6553ed0 sp 0xfffe6553fe8
READ of size 1 at 0x0fffe65540a4 thread T0
    #0 0xfff769dc388 in _ZN6__asan13GetStackTraceEPN11__sanitizer10StackTraceEmmm /home/bergner/gcc/gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc:160
    #1 0xfff769df4f0 in __asan_report_error /home/bergner/gcc/gcc-fsf-mainline-asan/libsanitizer/asan/asan_report.cc:471 (discriminator 5)
    #2 0xfff769d0dbc in __interceptor_memcmp /home/bergner/gcc/gcc-fsf-mainline-asan/libsanitizer/asan/asan_interceptors.cc:218 (discriminator 1)
    #3 0x10000b4c in main /home/bergner/gcc/gcc-fsf-mainline-asan/gcc/testsuite/c-c++-common/asan/memcmp-1.c:12

...which doesn't match:

/* { dg-output "    #0 0x\[0-9a-f\]+ (in _*(interceptor_|)memcmp |\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
/* { dg-output "    #1 0x\[0-9a-f\]+ (in _*main|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */

because the frame numbers are off.  Any ideas on how to fix that?

We cannot use the FastUnwindStack, since that uses exclusively the
frame pointer which ppc almost never uses.  We also can have stack
frames above or below the stack pointer, depending on whether the
function is a leaf or not.  And with shrink wrapping, can we even
be sure if the frame pointer is even setup even on those systems
that do use the frame pointer?  I've seen others say we should
use libbacktrace, and I think I have to agree with them for the
reasons above.

One question that I have is that the toplev.c test for port support
tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
defined as (flag_stack_protect != 0), so ASAN only works when we use
-fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
must be false?

I should also note, the code setting the page size is very fragile.
On PPC/PPC64 kernels, you can configure the kernel to use different
page sizes.  My systems are running 64K page sizes, but could just
as easily be 4K or some other number.  Shouldn't we be calling
getpagesize() or sysconf() to query the page size and then computing
the other values from that?

I also don't like all the #ifdef's sprinkling the code.  Can't we
use some configure fragments to help remove many/most of these?

Does anyone have any thoughts on the patch?  Does it look reasonable?

Peter


gcc/
	* config/rs6000/rs6000.c (TARGET_ASAN_SHADOW_OFFSET): Define.
	(rs6000_asan_shadow_offset): New function.

libsanitizer/
	* configure.tgt: Enable build on powerpc and powerpc64 linux.
	* sanitizer_common/sanitizer_common.h (kPageSizeBits): Set for powerpc
	and powerpc64.
	(kPageSize): Likewise.
	(kCacheLineSize): Likewise.
	(kMmapGranularity): Likewise.
	* sanitizer_common/sanitizer_linux.cc (__NR_mmap) Call for powerpc64.
	(__NR_fstat): Likewise.
	* sanitizer_common/sanitizer_stacktrace.cc (patch_pc): Align to 4 bytes.
	* asan/asan_mapping.h (SHADOW_OFFSET): Define for powerpc and powerpc64.
	(kHighMemEnd): Set for powerpc and powerpc64.
	* asan/asan_linux.cc (GetPcSpBp): Add powerpc and powerpc64 support.
	(GetStackTrace): Use _Unwind_Backtrace for powerpc and powerpc64.

Index: libsanitizer/configure.tgt
===================================================================
--- libsanitizer/configure.tgt	(revision 193575)
+++ libsanitizer/configure.tgt	(working copy)
@@ -20,6 +20,8 @@
 
 # Filter out unsupported systems.
 case "${target}" in
+  powerpc*-*-linux*)
+	;;
   x86_64-*-linux* | i?86-*-linux* | sparc*-*-linux*)
 	;;
   *)
Index: libsanitizer/sanitizer_common/sanitizer_common.h
===================================================================
--- libsanitizer/sanitizer_common/sanitizer_common.h	(revision 193575)
+++ libsanitizer/sanitizer_common/sanitizer_common.h	(working copy)
@@ -21,12 +21,24 @@
 // Constants.
 const uptr kWordSize = __WORDSIZE / 8;
 const uptr kWordSizeInBits = 8 * kWordSize;
+#if defined(__powerpc__) || defined(__powerpc64__)
+// Current PPC64 kernels use 64K pages sizes, but they can be
+// configured with 4K or even other sizes, so we should probably
+// use getpagesize() or sysconf(_SC_PAGESIZE) here rather than
+// hardcoding the values.
+const uptr kPageSizeBits = 16;
+const uptr kPageSize = 1UL << kPageSizeBits;
+const uptr kCacheLineSize = 128;
+const uptr kMmapGranularity = kPageSize;
+#elif !defined( _WIN32)
 const uptr kPageSizeBits = 12;
 const uptr kPageSize = 1UL << kPageSizeBits;
 const uptr kCacheLineSize = 64;
-#ifndef _WIN32
 const uptr kMmapGranularity = kPageSize;
 #else
+const uptr kPageSizeBits = 12;
+const uptr kPageSize = 1UL << kPageSizeBits;
+const uptr kCacheLineSize = 64;
 const uptr kMmapGranularity = 1UL << 16;
 #endif
 
Index: libsanitizer/sanitizer_common/sanitizer_linux.cc
===================================================================
--- libsanitizer/sanitizer_common/sanitizer_linux.cc	(revision 193575)
+++ libsanitizer/sanitizer_common/sanitizer_linux.cc	(working copy)
@@ -34,7 +34,7 @@
 // --------------- sanitizer_libc.h
 void *internal_mmap(void *addr, uptr length, int prot, int flags,
                     int fd, u64 offset) {
-#if defined __x86_64__
+#if defined(__x86_64__) || defined(__powerpc64__)
   return (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset);
 #else
   return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
@@ -67,7 +67,7 @@
 }
 
 uptr internal_filesize(fd_t fd) {
-#if defined __x86_64__
+#if defined(__x86_64__) || defined(__powerpc64__)
   struct stat st;
   if (syscall(__NR_fstat, fd, &st))
     return -1;
Index: libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
===================================================================
--- libsanitizer/sanitizer_common/sanitizer_stacktrace.cc	(revision 193575)
+++ libsanitizer/sanitizer_common/sanitizer_stacktrace.cc	(working copy)
@@ -31,7 +31,12 @@
   // Cancel Thumb bit.
   pc = pc & (~1);
 #endif
+#if defined(__powerpc__) || defined(__powerpc64__)
+  // PCs are always 4 byte aligned.
+  return pc - 4;
+#else
   return pc - 1;
+#endif
 }
 
 static void PrintStackFramePrefix(uptr frame_num, uptr pc) {
Index: libsanitizer/asan/asan_mapping.h
===================================================================
--- libsanitizer/asan/asan_mapping.h	(revision 193575)
+++ libsanitizer/asan/asan_mapping.h	(working copy)
@@ -31,7 +31,11 @@
 #  if __WORDSIZE == 32
 #   define SHADOW_OFFSET (1 << 29)
 #  else
-#   define SHADOW_OFFSET (1ULL << 44)
+#   if defined(__powerpc64__)
+#    define SHADOW_OFFSET (1ULL << 41)
+#   else
+#    define SHADOW_OFFSET (1ULL << 44)
+#   endif
 #  endif
 # endif
 #endif  // ASAN_FLEXIBLE_MAPPING_AND_OFFSET
@@ -41,7 +45,11 @@
 #define SHADOW_TO_MEM(shadow) (((shadow) - SHADOW_OFFSET) << SHADOW_SCALE)
 
 #if __WORDSIZE == 64
+# if defined(__powerpc64__)
+  static const uptr kHighMemEnd = 0x00000fffffffffffUL;
+# else
   static const uptr kHighMemEnd = 0x00007fffffffffffUL;
+# endif
 #else  // __WORDSIZE == 32
   static const uptr kHighMemEnd = 0xffffffff;
 #endif  // __WORDSIZE
Index: libsanitizer/asan/asan_linux.cc
===================================================================
--- libsanitizer/asan/asan_linux.cc	(revision 193575)
+++ libsanitizer/asan/asan_linux.cc	(working copy)
@@ -66,6 +66,13 @@
   *pc = ucontext->uc_mcontext.gregs[REG_EIP];
   *bp = ucontext->uc_mcontext.gregs[REG_EBP];
   *sp = ucontext->uc_mcontext.gregs[REG_ESP];
+# elif defined(__powerpc__) || defined(__powerpc64__)
+  ucontext_t *ucontext = (ucontext_t*)context;
+  *pc = ucontext->uc_mcontext.regs->nip;
+  *sp = ucontext->uc_mcontext.regs->gpr[PT_R1];
+  // The powerpc{,64}-linux ABIs do not specify r31 as the frame
+  // pointer, but GCC always uses r31 when we need a frame pointer.
+  *bp = ucontext->uc_mcontext.regs->gpr[PT_R31];
 # elif defined(__sparc__)
   ucontext_t *ucontext = (ucontext_t*)context;
   uptr *stk_ptr;
@@ -149,7 +156,7 @@
   stack->trace[0] = pc;
   if ((max_s) > 1) {
     stack->max_size = max_s;
-#ifdef __arm__
+#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
     _Unwind_Backtrace(Unwind_Trace, stack);
 #else
     if (!asan_inited) return;
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 193575)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -1186,6 +1186,9 @@
 #undef TARGET_DELEGITIMIZE_ADDRESS
 #define TARGET_DELEGITIMIZE_ADDRESS rs6000_delegitimize_address
 
+#undef TARGET_ASAN_SHADOW_OFFSET
+#define TARGET_ASAN_SHADOW_OFFSET rs6000_asan_shadow_offset
+
 #undef TARGET_CONST_NOT_OK_FOR_DEBUG_P
 #define TARGET_CONST_NOT_OK_FOR_DEBUG_P rs6000_const_not_ok_for_debug_p
 
@@ -27372,6 +27375,13 @@
     }
 }
 
+/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
+
+static unsigned HOST_WIDE_INT
+rs6000_asan_shadow_offset (void)
+{
+  return (unsigned HOST_WIDE_INT) 1 << (TARGET_64BIT ? 41 : 29);
+}
 \f
 /* Mask options that we want to support inside of attribute((target)) and
    #pragma GCC target operations.  Note, we do not include things like

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

On Fri, Nov 16, 2012 at 3:08 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> Attached below is an initial port of ASAN to powerpc*-linux.
> With the patch below along with Jakub's ASAN testsuite patch:
>
>   http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01365.html
>
> ASAN not only builds, but seems to be working.

Outstanding!

> The lone ASAN
> test case does fail, but it seems to be related to us using
> _Unwind_Backtrace() and we end up with two extra frames at the
> bottom of our stack frame, so we don't match the expected
> results.

Maybe just drop those two frames before reporting them?
(if we always have them)

>
> ==47550== ERROR: AddressSanitizer stack-buffer-overflow on address 0x0fffe65540a4 at pc 0xfff769d0d70 bp 0xfffe6553ed0 sp 0xfffe6553fe8
> READ of size 1 at 0x0fffe65540a4 thread T0
>     #0 0xfff769dc388 in _ZN6__asan13GetStackTraceEPN11__sanitizer10StackTraceEmmm /home/bergner/gcc/gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc:160
>     #1 0xfff769df4f0 in __asan_report_error /home/bergner/gcc/gcc-fsf-mainline-asan/libsanitizer/asan/asan_report.cc:471 (discriminator 5)
>     #2 0xfff769d0dbc in __interceptor_memcmp /home/bergner/gcc/gcc-fsf-mainline-asan/libsanitizer/asan/asan_interceptors.cc:218 (discriminator 1)
>     #3 0x10000b4c in main /home/bergner/gcc/gcc-fsf-mainline-asan/gcc/testsuite/c-c++-common/asan/memcmp-1.c:12
>
> ...which doesn't match:
>
> /* { dg-output "    #0 0x\[0-9a-f\]+ (in _*(interceptor_|)memcmp |\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
> /* { dg-output "    #1 0x\[0-9a-f\]+ (in _*main|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
>
> because the frame numbers are off.  Any ideas on how to fix that?
>
> We cannot use the FastUnwindStack, since that uses exclusively the
> frame pointer which ppc almost never uses.  We also can have stack
> frames above or below the stack pointer, depending on whether the
> function is a leaf or not.  And with shrink wrapping, can we even
> be sure if the frame pointer is even setup even on those systems
> that do use the frame pointer?  I've seen others say we should
> use libbacktrace, and I think I have to agree with them for the
> reasons above.

FastUnwindStack is clearly x86-only thing.
I'd love to have fast solutions for other architectures, but for now
libbacktrace (or any other library call)
is ok for non-x86.
We just need to make sure that those functions do not call malloc/etc.


>
> One question that I have is that the toplev.c test for port support
> tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
> defined as (flag_stack_protect != 0), so ASAN only works when we use
> -fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
> must be false?

<Hopefully some one else can comment here>

>
> I should also note, the code setting the page size is very fragile.
> On PPC/PPC64 kernels, you can configure the kernel to use different
> page sizes.  My systems are running 64K page sizes, but could just
> as easily be 4K or some other number.  Shouldn't we be calling
> getpagesize() or sysconf() to query the page size and then computing
> the other values from that?
>
> I also don't like all the #ifdef's sprinkling the code.  Can't we
> use some configure fragments to help remove many/most of these?

We'll probably have to add some config-like headers as we get more platforms.
Not necessarily now.

>
> Does anyone have any thoughts on the patch?  Does it look reasonable?
>
> Peter
>
>
> gcc/
>         * config/rs6000/rs6000.c (TARGET_ASAN_SHADOW_OFFSET): Define.
>         (rs6000_asan_shadow_offset): New function.
>
> libsanitizer/
>         * configure.tgt: Enable build on powerpc and powerpc64 linux.
>         * sanitizer_common/sanitizer_common.h (kPageSizeBits): Set for powerpc
>         and powerpc64.
>         (kPageSize): Likewise.
>         (kCacheLineSize): Likewise.
>         (kMmapGranularity): Likewise.
>         * sanitizer_common/sanitizer_linux.cc (__NR_mmap) Call for powerpc64.
>         (__NR_fstat): Likewise.
>         * sanitizer_common/sanitizer_stacktrace.cc (patch_pc): Align to 4 bytes.
>         * asan/asan_mapping.h (SHADOW_OFFSET): Define for powerpc and powerpc64.
>         (kHighMemEnd): Set for powerpc and powerpc64.
>         * asan/asan_linux.cc (GetPcSpBp): Add powerpc and powerpc64 support.
>         (GetStackTrace): Use _Unwind_Backtrace for powerpc and powerpc64.
>
> Index: libsanitizer/configure.tgt
> ===================================================================
> --- libsanitizer/configure.tgt  (revision 193575)
> +++ libsanitizer/configure.tgt  (working copy)
> @@ -20,6 +20,8 @@
>
>  # Filter out unsupported systems.
>  case "${target}" in
> +  powerpc*-*-linux*)
> +       ;;
>    x86_64-*-linux* | i?86-*-linux* | sparc*-*-linux*)
>         ;;
>    *)
> Index: libsanitizer/sanitizer_common/sanitizer_common.h
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_common.h    (revision 193575)
> +++ libsanitizer/sanitizer_common/sanitizer_common.h    (working copy)
> @@ -21,12 +21,24 @@
>  // Constants.
>  const uptr kWordSize = __WORDSIZE / 8;
>  const uptr kWordSizeInBits = 8 * kWordSize;
> +#if defined(__powerpc__) || defined(__powerpc64__)
> +// Current PPC64 kernels use 64K pages sizes, but they can be
> +// configured with 4K or even other sizes, so we should probably
> +// use getpagesize() or sysconf(_SC_PAGESIZE) here rather than
> +// hardcoding the values.
> +const uptr kPageSizeBits = 16;

Interesting. This may have some unexpected side effects. (or maybe not?)
It's of course ok to try like this and change it later if something got broken.

> +const uptr kPageSize = 1UL << kPageSizeBits;
> +const uptr kCacheLineSize = 128;
> +const uptr kMmapGranularity = kPageSize;
> +#elif !defined( _WIN32)
>  const uptr kPageSizeBits = 12;
>  const uptr kPageSize = 1UL << kPageSizeBits;
>  const uptr kCacheLineSize = 64;
> -#ifndef _WIN32
>  const uptr kMmapGranularity = kPageSize;
>  #else
> +const uptr kPageSizeBits = 12;
> +const uptr kPageSize = 1UL << kPageSizeBits;
> +const uptr kCacheLineSize = 64;
>  const uptr kMmapGranularity = 1UL << 16;
>  #endif
>
> Index: libsanitizer/sanitizer_common/sanitizer_linux.cc
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_linux.cc    (revision 193575)
> +++ libsanitizer/sanitizer_common/sanitizer_linux.cc    (working copy)
> @@ -34,7 +34,7 @@
>  // --------------- sanitizer_libc.h
>  void *internal_mmap(void *addr, uptr length, int prot, int flags,
>                      int fd, u64 offset) {
> -#if defined __x86_64__
> +#if defined(__x86_64__) || defined(__powerpc64__)
>    return (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset);
>  #else
>    return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
> @@ -67,7 +67,7 @@
>  }
>
>  uptr internal_filesize(fd_t fd) {
> -#if defined __x86_64__
> +#if defined(__x86_64__) || defined(__powerpc64__)
>    struct stat st;
>    if (syscall(__NR_fstat, fd, &st))
>      return -1;
> Index: libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_stacktrace.cc       (revision 193575)
> +++ libsanitizer/sanitizer_common/sanitizer_stacktrace.cc       (working copy)
> @@ -31,7 +31,12 @@
>    // Cancel Thumb bit.
>    pc = pc & (~1);
>  #endif
> +#if defined(__powerpc__) || defined(__powerpc64__)
> +  // PCs are always 4 byte aligned.
> +  return pc - 4;
> +#else
>    return pc - 1;
> +#endif
>  }
>
>  static void PrintStackFramePrefix(uptr frame_num, uptr pc) {
> Index: libsanitizer/asan/asan_mapping.h
> ===================================================================
> --- libsanitizer/asan/asan_mapping.h    (revision 193575)
> +++ libsanitizer/asan/asan_mapping.h    (working copy)
> @@ -31,7 +31,11 @@
>  #  if __WORDSIZE == 32
>  #   define SHADOW_OFFSET (1 << 29)
>  #  else
> -#   define SHADOW_OFFSET (1ULL << 44)
> +#   if defined(__powerpc64__)
> +#    define SHADOW_OFFSET (1ULL << 41)
> +#   else
> +#    define SHADOW_OFFSET (1ULL << 44)
> +#   endif
>  #  endif
>  # endif
>  #endif  // ASAN_FLEXIBLE_MAPPING_AND_OFFSET
> @@ -41,7 +45,11 @@
>  #define SHADOW_TO_MEM(shadow) (((shadow) - SHADOW_OFFSET) << SHADOW_SCALE)
>
>  #if __WORDSIZE == 64
> +# if defined(__powerpc64__)
> +  static const uptr kHighMemEnd = 0x00000fffffffffffUL;
> +# else
>    static const uptr kHighMemEnd = 0x00007fffffffffffUL;
> +# endif
>  #else  // __WORDSIZE == 32
>    static const uptr kHighMemEnd = 0xffffffff;
>  #endif  // __WORDSIZE
> Index: libsanitizer/asan/asan_linux.cc
> ===================================================================
> --- libsanitizer/asan/asan_linux.cc     (revision 193575)
> +++ libsanitizer/asan/asan_linux.cc     (working copy)
> @@ -66,6 +66,13 @@
>    *pc = ucontext->uc_mcontext.gregs[REG_EIP];
>    *bp = ucontext->uc_mcontext.gregs[REG_EBP];
>    *sp = ucontext->uc_mcontext.gregs[REG_ESP];
> +# elif defined(__powerpc__) || defined(__powerpc64__)
> +  ucontext_t *ucontext = (ucontext_t*)context;
> +  *pc = ucontext->uc_mcontext.regs->nip;
> +  *sp = ucontext->uc_mcontext.regs->gpr[PT_R1];
> +  // The powerpc{,64}-linux ABIs do not specify r31 as the frame
> +  // pointer, but GCC always uses r31 when we need a frame pointer.
> +  *bp = ucontext->uc_mcontext.regs->gpr[PT_R31];
>  # elif defined(__sparc__)
>    ucontext_t *ucontext = (ucontext_t*)context;
>    uptr *stk_ptr;
> @@ -149,7 +156,7 @@
>    stack->trace[0] = pc;
>    if ((max_s) > 1) {
>      stack->max_size = max_s;
> -#ifdef __arm__
> +#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
>      _Unwind_Backtrace(Unwind_Trace, stack);
>  #else
>      if (!asan_inited) return;
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 193575)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -1186,6 +1186,9 @@
>  #undef TARGET_DELEGITIMIZE_ADDRESS
>  #define TARGET_DELEGITIMIZE_ADDRESS rs6000_delegitimize_address
>
> +#undef TARGET_ASAN_SHADOW_OFFSET
> +#define TARGET_ASAN_SHADOW_OFFSET rs6000_asan_shadow_offset
> +
>  #undef TARGET_CONST_NOT_OK_FOR_DEBUG_P
>  #define TARGET_CONST_NOT_OK_FOR_DEBUG_P rs6000_const_not_ok_for_debug_p
>
> @@ -27372,6 +27375,13 @@
>      }
>  }
>
> +/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
> +
> +static unsigned HOST_WIDE_INT
> +rs6000_asan_shadow_offset (void)
> +{
> +  return (unsigned HOST_WIDE_INT) 1 << (TARGET_64BIT ? 41 : 29);
> +}
>
>  /* Mask options that we want to support inside of attribute((target)) and
>     #pragma GCC target operations.  Note, we do not include things like
>
>


overall the patch looks great.
I really want it to go through LLVM tree first (otherwise we'll lose
control of the code very soon),
but I am boarding a plane and will not be available for a few days after that.
Hopefully other asan folks (CC-ed) can finish the review and commit to llvm.

Thanks!
--kcc

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

On Sat, Nov 17, 2012 at 3:08 AM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> Attached below is an initial port of ASAN to powerpc*-linux.
> With the patch below along with Jakub's ASAN testsuite patch:
>
>   http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01365.html
>
> ASAN not only builds, but seems to be working.  The lone ASAN
> test case does fail, but it seems to be related to us using
> _Unwind_Backtrace() and we end up with two extra frames at the
> bottom of our stack frame, so we don't match the expected
> results.
>
> ==47550== ERROR: AddressSanitizer stack-buffer-overflow on address 0x0fffe65540a4 at pc 0xfff769d0d70 bp 0xfffe6553ed0 sp 0xfffe6553fe8
> READ of size 1 at 0x0fffe65540a4 thread T0
>     #0 0xfff769dc388 in _ZN6__asan13GetStackTraceEPN11__sanitizer10StackTraceEmmm /home/bergner/gcc/gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc:160
>     #1 0xfff769df4f0 in __asan_report_error /home/bergner/gcc/gcc-fsf-mainline-asan/libsanitizer/asan/asan_report.cc:471 (discriminator 5)
>     #2 0xfff769d0dbc in __interceptor_memcmp /home/bergner/gcc/gcc-fsf-mainline-asan/libsanitizer/asan/asan_interceptors.cc:218 (discriminator 1)
>     #3 0x10000b4c in main /home/bergner/gcc/gcc-fsf-mainline-asan/gcc/testsuite/c-c++-common/asan/memcmp-1.c:12
>
> ...which doesn't match:
>
> /* { dg-output "    #0 0x\[0-9a-f\]+ (in _*(interceptor_|)memcmp |\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
> /* { dg-output "    #1 0x\[0-9a-f\]+ (in _*main|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
>
> because the frame numbers are off.  Any ideas on how to fix that?
>
> We cannot use the FastUnwindStack, since that uses exclusively the
> frame pointer which ppc almost never uses.  We also can have stack
> frames above or below the stack pointer, depending on whether the
> function is a leaf or not.  And with shrink wrapping, can we even
> be sure if the frame pointer is even setup even on those systems
> that do use the frame pointer?  I've seen others say we should
> use libbacktrace, and I think I have to agree with them for the
> reasons above.
>
> One question that I have is that the toplev.c test for port support
> tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
> defined as (flag_stack_protect != 0), so ASAN only works when we use
> -fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
> must be false?
>
> I should also note, the code setting the page size is very fragile.
> On PPC/PPC64 kernels, you can configure the kernel to use different
> page sizes.  My systems are running 64K page sizes, but could just
> as easily be 4K or some other number.  Shouldn't we be calling
> getpagesize() or sysconf() to query the page size and then computing
> the other values from that?
>
> I also don't like all the #ifdef's sprinkling the code.  Can't we
> use some configure fragments to help remove many/most of these?
>
> Does anyone have any thoughts on the patch?  Does it look reasonable?
>
> Peter
>
>
> gcc/
>         * config/rs6000/rs6000.c (TARGET_ASAN_SHADOW_OFFSET): Define.
>         (rs6000_asan_shadow_offset): New function.
>
> libsanitizer/
>         * configure.tgt: Enable build on powerpc and powerpc64 linux.
>         * sanitizer_common/sanitizer_common.h (kPageSizeBits): Set for powerpc
>         and powerpc64.
>         (kPageSize): Likewise.
>         (kCacheLineSize): Likewise.
>         (kMmapGranularity): Likewise.
>         * sanitizer_common/sanitizer_linux.cc (__NR_mmap) Call for powerpc64.
>         (__NR_fstat): Likewise.
>         * sanitizer_common/sanitizer_stacktrace.cc (patch_pc): Align to 4 bytes.
>         * asan/asan_mapping.h (SHADOW_OFFSET): Define for powerpc and powerpc64.
>         (kHighMemEnd): Set for powerpc and powerpc64.
>         * asan/asan_linux.cc (GetPcSpBp): Add powerpc and powerpc64 support.
>         (GetStackTrace): Use _Unwind_Backtrace for powerpc and powerpc64.
>
> Index: libsanitizer/configure.tgt
> ===================================================================
> --- libsanitizer/configure.tgt  (revision 193575)
> +++ libsanitizer/configure.tgt  (working copy)
> @@ -20,6 +20,8 @@
>
>  # Filter out unsupported systems.
>  case "${target}" in
> +  powerpc*-*-linux*)
> +       ;;
>    x86_64-*-linux* | i?86-*-linux* | sparc*-*-linux*)
>         ;;
>    *)
> Index: libsanitizer/sanitizer_common/sanitizer_common.h
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_common.h    (revision 193575)
> +++ libsanitizer/sanitizer_common/sanitizer_common.h    (working copy)
> @@ -21,12 +21,24 @@
>  // Constants.
>  const uptr kWordSize = __WORDSIZE / 8;
>  const uptr kWordSizeInBits = 8 * kWordSize;
> +#if defined(__powerpc__) || defined(__powerpc64__)
> +// Current PPC64 kernels use 64K pages sizes, but they can be
> +// configured with 4K or even other sizes, so we should probably
> +// use getpagesize() or sysconf(_SC_PAGESIZE) here rather than
> +// hardcoding the values.
> +const uptr kPageSizeBits = 16;
> +const uptr kPageSize = 1UL << kPageSizeBits;
> +const uptr kCacheLineSize = 128;
> +const uptr kMmapGranularity = kPageSize;
> +#elif !defined( _WIN32)
>  const uptr kPageSizeBits = 12;
>  const uptr kPageSize = 1UL << kPageSizeBits;
>  const uptr kCacheLineSize = 64;
> -#ifndef _WIN32
>  const uptr kMmapGranularity = kPageSize;
>  #else
> +const uptr kPageSizeBits = 12;
> +const uptr kPageSize = 1UL << kPageSizeBits;
> +const uptr kCacheLineSize = 64;
>  const uptr kMmapGranularity = 1UL << 16;
>  #endif
>
> Index: libsanitizer/sanitizer_common/sanitizer_linux.cc
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_linux.cc    (revision 193575)
> +++ libsanitizer/sanitizer_common/sanitizer_linux.cc    (working copy)
> @@ -34,7 +34,7 @@
>  // --------------- sanitizer_libc.h
>  void *internal_mmap(void *addr, uptr length, int prot, int flags,
>                      int fd, u64 offset) {
> -#if defined __x86_64__
> +#if defined(__x86_64__) || defined(__powerpc64__)


I've changed this part and committed as a separate patch upstream:
http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc?r1=168301&r2=168300&pathrev=168301

--kcc

>    return (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset);
>  #else
>    return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
> @@ -67,7 +67,7 @@
>  }
>
>  uptr internal_filesize(fd_t fd) {
> -#if defined __x86_64__
> +#if defined(__x86_64__) || defined(__powerpc64__)
>    struct stat st;
>    if (syscall(__NR_fstat, fd, &st))
>      return -1;
> Index: libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_stacktrace.cc       (revision 193575)
> +++ libsanitizer/sanitizer_common/sanitizer_stacktrace.cc       (working copy)
> @@ -31,7 +31,12 @@
>    // Cancel Thumb bit.
>    pc = pc & (~1);
>  #endif
> +#if defined(__powerpc__) || defined(__powerpc64__)
> +  // PCs are always 4 byte aligned.
> +  return pc - 4;
> +#else
>    return pc - 1;
> +#endif
>  }
>
>  static void PrintStackFramePrefix(uptr frame_num, uptr pc) {
> Index: libsanitizer/asan/asan_mapping.h
> ===================================================================
> --- libsanitizer/asan/asan_mapping.h    (revision 193575)
> +++ libsanitizer/asan/asan_mapping.h    (working copy)
> @@ -31,7 +31,11 @@
>  #  if __WORDSIZE == 32
>  #   define SHADOW_OFFSET (1 << 29)
>  #  else
> -#   define SHADOW_OFFSET (1ULL << 44)
> +#   if defined(__powerpc64__)
> +#    define SHADOW_OFFSET (1ULL << 41)
> +#   else
> +#    define SHADOW_OFFSET (1ULL << 44)
> +#   endif
>  #  endif
>  # endif
>  #endif  // ASAN_FLEXIBLE_MAPPING_AND_OFFSET
> @@ -41,7 +45,11 @@
>  #define SHADOW_TO_MEM(shadow) (((shadow) - SHADOW_OFFSET) << SHADOW_SCALE)
>
>  #if __WORDSIZE == 64
> +# if defined(__powerpc64__)
> +  static const uptr kHighMemEnd = 0x00000fffffffffffUL;
> +# else
>    static const uptr kHighMemEnd = 0x00007fffffffffffUL;
> +# endif
>  #else  // __WORDSIZE == 32
>    static const uptr kHighMemEnd = 0xffffffff;
>  #endif  // __WORDSIZE
> Index: libsanitizer/asan/asan_linux.cc
> ===================================================================
> --- libsanitizer/asan/asan_linux.cc     (revision 193575)
> +++ libsanitizer/asan/asan_linux.cc     (working copy)
> @@ -66,6 +66,13 @@
>    *pc = ucontext->uc_mcontext.gregs[REG_EIP];
>    *bp = ucontext->uc_mcontext.gregs[REG_EBP];
>    *sp = ucontext->uc_mcontext.gregs[REG_ESP];
> +# elif defined(__powerpc__) || defined(__powerpc64__)
> +  ucontext_t *ucontext = (ucontext_t*)context;
> +  *pc = ucontext->uc_mcontext.regs->nip;
> +  *sp = ucontext->uc_mcontext.regs->gpr[PT_R1];
> +  // The powerpc{,64}-linux ABIs do not specify r31 as the frame
> +  // pointer, but GCC always uses r31 when we need a frame pointer.
> +  *bp = ucontext->uc_mcontext.regs->gpr[PT_R31];
>  # elif defined(__sparc__)
>    ucontext_t *ucontext = (ucontext_t*)context;
>    uptr *stk_ptr;
> @@ -149,7 +156,7 @@
>    stack->trace[0] = pc;
>    if ((max_s) > 1) {
>      stack->max_size = max_s;
> -#ifdef __arm__
> +#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
>      _Unwind_Backtrace(Unwind_Trace, stack);
>  #else
>      if (!asan_inited) return;
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 193575)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -1186,6 +1186,9 @@
>  #undef TARGET_DELEGITIMIZE_ADDRESS
>  #define TARGET_DELEGITIMIZE_ADDRESS rs6000_delegitimize_address
>
> +#undef TARGET_ASAN_SHADOW_OFFSET
> +#define TARGET_ASAN_SHADOW_OFFSET rs6000_asan_shadow_offset
> +
>  #undef TARGET_CONST_NOT_OK_FOR_DEBUG_P
>  #define TARGET_CONST_NOT_OK_FOR_DEBUG_P rs6000_const_not_ok_for_debug_p
>
> @@ -27372,6 +27375,13 @@
>      }
>  }
>
> +/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
> +
> +static unsigned HOST_WIDE_INT
> +rs6000_asan_shadow_offset (void)
> +{
> +  return (unsigned HOST_WIDE_INT) 1 << (TARGET_64BIT ? 41 : 29);
> +}
>
>  /* Mask options that we want to support inside of attribute((target)) and
>     #pragma GCC target operations.  Note, we do not include things like
>
>

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Mon, 2012-11-19 at 15:02 +0400, Dmitry Vyukov wrote:
> I am on a conference today and tomorrow, so I will be able to
> review the patch on Wed. Where can I see the whole patch?

You can find the patch here:

  http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01425.html

Peter

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Jakub Jelinek]

On Fri, Nov 16, 2012 at 05:08:06PM -0600, Peter Bergner wrote:
> One question that I have is that the toplev.c test for port support
> tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
> defined as (flag_stack_protect != 0), so ASAN only works when we use
> -fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
> must be false?

It would be way too much work to support FRAME_GROWS_DOWNWARD.
IMHO far simple for targets like ppc is to define
FRAME_GROWS_DOWNWARD as (flag_stack_protect != 0 || flag_address_sanitizer != 0).

	Jakub

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Mon, 2012-11-19 at 15:29 +0100, Jakub Jelinek wrote:
> On Fri, Nov 16, 2012 at 05:08:06PM -0600, Peter Bergner wrote:
> > One question that I have is that the toplev.c test for port support
> > tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
> > defined as (flag_stack_protect != 0), so ASAN only works when we use
> > -fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
> > must be false?
> 
> It would be way too much work to support FRAME_GROWS_DOWNWARD.

Do you you have a pointer or a reference that describes why ASAN
relies on that?  I don't doubt you are correct, but for my own
education, I'd like to know the reason.


> IMHO far simple for targets like ppc is to define
> FRAME_GROWS_DOWNWARD as (flag_stack_protect != 0 || flag_address_sanitizer != 0).

That looks like a better idea than what I was thinking of, so
I'll go ahead and add that to our target patch.  Thanks!

Peter

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Jakub Jelinek]

On Mon, Nov 19, 2012 at 08:49:30AM -0600, Peter Bergner wrote:
> On Mon, 2012-11-19 at 15:29 +0100, Jakub Jelinek wrote:
> > On Fri, Nov 16, 2012 at 05:08:06PM -0600, Peter Bergner wrote:
> > > One question that I have is that the toplev.c test for port support
> > > tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
> > > defined as (flag_stack_protect != 0), so ASAN only works when we use
> > > -fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
> > > must be false?
> > 
> > It would be way too much work to support FRAME_GROWS_DOWNWARD.
> 
> Do you you have a pointer or a reference that describes why ASAN
> relies on that?  I don't doubt you are correct, but for my own
> education, I'd like to know the reason.

Look at cfgexpand.c changes done for asan, it is modelled there after
the stack protector layout code, would need to take into account the other
direction.

	Jakub

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

On Mon, Nov 19, 2012 at 6:49 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Mon, 2012-11-19 at 15:29 +0100, Jakub Jelinek wrote:
>> On Fri, Nov 16, 2012 at 05:08:06PM -0600, Peter Bergner wrote:
>> > One question that I have is that the toplev.c test for port support
>> > tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
>> > defined as (flag_stack_protect != 0), so ASAN only works when we use
>> > -fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
>> > must be false?
>>
>> It would be way too much work to support FRAME_GROWS_DOWNWARD.
>
> Do you you have a pointer or a reference that describes why ASAN
> relies on that?

The only part where I know for sure that asan relies on stack growing
down is the custom unwinder, which is not applicable to PowerPC
anyway.
Another suspect is the way we poison redzones in the stack frame and
report an error if one found, but It may actually just work.
If the tests pass, I would say it indeed just works.


> I don't doubt you are correct, but for my own
> education, I'd like to know the reason.
>
>
>> IMHO far simple for targets like ppc is to define
>> FRAME_GROWS_DOWNWARD as (flag_stack_protect != 0 || flag_address_sanitizer != 0).
>
> That looks like a better idea than what I was thinking of, so
> I'll go ahead and add that to our target patch.  Thanks!
>
> Peter
>
>
>

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Jakub Jelinek]

On Mon, Nov 19, 2012 at 07:28:18PM +0400, Konstantin Serebryany wrote:
> On Mon, Nov 19, 2012 at 6:49 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > On Mon, 2012-11-19 at 15:29 +0100, Jakub Jelinek wrote:
> >> On Fri, Nov 16, 2012 at 05:08:06PM -0600, Peter Bergner wrote:
> >> > One question that I have is that the toplev.c test for port support
> >> > tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
> >> > defined as (flag_stack_protect != 0), so ASAN only works when we use
> >> > -fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
> >> > must be false?
> >>
> >> It would be way too much work to support FRAME_GROWS_DOWNWARD.
> >
> > Do you you have a pointer or a reference that describes why ASAN
> > relies on that?
> 
> The only part where I know for sure that asan relies on stack growing
> down is the custom unwinder, which is not applicable to PowerPC
> anyway.

FRAME_GROWS_DOWNWARD is still a different thing from STACK_GROWS_DOWNWARD,
all GCC supported targets but hppa are AFAIK STACK_GROWS_DOWNWARD,
FRAME_GROWS_DOWNWARD is about in which order stack slots are assigned to
automatic variables, with FRAME_GROWS_DOWNWARD it is top to bottom,
otherwise it is bottom to top.  This is generally just a GCC internal thing,
obviously user visible if you look at the order of variables, but there are
other aspects that influence it (stack slot sharing, stack protector, etc.).

	Jakub

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Fri, 2012-11-16 at 15:47 -0800, Konstantin Serebryany wrote:
> On Fri, Nov 16, 2012 at 3:08 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > The lone ASAN
> > test case does fail, but it seems to be related to us using
> > _Unwind_Backtrace() and we end up with two extra frames at the
> > bottom of our stack frame, so we don't match the expected
> > results.
> 
> Maybe just drop those two frames before reporting them?
> (if we always have them)

I was worried about whether we always have to extra frames from the
ASAN runtime or not, but looking at the code, it seems we do, so I
added a StackTrace::PopStackFrames(uptr count) method that can be
called after the call to _Unwind_Backtrace() pop off the offending
frames.  With this change, we now pass the ASAN testsuite...all 1
of them. :)



> > One question that I have is that the toplev.c test for port support
> > tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
> > defined as (flag_stack_protect != 0), so ASAN only works when we use
> > -fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
> > must be false?

Jakub answered this and I took his suggestion and redefined our target's
FRAME_GROWS_DOWNWARD to include !flag_asan so we no longer need to use
-fstack-protector.




> > I also don't like all the #ifdef's sprinkling the code.  Can't we
> > use some configure fragments to help remove many/most of these?
> 
> We'll probably have to add some config-like headers as we get more platforms.
> Not necessarily now.

With the SANITIZER_LINUX_USES_64BIT_SYSCALLS patch, I see you started
to address this.  Nice.  I look forward to more of that.



> > +#if defined(__powerpc__) || defined(__powerpc64__)
> > +// Current PPC64 kernels use 64K pages sizes, but they can be
> > +// configured with 4K or even other sizes, so we should probably
> > +// use getpagesize() or sysconf(_SC_PAGESIZE) here rather than
> > +// hardcoding the values.
> > +const uptr kPageSizeBits = 16;
> 
> Interesting. This may have some unexpected side effects. (or maybe not?)
> It's of course ok to try like this and change it later if something got broken.

Well, we _have_ to make this change, since without it, we attempt to do
an mmap of 4K and that will fail on any system with a page size larger
than 4k, since page size is the minimum mmap quantity.


I have attached our current patch which does not include your change that
added SANITIZER_LINUX_USES_64BIT_SYSCALLS, since that isn't upstream yet.

Peter


Index: libsanitizer/sanitizer_common/sanitizer_stacktrace.h
===================================================================
--- libsanitizer/sanitizer_common/sanitizer_stacktrace.h	(revision 193626)
+++ libsanitizer/sanitizer_common/sanitizer_stacktrace.h	(working copy)
@@ -43,6 +43,8 @@
 
   void FastUnwindStack(uptr pc, uptr bp, uptr stack_top, uptr stack_bottom);
 
+  void PopStackFrames(uptr count);
+
   static uptr GetCurrentPc();
 
   static uptr CompressStack(StackTrace *stack,
Index: libsanitizer/sanitizer_common/sanitizer_common.h
===================================================================
--- libsanitizer/sanitizer_common/sanitizer_common.h	(revision 193626)
+++ libsanitizer/sanitizer_common/sanitizer_common.h	(working copy)
@@ -21,12 +21,24 @@
 // Constants.
 const uptr kWordSize = __WORDSIZE / 8;
 const uptr kWordSizeInBits = 8 * kWordSize;
+#if defined(__powerpc__) || defined(__powerpc64__)
+// Current PPC64 kernels use 64K pages sizes, but they can be
+// configured with 4K or even other sizes, so we should probably
+// use getpagesize() or sysconf(_SC_PAGESIZE) here rather than
+// hardcoding the values.
+const uptr kPageSizeBits = 16;
+const uptr kPageSize = 1UL << kPageSizeBits;
+const uptr kCacheLineSize = 128;
+const uptr kMmapGranularity = kPageSize;
+#elif !defined( _WIN32)
 const uptr kPageSizeBits = 12;
 const uptr kPageSize = 1UL << kPageSizeBits;
 const uptr kCacheLineSize = 64;
-#ifndef _WIN32
 const uptr kMmapGranularity = kPageSize;
 #else
+const uptr kPageSizeBits = 12;
+const uptr kPageSize = 1UL << kPageSizeBits;
+const uptr kCacheLineSize = 64;
 const uptr kMmapGranularity = 1UL << 16;
 #endif
 
Index: libsanitizer/sanitizer_common/sanitizer_linux.cc
===================================================================
--- libsanitizer/sanitizer_common/sanitizer_linux.cc	(revision 193626)
+++ libsanitizer/sanitizer_common/sanitizer_linux.cc	(working copy)
@@ -34,7 +34,7 @@
 // --------------- sanitizer_libc.h
 void *internal_mmap(void *addr, uptr length, int prot, int flags,
                     int fd, u64 offset) {
-#if defined __x86_64__
+#if defined(__x86_64__) || defined(__powerpc64__)
   return (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset);
 #else
   return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
@@ -67,7 +67,7 @@
 }
 
 uptr internal_filesize(fd_t fd) {
-#if defined __x86_64__
+#if defined(__x86_64__) || defined(__powerpc64__)
   struct stat st;
   if (syscall(__NR_fstat, fd, &st))
     return -1;
Index: libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
===================================================================
--- libsanitizer/sanitizer_common/sanitizer_stacktrace.cc	(revision 193626)
+++ libsanitizer/sanitizer_common/sanitizer_stacktrace.cc	(working copy)
@@ -31,7 +31,12 @@
   // Cancel Thumb bit.
   pc = pc & (~1);
 #endif
+#if defined(__powerpc__) || defined(__powerpc64__)
+  // PCs are always 4 byte aligned.
+  return pc - 4;
+#else
   return pc - 1;
+#endif
 }
 
 static void PrintStackFramePrefix(uptr frame_num, uptr pc) {
@@ -135,6 +140,16 @@
   }
 }
 
+void StackTrace::PopStackFrames(uptr count) {
+  CHECK(size > count);
+  size -= count;
+  uptr i;
+
+  for (i = 0; i < size; i++) {
+    trace[i] = trace[i+count];
+  }
+}
+
 // On 32-bits we don't compress stack traces.
 // On 64-bits we compress stack traces: if a given pc differes slightly from
 // the previous one, we record a 31-bit offset instead of the full pc.
Index: libsanitizer/asan/asan_mapping.h
===================================================================
--- libsanitizer/asan/asan_mapping.h	(revision 193626)
+++ libsanitizer/asan/asan_mapping.h	(working copy)
@@ -31,7 +31,11 @@
 #  if __WORDSIZE == 32
 #   define SHADOW_OFFSET (1 << 29)
 #  else
-#   define SHADOW_OFFSET (1ULL << 44)
+#   if defined(__powerpc64__)
+#    define SHADOW_OFFSET (1ULL << 41)
+#   else
+#    define SHADOW_OFFSET (1ULL << 44)
+#   endif
 #  endif
 # endif
 #endif  // ASAN_FLEXIBLE_MAPPING_AND_OFFSET
@@ -41,7 +45,11 @@
 #define SHADOW_TO_MEM(shadow) (((shadow) - SHADOW_OFFSET) << SHADOW_SCALE)
 
 #if __WORDSIZE == 64
+# if defined(__powerpc64__)
+  static const uptr kHighMemEnd = 0x00000fffffffffffUL;
+# else
   static const uptr kHighMemEnd = 0x00007fffffffffffUL;
+# endif
 #else  // __WORDSIZE == 32
   static const uptr kHighMemEnd = 0xffffffff;
 #endif  // __WORDSIZE
Index: libsanitizer/asan/asan_linux.cc
===================================================================
--- libsanitizer/asan/asan_linux.cc	(revision 193626)
+++ libsanitizer/asan/asan_linux.cc	(working copy)
@@ -66,6 +66,13 @@
   *pc = ucontext->uc_mcontext.gregs[REG_EIP];
   *bp = ucontext->uc_mcontext.gregs[REG_EBP];
   *sp = ucontext->uc_mcontext.gregs[REG_ESP];
+# elif defined(__powerpc__) || defined(__powerpc64__)
+  ucontext_t *ucontext = (ucontext_t*)context;
+  *pc = ucontext->uc_mcontext.regs->nip;
+  *sp = ucontext->uc_mcontext.regs->gpr[PT_R1];
+  // The powerpc{,64}-linux ABIs do not specify r31 as the frame
+  // pointer, but GCC always uses r31 when we need a frame pointer.
+  *bp = ucontext->uc_mcontext.regs->gpr[PT_R31];
 # elif defined(__sparc__)
   ucontext_t *ucontext = (ucontext_t*)context;
   uptr *stk_ptr;
@@ -149,8 +156,10 @@
   stack->trace[0] = pc;
   if ((max_s) > 1) {
     stack->max_size = max_s;
-#ifdef __arm__
+#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
     _Unwind_Backtrace(Unwind_Trace, stack);
+    // Pop off the two ASAN functions from the backtrace.
+    stack->PopStackFrames(2);
 #else
     if (!asan_inited) return;
     if (AsanThread *t = asanThreadRegistry().GetCurrent())
Index: libsanitizer/configure.tgt
===================================================================
--- libsanitizer/configure.tgt	(revision 193626)
+++ libsanitizer/configure.tgt	(working copy)
@@ -20,6 +20,8 @@
 
 # Filter out unsupported systems.
 case "${target}" in
+  powerpc*-*-linux*)
+	;;
   x86_64-*-linux* | i?86-*-linux* | sparc*-*-linux*)
 	;;
   *)
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 193626)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -1186,6 +1186,9 @@
 #undef TARGET_DELEGITIMIZE_ADDRESS
 #define TARGET_DELEGITIMIZE_ADDRESS rs6000_delegitimize_address
 
+#undef TARGET_ASAN_SHADOW_OFFSET
+#define TARGET_ASAN_SHADOW_OFFSET rs6000_asan_shadow_offset
+
 #undef TARGET_CONST_NOT_OK_FOR_DEBUG_P
 #define TARGET_CONST_NOT_OK_FOR_DEBUG_P rs6000_const_not_ok_for_debug_p
 
@@ -27370,6 +27373,13 @@
     }
 }
 
+/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
+
+static unsigned HOST_WIDE_INT
+rs6000_asan_shadow_offset (void)
+{
+  return (unsigned HOST_WIDE_INT) 1 << (TARGET_64BIT ? 41 : 29);
+}
 \f
 /* Mask options that we want to support inside of attribute((target)) and
    #pragma GCC target operations.  Note, we do not include things like
Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h	(revision 193626)
+++ gcc/config/rs6000/rs6000.h	(working copy)
@@ -1406,7 +1406,7 @@
 
    On the RS/6000, we grow upwards, from the area after the outgoing
    arguments.  */
-#define FRAME_GROWS_DOWNWARD (flag_stack_protect != 0)
+#define FRAME_GROWS_DOWNWARD (flag_stack_protect != 0 || flag_asan != 0)
 
 /* Size of the outgoing register save area */
 #define RS6000_REG_SAVE ((DEFAULT_ABI == ABI_AIX			\

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

On Tue, Nov 20, 2012 at 12:04 AM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Fri, 2012-11-16 at 15:47 -0800, Konstantin Serebryany wrote:
>> On Fri, Nov 16, 2012 at 3:08 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
>> > The lone ASAN
>> > test case does fail, but it seems to be related to us using
>> > _Unwind_Backtrace() and we end up with two extra frames at the
>> > bottom of our stack frame, so we don't match the expected
>> > results.
>>
>> Maybe just drop those two frames before reporting them?
>> (if we always have them)
>
> I was worried about whether we always have to extra frames from the
> ASAN runtime or not, but looking at the code, it seems we do, so I
> added a StackTrace::PopStackFrames(uptr count) method that can be
> called after the call to _Unwind_Backtrace() pop off the offending
> frames.  With this change, we now pass the ASAN testsuite...all 1
> of them. :)

Test suite is a separate issue.
Our largest piece of testing in LLVM uses gtest (very convenient!),
which gcc doesn't have.


>
>
>
>> > One question that I have is that the toplev.c test for port support
>> > tests for !FRAME_GROWS_DOWNWARD.  The rs6000 port has FRAME_GROWS_DOWNWARD
>> > defined as (flag_stack_protect != 0), so ASAN only works when we use
>> > -fstack-protector.  Is there a technical reason why FRAME_GROWS_DOWNWARD
>> > must be false?
>
> Jakub answered this and I took his suggestion and redefined our target's
> FRAME_GROWS_DOWNWARD to include !flag_asan so we no longer need to use
> -fstack-protector.
>
>
>
>
>> > I also don't like all the #ifdef's sprinkling the code.  Can't we
>> > use some configure fragments to help remove many/most of these?
>>
>> We'll probably have to add some config-like headers as we get more platforms.
>> Not necessarily now.
>
> With the SANITIZER_LINUX_USES_64BIT_SYSCALLS patch, I see you started
> to address this.  Nice.  I look forward to more of that.
>
>
>
>> > +#if defined(__powerpc__) || defined(__powerpc64__)
>> > +// Current PPC64 kernels use 64K pages sizes, but they can be
>> > +// configured with 4K or even other sizes, so we should probably
>> > +// use getpagesize() or sysconf(_SC_PAGESIZE) here rather than
>> > +// hardcoding the values.
>> > +const uptr kPageSizeBits = 16;
>>
>> Interesting. This may have some unexpected side effects. (or maybe not?)
>> It's of course ok to try like this and change it later if something got broken.
>
> Well, we _have_ to make this change, since without it, we attempt to do
> an mmap of 4K and that will fail on any system with a page size larger
> than 4k, since page size is the minimum mmap quantity.

Or, sure we have to. I just say that this still may cause some troubles.

>
>
> I have attached our current patch which does not include your change that
> added SANITIZER_LINUX_USES_64BIT_SYSCALLS, since that isn't upstream yet.

I've applied your patch (with minor style and comment changes) upstream:
http://llvm.org/viewvc/llvm-project?view=rev&revision=168356
I did not have any way to test it though. Also, gmail does something
horrible with patches inlined in a message, so I might have missed
something.

Soon I hope to learn how to pull the upstream changes to gcc tree and
do it myself.
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55376)
In the meantime, you are welcome to apply the same patch to gcc manually.
Same for the gcc-specific parts of you patch.

Thanks!

--kcc

>
> Peter
>
>
> Index: libsanitizer/sanitizer_common/sanitizer_stacktrace.h
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_stacktrace.h        (revision 193626)
> +++ libsanitizer/sanitizer_common/sanitizer_stacktrace.h        (working copy)
> @@ -43,6 +43,8 @@
>
>    void FastUnwindStack(uptr pc, uptr bp, uptr stack_top, uptr stack_bottom);
>
> +  void PopStackFrames(uptr count);
> +
>    static uptr GetCurrentPc();
>
>    static uptr CompressStack(StackTrace *stack,
> Index: libsanitizer/sanitizer_common/sanitizer_common.h
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_common.h    (revision 193626)
> +++ libsanitizer/sanitizer_common/sanitizer_common.h    (working copy)
> @@ -21,12 +21,24 @@
>  // Constants.
>  const uptr kWordSize = __WORDSIZE / 8;
>  const uptr kWordSizeInBits = 8 * kWordSize;
> +#if defined(__powerpc__) || defined(__powerpc64__)
> +// Current PPC64 kernels use 64K pages sizes, but they can be
> +// configured with 4K or even other sizes, so we should probably
> +// use getpagesize() or sysconf(_SC_PAGESIZE) here rather than
> +// hardcoding the values.
> +const uptr kPageSizeBits = 16;
> +const uptr kPageSize = 1UL << kPageSizeBits;
> +const uptr kCacheLineSize = 128;
> +const uptr kMmapGranularity = kPageSize;
> +#elif !defined( _WIN32)
>  const uptr kPageSizeBits = 12;
>  const uptr kPageSize = 1UL << kPageSizeBits;
>  const uptr kCacheLineSize = 64;
> -#ifndef _WIN32
>  const uptr kMmapGranularity = kPageSize;
>  #else
> +const uptr kPageSizeBits = 12;
> +const uptr kPageSize = 1UL << kPageSizeBits;
> +const uptr kCacheLineSize = 64;
>  const uptr kMmapGranularity = 1UL << 16;
>  #endif
>
> Index: libsanitizer/sanitizer_common/sanitizer_linux.cc
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_linux.cc    (revision 193626)
> +++ libsanitizer/sanitizer_common/sanitizer_linux.cc    (working copy)
> @@ -34,7 +34,7 @@
>  // --------------- sanitizer_libc.h
>  void *internal_mmap(void *addr, uptr length, int prot, int flags,
>                      int fd, u64 offset) {
> -#if defined __x86_64__
> +#if defined(__x86_64__) || defined(__powerpc64__)
>    return (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset);
>  #else
>    return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
> @@ -67,7 +67,7 @@
>  }
>
>  uptr internal_filesize(fd_t fd) {
> -#if defined __x86_64__
> +#if defined(__x86_64__) || defined(__powerpc64__)
>    struct stat st;
>    if (syscall(__NR_fstat, fd, &st))
>      return -1;
> Index: libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_stacktrace.cc       (revision 193626)
> +++ libsanitizer/sanitizer_common/sanitizer_stacktrace.cc       (working copy)
> @@ -31,7 +31,12 @@
>    // Cancel Thumb bit.
>    pc = pc & (~1);
>  #endif
> +#if defined(__powerpc__) || defined(__powerpc64__)
> +  // PCs are always 4 byte aligned.
> +  return pc - 4;
> +#else
>    return pc - 1;
> +#endif
>  }
>
>  static void PrintStackFramePrefix(uptr frame_num, uptr pc) {
> @@ -135,6 +140,16 @@
>    }
>  }
>
> +void StackTrace::PopStackFrames(uptr count) {
> +  CHECK(size > count);
> +  size -= count;
> +  uptr i;
> +
> +  for (i = 0; i < size; i++) {
> +    trace[i] = trace[i+count];
> +  }
> +}
> +
>  // On 32-bits we don't compress stack traces.
>  // On 64-bits we compress stack traces: if a given pc differes slightly from
>  // the previous one, we record a 31-bit offset instead of the full pc.
> Index: libsanitizer/asan/asan_mapping.h
> ===================================================================
> --- libsanitizer/asan/asan_mapping.h    (revision 193626)
> +++ libsanitizer/asan/asan_mapping.h    (working copy)
> @@ -31,7 +31,11 @@
>  #  if __WORDSIZE == 32
>  #   define SHADOW_OFFSET (1 << 29)
>  #  else
> -#   define SHADOW_OFFSET (1ULL << 44)
> +#   if defined(__powerpc64__)
> +#    define SHADOW_OFFSET (1ULL << 41)
> +#   else
> +#    define SHADOW_OFFSET (1ULL << 44)
> +#   endif
>  #  endif
>  # endif
>  #endif  // ASAN_FLEXIBLE_MAPPING_AND_OFFSET
> @@ -41,7 +45,11 @@
>  #define SHADOW_TO_MEM(shadow) (((shadow) - SHADOW_OFFSET) << SHADOW_SCALE)
>
>  #if __WORDSIZE == 64
> +# if defined(__powerpc64__)
> +  static const uptr kHighMemEnd = 0x00000fffffffffffUL;
> +# else
>    static const uptr kHighMemEnd = 0x00007fffffffffffUL;
> +# endif
>  #else  // __WORDSIZE == 32
>    static const uptr kHighMemEnd = 0xffffffff;
>  #endif  // __WORDSIZE
> Index: libsanitizer/asan/asan_linux.cc
> ===================================================================
> --- libsanitizer/asan/asan_linux.cc     (revision 193626)
> +++ libsanitizer/asan/asan_linux.cc     (working copy)
> @@ -66,6 +66,13 @@
>    *pc = ucontext->uc_mcontext.gregs[REG_EIP];
>    *bp = ucontext->uc_mcontext.gregs[REG_EBP];
>    *sp = ucontext->uc_mcontext.gregs[REG_ESP];
> +# elif defined(__powerpc__) || defined(__powerpc64__)
> +  ucontext_t *ucontext = (ucontext_t*)context;
> +  *pc = ucontext->uc_mcontext.regs->nip;
> +  *sp = ucontext->uc_mcontext.regs->gpr[PT_R1];
> +  // The powerpc{,64}-linux ABIs do not specify r31 as the frame
> +  // pointer, but GCC always uses r31 when we need a frame pointer.
> +  *bp = ucontext->uc_mcontext.regs->gpr[PT_R31];
>  # elif defined(__sparc__)
>    ucontext_t *ucontext = (ucontext_t*)context;
>    uptr *stk_ptr;
> @@ -149,8 +156,10 @@
>    stack->trace[0] = pc;
>    if ((max_s) > 1) {
>      stack->max_size = max_s;
> -#ifdef __arm__
> +#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
>      _Unwind_Backtrace(Unwind_Trace, stack);
> +    // Pop off the two ASAN functions from the backtrace.
> +    stack->PopStackFrames(2);
>  #else
>      if (!asan_inited) return;
>      if (AsanThread *t = asanThreadRegistry().GetCurrent())
> Index: libsanitizer/configure.tgt
> ===================================================================
> --- libsanitizer/configure.tgt  (revision 193626)
> +++ libsanitizer/configure.tgt  (working copy)
> @@ -20,6 +20,8 @@
>
>  # Filter out unsupported systems.
>  case "${target}" in
> +  powerpc*-*-linux*)
> +       ;;
>    x86_64-*-linux* | i?86-*-linux* | sparc*-*-linux*)
>         ;;
>    *)
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 193626)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -1186,6 +1186,9 @@
>  #undef TARGET_DELEGITIMIZE_ADDRESS
>  #define TARGET_DELEGITIMIZE_ADDRESS rs6000_delegitimize_address
>
> +#undef TARGET_ASAN_SHADOW_OFFSET
> +#define TARGET_ASAN_SHADOW_OFFSET rs6000_asan_shadow_offset
> +
>  #undef TARGET_CONST_NOT_OK_FOR_DEBUG_P
>  #define TARGET_CONST_NOT_OK_FOR_DEBUG_P rs6000_const_not_ok_for_debug_p
>
> @@ -27370,6 +27373,13 @@
>      }
>  }
>
> +/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
> +
> +static unsigned HOST_WIDE_INT
> +rs6000_asan_shadow_offset (void)
> +{
> +  return (unsigned HOST_WIDE_INT) 1 << (TARGET_64BIT ? 41 : 29);
> +}
>
>  /* Mask options that we want to support inside of attribute((target)) and
>     #pragma GCC target operations.  Note, we do not include things like
> Index: gcc/config/rs6000/rs6000.h
> ===================================================================
> --- gcc/config/rs6000/rs6000.h  (revision 193626)
> +++ gcc/config/rs6000/rs6000.h  (working copy)
> @@ -1406,7 +1406,7 @@
>
>     On the RS/6000, we grow upwards, from the area after the outgoing
>     arguments.  */
> -#define FRAME_GROWS_DOWNWARD (flag_stack_protect != 0)
> +#define FRAME_GROWS_DOWNWARD (flag_stack_protect != 0 || flag_asan != 0)
>
>  /* Size of the outgoing register save area */
>  #define RS6000_REG_SAVE ((DEFAULT_ABI == ABI_AIX                       \
>
>
>

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Tue, 2012-11-20 at 11:07 +0400, Konstantin Serebryany wrote:
> I've applied your patch (with minor style and comment changes) upstream:
> http://llvm.org/viewvc/llvm-project?view=rev&revision=168356
> I did not have any way to test it though. Also, gmail does something
> horrible with patches inlined in a message, so I might have missed
> something.

Doing a quick peruse through your LLVM commit, I see you grabbed the
PopStackFrames() addition, but the asan_linux.cc changes do not include
the call to PopStackFrames() after the _Unwind_Backtrace() call.
Specifically, the following patch hunk:

>      _Unwind_Backtrace(Unwind_Trace, stack);
> > +    // Pop off the two ASAN functions from the backtrace.
> > +    stack->PopStackFrames(2);

I'll scan the reset of your commit looking for anything else that
is missing.


> Soon I hope to learn how to pull the upstream changes to gcc tree and
> do it myself.
> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55376)
> In the meantime, you are welcome to apply the same patch to gcc manually.
> Same for the gcc-specific parts of you patch.

I'll grab your changes from the LLVM tree so as to pick up your
style changes and add anything you inadvertently dropped and
commit it.  Thanks.

Peter

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

On Tue, Nov 20, 2012 at 5:41 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Tue, 2012-11-20 at 11:07 +0400, Konstantin Serebryany wrote:
>> I've applied your patch (with minor style and comment changes) upstream:
>> http://llvm.org/viewvc/llvm-project?view=rev&revision=168356
>> I did not have any way to test it though. Also, gmail does something
>> horrible with patches inlined in a message, so I might have missed
>> something.
>
> Doing a quick peruse through your LLVM commit, I see you grabbed the
> PopStackFrames() addition, but the asan_linux.cc changes do not include
> the call to PopStackFrames() after the _Unwind_Backtrace() call.
> Specifically, the following patch hunk:
>
>>      _Unwind_Backtrace(Unwind_Trace, stack);
>> > +    // Pop off the two ASAN functions from the backtrace.
>> > +    stack->PopStackFrames(2);

Ah, indeed, I missed that.
Since the patch also affects ARM, I'd like to hear from Evgeniy Stepanov
(or we may decouple powerpc from arm)

--kcc
>
> I'll scan the reset of your commit looking for anything else that
> is missing.
>
>
>> Soon I hope to learn how to pull the upstream changes to gcc tree and
>> do it myself.
>> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55376)
>> In the meantime, you are welcome to apply the same patch to gcc manually.
>> Same for the gcc-specific parts of you patch.
>
> I'll grab your changes from the LLVM tree so as to pick up your
> style changes and add anything you inadvertently dropped and
> commit it.  Thanks.
>
> Peter
>
>

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

On Tue, Nov 20, 2012 at 6:18 PM, Evgeniy Stepanov
<eugeni.stepanov@gmail.com> wrote:
> On Tue, Nov 20, 2012 at 5:52 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>>
>> On Tue, Nov 20, 2012 at 5:41 PM, Peter Bergner <bergner@vnet.ibm.com>
>> wrote:
>> > On Tue, 2012-11-20 at 11:07 +0400, Konstantin Serebryany wrote:
>> >> I've applied your patch (with minor style and comment changes)
>> >> upstream:
>> >> http://llvm.org/viewvc/llvm-project?view=rev&revision=168356
>> >> I did not have any way to test it though. Also, gmail does something
>> >> horrible with patches inlined in a message, so I might have missed
>> >> something.
>> >
>> > Doing a quick peruse through your LLVM commit, I see you grabbed the
>> > PopStackFrames() addition, but the asan_linux.cc changes do not include
>> > the call to PopStackFrames() after the _Unwind_Backtrace() call.
>> > Specifically, the following patch hunk:
>> >
>> >>      _Unwind_Backtrace(Unwind_Trace, stack);
>> >> > +    // Pop off the two ASAN functions from the backtrace.
>> >> > +    stack->PopStackFrames(2);
>
>
> I wonder if under some conditions we may get a different number of extra
> frames (inlining comes to mind). What do you think of removing any number of
> frames that belong to the runtime library - we have memory layout info for
> that?

Bad idea, imho.
Hard to implement, slower to run (remember, this *is* a hotspot).
The frames in question are in our run-time and we can fully control inlining.
What is the current number of redundant frames on ARM?

--kcc


>
>
>>
>>
>> Ah, indeed, I missed that.
>> Since the patch also affects ARM, I'd like to hear from Evgeniy Stepanov
>> (or we may decouple powerpc from arm)
>>
>> --kcc
>> >
>> > I'll scan the reset of your commit looking for anything else that
>> > is missing.
>> >
>> >
>> >> Soon I hope to learn how to pull the upstream changes to gcc tree and
>> >> do it myself.
>> >> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55376)
>> >> In the meantime, you are welcome to apply the same patch to gcc
>> >> manually.
>> >> Same for the gcc-specific parts of you patch.
>> >
>> > I'll grab your changes from the LLVM tree so as to pick up your
>> > style changes and add anything you inadvertently dropped and
>> > commit it.  Thanks.
>> >
>> > Peter
>> >
>> >
>
>

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Dmitry Vyukov]

On Tue, Nov 20, 2012 at 6:21 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Tue, Nov 20, 2012 at 6:18 PM, Evgeniy Stepanov
> <eugeni.stepanov@gmail.com> wrote:
>> On Tue, Nov 20, 2012 at 5:52 PM, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>>>
>>> On Tue, Nov 20, 2012 at 5:41 PM, Peter Bergner <bergner@vnet.ibm.com>
>>> wrote:
>>> > On Tue, 2012-11-20 at 11:07 +0400, Konstantin Serebryany wrote:
>>> >> I've applied your patch (with minor style and comment changes)
>>> >> upstream:
>>> >> http://llvm.org/viewvc/llvm-project?view=rev&revision=168356
>>> >> I did not have any way to test it though. Also, gmail does something
>>> >> horrible with patches inlined in a message, so I might have missed
>>> >> something.
>>> >
>>> > Doing a quick peruse through your LLVM commit, I see you grabbed the
>>> > PopStackFrames() addition, but the asan_linux.cc changes do not include
>>> > the call to PopStackFrames() after the _Unwind_Backtrace() call.
>>> > Specifically, the following patch hunk:
>>> >
>>> >>      _Unwind_Backtrace(Unwind_Trace, stack);
>>> >> > +    // Pop off the two ASAN functions from the backtrace.
>>> >> > +    stack->PopStackFrames(2);
>>
>>
>> I wonder if under some conditions we may get a different number of extra
>> frames (inlining comes to mind). What do you think of removing any number of
>> frames that belong to the runtime library - we have memory layout info for
>> that?
>
> Bad idea, imho.
> Hard to implement, slower to run (remember, this *is* a hotspot).
> The frames in question are in our run-time and we can fully control inlining.
> What is the current number of redundant frames on ARM?


And we can have output tests that verify that we remove the right
number of frames.

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Evgeniy Stepanov]

Ok, fine.


On Tue, Nov 20, 2012 at 6:47 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Nov 20, 2012 at 6:21 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> On Tue, Nov 20, 2012 at 6:18 PM, Evgeniy Stepanov
>> <eugeni.stepanov@gmail.com> wrote:
>>> On Tue, Nov 20, 2012 at 5:52 PM, Konstantin Serebryany
>>> <konstantin.s.serebryany@gmail.com> wrote:
>>>>
>>>> On Tue, Nov 20, 2012 at 5:41 PM, Peter Bergner <bergner@vnet.ibm.com>
>>>> wrote:
>>>> > On Tue, 2012-11-20 at 11:07 +0400, Konstantin Serebryany wrote:
>>>> >> I've applied your patch (with minor style and comment changes)
>>>> >> upstream:
>>>> >> http://llvm.org/viewvc/llvm-project?view=rev&revision=168356
>>>> >> I did not have any way to test it though. Also, gmail does something
>>>> >> horrible with patches inlined in a message, so I might have missed
>>>> >> something.
>>>> >
>>>> > Doing a quick peruse through your LLVM commit, I see you grabbed the
>>>> > PopStackFrames() addition, but the asan_linux.cc changes do not include
>>>> > the call to PopStackFrames() after the _Unwind_Backtrace() call.
>>>> > Specifically, the following patch hunk:
>>>> >
>>>> >>      _Unwind_Backtrace(Unwind_Trace, stack);
>>>> >> > +    // Pop off the two ASAN functions from the backtrace.
>>>> >> > +    stack->PopStackFrames(2);
>>>
>>>
>>> I wonder if under some conditions we may get a different number of extra
>>> frames (inlining comes to mind). What do you think of removing any number of
>>> frames that belong to the runtime library - we have memory layout info for
>>> that?
>>
>> Bad idea, imho.
>> Hard to implement, slower to run (remember, this *is* a hotspot).
>> The frames in question are in our run-time and we can fully control inlining.
>> What is the current number of redundant frames on ARM?
>
>
> And we can have output tests that verify that we remove the right
> number of frames.

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

http://llvm.org/viewvc/llvm-project?rev=168369&view=rev

On Tue, Nov 20, 2012 at 7:12 PM, Evgeniy Stepanov
<eugeni.stepanov@gmail.com> wrote:
> Ok, fine.
>
>
> On Tue, Nov 20, 2012 at 6:47 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Tue, Nov 20, 2012 at 6:21 PM, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>>> On Tue, Nov 20, 2012 at 6:18 PM, Evgeniy Stepanov
>>> <eugeni.stepanov@gmail.com> wrote:
>>>> On Tue, Nov 20, 2012 at 5:52 PM, Konstantin Serebryany
>>>> <konstantin.s.serebryany@gmail.com> wrote:
>>>>>
>>>>> On Tue, Nov 20, 2012 at 5:41 PM, Peter Bergner <bergner@vnet.ibm.com>
>>>>> wrote:
>>>>> > On Tue, 2012-11-20 at 11:07 +0400, Konstantin Serebryany wrote:
>>>>> >> I've applied your patch (with minor style and comment changes)
>>>>> >> upstream:
>>>>> >> http://llvm.org/viewvc/llvm-project?view=rev&revision=168356
>>>>> >> I did not have any way to test it though. Also, gmail does something
>>>>> >> horrible with patches inlined in a message, so I might have missed
>>>>> >> something.
>>>>> >
>>>>> > Doing a quick peruse through your LLVM commit, I see you grabbed the
>>>>> > PopStackFrames() addition, but the asan_linux.cc changes do not include
>>>>> > the call to PopStackFrames() after the _Unwind_Backtrace() call.
>>>>> > Specifically, the following patch hunk:
>>>>> >
>>>>> >>      _Unwind_Backtrace(Unwind_Trace, stack);
>>>>> >> > +    // Pop off the two ASAN functions from the backtrace.
>>>>> >> > +    stack->PopStackFrames(2);
>>>>
>>>>
>>>> I wonder if under some conditions we may get a different number of extra
>>>> frames (inlining comes to mind). What do you think of removing any number of
>>>> frames that belong to the runtime library - we have memory layout info for
>>>> that?
>>>
>>> Bad idea, imho.
>>> Hard to implement, slower to run (remember, this *is* a hotspot).
>>> The frames in question are in our run-time and we can fully control inlining.
>>> What is the current number of redundant frames on ARM?
>>
>>
>> And we can have output tests that verify that we remove the right
>> number of frames.

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Tue, 2012-11-20 at 17:52 +0400, Konstantin Serebryany wrote:
> On Tue, Nov 20, 2012 at 5:41 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > Doing a quick peruse through your LLVM commit, I see you grabbed the
> > PopStackFrames() addition, but the asan_linux.cc changes do not include
> > the call to PopStackFrames() after the _Unwind_Backtrace() call.
> > Specifically, the following patch hunk:
> >
> >>      _Unwind_Backtrace(Unwind_Trace, stack);
> >> > +    // Pop off the two ASAN functions from the backtrace.
> >> > +    stack->PopStackFrames(2);
> 
> Ah, indeed, I missed that.
> Since the patch also affects ARM, I'd like to hear from Evgeniy Stepanov
> (or we may decouple powerpc from arm)

I specifically added that call for all architectures that use
_Unwind_Backtrace, since I believe they will all see the two
extra ASAN functions on the call stack since that is an artifact
of how _Unwind_Backtrace works.  That being said, it would be good
for someone to confirm that change works for ARM...or SPARC
once David gets the SPARC changes ready since he seems to me
not far behind us (ie, POWER).

Peter

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

Evgeniy, my change broke the ARM Android runs:
sanitizer_common/sanitizer_stacktrace.cc:147 "((size > count)) != (0)"
(0x0, 0x0)

Could you please take a look?

On Tue, Nov 20, 2012 at 6:01 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Tue, 2012-11-20 at 17:52 +0400, Konstantin Serebryany wrote:
>> On Tue, Nov 20, 2012 at 5:41 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
>> > Doing a quick peruse through your LLVM commit, I see you grabbed the
>> > PopStackFrames() addition, but the asan_linux.cc changes do not include
>> > the call to PopStackFrames() after the _Unwind_Backtrace() call.
>> > Specifically, the following patch hunk:
>> >
>> >>      _Unwind_Backtrace(Unwind_Trace, stack);
>> >> > +    // Pop off the two ASAN functions from the backtrace.
>> >> > +    stack->PopStackFrames(2);
>>
>> Ah, indeed, I missed that.
>> Since the patch also affects ARM, I'd like to hear from Evgeniy Stepanov
>> (or we may decouple powerpc from arm)
>
> I specifically added that call for all architectures that use
> _Unwind_Backtrace, since I believe they will all see the two
> extra ASAN functions on the call stack since that is an artifact
> of how _Unwind_Backtrace works.  That being said, it would be good
> for someone to confirm that change works for ARM...or SPARC
> once David gets the SPARC changes ready since he seems to me
> not far behind us (ie, POWER).
>
> Peter
>
>

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Tue, 2012-11-20 at 18:18 +0400, Evgeniy Stepanov wrote:

> I wonder if under some conditions we may get a different number of
> extra frames (inlining comes to mind). What do you think of removing
> any number of frames that belong to the runtime library - we have
> memory layout info for that?

How about the following hack that needs to be cleaned up, but does
work for me.  It scans through the trace looking for the frame pointer
that is passed in and if it finds it, it pops the stack up to that
point.  If it doesn't find it (a bug?), it just leaves the trace
alone.

Maybe it fixes the ARM Android issue you just ran into?

Peter



diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/asan/asan_linux.cc gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc
--- gcc-fsf-mainline-kcc/libsanitizer/asan/asan_linux.cc	2012-11-20 12:52:33.961664485 -0600
+++ gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc	2012-11-20 11:28:00.646245908 -0600
@@ -141,12 +141,19 @@ uptr Unwind_GetIP(struct _Unwind_Context
 #endif
 }
 
+uptr Unwind_GetBP(struct _Unwind_Context *ctx) {
+  return _Unwind_GetCFA(ctx);
+}
+
 _Unwind_Reason_Code Unwind_Trace(struct _Unwind_Context *ctx,
     void *param) {
   StackTrace *b = (StackTrace*)param;
   CHECK(b->size < b->max_size);
   uptr pc = Unwind_GetIP(ctx);
-  b->trace[b->size++] = pc;
+  uptr bp = Unwind_GetBP(ctx);
+  b->trace[b->size] = pc;
+  b->frame[b->size] = bp;
+  b->size++;
   if (b->size == b->max_size) return UNWIND_STOP;
   return UNWIND_CONTINUE;
 }
@@ -158,8 +165,13 @@ void GetStackTrace(StackTrace *stack, up
     stack->max_size = max_s;
 #if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
     _Unwind_Backtrace(Unwind_Trace, stack);
-    // Pop off the two ASAN functions from the backtrace.
-    stack->PopStackFrames(2);
+    // Attempt to pop off the ASAN functions from the backtrace.
+    uptr cnt;
+    for (cnt = 0; cnt < stack->size; cnt++)
+      if (stack->frame[cnt] == bp) {
+	stack->PopStackFrames(cnt);
+	break;
+      }
 #else
     if (!asan_inited) return;
     if (AsanThread *t = asanThreadRegistry().GetCurrent())
diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
--- gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc	2012-11-20 11:42:08.834439704 -0600
+++ gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc	2012-11-20 12:59:29.896106625 -0600
@@ -144,7 +144,8 @@ void StackTrace::PopStackFrames(uptr cou
   CHECK(size > count);
   size -= count;
   for (uptr i = 0; i < size; i++) {
-    trace[i] = trace[i + count];
+    trace[i] = trace[i+count];
+    frame[i] = frame[i+count];
   }
 }
 
diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.h gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
--- gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.h	2012-11-20 11:42:08.821389243 -0600
+++ gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.h	2012-11-20 11:12:44.551390980 -0600
@@ -23,6 +23,7 @@ struct StackTrace {
   uptr size;
   uptr max_size;
   uptr trace[kStackTraceMax];
+  uptr frame[kStackTraceMax];	// For use by _Unwind_Backtrace architectures.
   static void PrintStack(const uptr *addr, uptr size,
                          bool symbolize, const char *strip_file_prefix,
                          SymbolizeCallback symbolize_callback);

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

On Tue, Nov 20, 2012 at 11:08 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Tue, 2012-11-20 at 18:18 +0400, Evgeniy Stepanov wrote:
>
>> I wonder if under some conditions we may get a different number of
>> extra frames (inlining comes to mind). What do you think of removing
>> any number of frames that belong to the runtime library - we have
>> memory layout info for that?
>
> How about the following hack that needs to be cleaned up, but does
> work for me.  It scans through the trace looking for the frame pointer
> that is passed in and if it finds it, it pops the stack up to that
> point.  If it doesn't find it (a bug?), it just leaves the trace
> alone.
>
> Maybe it fixes the ARM Android issue you just ran into?
>
> Peter
>
>
>
> diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/asan/asan_linux.cc gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc
> --- gcc-fsf-mainline-kcc/libsanitizer/asan/asan_linux.cc        2012-11-20 12:52:33.961664485 -0600
> +++ gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc       2012-11-20 11:28:00.646245908 -0600
> @@ -141,12 +141,19 @@ uptr Unwind_GetIP(struct _Unwind_Context
>  #endif
>  }
>
> +uptr Unwind_GetBP(struct _Unwind_Context *ctx) {
> +  return _Unwind_GetCFA(ctx);
> +}
> +
>  _Unwind_Reason_Code Unwind_Trace(struct _Unwind_Context *ctx,
>      void *param) {
>    StackTrace *b = (StackTrace*)param;
>    CHECK(b->size < b->max_size);
>    uptr pc = Unwind_GetIP(ctx);
> -  b->trace[b->size++] = pc;
> +  uptr bp = Unwind_GetBP(ctx);
> +  b->trace[b->size] = pc;
> +  b->frame[b->size] = bp;
> +  b->size++;
>    if (b->size == b->max_size) return UNWIND_STOP;
>    return UNWIND_CONTINUE;
>  }
> @@ -158,8 +165,13 @@ void GetStackTrace(StackTrace *stack, up
>      stack->max_size = max_s;
>  #if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
>      _Unwind_Backtrace(Unwind_Trace, stack);
> -    // Pop off the two ASAN functions from the backtrace.
> -    stack->PopStackFrames(2);
> +    // Attempt to pop off the ASAN functions from the backtrace.
> +    uptr cnt;
> +    for (cnt = 0; cnt < stack->size; cnt++)
> +      if (stack->frame[cnt] == bp) {
> +       stack->PopStackFrames(cnt);
> +       break;
> +      }
>  #else
>      if (!asan_inited) return;
>      if (AsanThread *t = asanThreadRegistry().GetCurrent())
> diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
> --- gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc  2012-11-20 11:42:08.834439704 -0600
> +++ gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc 2012-11-20 12:59:29.896106625 -0600
> @@ -144,7 +144,8 @@ void StackTrace::PopStackFrames(uptr cou
>    CHECK(size > count);
>    size -= count;
>    for (uptr i = 0; i < size; i++) {
> -    trace[i] = trace[i + count];
> +    trace[i] = trace[i+count];
> +    frame[i] = frame[i+count];
>    }
>  }
>
> diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.h gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
> --- gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.h   2012-11-20 11:42:08.821389243 -0600
> +++ gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.h  2012-11-20 11:12:44.551390980 -0600
> @@ -23,6 +23,7 @@ struct StackTrace {
>    uptr size;
>    uptr max_size;
>    uptr trace[kStackTraceMax];
> +  uptr frame[kStackTraceMax];  // For use by _Unwind_Backtrace architectures.

This is bad. The objects of this type are already too big, we should
not make them 2x larger.
Hopefully Evgeniy can handle this tomorrow.

--kcc


>    static void PrintStack(const uptr *addr, uptr size,
>                           bool symbolize, const char *strip_file_prefix,
>                           SymbolizeCallback symbolize_callback);
>
>

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Tue, 2012-11-20 at 23:24 +0400, Konstantin Serebryany wrote:
> On Tue, Nov 20, 2012 at 11:08 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.h gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
> > --- gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.h   2012-11-20 11:42:08.821389243 -0600
> > +++ gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.h  2012-11-20 11:12:44.551390980 -0600
> > @@ -23,6 +23,7 @@ struct StackTrace {
> >    uptr size;
> >    uptr max_size;
> >    uptr trace[kStackTraceMax];
> > +  uptr frame[kStackTraceMax];  // For use by _Unwind_Backtrace architectures.
> 
> This is bad. The objects of this type are already too big, we should
> not make them 2x larger.
> Hopefully Evgeniy can handle this tomorrow.

Ok, here's another attempt that doesn't require storing the frame pointers.
In this case, we pass down the frame pointer we're looking for into the
unwind code and if we come across it while building up the trace, we immediately
empty the trace and start over, effectively popping the ASAN functions from
the trace.  If we never encounter the passed down frame pointer, then the code
will just behave as before.  Thoughts?

Peter


diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/LAST_UPDATED gcc-fsf-mainline-asan/LAST_UPDATED
--- gcc-fsf-mainline-kcc/LAST_UPDATED	2012-11-20 11:40:17.232777673 -0600
+++ gcc-fsf-mainline-asan/LAST_UPDATED	2012-11-19 10:33:36.362778406 -0600
@@ -1,2 +1,2 @@
-Tue Nov 20 11:40:17 CST 2012
-Tue Nov 20 17:40:17 UTC 2012 (revision 193626)
+Mon Nov 19 10:33:36 CST 2012
+Mon Nov 19 16:33:36 UTC 2012 (revision 193626)
diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/asan/asan_linux.cc gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc
--- gcc-fsf-mainline-kcc/libsanitizer/asan/asan_linux.cc	2012-11-20 12:52:33.961664485 -0600
+++ gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc	2012-11-20 14:12:31.231751746 -0600
@@ -141,11 +141,27 @@ uptr Unwind_GetIP(struct _Unwind_Context
 #endif
 }
 
+uptr Unwind_GetBP(struct _Unwind_Context *ctx) {
+  return _Unwind_GetCFA(ctx);
+}
+
+struct Unwind_Trace_Info {
+  StackTrace *stack;
+  uptr bp;
+};
+
 _Unwind_Reason_Code Unwind_Trace(struct _Unwind_Context *ctx,
     void *param) {
-  StackTrace *b = (StackTrace*)param;
-  CHECK(b->size < b->max_size);
+  Unwind_Trace_Info *p = (Unwind_Trace_Info *)param;
+  StackTrace *b = p->stack;
   uptr pc = Unwind_GetIP(ctx);
+  if (Unwind_GetBP(ctx) == p->bp) {
+    // We just encountered the frame pointer we want to start
+    // our backtrace with, so empty the backtrace before adding
+    // this frame to the backtrace.
+    b->size = 0;
+  }
+  CHECK(b->size < b->max_size);
   b->trace[b->size++] = pc;
   if (b->size == b->max_size) return UNWIND_STOP;
   return UNWIND_CONTINUE;
@@ -157,9 +173,10 @@ void GetStackTrace(StackTrace *stack, up
   if ((max_s) > 1) {
     stack->max_size = max_s;
 #if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
-    _Unwind_Backtrace(Unwind_Trace, stack);
-    // Pop off the two ASAN functions from the backtrace.
-    stack->PopStackFrames(2);
+    Unwind_Trace_Info param;
+    param.stack = stack;
+    param.bp = bp;
+    _Unwind_Backtrace(Unwind_Trace, &param);
 #else
     if (!asan_inited) return;
     if (AsanThread *t = asanThreadRegistry().GetCurrent())

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Richard Henderson]

On 11/20/2012 12:16 PM, Peter Bergner wrote:
> +uptr Unwind_GetBP(struct _Unwind_Context *ctx) {
> +  return _Unwind_GetCFA(ctx);
> +}
> +
> +struct Unwind_Trace_Info {
> +  StackTrace *stack;
> +  uptr bp;
> +};
> +
>  _Unwind_Reason_Code Unwind_Trace(struct _Unwind_Context *ctx,
>      void *param) {
> -  StackTrace *b = (StackTrace*)param;
> -  CHECK(b->size < b->max_size);
> +  Unwind_Trace_Info *p = (Unwind_Trace_Info *)param;
> +  StackTrace *b = p->stack;
>    uptr pc = Unwind_GetIP(ctx);
> +  if (Unwind_GetBP(ctx) == p->bp) {

BP will only equal the CFA on some targets.  It really depends on how the target sets up the stack frame.  It will also depend on the target actually using frame pointers.

On the other hand, CFA = SP on the next frame up.  And that's rather more reliable based on how we work with dwarf2 and define the CFA.  Only very unusual functions have CFA != the incoming SP -- asm versions of longjmp for example.


r~

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Tue, 2012-11-20 at 12:36 -0800, Richard Henderson wrote:
> BP will only equal the CFA on some targets.  It really depends on how the
> target sets up the stack frame.

Are you talking about leaf routines like on ppc64 where we don't
decrement the stack pointer?  If so, that's not a concern here
because the ASAN tests insert a call so none of the instrumented
functions will be leaf routines.


> It will also depend on the target actually using frame pointers.

That is problematical, except for...


> On the other hand, CFA = SP on the next frame up.  And that's rather more
> reliable based on how we work with dwarf2 and define the CFA.  Only very
> unusual functions have CFA != the incoming SP -- asm versions of longjmp
> for example.

Doesn't this save us, since the bottom frame in the backtrace will always
be an ASAN functionand the frame we're interested in will always be higher
in the backtrace?

I guess I'm wondering, in this specific use case, do you think using
the CFA to compare against is safe or not?

Peter

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Richard Henderson]

On 11/20/2012 02:14 PM, Peter Bergner wrote:
> Doesn't this save us, since the bottom frame in the backtrace will always
> be an ASAN functionand the frame we're interested in will always be higher
> in the backtrace?
> 
> I guess I'm wondering, in this specific use case, do you think using
> the CFA to compare against is safe or not?

Yes it saves us.  I believe using the value of __builtin_dwarf_cfa from
the outermost ASAN function will reliably match the SP value of the
interesting user function outer of ASAN.


r~

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Evgeniy Stepanov]

The ARM/Android failure is due to libstdc++ in android-ndk-r8b not
containing debug info. As a result, stack unwinding breaks in
"operator new", after exactly 2 frames. I guess we can simply tweak
the assert to be OK with empty stack traces when user code stack can
not be unwinded.

Matching FP or SP also sounds good, and perhaps more reliable than
just popping 2 frames from the top of the stack.

AFAIK, the debug info issue is fixed in the latest NDK release.


On Wed, Nov 21, 2012 at 2:27 AM, Richard Henderson <rth@redhat.com> wrote:
> On 11/20/2012 02:14 PM, Peter Bergner wrote:
>> Doesn't this save us, since the bottom frame in the backtrace will always
>> be an ASAN functionand the frame we're interested in will always be higher
>> in the backtrace?
>>
>> I guess I'm wondering, in this specific use case, do you think using
>> the CFA to compare against is safe or not?
>
> Yes it saves us.  I believe using the value of __builtin_dwarf_cfa from
> the outermost ASAN function will reliably match the SP value of the
> interesting user function outer of ASAN.
>
>
> r~

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
> Matching FP or SP also sounds good, and perhaps more reliable than
> just popping 2 frames from the top of the stack.

Agreed.  Can you try my second patch that searches for the frame
address we want our backtrace to start with and see if that works
for ARM?  The nice thing about that patch is that we won't have
to play any games with forcing or disabling inlining of any of
the ASAN functions which me might have to do if we always pop
2 frames off the stack.  It would also be tolerant of adding
any number of new function calls in between the current two
ASAN function at the top of the backtrace.

http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html

Bah, ignore that first diff of the LAST_UPDATED file. :(

Peter

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

On Wed, Nov 21, 2012 at 8:16 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
>> Matching FP or SP also sounds good, and perhaps more reliable than
>> just popping 2 frames from the top of the stack.
>
> Agreed.  Can you try my second patch that searches for the frame
> address we want our backtrace to start with and see if that works
> for ARM?  The nice thing about that patch is that we won't have
> to play any games with forcing or disabling inlining of any of
> the ASAN functions which me might have to do if we always pop
> 2 frames off the stack.  It would also be tolerant of adding
> any number of new function calls in between the current two
> ASAN function at the top of the backtrace.
>
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html
>
> Bah, ignore that first diff of the LAST_UPDATED file. :(

I'd actually prefer to keep the current code that pops two frames (if
it works for you)
Evgeniy seems to know how to fix the ARM case.

--kcc

>
> Peter
>
>
>

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Peter Bergner]

On Wed, 2012-11-21 at 20:22 +0400, Konstantin Serebryany wrote:
> On Wed, Nov 21, 2012 at 8:16 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
> >> Matching FP or SP also sounds good, and perhaps more reliable than
> >> just popping 2 frames from the top of the stack.
> >
> > Agreed.  Can you try my second patch that searches for the frame
> > address we want our backtrace to start with and see if that works
> > for ARM?  The nice thing about that patch is that we won't have
> > to play any games with forcing or disabling inlining of any of
> > the ASAN functions which me might have to do if we always pop
> > 2 frames off the stack.  It would also be tolerant of adding
> > any number of new function calls in between the current two
> > ASAN function at the top of the backtrace.
> >
> > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html
> >
> > Bah, ignore that first diff of the LAST_UPDATED file. :(
> 
> I'd actually prefer to keep the current code that pops two frames
> (if it works for you)

Well it does work for me, since I wrote it. :)  That being said, the
change where I always pop two frames off of the backtrace was more of
a proof of concept that if I can remove those ASAN functions from the
backtrace, do we pass the test case in the testsuite.  It did, but I
have to admit that code is extremely fragile.  It is dependent not
only on the inlining heuristics of one compiler, but of two compilers!
Not to mention people building debugable compilers with -O0 -fno-inline,
etc. etc.  We'd also have to make sure no one in the future adds any
ASAN function calls in between the report function and the GetBackTrace
calls.  It just seems like there are so many things that could go wrong,
that something is bound to.


> Evgeniy seems to know how to fix the ARM case.

His fix was to do:

 void StackTrace::PopStackFrames(uptr count) {
-  CHECK(size > count);
+  CHECK(size >= count);
   size -= count;
   for (uptr i = 0; i < size; i++) {
     trace[i] = trace[i + count];

Basically, that is allowing for us to pop off all of the frames from
the backtrace leaving an empty backtrace.  That can't be helpful in
tracking down the address violation, can it?  With my patch above,
either we find the frame we want to start our backtrace with, or
it returns the entire backtrace, ASAN functions and all.  Isn't that
better from a diagnostic point of view?

That being said, I'd still like to hear from Evgeniy whether my
patch above helps ARM or not.

Peter

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Evgeniy Stepanov]

Yes, it has 3 asan-rtl frames on top. I'm not sure why this does not
happen on ppc.

    #0 0x40122cdb in __asan::GetStackTrace(__sanitizer::StackTrace*,
unsigned long, unsigned long, unsigned long)
    #1 0x40125167 in __asan_report_error
    #2 0x40125af3 in __asan_report_load1


On Thu, Nov 22, 2012 at 12:10 AM, Evgeniy Stepanov
<eugeni.stepanov@gmail.com> wrote:
> I'm looking into the empty stack issue, at this point it looks like a weird
> linker bug. But its completely orthogonal to this discussion.
>
> I recall that the stack trace of the offending memory access has in fact
> three extra frames on top. I'll verify tomorrow. If so, FP/SP matching
> solution is preferable.
>
> On Nov 21, 2012 9:08 PM, "Peter Bergner" <bergner@vnet.ibm.com> wrote:
>>
>> On Wed, 2012-11-21 at 20:22 +0400, Konstantin Serebryany wrote:
>> > On Wed, Nov 21, 2012 at 8:16 PM, Peter Bergner <bergner@vnet.ibm.com>
>> > wrote:
>> > > On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
>> > >> Matching FP or SP also sounds good, and perhaps more reliable than
>> > >> just popping 2 frames from the top of the stack.
>> > >
>> > > Agreed.  Can you try my second patch that searches for the frame
>> > > address we want our backtrace to start with and see if that works
>> > > for ARM?  The nice thing about that patch is that we won't have
>> > > to play any games with forcing or disabling inlining of any of
>> > > the ASAN functions which me might have to do if we always pop
>> > > 2 frames off the stack.  It would also be tolerant of adding
>> > > any number of new function calls in between the current two
>> > > ASAN function at the top of the backtrace.
>> > >
>> > > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html
>> > >
>> > > Bah, ignore that first diff of the LAST_UPDATED file. :(
>> >
>> > I'd actually prefer to keep the current code that pops two frames
>> > (if it works for you)
>>
>> Well it does work for me, since I wrote it. :)  That being said, the
>> change where I always pop two frames off of the backtrace was more of
>> a proof of concept that if I can remove those ASAN functions from the
>> backtrace, do we pass the test case in the testsuite.  It did, but I
>> have to admit that code is extremely fragile.  It is dependent not
>> only on the inlining heuristics of one compiler, but of two compilers!
>> Not to mention people building debugable compilers with -O0 -fno-inline,
>> etc. etc.  We'd also have to make sure no one in the future adds any
>> ASAN function calls in between the report function and the GetBackTrace
>> calls.  It just seems like there are so many things that could go wrong,
>> that something is bound to.
>>
>>
>> > Evgeniy seems to know how to fix the ARM case.
>>
>> His fix was to do:
>>
>>  void StackTrace::PopStackFrames(uptr count) {
>> -  CHECK(size > count);
>> +  CHECK(size >= count);
>>    size -= count;
>>    for (uptr i = 0; i < size; i++) {
>>      trace[i] = trace[i + count];
>>
>> Basically, that is allowing for us to pop off all of the frames from
>> the backtrace leaving an empty backtrace.  That can't be helpful in
>> tracking down the address violation, can it?  With my patch above,
>> either we find the frame we want to start our backtrace with, or
>> it returns the entire backtrace, ASAN functions and all.  Isn't that
>> better from a diagnostic point of view?
>>
>> That being said, I'd still like to hear from Evgeniy whether my
>> patch above helps ARM or not.
>>
>> Peter
>>
>>
>>
>

Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany]

On Thu, Nov 22, 2012 at 12:38 PM, Evgeniy Stepanov
<eugeni.stepanov@gmail.com> wrote:
> Yes, it has 3 asan-rtl frames on top. I'm not sure why this does not
> happen on ppc.

Different inlining inside run-time.
I still prefer a simple PopFrames to anything else, I am sure we can
make it reliable.
the asan run-time should always be build with optimizations on and we
can force or (better) forbid inlining for any function.

--kcc

>
>     #0 0x40122cdb in __asan::GetStackTrace(__sanitizer::StackTrace*,
> unsigned long, unsigned long, unsigned long)
>     #1 0x40125167 in __asan_report_error
>     #2 0x40125af3 in __asan_report_load1
>
>
> On Thu, Nov 22, 2012 at 12:10 AM, Evgeniy Stepanov
> <eugeni.stepanov@gmail.com> wrote:
>> I'm looking into the empty stack issue, at this point it looks like a weird
>> linker bug. But its completely orthogonal to this discussion.
>>
>> I recall that the stack trace of the offending memory access has in fact
>> three extra frames on top. I'll verify tomorrow. If so, FP/SP matching
>> solution is preferable.
>>
>> On Nov 21, 2012 9:08 PM, "Peter Bergner" <bergner@vnet.ibm.com> wrote:
>>>
>>> On Wed, 2012-11-21 at 20:22 +0400, Konstantin Serebryany wrote:
>>> > On Wed, Nov 21, 2012 at 8:16 PM, Peter Bergner <bergner@vnet.ibm.com>
>>> > wrote:
>>> > > On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
>>> > >> Matching FP or SP also sounds good, and perhaps more reliable than
>>> > >> just popping 2 frames from the top of the stack.
>>> > >
>>> > > Agreed.  Can you try my second patch that searches for the frame
>>> > > address we want our backtrace to start with and see if that works
>>> > > for ARM?  The nice thing about that patch is that we won't have
>>> > > to play any games with forcing or disabling inlining of any of
>>> > > the ASAN functions which me might have to do if we always pop
>>> > > 2 frames off the stack.  It would also be tolerant of adding
>>> > > any number of new function calls in between the current two
>>> > > ASAN function at the top of the backtrace.
>>> > >
>>> > > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html
>>> > >
>>> > > Bah, ignore that first diff of the LAST_UPDATED file. :(
>>> >
>>> > I'd actually prefer to keep the current code that pops two frames
>>> > (if it works for you)
>>>
>>> Well it does work for me, since I wrote it. :)  That being said, the
>>> change where I always pop two frames off of the backtrace was more of
>>> a proof of concept that if I can remove those ASAN functions from the
>>> backtrace, do we pass the test case in the testsuite.  It did, but I
>>> have to admit that code is extremely fragile.  It is dependent not
>>> only on the inlining heuristics of one compiler, but of two compilers!
>>> Not to mention people building debugable compilers with -O0 -fno-inline,
>>> etc. etc.  We'd also have to make sure no one in the future adds any
>>> ASAN function calls in between the report function and the GetBackTrace
>>> calls.  It just seems like there are so many things that could go wrong,
>>> that something is bound to.
>>>
>>>
>>> > Evgeniy seems to know how to fix the ARM case.
>>>
>>> His fix was to do:
>>>
>>>  void StackTrace::PopStackFrames(uptr count) {
>>> -  CHECK(size > count);
>>> +  CHECK(size >= count);
>>>    size -= count;
>>>    for (uptr i = 0; i < size; i++) {
>>>      trace[i] = trace[i + count];
>>>
>>> Basically, that is allowing for us to pop off all of the frames from
>>> the backtrace leaving an empty backtrace.  That can't be helpful in
>>> tracking down the address violation, can it?  With my patch above,
>>> either we find the frame we want to start our backtrace with, or
>>> it returns the entire backtrace, ASAN functions and all.  Isn't that
>>> better from a diagnostic point of view?
>>>
>>> That being said, I'd still like to hear from Evgeniy whether my
>>> patch above helps ARM or not.
>>>
>>> Peter
>>>
>>>
>>>
>>