This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 4/6] gdbserver: Delimit debugging output for readability
- From: Yao Qi <yao at codesourcery dot com>
- To: Doug Evans <dje at google dot com>
- Cc: Pedro Alves <palves at redhat dot com>, gdb-patches <gdb-patches at sourceware dot org>
- Date: Fri, 17 Jan 2014 20:57:10 +0800
- Subject: Re: [PATCH 4/6] gdbserver: Delimit debugging output for readability
- Authentication-results: sourceware.org; auth=none
- References: <yjt2zjnztait dot fsf at ruffy dot mtv dot corp dot google dot com> <52B1842F dot 5020401 at redhat dot com> <21205 dot 55987 dot 69477 dot 892571 at ruffy dot mtv dot corp dot google dot com> <52D826DF dot 4000505 at codesourcery dot com> <CADPb22Ts6CMhb_cEBJRDvWPFqgO7YBZaJ1PGEqj0HDVS=tKzGA at mail dot gmail dot 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 (éå)