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

[Andrew Pinski <pinskia at gmail dot com>]

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