| Message ID | 1353107286.17833.36.camel@otta |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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
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
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
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
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 > > >
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
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); +} /* Mask options that we want to support inside of attribute((target)) and #pragma GCC target operations. Note, we do not include things like
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.