Re: [PATCH] PowerPC: use libgcc _Unwind functions to get backtrace

["Ryan S. Arnold" <ryan dot arnold at gmail dot com>]

On Tue, Jul 23, 2013 at 8:49 AM, Adhemerval Zanella
<azanella@linux.vnet.ibm.com> wrote:
> Thanks for the review, patch revised below (the ChangeLog is the same as before).
> Ok to apply?
>
> ---
>
> diff --git a/sysdeps/powerpc/powerpc32/backtrace.c b/sysdeps/powerpc/powerpc32/backtrace.c
> index b4b11dd..ac012a4 100644
> --- a/sysdeps/powerpc/powerpc32/backtrace.c
> +++ b/sysdeps/powerpc/powerpc32/backtrace.c
> @@ -18,6 +18,9 @@
>
>  #include <execinfo.h>
>  #include <stddef.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <bits/libc-vdso.h>
>
>  /* This is the stack layout we see with every stack frame.
>     Note that every routine is required by the ABI to lay out the stack
> @@ -35,6 +38,43 @@ struct layout
>    void *return_address;
>  };
>
> +#define SIGNAL_FRAMESIZE 64

I had to hunt in the kernel sources for an explanation of what this
is.. and now it's kind of obvious but....

Here's a good explanation that Peter Bergner provided.  Perhaps it can
become a comment?

"A signal handler is just like any other function compiled by the
compiler, so if it needs to save/restore the LR, it will save it into
the callers stack frame.  Since a signal handler doesn't have a
caller, the kernel creates a dummy frame to make it look like it has a
caller.  If we didn't create a dummy frame, the save of the LR could
trash the stack frame."


> +
> +/* signal trampoline saved area.  */
> +struct signal_frame_32 {
> +  char               dummy[SIGNAL_FRAMESIZE];
> +  struct sigcontext  sctx;
> +  mcontext_t         mctx;
> +  /* We don't care about the rest.  */
> +};

I assume the "we don't care..." comment means that this gets overlaid
on top of the 'current' frame pointer since we only need to inspect
the context?  Perhaps a comment on this would be useful.

> +
> +static inline int
> +is_sigtramp_address (unsigned int nip, unsigned int fp)
> +{
> +#ifdef SHARED
> +  if (nip == (unsigned int)__vdso_sigtramp32)
> +    return 1;
> +#endif
> +  return 0;
> +}

Why do you need 'fp' if it isn't used?

> +
> +struct rt_signal_frame_32 {
> +  char               dummy[SIGNAL_FRAMESIZE + 16];
> +  siginfo_t          info;
> +  struct ucontext    uc;
> +  /* We don't care about the rest.  */
> +};
> +
> +static inline int
> +is_sigtramp_address_rt (unsigned int nip, unsigned int fp)
> +{
> +#ifdef SHARED
> +  if (nip == (unsigned int)__vdso_sigtramp_rt32)
> +    return 1;
> +#endif
> +  return 0;
> +}
> +
Likewise regarding FP.

>  int
>  __backtrace (void **array, int size)
>  {
> @@ -50,7 +90,30 @@ __backtrace (void **array, int size)
>    for (                                count = 0;
>         current != NULL &&      count < size;
>         current = current->next, count++)
> -    array[count] = current->return_address;
> +    {
> +      gregset_t *gregset = NULL;

Should this properly require #include <sys/ucontext.h>?

> +
> +      array[count] = current->return_address;
> +
> +      /* Check if the symbol is the signal trampoline and get the interrupted
> +       * symbol address from the trampoline saved area.  */
> +      if (is_sigtramp_address ((unsigned int)current->return_address,
> +                               (unsigned int)current))
> +       {
> +         struct signal_frame_32 *sigframe =
> +           (struct signal_frame_32*) current;
> +          gregset = &sigframe->mctx.gregs;
> +        }
> +      if (is_sigtramp_address ((unsigned int)current->return_address,
> +                               (unsigned int)current))
> +       {
> +         struct signal_frame_32 *sigframe =
> +            (struct signal_frame_32*) current;
> +          gregset = &sigframe->mctx.gregs;
> +        }

Is this second block supposed to be:

if (is_sigtramp_address_rt...

If so, rt_signal_frame_32 doesn't have an mctx element, because it
only goes up to the ucontext.  I think the rt_signal_frame_32 needs to
be larger.

I'd suggest a testcase to test this but it'd require a realtime kernel?

> +      if (gregset)
> +       array[++count] = (void*)((*gregset)[PT_NIP]);
> +    }
>
>    /* It's possible the second-last stack frame can't return
>       (that is, it's __libc_start_main), in which case
> diff --git a/sysdeps/powerpc/powerpc64/backtrace.c b/sysdeps/powerpc/powerpc64/backtrace.c
> index 2d3e051..075732f 100644
> --- a/sysdeps/powerpc/powerpc64/backtrace.c
> +++ b/sysdeps/powerpc/powerpc64/backtrace.c
> @@ -18,6 +18,9 @@
>
>  #include <execinfo.h>
>  #include <stddef.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <bits/libc-vdso.h>
>
>  /* This is the stack layout we see with every stack frame.
>     Note that every routine is required by the ABI to lay out the stack
> @@ -38,6 +41,28 @@ struct layout
>    void *return_address;
>  };
>
> +/* signal trampoline saved area.  */
> +struct signal_frame_64 {
> +#define SIGNAL_FRAMESIZE 128
> +  char            dummy[SIGNAL_FRAMESIZE];
> +  struct ucontext uc;

You need to account for the possibility of the transaction memory
context 'uc_transact'  added for POWER8 in order to properly get at
'tramp'.

Also, how does this work if the VSX regs are included in the ucontext
coming from the kernel?  Can we even tell?  Can we write a test for
this?

> +  unsigned long   unused[2];
> +  unsigned int    tramp[6];
> +  /* We don't care about the rest.  */
> +};
> +
> +static inline int
> +is_sigtramp_address (unsigned long nip, unsigned long fp)
> +{
> +  if (nip == fp + offsetof(struct signal_frame_64, tramp))
> +    return 1;
> +#ifdef SHARED
> +  if (nip == (unsigned long)__vdso_sigtramp_rt64)
> +    return 1;
> +#endif
> +  return 0;
> +}
> +
>  int
>  __backtrace (void **array, int size)
>  {
> @@ -53,7 +78,18 @@ __backtrace (void **array, int size)
>    for (                                count = 0;
>         current != NULL &&      count < size;
>         current = current->next, count++)
> -    array[count] = current->return_address;
> +    {
> +      array[count] = current->return_address;
> +
> +      /* Check if the symbol is the signal trampoline and get the interrupted
> +       * symbol address from the trampoline saved area.  */
> +      if (is_sigtramp_address ((unsigned long)current->return_address,
> +                               (unsigned long)current))
> +        {
> +         struct signal_frame_64 *sigframe = (struct signal_frame_64*) current;
> +          array[++count] = (void*)sigframe->uc.uc_mcontext.gp_regs[PT_NIP];
> +       }
> +    }
>
>    /* It's possible the second-last stack frame can't return
>       (that is, it's __libc_start_main), in which case
> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h b/sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h
> index 8b195db..ba54de4 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h
> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h
> @@ -34,6 +34,13 @@ extern void *__vdso_getcpu;
>
>  extern void *__vdso_time;
>
> +#if defined(__PPC64__) || defined(__powerpc64__)
> +extern void *__vdso_sigtramp_rt64;
> +#else
> +extern void *__vdso_sigtramp32;
> +extern void *__vdso_sigtramp_rt32;
> +#endif
> +
>  /* This macro is needed for PPC64 to return a skeleton OPD entry of a vDSO
>     symbol.  This works because _dl_vdso_vsym always return the function
>     address, and no vDSO symbols use the TOC or chain pointers from the OPD
> diff --git a/sysdeps/unix/sysv/linux/powerpc/init-first.c b/sysdeps/unix/sysv/linux/powerpc/init-first.c
> index f6f05f0..594eb50 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/init-first.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/init-first.c
> @@ -29,6 +29,12 @@ void *__vdso_clock_getres;
>  void *__vdso_get_tbfreq;
>  void *__vdso_getcpu;
>  void *__vdso_time;
> +#if defined(__PPC64__) || defined(__powerpc64__)
> +void *__vdso_sigtramp_rt64;
> +#else
> +void *__vdso_sigtramp32;
> +void *__vdso_sigtramp_rt32;
> +#endif

Do these require a symbol definition to
sysdeps/unix/sysv/linux/powerpc/Versions under GLIBC_PRIVATE like we
do for the other vdso routines?

>
>  static inline void
>  _libc_vdso_platform_setup (void)
> @@ -46,6 +52,13 @@ _libc_vdso_platform_setup (void)
>    __vdso_getcpu = _dl_vdso_vsym ("__kernel_getcpu", &linux2615);
>
>    __vdso_time = _dl_vdso_vsym ("__kernel_time", &linux2615);
> +
> +#if defined(__PPC64__) || defined(__powerpc64__)
> +  __vdso_sigtramp_rt64 = _dl_vdso_vsym ("__kernel_sigtramp_rt64", &linux2615);

Care to add a comment on why there isn't a __kernel_sigtramp64 and
equivalent __vdso_sigtramp64 like there is for powerpc32?

Ryan S. Arnold