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

[Konstantin Serebryany <konstantin dot s dot serebryany at gmail dot com>]

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