| Message ID | 1353355471.17833.58.camel@otta |
|---|---|
| State | New |
| Headers | show |
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 \ > > >
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
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 > >
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
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 >> > >> > > >
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.
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.
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.
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 > >
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 \