| Message ID | 20121115035105.GA17255@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 14, 2012 at 7:51 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > Hi, > > X32 uses 32-bit pointer in software. But its hardware pointer is > 64-bit. We must use hardware pointer to unwind frames. This patch > adds uhwptr for hardware pointer and uses it to unwind stack frames. > Tested on Linux/x32, Linux/x86-64 and Linux/ia32. Please review it > for upstream. > > Thanks. > > > H.J. > --- > 2012-11-14 H.J. Lu <hongjiu.lu@intel.com> > > PR other/55333 > * include/sanitizer/common_interface_defs.h (uhwptr): New type > for hardware pointer. > * sanitizer_common/sanitizer_stacktrace.cc (StackTrace::FastUnwindStack): > Replace uptr with uhwptr for frame unwind. Can't you use the mode __unwind_word__ instead if we are compiling with GCC? This way it will always be correct even for x86_64 and x32? Also I think StackTrace::FastUnwindStack should never be enabled in general for GCC since it depends on the frame pointer being enabled which is not always true on either i686 or x86_64 and it is not very portable at all. Thanks, Andrew Pinski > > diff --git a/libsanitizer/include/sanitizer/common_interface_defs.h b/libsanitizer/include/sanitizer/common_interface_defs.h > index 4ac7609..64b8232 100644 > --- a/libsanitizer/include/sanitizer/common_interface_defs.h > +++ b/libsanitizer/include/sanitizer/common_interface_defs.h > @@ -46,6 +46,11 @@ typedef signed long long sptr; // NOLINT > typedef unsigned long uptr; // NOLINT > typedef signed long sptr; // NOLINT > #endif // defined(_WIN64) > +#if defined(__x86_64__) > +typedef unsigned long long uhwptr; // NOLINT > +#else > +typedef uptr uhwptr; // NOLINT > +#endif > typedef unsigned char u8; > typedef unsigned short u16; // NOLINT > typedef unsigned int u32; > diff --git a/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc b/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc > index f6d7a09..915c4b8 100644 > --- a/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc > +++ b/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc > @@ -120,18 +120,18 @@ void StackTrace::FastUnwindStack(uptr pc, uptr bp, > uptr stack_top, uptr stack_bottom) { > CHECK(size == 0 && trace[0] == pc); > size = 1; > - uptr *frame = (uptr*)bp; > - uptr *prev_frame = frame; > + uhwptr *frame = (uhwptr *)bp; > + uhwptr *prev_frame = frame; > while (frame >= prev_frame && > - frame < (uptr*)stack_top - 2 && > - frame > (uptr*)stack_bottom && > + frame < (uhwptr *)stack_top - 2 && > + frame > (uhwptr *)stack_bottom && > size < max_size) { > - uptr pc1 = frame[1]; > + uhwptr pc1 = frame[1]; > if (pc1 != pc) { > - trace[size++] = pc1; > + trace[size++] = (uptr) pc1; > } > prev_frame = frame; > - frame = (uptr*)frame[0]; > + frame = (uhwptr *)frame[0]; > } > } >
On Wed, Nov 14, 2012 at 07:51:05PM -0800, H.J. Lu wrote: > X32 uses 32-bit pointer in software. But its hardware pointer is > 64-bit. We must use hardware pointer to unwind frames. This patch > adds uhwptr for hardware pointer and uses it to unwind stack frames. > Tested on Linux/x32, Linux/x86-64 and Linux/ia32. Please review it > for upstream. IMNSHO libasan should just use libbacktrace for building backtraces (if available, or backtrace (3)), libbacktrace can be used even to symbolize the backtrace on the fly, without the need for additional scripts. Jakub
On Thu, Nov 15, 2012 at 1:26 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Nov 14, 2012 at 07:51:05PM -0800, H.J. Lu wrote: >> X32 uses 32-bit pointer in software. But its hardware pointer is >> 64-bit. We must use hardware pointer to unwind frames. This patch >> adds uhwptr for hardware pointer and uses it to unwind stack frames. >> Tested on Linux/x32, Linux/x86-64 and Linux/ia32. Please review it >> for upstream. > > IMNSHO libasan should just use libbacktrace for building backtraces (if > available, or backtrace (3)), libbacktrace can be used even to symbolize > the backtrace on the fly, without the need for additional scripts. > It is a good idea. In the meantime, we should fix libsanitizer for x32.
+dvyukov, +glider, +samsonov Sorry I am lagging behind e-mail, but I am sure Dmitry, Alexander or Alexey may submit the patch upstream. Please make sure to comment the reason for using a separate typedef. We need our custom unwinder based on frame pointers to remain the default choice on x86[_64] because this is a hotspot and replacing it with any library call (especially if that call does not use frame pointers but instead uses debug info) will slow down the tool significantly. The asan docs explicitly say that you need -fno-omit-frame-pointers to get reasonable bug reports. Note that on ARM (and on Windows) we are using a library call. --kcc On Thu, Nov 15, 2012 at 6:02 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Thu, Nov 15, 2012 at 1:26 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Wed, Nov 14, 2012 at 07:51:05PM -0800, H.J. Lu wrote: >>> X32 uses 32-bit pointer in software. But its hardware pointer is >>> 64-bit. We must use hardware pointer to unwind frames. This patch >>> adds uhwptr for hardware pointer and uses it to unwind stack frames. >>> Tested on Linux/x32, Linux/x86-64 and Linux/ia32. Please review it >>> for upstream. >> >> IMNSHO libasan should just use libbacktrace for building backtraces (if >> available, or backtrace (3)), libbacktrace can be used even to symbolize >> the backtrace on the fly, without the need for additional scripts. >> > > It is a good idea. In the meantime, we should fix libsanitizer > for x32. > > -- > H.J.
On Thu, Nov 15, 2012 at 09:05:13AM -0800, Konstantin Serebryany wrote: > +dvyukov, +glider, +samsonov > > Sorry I am lagging behind e-mail, but I am sure Dmitry, Alexander or > Alexey may submit the patch upstream. > Please make sure to comment the reason for using a separate typedef. > > We need our custom unwinder based on frame pointers to remain the > default choice on x86[_64] because this is a hotspot > and replacing it with any library call (especially if that call does > not use frame pointers but instead uses debug info) will slow down > the tool significantly. > The asan docs explicitly say that you need -fno-omit-frame-pointers to > get reasonable bug reports. The backtrace at asan crash time definitely isn't a hotspot, so that IMHO is a place which definitely should use a proper and accurrate library unwinding. Frame pointers aren't emitted by default on either x86_64 or i?86, system libraries likely won't have them, unless people rebuild everything with -fsanitize=address -fno-omit-frame-pointer (unlikely to happen) etc. So relying on frame pointers is definitely very risky. Even libasan itself is right now built with, even explicit, -fomit-frame-pointer. Jakub
On Thu, Nov 15, 2012 at 9:19 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Nov 15, 2012 at 09:05:13AM -0800, Konstantin Serebryany wrote: >> +dvyukov, +glider, +samsonov >> >> Sorry I am lagging behind e-mail, but I am sure Dmitry, Alexander or >> Alexey may submit the patch upstream. >> Please make sure to comment the reason for using a separate typedef. >> >> We need our custom unwinder based on frame pointers to remain the >> default choice on x86[_64] because this is a hotspot >> and replacing it with any library call (especially if that call does >> not use frame pointers but instead uses debug info) will slow down >> the tool significantly. >> The asan docs explicitly say that you need -fno-omit-frame-pointers to >> get reasonable bug reports. > > The backtrace at asan crash time definitely isn't a hotspot, so that IMHO is > a place which definitely should use a proper and accurrate library > unwinding. Asan collects stack traces on every malloc/free call. > Frame pointers aren't emitted by default on either x86_64 or > i?86, system libraries likely won't have them, unless people rebuild > everything with -fsanitize=address -fno-omit-frame-pointer (unlikely to > happen) etc. So relying on frame pointers is definitely very risky. If a user has asan build where he adds -fsanitize=address, it seems trivial to add -fno-omit-frame-pointer as well. > Even > libasan itself is right now built with, even explicit, -fomit-frame-pointer. That's the right thing. Asan does not unwind itself and performance critical.
On Thu, Nov 15, 2012 at 9:25 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Thu, Nov 15, 2012 at 9:19 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Thu, Nov 15, 2012 at 09:05:13AM -0800, Konstantin Serebryany wrote: >>> +dvyukov, +glider, +samsonov >>> >>> Sorry I am lagging behind e-mail, but I am sure Dmitry, Alexander or >>> Alexey may submit the patch upstream. >>> Please make sure to comment the reason for using a separate typedef. >>> >>> We need our custom unwinder based on frame pointers to remain the >>> default choice on x86[_64] because this is a hotspot >>> and replacing it with any library call (especially if that call does >>> not use frame pointers but instead uses debug info) will slow down >>> the tool significantly. >>> The asan docs explicitly say that you need -fno-omit-frame-pointers to >>> get reasonable bug reports. >> >> The backtrace at asan crash time definitely isn't a hotspot, so that IMHO is >> a place which definitely should use a proper and accurrate library >> unwinding. > > Asan collects stack traces on every malloc/free call. Right. A simple exercise: run 400.perlbench or 483.xalancbmk with asan. If the stack traces in malloc/free are enabled (which is the default), they consume 10-30% of time (don't remember the exact numbers). If we replace fp-based unwinder with something else, this will become 80%. Most programs we care about (e.g. Chrome or Firefox) behave very similar to 483.xalancbmk, i.e. they are very malloc intensive and have deep calls stacks. --kcc > > >> Frame pointers aren't emitted by default on either x86_64 or >> i?86, system libraries likely won't have them, unless people rebuild >> everything with -fsanitize=address -fno-omit-frame-pointer (unlikely to >> happen) etc. So relying on frame pointers is definitely very risky. > > If a user has asan build where he adds -fsanitize=address, it seems > trivial to add -fno-omit-frame-pointer as well. > >> Even >> libasan itself is right now built with, even explicit, -fomit-frame-pointer. > > That's the right thing. Asan does not unwind itself and performance critical.
I am sensing some optimization here :) Is it really important to collect the full stack trace for the allocation context? You care about actual site where the allocation happens -- which might usually be the calls to the malloc like wrappers. The distance from there to the leaf should not he too far, right? David On Thu, Nov 15, 2012 at 9:36 AM, Konstantin Serebryany <konstantin.s.serebryany@gmail.com> wrote: > On Thu, Nov 15, 2012 at 9:25 AM, Dmitry Vyukov <dvyukov@google.com> wrote: >> On Thu, Nov 15, 2012 at 9:19 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Thu, Nov 15, 2012 at 09:05:13AM -0800, Konstantin Serebryany wrote: >>>> +dvyukov, +glider, +samsonov >>>> >>>> Sorry I am lagging behind e-mail, but I am sure Dmitry, Alexander or >>>> Alexey may submit the patch upstream. >>>> Please make sure to comment the reason for using a separate typedef. >>>> >>>> We need our custom unwinder based on frame pointers to remain the >>>> default choice on x86[_64] because this is a hotspot >>>> and replacing it with any library call (especially if that call does >>>> not use frame pointers but instead uses debug info) will slow down >>>> the tool significantly. >>>> The asan docs explicitly say that you need -fno-omit-frame-pointers to >>>> get reasonable bug reports. >>> >>> The backtrace at asan crash time definitely isn't a hotspot, so that IMHO is >>> a place which definitely should use a proper and accurrate library >>> unwinding. >> >> Asan collects stack traces on every malloc/free call. > > Right. > A simple exercise: run 400.perlbench or 483.xalancbmk with asan. > If the stack traces in malloc/free are enabled (which is the default), > they consume 10-30% of time (don't remember the exact numbers). > If we replace fp-based unwinder with something else, this will become 80%. > Most programs we care about (e.g. Chrome or Firefox) behave very > similar to 483.xalancbmk, i.e. they are very malloc intensive and have > deep calls stacks. > > --kcc > >> >> >>> Frame pointers aren't emitted by default on either x86_64 or >>> i?86, system libraries likely won't have them, unless people rebuild >>> everything with -fsanitize=address -fno-omit-frame-pointer (unlikely to >>> happen) etc. So relying on frame pointers is definitely very risky. >> >> If a user has asan build where he adds -fsanitize=address, it seems >> trivial to add -fno-omit-frame-pointer as well. >> >>> Even >>> libasan itself is right now built with, even explicit, -fomit-frame-pointer. >> >> That's the right thing. Asan does not unwind itself and performance critical.
On Thu, Nov 15, 2012 at 9:49 AM, Xinliang David Li <davidxl@google.com> wrote: > I am sensing some optimization here :) Is it really important to > collect the full stack trace for the allocation context? Not important if you want to *find* a bug. Deadly important if you want to *analyze* the bug. The free() traces are even more important than malloc() traces. Trace collection could be disable or reduces for performance. http://code.google.com/p/address-sanitizer/wiki/AddressSanitizer#Flags > You care > about actual site where the allocation happens -- which might usually > be the calls to the malloc like wrappers. The distance from there to > the leaf should not he too far, right? Hm? You mean the code has xalloc() which calls malloc()? No problem, just one extra frame to unwind and store. --kcc > > David > > > On Thu, Nov 15, 2012 at 9:36 AM, Konstantin Serebryany > <konstantin.s.serebryany@gmail.com> wrote: >> On Thu, Nov 15, 2012 at 9:25 AM, Dmitry Vyukov <dvyukov@google.com> wrote: >>> On Thu, Nov 15, 2012 at 9:19 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>> On Thu, Nov 15, 2012 at 09:05:13AM -0800, Konstantin Serebryany wrote: >>>>> +dvyukov, +glider, +samsonov >>>>> >>>>> Sorry I am lagging behind e-mail, but I am sure Dmitry, Alexander or >>>>> Alexey may submit the patch upstream. >>>>> Please make sure to comment the reason for using a separate typedef. >>>>> >>>>> We need our custom unwinder based on frame pointers to remain the >>>>> default choice on x86[_64] because this is a hotspot >>>>> and replacing it with any library call (especially if that call does >>>>> not use frame pointers but instead uses debug info) will slow down >>>>> the tool significantly. >>>>> The asan docs explicitly say that you need -fno-omit-frame-pointers to >>>>> get reasonable bug reports. >>>> >>>> The backtrace at asan crash time definitely isn't a hotspot, so that IMHO is >>>> a place which definitely should use a proper and accurrate library >>>> unwinding. >>> >>> Asan collects stack traces on every malloc/free call. >> >> Right. >> A simple exercise: run 400.perlbench or 483.xalancbmk with asan. >> If the stack traces in malloc/free are enabled (which is the default), >> they consume 10-30% of time (don't remember the exact numbers). >> If we replace fp-based unwinder with something else, this will become 80%. >> Most programs we care about (e.g. Chrome or Firefox) behave very >> similar to 483.xalancbmk, i.e. they are very malloc intensive and have >> deep calls stacks. >> >> --kcc >> >>> >>> >>>> Frame pointers aren't emitted by default on either x86_64 or >>>> i?86, system libraries likely won't have them, unless people rebuild >>>> everything with -fsanitize=address -fno-omit-frame-pointer (unlikely to >>>> happen) etc. So relying on frame pointers is definitely very risky. >>> >>> If a user has asan build where he adds -fsanitize=address, it seems >>> trivial to add -fno-omit-frame-pointer as well. >>> >>>> Even >>>> libasan itself is right now built with, even explicit, -fomit-frame-pointer. >>> >>> That's the right thing. Asan does not unwind itself and performance critical.
diff --git a/libsanitizer/include/sanitizer/common_interface_defs.h b/libsanitizer/include/sanitizer/common_interface_defs.h index 4ac7609..64b8232 100644 --- a/libsanitizer/include/sanitizer/common_interface_defs.h +++ b/libsanitizer/include/sanitizer/common_interface_defs.h @@ -46,6 +46,11 @@ typedef signed long long sptr; // NOLINT typedef unsigned long uptr; // NOLINT typedef signed long sptr; // NOLINT #endif // defined(_WIN64) +#if defined(__x86_64__) +typedef unsigned long long uhwptr; // NOLINT +#else +typedef uptr uhwptr; // NOLINT +#endif typedef unsigned char u8; typedef unsigned short u16; // NOLINT typedef unsigned int u32; diff --git a/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc b/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc index f6d7a09..915c4b8 100644 --- a/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc +++ b/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc @@ -120,18 +120,18 @@ void StackTrace::FastUnwindStack(uptr pc, uptr bp, uptr stack_top, uptr stack_bottom) { CHECK(size == 0 && trace[0] == pc); size = 1; - uptr *frame = (uptr*)bp; - uptr *prev_frame = frame; + uhwptr *frame = (uhwptr *)bp; + uhwptr *prev_frame = frame; while (frame >= prev_frame && - frame < (uptr*)stack_top - 2 && - frame > (uptr*)stack_bottom && + frame < (uhwptr *)stack_top - 2 && + frame > (uhwptr *)stack_bottom && size < max_size) { - uptr pc1 = frame[1]; + uhwptr pc1 = frame[1]; if (pc1 != pc) { - trace[size++] = pc1; + trace[size++] = (uptr) pc1; } prev_frame = frame; - frame = (uptr*)frame[0]; + frame = (uhwptr *)frame[0]; } }