Patchwork PATCH: PR other/55333: libsanitizer StackTrace::FastUnwindStack wrong x32

login
register
mail settings
Submitter H.J. Lu
Date Nov. 15, 2012, 3:51 a.m.
Message ID <20121115035105.GA17255@gmail.com>
Download mbox | patch
Permalink /patch/199125/
State New
Headers show

Comments

H.J. Lu - Nov. 15, 2012, 3:51 a.m.
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.
Andrew Pinski - Nov. 15, 2012, 4:07 a.m.
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];
>    }
>  }
>
Jakub Jelinek - Nov. 15, 2012, 9:26 a.m.
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
H.J. Lu - Nov. 15, 2012, 2:02 p.m.
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.
Konstantin Serebryany - Nov. 15, 2012, 5:05 p.m.
+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.
Jakub Jelinek - Nov. 15, 2012, 5:19 p.m.
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
Dmitriy Vyukov - Nov. 15, 2012, 5:25 p.m.
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.
Konstantin Serebryany - Nov. 15, 2012, 5:36 p.m.
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.
Xinliang David Li - Nov. 15, 2012, 5:49 p.m.
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.
Konstantin Serebryany - Nov. 15, 2012, 6:04 p.m.
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.

Patch

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];
   }
 }