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

[Pedro Alves <palves at redhat dot com>]

On 01/15/2014 12:47 AM, Doug Evans wrote:

> Hi.
> While going through the reviews, I found myself wanting something more,
> namely timestamps (like "set debug timestamp on" in gdb).
> 
> This patch adds a check for gettimeofday and doesn't fall back to using one
> in libiberty or gnulib, leaving that for another day.
> 
> It also adds macro FUNCTION_NAME, based on the implementation in gdb_assert.h.
> A subsequent patch will use this when replacing abbreviations with
> function names.
> 
> Unlike gdb, I didn't add an option to enable timestamps, you get them
> automagically with --debug.  I can do that if people want.

I'd like that.  I've had to diff logs (side by side in e.g., kdiff3)
more than once in the past to track tricky stuff, and timestamps
unfortunately make that impossible.

> I had a version of this patch that used indentation to represent
> the heirarchy of debug_level_{enter,exit}.  I left the basic support
> in thinking it might be useful.  I can either remove it or add the indentation,
> but I found myself not really wanting the indentation.

I don't have a strong opinion, as I didn't make use of this
yet, but I think I wouldn't want indentation.  (E.g., maybe printing
a level counter instead of indentation could be better.).

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

> +#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 ?

Otherwise this all looks good to me.

>  /* 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?

-- 
Pedro Alves