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

[Yao Qi <yao@codesourcery.com>]

On 01/17/2014 03:01 AM, Doug Evans wrote:
> 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.]
> 

I thought gettimeofday module has been imported, but it wasn't.  Then,
we can import it in next step.

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

FAOD, current pattern ("if (debug_threads) debug_printf ()") works fine
by me.  It has been good enough, IMO.

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

debug_printf doesn't have any issues used in ipa, but debug_level_incr
has.  Probably we can use "#ifndef IN_PROCESS_AGENT" to guarantee
debug_level stuffs can't be used in ipa.

-- 
Yao (齐尧)