Re: [PATCH 4/6] gdbserver: Delimit debugging output for readability

[Doug Evans <dje at google dot com>]

On Thu, Jan 16, 2014 at 9:22 AM, Pedro Alves <palves@redhat.com> wrote:
>> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
>> index 13b3ff6..3706577 100644
>> --- a/gdb/gdbserver/tracepoint.c
>> +++ b/gdb/gdbserver/tracepoint.c
>> @@ -61,6 +61,8 @@
>>
>>  */
>>
>> +#ifdef IN_PROCESS_AGENT
>> +
>>  static void trace_vdebug (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
>>
>>  static void
>> @@ -81,6 +83,19 @@ trace_vdebug (const char *fmt, ...)
>>        trace_vdebug ((fmt), ##args);          \
>>    } while (0)
>>
>> +#else
>> +
>> +#define trace_debug_1(level, fmt, args...)   \
>> +  do {                                               \
>> +    if (level <= debug_threads)                      \
>> +      {                                              \
>> +     debug_printf ((fmt), ##args);           \
>> +     debug_printf ("\n");                    \
>> +      }                                              \
>> +  } while (0)
>> +
>
> Please write this in a way that preserves printing PROG
> in gdbserver.  When debugging gdbserver and the IPA both
> tracing at the same time (fast tracepoints and regular
> tracepoints), it's confusing not to have there explicitly
> who printed what, because both programs print the
> same messages.

A counter-proposal is that no information is lost given that if PROG
isn't present you know it's gdbserver, and it makes the debugging
output consistent with the rest of gdbserver.
Otherwise "Consistency Is Good" is going to make me want to prepend
PROG to all gdbserver output, which I don't have a problem with, but
thought I'd double check.

>
>> +#endif
>> +
>>  #define trace_debug(FMT, args...)            \
>>    trace_debug_1 (1, FMT, ##args)
>>
>> @@ -338,7 +353,7 @@ tracepoint_look_up_symbols (void)
>>        if (look_up_one_symbol (symbol_list[i].name, addrp, 1) == 0)
>>       {
>>         if (debug_threads)
>> -         fprintf (stderr, "symbol `%s' not found\n", symbol_list[i].name);
>> +         debug_printf ("symbol `%s' not found\n", symbol_list[i].name);
>>         return;
>>       }
>>      }
>> diff --git a/gdb/gdbserver/utils.c b/gdb/gdbserver/utils.c
>> index eff4499..1ce5512 100644
>> --- a/gdb/gdbserver/utils.c
>> +++ b/gdb/gdbserver/utils.c
>
> Could this new debug support code be put in a new file
> instead?  E.g., gdbserver/debug.c ?

It's a pretty-small file, and utils.c is kind of our collective
kitchen sink for such things.
I have no preference, just double checking that that's what you want.
For one, Consistency Is Good means I'd want to push for gdb/debug.c
(if/when there is something to put in there) and I want to make sure
everyone is ok with that - no reason why they wouldn't be, per se,
just checking.

> Otherwise this all looks good to me.

Thanks.

>>  /* Temporary storage using circular buffer.  */
>>  #define NUMCELLS 10
>>  #define CELLSIZE 50
>> diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
>> index 6d3df71..6c781c0 100644
>> --- a/gdb/gdbserver/utils.h
>> +++ b/gdb/gdbserver/utils.h
>> @@ -19,10 +19,44 @@
>>  #ifndef UTILS_H
>>  #define UTILS_H
>>
>> +/* Version 2.4 and later of GCC define a magical variable `__PRETTY_FUNCTION__'
>> +   which contains the name of the function currently being defined.
>> +   This is broken in G++ before version 2.6.
>> +   C9x has a similar variable called __func__, but prefer the GCC one since
>> +   it demangles C++ function names.  */
>> +#if (GCC_VERSION >= 2004)
>> +#define FUNCTION_NAME __PRETTY_FUNCTION__
>> +#else
>> +#if defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L
>> +#define FUNCTION_NAME __func__
>> +#endif
>> +#endif
>
> It'd be nice if this and gdb_assert.h's version of the same were
> shared.  That is, e.g., put this in common/common-utils.h instead,
> and make gdb_assert.h define ASSERT_FUNCTION as FUNCTION_NAME
> (or eliminate ASSERT_FUNCTION entirely).  Are you planning on
> doing it?

Done in v2.