Re: bk commit into 5.1 tree (davi:1.2644) BUG#31891

[Sergei Golubchik]

Hi!

On Dec 27, Davi Arnaut wrote:
> Sergei Golubchik wrote:
> 
> >> 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 ?
> 
> The __weak__ symbol check is a compiler check and __cxa_demangle a
> library one. Thus, for the sake of usefulness and modularity, I chose to
> separate both.

I didn't mean one combined check. I mean either one of the three:

1. *only* __cxa_demangle check in configure, that is only
AC_CHECK_FUNC(__cxa_demangle). No weak symbols, no nothing. Only

#ifdef HAVE_CXA_DEMANGLE
    return __cxa_demangle(mangled_name, NULL, NULL, status);
#endif

(disclaimer: I don't know if configure's generated symbol for
__cxa_demangle will be HAVE_CXA_DEMANGLE, see the above as an example
only)

2. Only __weak__ check, and in the code you do

char *__cxa_demangle(const char *mangled_name, char *output_buffer,
                     unsigned int *length, int *status) __attribute__((__weak__))
{
  return mangled_name;
}

and then use __cxa_demangle without any checks.

3. same as 2. but without configure check for __weak__, using __GNUC__
instead.

> >> +#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.
> 
> My magic 8-Ball tells me that 128 shall be enough. Agree?

Yes.
 
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