[PATCH] os: use libunwind to generate backtraces

[Peter Hutterer]

On Mon, Feb 18, 2013 at 10:40:13PM +0100, Marcin Slusarz wrote:
> On Mon, Feb 18, 2013 at 10:22:20PM +0100, Julien Cristau wrote:
> > On Sat, Feb 16, 2013 at 16:45:54 +0100, Marcin Slusarz wrote:
> > 
> > > Libunwind generates backtraces much more reliably than glibc's "backtrace".
> > > 
> > mind adding an example in the commit message (before/after)?
> Sure.
> 
> ---
> From: Marcin Slusarz <marcin.slusarz at gmail.com>
> Subject: [PATCH] os: use libunwind to generate backtraces
> 
> Libunwind generates backtraces much more reliably than glibc "backtrace".
> 
> Before:
> 0: /opt/xserver/bin/X (0x400000+0x18ce36) [0x58ce36]
> 1: /opt/xserver/bin/X (xorg_backtrace+0x9) [0x58d119]
> 2: /opt/xserver/bin/X (0x400000+0x190d69) [0x590d69]
> 3: /lib64/libpthread.so.0 (0x7fb904268000+0x10a90) [0x7fb904278a90]
> 4: /lib64/libc.so.6 (ioctl+0x7) [0x7fb902fbf987]
> 5: /usr/lib64/libdrm.so.2 (drmIoctl+0x28) [0x7fb90405ffa8]
> 6: /usr/lib64/libdrm.so.2 (drmCommandWrite+0x1b) [0x7fb90406235b]
> 7: /usr/lib64/libdrm_nouveau.so.2 (nouveau_bo_wait+0x89) [0x7fb902009719]
> 8: /opt/xserver/lib/xorg/modules/drivers/nouveau_drv.so (0x7fb90220e000+0x76f3) [0x7fb9022156f3]
> 9: /opt/xserver/lib/xorg/modules/libexa.so (0x7fb9019c7000+0xbae0) [0x7fb9019d2ae0]
> 10: /opt/xserver/bin/X (0x400000+0x17d2b3) [0x57d2b3]
> 11: /opt/xserver/bin/X (0x400000+0xc9930) [0x4c9930]
> 12: /opt/xserver/bin/X (0x400000+0x3a81a) [0x43a81a]
> 13: /opt/xserver/bin/X (0x400000+0x3d6a1) [0x43d6a1]
> 14: /opt/xserver/bin/X (0x400000+0x2c2ca) [0x42c2ca]
> 15: /lib64/libc.so.6 (__libc_start_main+0xf5) [0x7fb902f019b5]
> 16: /opt/xserver/bin/X (0x400000+0x2c60d) [0x42c60d]
> 17: ?? [0x0]
> 
> After:
> 0: /opt/xserver/bin/X (OsSigHandler+0x39) [0x590d69]
> 1: /lib64/libpthread.so.0 (__restore_rt+0x0) [0x7fb904278a8f]
> 2: /lib64/libc.so.6 (ioctl+0x7) [0x7fb902fbf987]
> 3: /usr/lib64/libdrm.so.2 (drmIoctl+0x28) [0x7fb90405ffa8]
> 4: /usr/lib64/libdrm.so.2 (drmCommandWrite+0x1b) [0x7fb90406235b]
> 5: /usr/lib64/libdrm_nouveau.so.2 (nouveau_bo_wait+0x89) [0x7fb902009719]
> 6: /opt/xserver/lib/xorg/modules/drivers/nouveau_drv.so (nouveau_exa_download_from_screen+0x1a3) [0x7fb9022156f3]
> 7: /opt/xserver/lib/xorg/modules/libexa.so (exaGetImage+0x1f0) [0x7fb9019d2ae0]
> 8: /opt/xserver/bin/X (miSpriteGetImage+0x173) [0x57d2b3]
> 9: /opt/xserver/bin/X (compGetImage+0xb0) [0x4c9930]
> 10: /opt/xserver/bin/X (ProcGetImage+0x55a) [0x43a81a]
> 11: /opt/xserver/bin/X (Dispatch+0x341) [0x43d6a1]
> 12: /opt/xserver/bin/X (main+0x3ba) [0x42c2ca]
> 13: /lib64/libc.so.6 (__libc_start_main+0xf5) [0x7fb902f019b5]
> 14: /opt/xserver/bin/X (_start+0x29) [0x42c60d]
> 15: ? (?+0x29) [0x29]
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz at gmail.com>
> ---
>  configure.ac            |  7 +++++
>  include/dix-config.h.in |  3 +++
>  os/Makefile.am          |  5 ++++
>  os/backtrace.c          | 70 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 85 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 9415a54..4a292da 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -309,6 +309,13 @@ AC_CHECK_HEADER([execinfo.h],[
>      ])]
>  )
>  
> +PKG_CHECK_MODULES(LIBUNWIND, libunwind, [HAVE_LIBUNWIND=yes], [HAVE_LIBUNWIND=no])
> +if test "x$HAVE_LIBUNWIND" = xyes; then
> +	AC_DEFINE(HAVE_LIBUNWIND, 1, [Have libunwind support])
> +fi
> +AM_CONDITIONAL(HAVE_LIBUNWIND, [test "x$HAVE_LIBUNWIND" = xyes])
> +
> +
>  dnl ---------------------------------------------------------------------------
>  dnl Bus options and CPU capabilities.  Replaces logic in
>  dnl hw/xfree86/os-support/bus/Makefile.am, among others.
> diff --git a/include/dix-config.h.in b/include/dix-config.h.in
> index 578f249..5102263 100644
> --- a/include/dix-config.h.in
> +++ b/include/dix-config.h.in
> @@ -60,6 +60,9 @@
>  /* Has backtrace support */
>  #undef HAVE_BACKTRACE
>  
> +/* Has libunwind support */
> +#undef HAVE_LIBUNWIND
> +
>  /* Define to 1 if you have the <byteswap.h> header file. */
>  #undef HAVE_BYTESWAP_H
>  
> diff --git a/os/Makefile.am b/os/Makefile.am
> index 8891485..364b6da 100644
> --- a/os/Makefile.am
> +++ b/os/Makefile.am
> @@ -34,6 +34,11 @@ if XDMCP
>  libos_la_SOURCES += $(XDMCP_SRCS)
>  endif
>  
> +if HAVE_LIBUNWIND
> +AM_CFLAGS += $(LIBUNWIND_CFLAGS)
> +libos_la_LIBADD += $(LIBUNWIND_LIBS)
> +endif
> +
>  EXTRA_DIST = $(SECURERPC_SRCS) $(XDMCP_SRCS)
>  
>  if SPECIAL_DTRACE_OBJECTS
> diff --git a/os/backtrace.c b/os/backtrace.c
> index daac60c..02aeb03 100644
> --- a/os/backtrace.c
> +++ b/os/backtrace.c
> @@ -30,6 +30,75 @@
>  #include <errno.h>
>  #include <string.h>
>  
> +#ifdef HAVE_LIBUNWIND
> +
> +#define UNW_LOCAL_ONLY
> +#include <libunwind.h>
> +
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif
> +#include <dlfcn.h>
> +
> +void
> +xorg_backtrace(void)
> +{
> +    unw_cursor_t cursor;
> +    unw_context_t context;
> +    unw_word_t off;
> +    unw_proc_info_t pip;
> +    int ret, i = 0;
> +    char procname[256];
> +    const char *filename;
> +    Dl_info dlinfo;
> +
> +    pip.unwind_info = NULL;
> +    ret = unw_getcontext(&context);
> +    if (ret) {
> +        ErrorFSigSafe("unw_getcontext: %d\n", ret);
> +        return;
> +    }
> +
> +    ret = unw_init_local(&cursor, &context);
> +    if (ret) {
> +        ErrorFSigSafe("unw_init_local: %d\n", ret);
> +        return;
> +    }
> +
> +    ErrorFSigSafe("\n");
> +    ErrorFSigSafe("Backtrace:\n");
> +    ret = unw_step(&cursor);
> +    while (ret > 0) {
> +        ret = unw_get_proc_info(&cursor, &pip);
> +        if (ret) {
> +            ErrorFSigSafe("unw_get_proc_info: %d\n", ret);

since we use ErrorFSigSafe for errors and backtrace can you add some hint
that this is an error, not part of the backtrace, to the message?
same goes for the few cases below that aren't part of the backtrace.
also, maybe use unw_strerror in addition to the error code.

> +            break;
> +        }
> +
> +        ret = unw_get_proc_name(&cursor, procname, 256, &off);
> +        if (ret && ret != -UNW_ENOMEM) {
> +            if (ret != -UNW_EUNSPEC)
> +                ErrorFSigSafe("unw_get_proc_name: %d\n", ret);
> +            procname[0] = '?';
> +            procname[1] = 0;
> +        }
> +
> +        if (dladdr((void *)(pip.start_ip + off), &dlinfo) && dlinfo.dli_fname &&
> +                *dlinfo.dli_fname)
> +            filename = dlinfo.dli_fname;
> +        else
> +            filename = "?";
> +
> +        ErrorFSigSafe("%u: %s (%s%s+0x%x) [%p]\n", i++, filename, procname,
> +            ret == -UNW_ENOMEM ? "..." : "", (int)off, (void *)(pip.start_ip + off));
> +
> +        ret = unw_step(&cursor);
> +        if (ret < 0)
> +            ErrorFSigSafe("unw_step: %d\n", ret);
> +    }
> +    ErrorFSigSafe("\n");
> +}
> +#else

please add a /* HAVE_LIBUNWIND */ here

looks good otherwise though, thanks.

Cheers,
   Peter

>  #ifdef HAVE_BACKTRACE
>  #ifndef _GNU_SOURCE
>  #define _GNU_SOURCE
> @@ -246,3 +315,4 @@ xorg_backtrace(void)
>  
>  #endif
>  #endif
> +#endif
> -- 
> 1.8.1