Sergei Golubchik wrote:
> 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
[..]
> I would prefer a shorter changeset comment. It doesn't need to be a
> novel, a short summary is enough. For example:
Novel.. heh.
> 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.
OK.
>> 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'd say that only configure check for __cxa_demangle is enough.
Breaks if the compiler doesn't support the weak symbol/attribute syntax
or other problems. But this is a very minor detail, I don't see any
downside/complexity in keeping those two checks separated.
> 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 ?
As explained in the per-file comment, to check the function call
"signature". Not strictly necessary but there is no downside.
>> +
>> +
>> +#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?
Regards,
--
Davi Arnaut, Software Engineer
MySQL Inc, www.mysql.com
Are you MySQL certified? www.mysql.com/certification
