* [PATCH, RFC] Enable libsanitizer on powerpc{,64} @ 2012-11-16 23:08 Peter Bergner 2012-11-16 23:48 ` Konstantin Serebryany ` (2 more replies) 0 siblings, 3 replies; 31+ messages in thread From: Peter Bergner @ 2012-11-16 23:08 UTC (permalink / raw) To: gcc-patches 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-16 23:08 [PATCH, RFC] Enable libsanitizer on powerpc{,64} Peter Bergner @ 2012-11-16 23:48 ` Konstantin Serebryany [not found] ` <CACT4Y+YzjEMeo0ZaFUcaiO4QNAJ-EhfAO+zeh0XXYaT70-sdvA@mail.gmail.com> 2012-11-19 20:07 ` Peter Bergner 2012-11-19 8:23 ` Konstantin Serebryany 2012-11-19 14:30 ` Jakub Jelinek 2 siblings, 2 replies; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-16 23:48 UTC (permalink / raw) To: Peter Bergner Cc: gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov, Evgeniy Stepanov 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CACT4Y+YzjEMeo0ZaFUcaiO4QNAJ-EhfAO+zeh0XXYaT70-sdvA@mail.gmail.com>]
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} [not found] ` <CACT4Y+YzjEMeo0ZaFUcaiO4QNAJ-EhfAO+zeh0XXYaT70-sdvA@mail.gmail.com> @ 2012-11-19 14:24 ` Peter Bergner 0 siblings, 0 replies; 31+ messages in thread From: Peter Bergner @ 2012-11-19 14:24 UTC (permalink / raw) To: Dmitry Vyukov Cc: Konstantin Serebryany, gcc-patches, Alexey Samsonov, Alexander Potapenko, Evgeniy Stepanov 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-16 23:48 ` Konstantin Serebryany [not found] ` <CACT4Y+YzjEMeo0ZaFUcaiO4QNAJ-EhfAO+zeh0XXYaT70-sdvA@mail.gmail.com> @ 2012-11-19 20:07 ` Peter Bergner 2012-11-20 7:08 ` Konstantin Serebryany 1 sibling, 1 reply; 31+ messages in thread From: Peter Bergner @ 2012-11-19 20:07 UTC (permalink / raw) To: Konstantin Serebryany Cc: gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov, Evgeniy Stepanov 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 \ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-19 20:07 ` Peter Bergner @ 2012-11-20 7:08 ` Konstantin Serebryany 2012-11-20 13:41 ` Peter Bergner 0 siblings, 1 reply; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-20 7:08 UTC (permalink / raw) To: Peter Bergner Cc: gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov, Evgeniy Stepanov 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 \ > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 7:08 ` Konstantin Serebryany @ 2012-11-20 13:41 ` Peter Bergner 2012-11-20 13:53 ` Konstantin Serebryany 0 siblings, 1 reply; 31+ messages in thread From: Peter Bergner @ 2012-11-20 13:41 UTC (permalink / raw) To: Konstantin Serebryany Cc: gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov, Evgeniy Stepanov 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 13:41 ` Peter Bergner @ 2012-11-20 13:53 ` Konstantin Serebryany [not found] ` <CABMLtrioRsU18HqOqWzJ2toNOGUFOBZSp-ODRFrgzAnp84c0dw@mail.gmail.com> 2012-11-20 18:09 ` Peter Bergner 0 siblings, 2 replies; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-20 13:53 UTC (permalink / raw) To: Peter Bergner Cc: gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov, Evgeniy Stepanov 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 > > ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CABMLtrioRsU18HqOqWzJ2toNOGUFOBZSp-ODRFrgzAnp84c0dw@mail.gmail.com>]
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} [not found] ` <CABMLtrioRsU18HqOqWzJ2toNOGUFOBZSp-ODRFrgzAnp84c0dw@mail.gmail.com> @ 2012-11-20 14:21 ` Konstantin Serebryany 2012-11-20 14:47 ` Dmitry Vyukov 2012-11-20 19:08 ` Peter Bergner 1 sibling, 1 reply; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-20 14:21 UTC (permalink / raw) To: Evgeniy Stepanov Cc: Peter Bergner, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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 >> > >> > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 14:21 ` Konstantin Serebryany @ 2012-11-20 14:47 ` Dmitry Vyukov 2012-11-20 15:12 ` Evgeniy Stepanov 0 siblings, 1 reply; 31+ messages in thread From: Dmitry Vyukov @ 2012-11-20 14:47 UTC (permalink / raw) To: Konstantin Serebryany Cc: Evgeniy Stepanov, Peter Bergner, gcc-patches, Alexey Samsonov, Alexander Potapenko 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 14:47 ` Dmitry Vyukov @ 2012-11-20 15:12 ` Evgeniy Stepanov 2012-11-20 15:17 ` Konstantin Serebryany 0 siblings, 1 reply; 31+ messages in thread From: Evgeniy Stepanov @ 2012-11-20 15:12 UTC (permalink / raw) To: Dmitry Vyukov Cc: Konstantin Serebryany, Peter Bergner, gcc-patches, Alexey Samsonov, Alexander Potapenko 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 15:12 ` Evgeniy Stepanov @ 2012-11-20 15:17 ` Konstantin Serebryany 0 siblings, 0 replies; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-20 15:17 UTC (permalink / raw) To: Evgeniy Stepanov Cc: Dmitry Vyukov, Peter Bergner, gcc-patches, Alexey Samsonov, Alexander Potapenko 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} [not found] ` <CABMLtrioRsU18HqOqWzJ2toNOGUFOBZSp-ODRFrgzAnp84c0dw@mail.gmail.com> 2012-11-20 14:21 ` Konstantin Serebryany @ 2012-11-20 19:08 ` Peter Bergner 2012-11-20 19:24 ` Konstantin Serebryany 1 sibling, 1 reply; 31+ messages in thread From: Peter Bergner @ 2012-11-20 19:08 UTC (permalink / raw) To: Evgeniy Stepanov Cc: Konstantin Serebryany, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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); ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 19:08 ` Peter Bergner @ 2012-11-20 19:24 ` Konstantin Serebryany 2012-11-20 20:20 ` Peter Bergner 0 siblings, 1 reply; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-20 19:24 UTC (permalink / raw) To: Peter Bergner Cc: Evgeniy Stepanov, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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); > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 19:24 ` Konstantin Serebryany @ 2012-11-20 20:20 ` Peter Bergner 2012-11-20 20:36 ` Richard Henderson 0 siblings, 1 reply; 31+ messages in thread From: Peter Bergner @ 2012-11-20 20:20 UTC (permalink / raw) To: Konstantin Serebryany Cc: Evgeniy Stepanov, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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, ¶m); #else if (!asan_inited) return; if (AsanThread *t = asanThreadRegistry().GetCurrent()) ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 20:20 ` Peter Bergner @ 2012-11-20 20:36 ` Richard Henderson 2012-11-20 22:15 ` Peter Bergner 0 siblings, 1 reply; 31+ messages in thread From: Richard Henderson @ 2012-11-20 20:36 UTC (permalink / raw) To: Peter Bergner Cc: Konstantin Serebryany, Evgeniy Stepanov, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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~ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 20:36 ` Richard Henderson @ 2012-11-20 22:15 ` Peter Bergner 2012-11-20 22:27 ` Richard Henderson 0 siblings, 1 reply; 31+ messages in thread From: Peter Bergner @ 2012-11-20 22:15 UTC (permalink / raw) To: Richard Henderson Cc: Konstantin Serebryany, Evgeniy Stepanov, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 22:15 ` Peter Bergner @ 2012-11-20 22:27 ` Richard Henderson 2012-11-21 9:47 ` Evgeniy Stepanov 0 siblings, 1 reply; 31+ messages in thread From: Richard Henderson @ 2012-11-20 22:27 UTC (permalink / raw) To: Peter Bergner Cc: Konstantin Serebryany, Evgeniy Stepanov, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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~ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 22:27 ` Richard Henderson @ 2012-11-21 9:47 ` Evgeniy Stepanov 2012-11-21 16:16 ` Peter Bergner 0 siblings, 1 reply; 31+ messages in thread From: Evgeniy Stepanov @ 2012-11-21 9:47 UTC (permalink / raw) To: Richard Henderson Cc: Peter Bergner, Konstantin Serebryany, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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~ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-21 9:47 ` Evgeniy Stepanov @ 2012-11-21 16:16 ` Peter Bergner 2012-11-21 16:23 ` Konstantin Serebryany 0 siblings, 1 reply; 31+ messages in thread From: Peter Bergner @ 2012-11-21 16:16 UTC (permalink / raw) To: Evgeniy Stepanov Cc: Richard Henderson, Konstantin Serebryany, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-21 16:16 ` Peter Bergner @ 2012-11-21 16:23 ` Konstantin Serebryany 2012-11-21 17:14 ` Peter Bergner 0 siblings, 1 reply; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-21 16:23 UTC (permalink / raw) To: Peter Bergner Cc: Evgeniy Stepanov, Richard Henderson, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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 > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-21 16:23 ` Konstantin Serebryany @ 2012-11-21 17:14 ` Peter Bergner [not found] ` <CABMLtriDyx+38pvRRQma+rrb0V0mLpU8CnCzVgcv9VZc12Ohew@mail.gmail.com> 0 siblings, 1 reply; 31+ messages in thread From: Peter Bergner @ 2012-11-21 17:14 UTC (permalink / raw) To: Konstantin Serebryany Cc: Evgeniy Stepanov, Richard Henderson, gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CABMLtriDyx+38pvRRQma+rrb0V0mLpU8CnCzVgcv9VZc12Ohew@mail.gmail.com>]
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} [not found] ` <CABMLtriDyx+38pvRRQma+rrb0V0mLpU8CnCzVgcv9VZc12Ohew@mail.gmail.com> @ 2012-11-22 8:38 ` Evgeniy Stepanov 2012-11-22 17:36 ` Konstantin Serebryany 0 siblings, 1 reply; 31+ messages in thread From: Evgeniy Stepanov @ 2012-11-22 8:38 UTC (permalink / raw) To: Peter Bergner Cc: gcc-patches, Richard Henderson, Dmitry Vyukov, Alexey Samsonov, Konstantin Serebryany, Alexander Potapenko 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 >> >> >> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-22 8:38 ` Evgeniy Stepanov @ 2012-11-22 17:36 ` Konstantin Serebryany 0 siblings, 0 replies; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-22 17:36 UTC (permalink / raw) To: Evgeniy Stepanov Cc: Peter Bergner, gcc-patches, Richard Henderson, Dmitry Vyukov, Alexey Samsonov, Alexander Potapenko 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 >>> >>> >>> >> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 13:53 ` Konstantin Serebryany [not found] ` <CABMLtrioRsU18HqOqWzJ2toNOGUFOBZSp-ODRFrgzAnp84c0dw@mail.gmail.com> @ 2012-11-20 18:09 ` Peter Bergner 2012-11-20 18:54 ` Konstantin Serebryany 1 sibling, 1 reply; 31+ messages in thread From: Peter Bergner @ 2012-11-20 18:09 UTC (permalink / raw) To: Konstantin Serebryany Cc: gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov, Evgeniy Stepanov, David Miller 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-20 18:09 ` Peter Bergner @ 2012-11-20 18:54 ` Konstantin Serebryany 0 siblings, 0 replies; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-20 18:54 UTC (permalink / raw) To: Peter Bergner, Evgeniy Stepanov Cc: gcc-patches, Alexey Samsonov, Alexander Potapenko, Dmitry Vyukov, David Miller 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 > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-16 23:08 [PATCH, RFC] Enable libsanitizer on powerpc{,64} Peter Bergner 2012-11-16 23:48 ` Konstantin Serebryany @ 2012-11-19 8:23 ` Konstantin Serebryany 2012-11-19 14:30 ` Jakub Jelinek 2 siblings, 0 replies; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-19 8:23 UTC (permalink / raw) To: Peter Bergner; +Cc: gcc-patches 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 > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-16 23:08 [PATCH, RFC] Enable libsanitizer on powerpc{,64} Peter Bergner 2012-11-16 23:48 ` Konstantin Serebryany 2012-11-19 8:23 ` Konstantin Serebryany @ 2012-11-19 14:30 ` Jakub Jelinek 2012-11-19 14:50 ` Peter Bergner 2 siblings, 1 reply; 31+ messages in thread From: Jakub Jelinek @ 2012-11-19 14:30 UTC (permalink / raw) To: Peter Bergner; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-19 14:30 ` Jakub Jelinek @ 2012-11-19 14:50 ` Peter Bergner 2012-11-19 15:06 ` Jakub Jelinek 2012-11-19 15:29 ` Konstantin Serebryany 0 siblings, 2 replies; 31+ messages in thread From: Peter Bergner @ 2012-11-19 14:50 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-19 14:50 ` Peter Bergner @ 2012-11-19 15:06 ` Jakub Jelinek 2012-11-19 15:29 ` Konstantin Serebryany 1 sibling, 0 replies; 31+ messages in thread From: Jakub Jelinek @ 2012-11-19 15:06 UTC (permalink / raw) To: Peter Bergner; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-19 14:50 ` Peter Bergner 2012-11-19 15:06 ` Jakub Jelinek @ 2012-11-19 15:29 ` Konstantin Serebryany 2012-11-19 15:32 ` Jakub Jelinek 1 sibling, 1 reply; 31+ messages in thread From: Konstantin Serebryany @ 2012-11-19 15:29 UTC (permalink / raw) To: Peter Bergner; +Cc: Jakub Jelinek, gcc-patches 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 > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64} 2012-11-19 15:29 ` Konstantin Serebryany @ 2012-11-19 15:32 ` Jakub Jelinek 0 siblings, 0 replies; 31+ messages in thread From: Jakub Jelinek @ 2012-11-19 15:32 UTC (permalink / raw) To: Konstantin Serebryany; +Cc: Peter Bergner, gcc-patches 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2012-11-22 17:36 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-16 23:08 [PATCH, RFC] Enable libsanitizer on powerpc{,64} Peter Bergner 2012-11-16 23:48 ` Konstantin Serebryany [not found] ` <CACT4Y+YzjEMeo0ZaFUcaiO4QNAJ-EhfAO+zeh0XXYaT70-sdvA@mail.gmail.com> 2012-11-19 14:24 ` Peter Bergner 2012-11-19 20:07 ` Peter Bergner 2012-11-20 7:08 ` Konstantin Serebryany 2012-11-20 13:41 ` Peter Bergner 2012-11-20 13:53 ` Konstantin Serebryany [not found] ` <CABMLtrioRsU18HqOqWzJ2toNOGUFOBZSp-ODRFrgzAnp84c0dw@mail.gmail.com> 2012-11-20 14:21 ` Konstantin Serebryany 2012-11-20 14:47 ` Dmitry Vyukov 2012-11-20 15:12 ` Evgeniy Stepanov 2012-11-20 15:17 ` Konstantin Serebryany 2012-11-20 19:08 ` Peter Bergner 2012-11-20 19:24 ` Konstantin Serebryany 2012-11-20 20:20 ` Peter Bergner 2012-11-20 20:36 ` Richard Henderson 2012-11-20 22:15 ` Peter Bergner 2012-11-20 22:27 ` Richard Henderson 2012-11-21 9:47 ` Evgeniy Stepanov 2012-11-21 16:16 ` Peter Bergner 2012-11-21 16:23 ` Konstantin Serebryany 2012-11-21 17:14 ` Peter Bergner [not found] ` <CABMLtriDyx+38pvRRQma+rrb0V0mLpU8CnCzVgcv9VZc12Ohew@mail.gmail.com> 2012-11-22 8:38 ` Evgeniy Stepanov 2012-11-22 17:36 ` Konstantin Serebryany 2012-11-20 18:09 ` Peter Bergner 2012-11-20 18:54 ` Konstantin Serebryany 2012-11-19 8:23 ` Konstantin Serebryany 2012-11-19 14:30 ` Jakub Jelinek 2012-11-19 14:50 ` Peter Bergner 2012-11-19 15:06 ` Jakub Jelinek 2012-11-19 15:29 ` Konstantin Serebryany 2012-11-19 15:32 ` Jakub Jelinek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).