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

[Doug Evans <dje at google dot com>]

On Thu, Jan 16, 2014 at 10:37 AM, Yao Qi <yao@codesourcery.com> wrote:
> On 01/15/2014 08:47 AM, Doug Evans wrote:
>> While going through the reviews, I found myself wanting something more,
>> namely timestamps (like "set debug timestamp on" in gdb).
>>
>
> Hi Doug,
> Could you help me to understand how useful timestamps are for
> measuring performance? or you use timestamps for other purposes?

All of the above.
I can use it for measuring performance, debugging(!) performance
issues, and debugging in general.

>> This patch adds a check for gettimeofday and doesn't fall back to using one
>> in libiberty or gnulib, leaving that for another day.
>
> gdbserver has used gnulib, which means we can use gettimeofday
> unconditionally in gdbserver?

Left for another day.
There's no loss in splitting this up into steps.
[The next step will itself need to be spit up into several steps: get
gettimeofday from gnulib, and then have gdbserver (and gdb) use it -
it may be a nop for gdb, say gnulib being preferred over libiberty,
but something to be confirmed nonetheless.]

>> diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
>> index 1b0da6c..e7d3e4f 100644
>> --- a/gdb/gdbserver/linux-aarch64-low.c
>> +++ b/gdb/gdbserver/linux-aarch64-low.c
>> @@ -323,7 +323,7 @@ aarch64_get_pc (struct regcache *regcache)
>>
>>    collect_register_by_name (regcache, "pc", &pc);
>>    if (debug_threads)
>> -    fprintf (stderr, "stop pc is %08lx\n", pc);
>> +    debug_printf ("stop pc is %08lx\n", pc);
>>    return pc;
>
> IWBN to move "if (debug_threads)" into debug_printf too.

I thought of that, but there are times when you want to check
debug_threads before calling debug_printf.
So then it's a case of some code checking debug_threads and some not,
and then how often will cut-n-paste hacking proliferate unnecessary
tests of debug_printf.  Another alternative is of course to call a
different function that does the check, but now we have two debug
printf functions instead of one.

btw, One thought I have is to make debug_printf a macro (or create
DEBUG_PRINTF or use a different name) so that adding FUNCTION_NAME
becomes automagic.  OTOH, I'm not sure always including the function
name will be a net win - it could be, guess it might depend on
personal preference (--debug=timestamp,funcname ? :-)).

If people really want debug_threads tests in debug_printf (which is
fine by me), that can be left for another day too.
It is arguably a separate step since it's an additional change on top
of replacing fprintf (stderr, ...) with debug_printf.

>> 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
>> @@ -17,6 +17,7 @@
>
>> +
>> +/* Increase or decrease the debug printf call nesting level.
>> +   FUNCTION_NAME is the name of the calling function, or NULL if unknown.
>> +   Call this when entering major routines that can trigger a lot of debug
>> +   output before it exits.  It allows the reader to associate subsequent
>> +   debug output to the call that ultimately triggered it.  */
>> +
>> +void
>> +debug_level_incr (int incr, const char *function_name)
>> +{
>> +  gdb_assert (incr == 1 || incr == -1);
>> +
>> +  /* Increment(/decrement) the level by one before printing the function name,
>> +     to distinguish this as an entry(/exit) point.
>> +     Then increment(/decrement) it again so that debugging printfs within
>> +     the function are recognized as such.  */
>> +  if (function_name != NULL)
>> +    {
>> +      debug_nesting_level += incr;
>> +      debug_printf ("%s %s\n",
>> +                 incr > 0 ? ">>>>entering" : "<<<<exiting",
>> +                 function_name);
>> +    }
>> +  debug_nesting_level += incr;
>> +
>> +  /* Don't crash on mismatched enter/exit, but still inform the user.  */
>> +  if (debug_nesting_level < 0)
>> +    {
>> +      debug_printf ("ERROR: mismatch in debug_level_enter/exit, level < 0\n");
>> +      debug_nesting_level = 0;
>> +    }
>> +}
>> +
>
> utils.c is compiled to both gdbserver and ipa.  IMO,
> ipa code should be thread-safe, because it can be used by a
> multi-threaded program.

That would argue for removing the indentation support, at least for now.
Fine by me.

OTOH, it seemed like ipa code has its own debug printf'ing so it can
prepend PROG, so I'm not sure this is necessary.
OTOOH, it'd be less preferable to assume(!) ipa code would never call
debug_printf.
Thoughts?