List:Commits« Previous MessageNext Message »
From:Sergei Golubchik Date:December 27 2007 3:57pm
Subject:Re: bk commit into 5.1 tree (davi:1.2644) BUG#31891
View as plain text  
Hi!

On Dec 20, Davi Arnaut wrote:
> ChangeSet@stripped, 2007-12-20 15:11:16-02:00, davi@stripped +3 -0
>   Bug#31891 Meaningful stack trace
>   
>   Currently, when the MySQL server crashes, the generated stack
>   dump is numeric and needs external tools to properly resolve
>   the functions names. The problem is that this is not very helpful
>   to someone who has a limited knowledge of debugging techniques.
>   Also the generated stack trace only contains the functions names
>   and requires special code for each platform (because of different
>   stack layouts).
>   
>   Nowadays, newer versions of the GNU C Library provides a set of
>   functions to obtain and manipulate a stack traces from within the
>   program. On systems that use teh ELF binary format, the stack trace
>   will contain important informations such as the shared object where
>   the call was generated, an offset into the function, and the actual
>   return address, and on other systems, only the return address is
>   presented -- which equals the current MySQL stack trace. Currently,
>   the library generates meaningful stack traces on the following
>   platforms: i386, x86_64, powerpc, ia64, alpha and s390. Having
>   the function name also makes possible the name demangling of
>   C++ functions.

I would prefer a shorter changeset comment. It doesn't need to be a
novel, a short summary is enough. For example:

   Bug#31891 Meaningful stack trace

   On crashes generate a user-friendly resolved and demangled stack
   trace when libc provides the necessary functions (newer libc on i386,
   x86_64, powerpc, ia64, alpha and s390). Otherwise print a numeric
   stack trace as before, relying on resolve_stack_dump utility.

> diff -Nrup a/sql/stacktrace.c b/sql/stacktrace.c
> --- a/sql/stacktrace.c	2007-07-23 14:39:47 -03:00
> +++ b/sql/stacktrace.c	2007-12-20 15:11:14 -02:00
> @@ -93,9 +98,82 @@ inline uint32* find_prev_pc(uint32* pc, 
>  }
>  #endif /* defined(__alpha__) && defined(__GNUC__) */
>  
> +#if HAVE_BACKTRACE && HAVE_ABI_CXA_DEMANGLE && HAVE_WEAK_SYMBOL
> +#define BACKTRACE_DEMANGLE 1
> +#endif
> +
> +#ifdef BACKTRACE_DEMANGLE
> +
> +char *__cxa_demangle(const char *mangled_name, char *output_buffer,
> +                     unsigned int *length, int *status) __attribute__((__weak__));
> +
> +static char *my_demangle(const char *mangled_name, int *status)
> +{
> +  if (__cxa_demangle)
> +    return __cxa_demangle(mangled_name, NULL, NULL, status);
> +  else
> +    return NULL;
> +}

Aren't you overdoing it ?

Why are you doing configure check for __cxa_demangle and configure check
for __weak__ and runtime detection of __cxa_demangle ?

I'd say that only configure check for __cxa_demangle is enough.

Or - if you really believe that it's widespread to have libraries with
and without __cxa_demangle on systems where the same mysqld binary can
be run (and thus runtime detection is necessary) - why do you do
configure-time check for __cxa_demangle and weak ?

> +static void my_demangle_symbols(char **addrs, int n)
> +{
> +  int status, i;
> +  char *begin, *end, *demangled;
> +
> +  for (i= 0; i < n; i++)
> +  {
> +    demangled= NULL;
> +    begin= strchr(addrs[i], '(');
> +    end= begin ? strchr(begin, '+') : NULL;
> +
> +    if (begin && end)
> +    {
> +      *begin++= *end++= '\0';
> +      demangled= my_demangle(begin, &status);
> +      if (!demangled || status)
> +      {
> +        demangled= NULL;
> +        begin[-1]= '(';
> +        end[-1]= '+';
> +      }
> +    }
> +
> +    if (demangled)
> +      fprintf(stderr, "%s(%s+%s\n", addrs[i], demangled, end);
> +    else
> +      fprintf(stderr, "%s\n", addrs[i]);
> +  }
> +}
> +#endif
> +
> +
> +#if HAVE_BACKTRACE
> +static void backtrace_current_thread(void)
> +{
> +  void *addrs[32];

I'd have a larger array here - we have few recursive functions, and I
believe 32 may be not enough.

> +  char **strings= NULL;
> +  int n = backtrace(addrs, array_elements(addrs));
> +#ifdef BACKTRACE_DEMANGLE
> +  if ((strings= backtrace_symbols(addrs, n)))
> +  {
> +    my_demangle_symbols(strings, n);
> +    free(strings);
> +  }
> +  else
> +#endif
> +  {
> +    backtrace_symbols_fd(addrs, n, fileno(stderr));
> +  }
> +}
> +#endif
> +
  
Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Dachauer Str. 37, D-80335 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140
Thread
bk commit into 5.1 tree (davi:1.2644) BUG#31891Davi Arnaut20 Dec
  • Re: bk commit into 5.1 tree (davi:1.2644) BUG#31891Sergei Golubchik27 Dec
    • Re: bk commit into 5.1 tree (davi:1.2644) BUG#31891Davi Arnaut27 Dec
      • Re: bk commit into 5.1 tree (davi:1.2644) BUG#31891Sergei Golubchik27 Dec
        • Re: bk commit into 5.1 tree (davi:1.2644) BUG#31891Davi Arnaut27 Dec
          • Re: bk commit into 5.1 tree (davi:1.2644) BUG#31891Sergei Golubchik28 Dec
            • Re: bk commit into 5.1 tree (davi:1.2644) BUG#31891Davi Arnaut28 Dec