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:42 PM, Adhemerval Zanella
<azanella@linux.vnet.ibm.com> wrote:

> 2013-07-23  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
>
>         * sysdeps/powerpc/powerpc32/backtrace.c (__backtrace): Handle signal
>         trampoline stack frame information.
>         * sysdeps/powerpc/powerpc64/backtrace.c (__backtrace): Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h
>         (__vdso_sigtramp_rt64): New variable: PPC64 signal trampoline.
>         (__vdso_sigtramp32): New variable: PPC32 signal trampoline.
>         (__vdso_sigtramp_rt32): New variable: PPC32 signal trampoline.
>         * sysdeps/unix/sysv/linux/powerpc/init-first.c
>         (_libc_vdso_platform_setup): Initialize the signal trampolines.
>         * debug/tst-backtrace5.c: Add a new test for SA_SIGINFO option.
>
> --
>
> diff --git a/debug/tst-backtrace5.c b/debug/tst-backtrace5.c
> index ca47437..f8984df 100644
> --- a/debug/tst-backtrace5.c
> +++ b/debug/tst-backtrace5.c
> @@ -29,6 +29,7 @@
>  #include "tst-backtrace.h"
>
>  static int do_test (void);
> +#define TIMEOUT 5
>  #define TEST_FUNCTION do_test ()
>  #include "../test-skeleton.c"
>
> @@ -91,7 +92,7 @@ handle_signal (int signum)
>  }
>
>  NO_INLINE int
> -fn (int c)
> +fn (int c, int flags)
>  {
>    pid_t parent_pid, child_pid;
>    int pipefd[2];
> @@ -100,12 +101,13 @@ fn (int c)
>
>    if (c > 0)
>      {
> -      fn (c - 1);
> +      fn (c - 1, flags);
>        return x;
>      }
>
>    memset (&act, 0, sizeof (act));
>    act.sa_handler = handle_signal;
> +  act.sa_flags = flags;
>    sigemptyset (&act.sa_mask);
>    sigaction (SIGUSR1, &act, NULL);
>    parent_pid = getpid ();
> @@ -131,6 +133,7 @@ fn (int c)
>  NO_INLINE static int
>  do_test (void)
>  {
> -  fn (2);
> +  fn (2, 0);
> +  fn (2, SA_SIGINFO);
>    return ret;
>  }
> diff --git a/sysdeps/powerpc/powerpc32/backtrace.c b/sysdeps/powerpc/powerpc32/backtrace.c
> index b4b11dd..cb9c9ca 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,46 @@ struct layout
>    void *return_address;
>  };
>
> +#define SIGNAL_FRAMESIZE 64
> +
> +/* Since the signal handler is just like any other function it needs to
> +   save/restore its LR and it will save it into 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.  */
> +struct signal_frame_32 {
> +  char               dummy[SIGNAL_FRAMESIZE];
> +  struct sigcontext  sctx;
> +  mcontext_t         mctx;
> +  /* We don't care about the rest, since the IP value is at 'uc' field.  */

This comment seems to be the text for the rt_signal_frame_32 struct,
and this one should be
".... value is in the 'mctx' field."

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

As mentioned earlier this should be "... the rest, since the IP value
is in the 'uc' field.".

> +};
> +
> +static inline int
> +is_sigtramp_address_rt (unsigned int nip)
> +{
> +#ifdef SHARED
> +  if (nip == (unsigned int)__vdso_sigtramp_rt32)
> +    return 1;
> +#endif
> +  return 0;
> +}
> +
>  int
>  __backtrace (void **array, int size)
>  {
> @@ -50,7 +93,28 @@ __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;
> +
> +      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))
> +       {
> +         struct signal_frame_32 *sigframe =
> +           (struct signal_frame_32*) current;
> +          gregset = &sigframe->mctx.gregs;
> +        }
> +      else if (is_sigtramp_address_rt ((unsigned int)current->return_address))
> +       {
> +         struct rt_signal_frame_32 *sigframe =
> +            (struct rt_signal_frame_32*) current;
> +          gregset = &sigframe->uc.uc_mcontext.uc_regs->gregs;
> +        }
> +      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..0309493 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,31 @@ struct layout
>    void *return_address;
>  };
>
> +/* Since the signal handler is just like any other function it needs to
> +   save/restore its LR and it will save it into 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.  */
> +struct signal_frame_64 {
> +#define SIGNAL_FRAMESIZE 128
> +  char            dummy[SIGNAL_FRAMESIZE];
> +  struct ucontext uc;
> +  unsigned long   unused[2];
> +  unsigned int    tramp[6];
> +  /* We don't care about the rest, since the IP value is at 'uc' field.  */
> +};
> +
> +static inline int
> +is_sigtramp_address (unsigned long nip, unsigned long fp)
> +{
> +  if (nip == fp + offsetof(struct signal_frame_64, tramp))

On powerpc64 the ucontext contains the mcontext_t, which holds the
register states.

You'll notice that the last entry in that structure in the glibc
header is the vmx_reserve field: (look at
glibc/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h):

long            vmx_reserve[NVRREG+NVRREG+1];

This means there is no Vector Scalar Register reserve area accounted
for in the glibc mcontext structure.  The [get|set] context structures
have been deprecated in POSIX so Uli Drepper wouldn't allow us to
extend the structure definitions for VSRs (due to complications with
versioning structures in APIs).

So in your code, using offsetof (struct signal_frame_64, tramp) will
presumably compute the size of the ucontext structure, as informed by
the glibc ucontext.h header, when determining the offset of 'tramp'
from the frame pointer.

Newer kernels  unconditionally extend the vmx_reserve field for the
vector scalar registers (look at
linux/arch/powerpc/include/uapi/asm/sigcontext.h):

" * Part of the VSX data is stored here also by extending vmx_restore
 * by an additional 32 double words.  Architecturally the layout of
 * the VSR registers and how they overlap on top of the legacy FPR and
 * VR registers is shown below:"
...

long            vmx_reserve[ELF_NVRREG+ELF_NVRREG+32+1];

Doesn't this mean that when running this on POWER7 or POWER8 that
offset you've computed for 'tramp' will actually come up short and
will point into the Vector Scalar Register section of vmx_reserve as
allocated by the kernel and not at 'tramp' as you've intended?

So on a system configured with VSX support you'll need to account for
this extended vmx_reserve.

I'm not sure of the best way to do this.  You could wrap it in a
kernel guard and assume that vmx_reserve has been extended based on
when this was added to the kernel (version).  If not 'assuming' a
kernel version you could check the dl_hwcap for PPC_FEATURE_VSX.  I
wonder how much this will slow down the backtrace?

Additionally, if you look in the Linux kernel's
linux/arch/powerpc/kernel/signal_64.c at the definition of struct
rt_sigframe:

struct rt_sigframe {
        /* sys_rt_sigreturn requires the ucontext be the first field */
        struct ucontext uc;
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
        struct ucontext uc_transact;
#endif
        unsigned long _unused[2];
        unsigned int tramp[TRAMP_SIZE];
....

The addition of the uc_transact structure on POWER8 system will
additionally throw off your 'tramp' offset computation unless you
account for it in your computation on POWER8 by checking hwcap2 for
PPC_FEATURE2_HTM and compensating.

Let me know if I'm completely wrong about all of this.

Ryan S. Arnold