Hi Pedro,
would you be so kind to review/approve whole 3 tracepoints patches 
together. They are interdependent, so it is much easier to look at them 
together. I did my best to satisfy all your and Yao's comments.

Stan, I will add tests after these 3 patches will be approved because I 
don't want to rewrite tests all time patches change.

Thank you,
Dmitry

On 10/11/2012 09:09 PM, Pedro Alves wrote:
> 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);
>> +    }