On Tue, Nov 20, 2012 at 12:04 AM, Peter Bergner <berg...@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 <berg...@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 \ > > >
