Re: [PATCH] Extend tsave: save start time, stop time, user and notes

[Pedro Alves <palves@redhat.com>]

On 10/11/2012 04:53 PM, Dmitry Kozlov wrote:

>> Logically, this patch depends on your previous patch
>>
>>   PATCH fix start-time and stop-time in trace-status
>>   http://sourceware.org/ml/gdb-patches/2012-09/msg00621.html
>>
>> otherwise, we generate start-time and stop-time in trace file in decimal format, while gdb still interprets it as hex.

Since gdbserver and gdb have always been in disagreement (IOW, this
never worked correctly with gdbserver, so we're free to change it), and
the RSP uses hex everywhere (*), let's fix gdbserver instead.

  @cindex remote protocol, field separator
  Fields within the packet should be separated using @samp{,} @samp{;} or
  @samp{:}.  Except where otherwise noted all numbers are represented in
  @sc{hex} with leading zeros suppressed.

> +2012-10-04  Dmitry Kozlov  <ddk@codesourcery.com>
> +
> +	* tracepoint.c (trace_save): Add saving starttime, stoptime, user and notes.
> +
> +2012-10-04  Dmitry Kozlov  <ddk@mentor.com>
> +
> +	* tracepoint.c (trace_status_command): Fix type of printf arg.
> +	(trace_status_mi): Likewise.

Looks like something stale (two entries).

> +
>  2012-10-03  Doug Evans  <dje@google.com>
>  
>  	PR symtab/14601
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 0f94150..959ede5 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -3018,10 +3018,11 @@ trace_save (const char *filename, int target_does_save)
>  	   (ts->running ? '1' : '0'), stop_reason_names[ts->stop_reason]);
>    if (ts->stop_reason == tracepoint_error)
>      {
> -      char *buf = (char *) alloca (strlen (ts->stop_desc) * 2 + 1);
> +      char *buf = (char *) xmalloc (strlen (ts->stop_desc) * 2 + 1);
>  
>        bin2hex ((gdb_byte *) ts->stop_desc, buf, 0);
>        fprintf (fp, ":%s", buf);
> +      xfree (buf);
>      }
>    fprintf (fp, ":%x", ts->stopping_tracepoint);
>    if (ts->traceframe_count >= 0)
> @@ -3036,6 +3037,35 @@ trace_save (const char *filename, int target_does_save)
>      fprintf (fp, ";disconn:%x", ts->disconnected_tracing);
>    if (ts->circular_buffer)
>      fprintf (fp, ";circular:%x", ts->circular_buffer);
> +  if (ts->start_time)
> +    {
> +      fprintf (fp, ";starttime:%ld%06ld",
> +    		(long int) (ts->start_time / 1000000),
> +    		(long int) (ts->start_time % 1000000));

See above.  Numbers in the remote protocol are represented in hex.

> +    }
> +  if (ts->stop_time)
> +    {
> +      fprintf (fp, ";stoptime:%ld%06ld",
> +    		(long int) (ts->stop_time / 1000000),
> +    		(long int) (ts->stop_time % 1000000));
> +    }
> +  if (ts->notes && ts->notes[0] != 0 )

Spurious space after 0.

> +    {
> +	  char *buf = (char *) xmalloc (strlen (ts->notes) * 2 + 1);

Unnecessary cast.

> +
> +	  bin2hex ((gdb_byte *) ts->notes, buf, 0);
> +	  fprintf (fp, ";notes:%s", buf);
> +	  xfree (buf);


Indentation is off (compared to ts->stop_time branch above).
Check tabs vs spaces.

> +    }
> +  if (ts->user_name)

As Yao pointed out, shouldn't this get the same check for empty string?

> +    {
> +	  char *buf = (char *) xmalloc (strlen (ts->user_name) * 2 + 1);
> +
> +	  bin2hex ((gdb_byte *) ts->user_name, buf, 0);
> +	  fprintf (fp, ";username:%s", buf);
> +	  xfree (buf);
> +    }

-- 
Pedro Alves