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

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

[Dmitry Kozlov]

[-- Attachment #1: Type: text/plain, Size: 170 bytes --]

Hi,
This patch extends tsave command to save some extra trace status 
information: start time, stop time, user and notes.
Please consider applying it.

Thank you,
Dmitry

[-- Attachment #2: gdb_tsave.txt --]
[-- Type: text/plain, Size: 1219 bytes --]

2012-10-03  Dmitry Kozlov  <ddk@mentor.com>
 
	* tracepoint.c (trace_save): Add saving starttime, stoptime, user and notes.
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 0f94150..f7e96eb 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -3036,6 +3036,24 @@ 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));
+  if (ts->stop_time)
+    fprintf (fp, ";stoptime:%ld%06ld", (long int) (ts->stop_time / 1000000), (long int) (ts->stop_time % 1000000));
+  if (ts->notes)
+  {
+    char *buf = (char *) alloca (strlen (ts->notes) * 2 + 1);
+    bin2hex ((gdb_byte *) ts->notes, buf, 0);
+    fprintf (fp, ";notes:%s", buf);
+
+  }
+  if (ts->user_name)
+  {
+    char *buf = (char *) alloca (strlen (ts->user_name) * 2 + 1);
+    bin2hex ((gdb_byte *) ts->user_name, buf, 0);
+    fprintf (fp, ";username:%s", buf);
+
+  }
   fprintf (fp, "\n");
 
   /* Note that we want to upload tracepoints and save those, rather

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

[Yao Qi]

On 10/03/2012 10:09 PM, Dmitry Kozlov wrote:

In general, I think it is useful to include these attributes in 
tracefile saved by command 'tsave'.

> 2012-10-03  Dmitry Kozlov<ddk@mentor.com>
>
> 	* tracepoint.c (trace_save): Add saving starttime, stoptime, user and notes.
>
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 0f94150..f7e96eb 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -3036,6 +3036,24 @@ 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));

This line is too long...

> +  if (ts->stop_time)
> +    fprintf (fp, ";stoptime:%ld%06ld", (long int) (ts->stop_time / 1000000), (long int) (ts->stop_time % 1000000));

and this line.

> +  if (ts->notes)

ts->notes will never be NULL, because GDBserver will always send 
"notes:" in qTStatus packet (See gdbserver/tracepoint.c:cmd_qtstatus), 
but other stubs may don't include "notes:" in qTStatus packet, so  we 
may do the check here like this,

    if (ts->notes && ts->notes[0] != 0)

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

We need a blank line here.  Looks 'alloca' is not encouraged to use, and 
it should be replaced with xmalloc/xfree.

   [PATCH] Replace potentially unsafe alloca with xmalloc/xfree in 
value_concat
   http://sourceware.org/ml/gdb-patches/2012-09/msg00274.html

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

Likewise.

> +    bin2hex ((gdb_byte *) ts->user_name, buf, 0);
> +    fprintf (fp, ";username:%s", buf);
> +
> +  }
>     fprintf (fp, "\n");
>
>     /* Note that we want to upload tracepoints and save those, rather

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

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

[Dmitry Kozlov]

[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]

Hello Yao,
thank you for review,
I have updated patch based on your recommendations. Please see attached 
file.

And you are right this patch depends on my previous patch

   PATCH fix start-time and stop-time in trace-status
   http://sourceware.org/ml/gdb-patches/2012-09/msg00621.htm

Thank you,
Dmitry

On 10/08/2012 04:47 PM, Yao Qi wrote:
> On 10/03/2012 10:09 PM, Dmitry Kozlov wrote:
>
> In general, I think it is useful to include these attributes in 
> tracefile saved by command 'tsave'.
>
>> 2012-10-03  Dmitry Kozlov<ddk@mentor.com>
>>
>>     * tracepoint.c (trace_save): Add saving starttime, stoptime, user 
>> and notes.
>>
>> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
>> index 0f94150..f7e96eb 100644
>> --- a/gdb/tracepoint.c
>> +++ b/gdb/tracepoint.c
>> @@ -3036,6 +3036,24 @@ 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));
>
> This line is too long...
>
>> +  if (ts->stop_time)
>> +    fprintf (fp, ";stoptime:%ld%06ld", (long int) (ts->stop_time / 
>> 1000000), (long int) (ts->stop_time % 1000000));
>
> and this line.
>
>> +  if (ts->notes)
>
> ts->notes will never be NULL, because GDBserver will always send 
> "notes:" in qTStatus packet (See gdbserver/tracepoint.c:cmd_qtstatus), 
> but other stubs may don't include "notes:" in qTStatus packet, so  we 
> may do the check here like this,
>
>    if (ts->notes && ts->notes[0] != 0)
>
>> +  {
>> +    char *buf = (char *) alloca (strlen (ts->notes) * 2 + 1);
>
> We need a blank line here.  Looks 'alloca' is not encouraged to use, 
> and it should be replaced with xmalloc/xfree.
>
>   [PATCH] Replace potentially unsafe alloca with xmalloc/xfree in 
> value_concat
>   http://sourceware.org/ml/gdb-patches/2012-09/msg00274.html
>
>> +    bin2hex ((gdb_byte *) ts->notes, buf, 0);
>> +    fprintf (fp, ";notes:%s", buf);
>> +
>> +  }
>> +  if (ts->user_name)
>> +  {
>> +    char *buf = (char *) alloca (strlen (ts->user_name) * 2 + 1);
>
> Likewise.
>
>> +    bin2hex ((gdb_byte *) ts->user_name, buf, 0);
>> +    fprintf (fp, ";username:%s", buf);
>> +
>> +  }
>>     fprintf (fp, "\n");
>>
>>     /* Note that we want to upload tracepoints and save those, rather
>
> 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.


[-- Attachment #2: gdb_tsave_v2.diff --]
[-- Type: text/x-patch, Size: 2209 bytes --]

commit e150b4b622d5f4ebf4e1d7c1f35510fd17c0ec8b
Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
Date:   Thu Oct 4 13:06:38 2012 +0400

    Extend tsave to save trace start time, stop time and notes.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fe3728b..135f566 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2012-10-04  Dmitry Kozlov  <ddk@mentor.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.
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 0f94150..0e44316 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -3018,7 +3018,7 @@ 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);
@@ -3036,6 +3036,33 @@ 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));
+    }
+  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 )
+    {
+	  char *buf = (char *) xmalloc (strlen (ts->notes) * 2 + 1);
+
+	  bin2hex ((gdb_byte *) ts->notes, buf, 0);
+	  fprintf (fp, ";notes:%s", buf);
+    }
+  if (ts->user_name)
+    {
+	  char *buf = (char *) xmalloc (strlen (ts->user_name) * 2 + 1);
+
+	  bin2hex ((gdb_byte *) ts->user_name, buf, 0);
+	  fprintf (fp, ";username:%s", buf);
+    }
+
   fprintf (fp, "\n");
 
   /* Note that we want to upload tracepoints and save those, rather

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

[Dmitry Kozlov]

[-- Attachment #1: Type: text/plain, Size: 2563 bytes --]

Hi Yao,
I have added xfree to this patch also. Please se attached patch.

Thank you,
Dmitry

On 10/08/2012 04:47 PM, Yao Qi wrote:
> On 10/03/2012 10:09 PM, Dmitry Kozlov wrote:
>
> In general, I think it is useful to include these attributes in 
> tracefile saved by command 'tsave'.
>
>> 2012-10-03  Dmitry Kozlov<ddk@mentor.com>
>>
>>     * tracepoint.c (trace_save): Add saving starttime, stoptime, user 
>> and notes.
>>
>> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
>> index 0f94150..f7e96eb 100644
>> --- a/gdb/tracepoint.c
>> +++ b/gdb/tracepoint.c
>> @@ -3036,6 +3036,24 @@ 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));
>
> This line is too long...
>
>> +  if (ts->stop_time)
>> +    fprintf (fp, ";stoptime:%ld%06ld", (long int) (ts->stop_time / 
>> 1000000), (long int) (ts->stop_time % 1000000));
>
> and this line.
>
>> +  if (ts->notes)
>
> ts->notes will never be NULL, because GDBserver will always send 
> "notes:" in qTStatus packet (See gdbserver/tracepoint.c:cmd_qtstatus), 
> but other stubs may don't include "notes:" in qTStatus packet, so  we 
> may do the check here like this,
>
>    if (ts->notes && ts->notes[0] != 0)
>
>> +  {
>> +    char *buf = (char *) alloca (strlen (ts->notes) * 2 + 1);
>
> We need a blank line here.  Looks 'alloca' is not encouraged to use, 
> and it should be replaced with xmalloc/xfree.
>
>   [PATCH] Replace potentially unsafe alloca with xmalloc/xfree in 
> value_concat
>   http://sourceware.org/ml/gdb-patches/2012-09/msg00274.html
>
>> +    bin2hex ((gdb_byte *) ts->notes, buf, 0);
>> +    fprintf (fp, ";notes:%s", buf);
>> +
>> +  }
>> +  if (ts->user_name)
>> +  {
>> +    char *buf = (char *) alloca (strlen (ts->user_name) * 2 + 1);
>
> Likewise.
>
>> +    bin2hex ((gdb_byte *) ts->user_name, buf, 0);
>> +    fprintf (fp, ";username:%s", buf);
>> +
>> +  }
>>     fprintf (fp, "\n");
>>
>>     /* Note that we want to upload tracepoints and save those, rather
>
> 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.


[-- Attachment #2: gdb_tsave_v3.diff --]
[-- Type: text/x-patch, Size: 2424 bytes --]

commit 06786b04e1948d0037909e7fa6834ddd7c171968
Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
Date:   Thu Oct 4 13:06:38 2012 +0400

    Extend tsave to save trace start time, stop time and notes.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 53f2078..9196bc2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+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.
+
 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));
+    }
+  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 )
+    {
+	  char *buf = (char *) xmalloc (strlen (ts->notes) * 2 + 1);
+
+	  bin2hex ((gdb_byte *) ts->notes, buf, 0);
+	  fprintf (fp, ";notes:%s", buf);
+	  xfree (buf);
+    }
+  if (ts->user_name)
+    {
+	  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);
+    }
+
   fprintf (fp, "\n");
 
   /* Note that we want to upload tracepoints and save those, rather

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

[Pedro Alves]

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

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

[Dmitry Kozlov]

[-- Attachment #1: Type: text/plain, Size: 3752 bytes --]

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);
>> +    }


[-- Attachment #2: gdb_dates_v4.diff --]
[-- Type: text/x-patch, Size: 3646 bytes --]

commit b256b0abcf8e45a615c4ce763d694d371e988f83
Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
Date:   Fri Oct 12 11:20:50 2012 +0400

    Fix trace-status to output proper start-time and stop-time.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e6867c6..27ad6ab 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
+
+	* tracepoint.c (trace_status_command): Fix type of printf arg to
+	prevent improper type conversion.
+	(trace_status_mi): Likewise.
+
 2012-10-11  Andrew Burgess  <aburgess@broadcom.com>
 
 	* remote-sim.c (gdbsim_create_inferior): Call init_thread_list to
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 8bfbe9c..bb4ace8 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@
+2012-10-12 Dmitry Kozlov <ddk@codesourcery.com>
+
+	* gdbserver/tracepoint.c (cmd_qtstatus): Modify trace-status output to
+	output start time and stop time in hex as gdb expects.
+
 2012-10-01  Andrew Burgess  <aburgess@broadcom.com>
 
 	* server.c (handle_search_memory_1): Include access length in
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 201a25b..5b56f81 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3664,7 +3664,8 @@ cmd_qtstatus (char *packet)
 	   free_space (), phex_nz (trace_buffer_hi - trace_buffer_lo, 0),
 	   circular_trace_buffer,
 	   disconnected_tracing,
-	   plongest (tracing_start_time), plongest (tracing_stop_time),
+	   phex_nz (tracing_start_time, sizeof (tracing_start_time)),
+	   phex_nz (tracing_stop_time, sizeof (tracing_stop_time)),
 	   buf1, buf2);
 }
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index cce8d00..1577ad3 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2041,20 +2041,20 @@ trace_status_command (char *args, int from_tty)
 
 	  /* Reporting a run time is more readable than two long numbers.  */
 	  printf_filtered (_("Trace started at %ld.%06ld secs, stopped %ld.%06ld secs later.\n"),
-			   (long int) ts->start_time / 1000000,
-			   (long int) ts->start_time % 1000000,
-			   (long int) run_time / 1000000,
-			   (long int) run_time % 1000000);
+			   (long int) (ts->start_time / 1000000),
+			   (long int) (ts->start_time % 1000000),
+			   (long int) (run_time / 1000000),
+			   (long int) (run_time % 1000000));
 	}
       else
 	printf_filtered (_("Trace started at %ld.%06ld secs.\n"),
-			 (long int) ts->start_time / 1000000,
-			 (long int) ts->start_time % 1000000);
+			 (long int) (ts->start_time / 1000000),
+			 (long int) (ts->start_time % 1000000));
     }
   else if (ts->stop_time)
     printf_filtered (_("Trace stopped at %ld.%06ld secs.\n"),
-		     (long int) ts->stop_time / 1000000,
-		     (long int) ts->stop_time % 1000000);
+		     (long int) (ts->stop_time / 1000000),
+		     (long int) (ts->stop_time % 1000000));
 
   /* Now report any per-tracepoint status available.  */
   tp_vec = all_tracepoints ();
@@ -2169,12 +2169,12 @@ trace_status_mi (int on_stop)
     char buf[100];
 
     xsnprintf (buf, sizeof buf, "%ld.%06ld",
-	       (long int) ts->start_time / 1000000,
-	       (long int) ts->start_time % 1000000);
+	       (long int) (ts->start_time / 1000000),
+	       (long int) (ts->start_time % 1000000));
     ui_out_field_string (uiout, "start-time", buf);
     xsnprintf (buf, sizeof buf, "%ld.%06ld",
-	       (long int) ts->stop_time / 1000000,
-	       (long int) ts->stop_time % 1000000);
+	       (long int) (ts->stop_time / 1000000),
+	       (long int) (ts->stop_time % 1000000));
     ui_out_field_string (uiout, "stop-time", buf);
   }
 }

[-- Attachment #3: gdb-stop-notes_v2.diff --]
[-- Type: text/x-patch, Size: 3417 bytes --]

commit d9cc6813eccfc2afa35be6b201a222a44c657b55
Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
Date:   Wed Oct 10 14:56:52 2012 +0400

    Modify trace-status output to always output stop-notes.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9196bc2..f34f04e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,8 +1,19 @@
+2012-10-10  Dmitry Kozlov  <ddk@codesourcery.com>
+
+	* tracepoint.c (trace_status_mi):  Modify trace status MI command output to 
+	always output stop-notes.
+	(trace_save): Save stop-notes if any by tsave.
+	(parse_trace_status): Add parsing stop-notes.
+
 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>
+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@codesourcery.com>
 
 	* tracepoint.c (trace_status_command): Fix type of printf arg.
 	(trace_status_mi): Likewise.
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 8bfbe9c..a23bb6d 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@
+2012-10-11 Dmitry Kozlov <ddk@codesourcery.com>
+
+	* gdbserver/tracepoint.c (cmd_qtstatus): Modify trace-status output to
+	always output stop-notes.
+
 2012-10-01  Andrew Burgess  <aburgess@broadcom.com>
 
 	* server.c (handle_search_memory_1): Include access length in
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 201a25b..60ba775 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3657,7 +3657,7 @@ cmd_qtstatus (char *packet)
 	   "circular:%d;"
 	   "disconn:%d;"
 	   "starttime:%s;stoptime:%s;"
-	   "username:%s:;notes:%s:",
+	   "username:%s;notes:%s;stop-notes:%s",
 	   tracing ? 1 : 0,
 	   stop_reason_rsp, tracing_stop_tpnum,
 	   traceframe_count, traceframes_created,
@@ -3665,7 +3665,7 @@ cmd_qtstatus (char *packet)
 	   circular_trace_buffer,
 	   disconnected_tracing,
 	   plongest (tracing_start_time), plongest (tracing_stop_time),
-	   buf1, buf2);
+	   buf1, buf2, buf3);
 }
 
 static void
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 959ede5..507ca41 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2164,6 +2164,8 @@ trace_status_mi (int on_stop)
 
   ui_out_field_string (uiout, "user-name", ts->user_name);
   ui_out_field_string (uiout, "notes", ts->notes);
+  // Always output stop-notes to MI to unify IDE processing of start and stop notes
+  ui_out_field_string (uiout, "stop-notes", ts->stop_desc);
 
   {
     char buf[100];
@@ -3065,6 +3067,14 @@ trace_save (const char *filename, int target_does_save)
 	  fprintf (fp, ";username:%s", buf);
 	  xfree (buf);
     }
+  if (ts->stop_desc)
+    {
+	  char *buf = (char *) xmalloc (strlen (ts->stop_desc) * 2 + 1);
+
+	  bin2hex ((gdb_byte *) ts->stop_desc, buf, 0);
+	  fprintf (fp, ";stop-notes:%s", buf);
+	  xfree (buf);
+    }
 
   fprintf (fp, "\n");
 
@@ -3994,6 +4004,14 @@ Status line: '%s'\n"), p, line);
 	  ts->notes[end] = '\0';
 	  p = p3;
 	}
+      else if (strncmp (p, "stop-notes", p1 - p) == 0)
+	{
+	  ++p1;
+	  ts->stop_desc = xmalloc (strlen (p) / 2);
+	  end = hex2bin (p1, ts->stop_desc, (p3 - p1) / 2);
+	  ts->stop_desc[end] = '\0';
+	  p = p3;
+	}
       else
 	{
 	  /* Silently skip unknown optional info.  */

[-- Attachment #4: gdb_tsave_v4.diff --]
[-- Type: text/x-patch, Size: 2347 bytes --]

commit 4ce80946bb684913900b5e4db3c627b2acd91399
Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
Date:   Fri Oct 12 11:32:25 2012 +0400

    Extend tsave to save trace start time, stop time and notes.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 27ad6ab..3e2a818 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
 
+	* tracepoint.c (trace_save): Add saving starttime, stoptime, user and notes.
+
+2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
+
 	* tracepoint.c (trace_status_command): Fix type of printf arg to
 	prevent improper type conversion.
 	(trace_status_mi): Likewise.
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 1577ad3..8f4f18c 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 = 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,33 @@ 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:%s",
+    		  phex_nz (ts->start_time, sizeof (ts->start_time)));
+    }
+  if (ts->stop_time)
+    {
+      fprintf (fp, ";stoptime:%s",
+    		phex_nz (ts->stop_time, sizeof (ts->stop_time)));
+    }
+  if (ts->notes && ts->notes[0] != 0)
+    {
+      char *buf = xmalloc (strlen (ts->notes) * 2 + 1);
+
+      bin2hex ((gdb_byte *) ts->notes, buf, 0);
+      fprintf (fp, ";notes:%s", buf);
+      xfree (buf);
+    }
+  if (ts->user_name && ts->user_name[0] != 0)
+    {
+      char *buf = xmalloc (strlen (ts->user_name) * 2 + 1);
+
+      bin2hex ((gdb_byte *) ts->user_name, buf, 0);
+      fprintf (fp, ";username:%s", buf);
+      xfree (buf);
+    }
+
   fprintf (fp, "\n");
 
   /* Note that we want to upload tracepoints and save those, rather

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

[Dmitry Kozlov]

[-- Attachment #1: Type: text/plain, Size: 3847 bytes --]

This is duplicate because I made wrong attachment in previous email. All 
patches are v4.

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);
>> +    }


[-- Attachment #2: gdb_dates_v4.diff --]
[-- Type: text/x-patch, Size: 3646 bytes --]

commit b256b0abcf8e45a615c4ce763d694d371e988f83
Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
Date:   Fri Oct 12 11:20:50 2012 +0400

    Fix trace-status to output proper start-time and stop-time.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e6867c6..27ad6ab 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
+
+	* tracepoint.c (trace_status_command): Fix type of printf arg to
+	prevent improper type conversion.
+	(trace_status_mi): Likewise.
+
 2012-10-11  Andrew Burgess  <aburgess@broadcom.com>
 
 	* remote-sim.c (gdbsim_create_inferior): Call init_thread_list to
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 8bfbe9c..bb4ace8 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@
+2012-10-12 Dmitry Kozlov <ddk@codesourcery.com>
+
+	* gdbserver/tracepoint.c (cmd_qtstatus): Modify trace-status output to
+	output start time and stop time in hex as gdb expects.
+
 2012-10-01  Andrew Burgess  <aburgess@broadcom.com>
 
 	* server.c (handle_search_memory_1): Include access length in
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 201a25b..5b56f81 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3664,7 +3664,8 @@ cmd_qtstatus (char *packet)
 	   free_space (), phex_nz (trace_buffer_hi - trace_buffer_lo, 0),
 	   circular_trace_buffer,
 	   disconnected_tracing,
-	   plongest (tracing_start_time), plongest (tracing_stop_time),
+	   phex_nz (tracing_start_time, sizeof (tracing_start_time)),
+	   phex_nz (tracing_stop_time, sizeof (tracing_stop_time)),
 	   buf1, buf2);
 }
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index cce8d00..1577ad3 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2041,20 +2041,20 @@ trace_status_command (char *args, int from_tty)
 
 	  /* Reporting a run time is more readable than two long numbers.  */
 	  printf_filtered (_("Trace started at %ld.%06ld secs, stopped %ld.%06ld secs later.\n"),
-			   (long int) ts->start_time / 1000000,
-			   (long int) ts->start_time % 1000000,
-			   (long int) run_time / 1000000,
-			   (long int) run_time % 1000000);
+			   (long int) (ts->start_time / 1000000),
+			   (long int) (ts->start_time % 1000000),
+			   (long int) (run_time / 1000000),
+			   (long int) (run_time % 1000000));
 	}
       else
 	printf_filtered (_("Trace started at %ld.%06ld secs.\n"),
-			 (long int) ts->start_time / 1000000,
-			 (long int) ts->start_time % 1000000);
+			 (long int) (ts->start_time / 1000000),
+			 (long int) (ts->start_time % 1000000));
     }
   else if (ts->stop_time)
     printf_filtered (_("Trace stopped at %ld.%06ld secs.\n"),
-		     (long int) ts->stop_time / 1000000,
-		     (long int) ts->stop_time % 1000000);
+		     (long int) (ts->stop_time / 1000000),
+		     (long int) (ts->stop_time % 1000000));
 
   /* Now report any per-tracepoint status available.  */
   tp_vec = all_tracepoints ();
@@ -2169,12 +2169,12 @@ trace_status_mi (int on_stop)
     char buf[100];
 
     xsnprintf (buf, sizeof buf, "%ld.%06ld",
-	       (long int) ts->start_time / 1000000,
-	       (long int) ts->start_time % 1000000);
+	       (long int) (ts->start_time / 1000000),
+	       (long int) (ts->start_time % 1000000));
     ui_out_field_string (uiout, "start-time", buf);
     xsnprintf (buf, sizeof buf, "%ld.%06ld",
-	       (long int) ts->stop_time / 1000000,
-	       (long int) ts->stop_time % 1000000);
+	       (long int) (ts->stop_time / 1000000),
+	       (long int) (ts->stop_time % 1000000));
     ui_out_field_string (uiout, "stop-time", buf);
   }
 }

[-- Attachment #3: gdb_stop_notes_v4.diff --]
[-- Type: text/x-patch, Size: 3258 bytes --]

commit 570ec36c0dfd2d19cde2f779ee3f146fc526e44e
Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
Date:   Fri Oct 12 11:37:31 2012 +0400

    Modify trace-status output to always output stop-notes.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3e2a818..dd9edde 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
 
+	* tracepoint.c (trace_status_mi):  Modify trace status MI command to 
+	always output stop-notes.
+	(trace_save): Save stop-notes if any by tsave.
+	(parse_trace_status): Add parsing stop-notes.
+
+2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
+
 	* tracepoint.c (trace_save): Add saving starttime, stoptime, user and notes.
 
 2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index bb4ace8..58a8827 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,10 @@
 2012-10-12 Dmitry Kozlov <ddk@codesourcery.com>
 
+	* gdbserver/tracepoint.c (cmd_qtstatus): Modify trace-status 
+	to always output stop-notes.
+
+2012-10-12 Dmitry Kozlov <ddk@codesourcery.com>
+
 	* gdbserver/tracepoint.c (cmd_qtstatus): Modify trace-status output to
 	output start time and stop time in hex as gdb expects.
 
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 5b56f81..72fca51 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3657,7 +3657,7 @@ cmd_qtstatus (char *packet)
 	   "circular:%d;"
 	   "disconn:%d;"
 	   "starttime:%s;stoptime:%s;"
-	   "username:%s:;notes:%s:",
+	   "username:%s;notes:%s;stop-notes:%s",
 	   tracing ? 1 : 0,
 	   stop_reason_rsp, tracing_stop_tpnum,
 	   traceframe_count, traceframes_created,
@@ -3666,7 +3666,7 @@ cmd_qtstatus (char *packet)
 	   disconnected_tracing,
 	   phex_nz (tracing_start_time, sizeof (tracing_start_time)),
 	   phex_nz (tracing_stop_time, sizeof (tracing_stop_time)),
-	   buf1, buf2);
+	   buf1, buf2, buf3);
 }
 
 static void
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 8f4f18c..0905e89 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2164,6 +2164,8 @@ trace_status_mi (int on_stop)
 
   ui_out_field_string (uiout, "user-name", ts->user_name);
   ui_out_field_string (uiout, "notes", ts->notes);
+  // Always output stop-notes to MI to unify IDE processing of start and stop notes
+  ui_out_field_string (uiout, "stop-notes", ts->stop_desc);
 
   {
     char buf[100];
@@ -3063,6 +3065,14 @@ trace_save (const char *filename, int target_does_save)
       fprintf (fp, ";username:%s", buf);
       xfree (buf);
     }
+  if (ts->stop_desc && ts->stop_desc[0] != 0)
+    {
+      char *buf = xmalloc (strlen (ts->stop_desc) * 2 + 1);
+
+      bin2hex ((gdb_byte *) ts->stop_desc, buf, 0);
+      fprintf (fp, ";stop-notes:%s", buf);
+      xfree (buf);
+    }
 
   fprintf (fp, "\n");
 
@@ -3992,6 +4002,14 @@ Status line: '%s'\n"), p, line);
 	  ts->notes[end] = '\0';
 	  p = p3;
 	}
+      else if (strncmp (p, "stop-notes", p1 - p) == 0)
+	{
+	  ++p1;
+	  ts->stop_desc = xmalloc (strlen (p) / 2);
+	  end = hex2bin (p1, ts->stop_desc, (p3 - p1) / 2);
+	  ts->stop_desc[end] = '\0';
+	  p = p3;
+	}
       else
 	{
 	  /* Silently skip unknown optional info.  */

[-- Attachment #4: gdb_tsave_v4.diff --]
[-- Type: text/x-patch, Size: 2347 bytes --]

commit 4ce80946bb684913900b5e4db3c627b2acd91399
Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
Date:   Fri Oct 12 11:32:25 2012 +0400

    Extend tsave to save trace start time, stop time and notes.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 27ad6ab..3e2a818 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
 
+	* tracepoint.c (trace_save): Add saving starttime, stoptime, user and notes.
+
+2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
+
 	* tracepoint.c (trace_status_command): Fix type of printf arg to
 	prevent improper type conversion.
 	(trace_status_mi): Likewise.
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 1577ad3..8f4f18c 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 = 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,33 @@ 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:%s",
+    		  phex_nz (ts->start_time, sizeof (ts->start_time)));
+    }
+  if (ts->stop_time)
+    {
+      fprintf (fp, ";stoptime:%s",
+    		phex_nz (ts->stop_time, sizeof (ts->stop_time)));
+    }
+  if (ts->notes && ts->notes[0] != 0)
+    {
+      char *buf = xmalloc (strlen (ts->notes) * 2 + 1);
+
+      bin2hex ((gdb_byte *) ts->notes, buf, 0);
+      fprintf (fp, ";notes:%s", buf);
+      xfree (buf);
+    }
+  if (ts->user_name && ts->user_name[0] != 0)
+    {
+      char *buf = xmalloc (strlen (ts->user_name) * 2 + 1);
+
+      bin2hex ((gdb_byte *) ts->user_name, buf, 0);
+      fprintf (fp, ";username:%s", buf);
+      xfree (buf);
+    }
+
   fprintf (fp, "\n");
 
   /* Note that we want to upload tracepoints and save those, rather

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

[Pedro Alves]

On 10/12/2012 09:46 AM, Dmitry Kozlov wrote:
> This is duplicate because I made wrong attachment in previous email. All patches are v4.
> 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.

It's really not.  And I'm not trying to difficult -- I really got confused reading the
single patch.  It's much easier to read one patch per email, and one email per logical
change.  If the patches are related, send them in a patch series (with patches numbered
N/M).  If a series comes as response to a cover email, even better, as then mail clients
will group the patches of the series neatly.  Using git (as you are) it's quite easy to
send series like that (as git-send-email does all that mechanical stuff for you).

In addition of the tests point Stan raised, there is a missing documentation issue.
All MI, RSP, and trace file format changes should be documented in the manual,
and also in NEWS, as otherwise it's as if they didn't exist.

On 10/12/2012 09:46 AM, Dmitry Kozlov wrote:>
> commit b256b0abcf8e45a615c4ce763d694d371e988f83
> Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
> Date:   Fri Oct 12 11:20:50 2012 +0400
>
>     Fix trace-status to output proper start-time and stop-time.
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index e6867c6..27ad6ab 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
> +
> +	* tracepoint.c (trace_status_command): Fix type of printf arg to
> +	prevent improper type conversion.
> +	(trace_status_mi): Likewise.

OK.

> +
>  2012-10-11  Andrew Burgess  <aburgess@broadcom.com>
>
>  	* remote-sim.c (gdbsim_create_inferior): Call init_thread_list to
> diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
> index 8bfbe9c..bb4ace8 100644
> --- a/gdb/gdbserver/ChangeLog
> +++ b/gdb/gdbserver/ChangeLog
> @@ -1,3 +1,8 @@
> +2012-10-12 Dmitry Kozlov <ddk@codesourcery.com>
> +
> +	* gdbserver/tracepoint.c (cmd_qtstatus): Modify trace-status output to
> +	output start time and stop time in hex as gdb expects.

OK.

> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index cce8d00..1577ad3 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -2041,20 +2041,20 @@ trace_status_command (char *args, int from_tty)
>
>  	  /* Reporting a run time is more readable than two long numbers.  */
>  	  printf_filtered (_("Trace started at %ld.%06ld secs, stopped %ld.%06ld secs later.\n"),
> -			   (long int) ts->start_time / 1000000,
> -			   (long int) ts->start_time % 1000000,
> -			   (long int) run_time / 1000000,
> -			   (long int) run_time % 1000000);
> +			   (long int) (ts->start_time / 1000000),
> +			   (long int) (ts->start_time % 1000000),
> +			   (long int) (run_time / 1000000),
> +			   (long int) (run_time % 1000000));
>  	}
>        else
>  	printf_filtered (_("Trace started at %ld.%06ld secs.\n"),
> -			 (long int) ts->start_time / 1000000,
> -			 (long int) ts->start_time % 1000000);
> +			 (long int) (ts->start_time / 1000000),
> +			 (long int) (ts->start_time % 1000000));
>      }
>    else if (ts->stop_time)
>      printf_filtered (_("Trace stopped at %ld.%06ld secs.\n"),
> -		     (long int) ts->stop_time / 1000000,
> -		     (long int) ts->stop_time % 1000000);
> +		     (long int) (ts->stop_time / 1000000),
> +		     (long int) (ts->stop_time % 1000000));
>
>    /* Now report any per-tracepoint status available.  */
>    tp_vec = all_tracepoints ();
> @@ -2169,12 +2169,12 @@ trace_status_mi (int on_stop)
>      char buf[100];
>
>      xsnprintf (buf, sizeof buf, "%ld.%06ld",
> -	       (long int) ts->start_time / 1000000,
> -	       (long int) ts->start_time % 1000000);
> +	       (long int) (ts->start_time / 1000000),
> +	       (long int) (ts->start_time % 1000000));
>      ui_out_field_string (uiout, "start-time", buf);
>      xsnprintf (buf, sizeof buf, "%ld.%06ld",
> -	       (long int) ts->stop_time / 1000000,
> -	       (long int) ts->stop_time % 1000000);
> +	       (long int) (ts->stop_time / 1000000),
> +	       (long int) (ts->stop_time % 1000000));
>      ui_out_field_string (uiout, "stop-time", buf);
>    }
>  }
>
>
> gdb_stop_notes_v4.diff
>
>
> commit 570ec36c0dfd2d19cde2f779ee3f146fc526e44e
> Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
> Date:   Fri Oct 12 11:37:31 2012 +0400
>
>     Modify trace-status output to always output stop-notes.
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 3e2a818..dd9edde 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,12 @@
>  2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
>
> +	* tracepoint.c (trace_status_mi):  Modify trace status MI command to
> +	always output stop-notes.

Single space after ':'.

> +	(trace_save): Save stop-notes if any by tsave.

s/ by tsave//

> +	(parse_trace_status): Add parsing stop-notes.

"parsing of"

> +
> +2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
> +
>  	* tracepoint.c (trace_save): Add saving starttime, stoptime, user and notes.

"saving of".

>
>  2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
> diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
> index bb4ace8..58a8827 100644
> --- a/gdb/gdbserver/ChangeLog
> +++ b/gdb/gdbserver/ChangeLog
> @@ -1,5 +1,10 @@
>  2012-10-12 Dmitry Kozlov <ddk@codesourcery.com>
>
> +	* gdbserver/tracepoint.c (cmd_qtstatus): Modify trace-status
> +	to always output stop-notes.
> +
> +2012-10-12 Dmitry Kozlov <ddk@codesourcery.com>
> +
>  	* gdbserver/tracepoint.c (cmd_qtstatus): Modify trace-status output to
>  	output start time and stop time in hex as gdb expects.
>
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 5b56f81..72fca51 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -3657,7 +3657,7 @@ cmd_qtstatus (char *packet)
>  	   "circular:%d;"
>  	   "disconn:%d;"
>  	   "starttime:%s;stoptime:%s;"
> -	   "username:%s:;notes:%s:",
> +	   "username:%s;notes:%s;stop-notes:%s",

This removes what looks like was a spurious ':', but it isn't mentioned
in the ChangeLog.  It's arguably a separate logical change.  If you split
that out into a separate patch, it's pre-approved (go ahead and commit it,
with ChangeLog entry).

>  	   tracing ? 1 : 0,
>  	   stop_reason_rsp, tracing_stop_tpnum,
>  	   traceframe_count, traceframes_created,
> @@ -3666,7 +3666,7 @@ cmd_qtstatus (char *packet)
>  	   disconnected_tracing,
>  	   phex_nz (tracing_start_time, sizeof (tracing_start_time)),
>  	   phex_nz (tracing_stop_time, sizeof (tracing_stop_time)),
> -	   buf1, buf2);
> +	   buf1, buf2, buf3);
>  }

We're already sending the stop-notes in the "tstop" case:

  /* If this was a forced stop, include any stop note that was supplied.  */
  if (strcmp (stop_reason_rsp, "tstop") == 0)
    {
      stop_reason_rsp = alloca (strlen ("tstop:") + strlen (buf3) + 1);
      strcpy (stop_reason_rsp, "tstop:");
      strcat (stop_reason_rsp, buf3);
    }

Should we really be putting the stop notes twice in the packet?  If not,
to avoid breaking compatibility, we'd skip sending "stop-notes:" in the
tstop case.

>
>  static void
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 8f4f18c..0905e89 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -2164,6 +2164,8 @@ trace_status_mi (int on_stop)
>
>    ui_out_field_string (uiout, "user-name", ts->user_name);
>    ui_out_field_string (uiout, "notes", ts->notes);
> +  // Always output stop-notes to MI to unify IDE processing of start and stop notes
> +  ui_out_field_string (uiout, "stop-notes", ts->stop_desc);

Use /* */ for comments.  Finish sentence with period and double space.


>
>    {
>      char buf[100];
> @@ -3063,6 +3065,14 @@ trace_save (const char *filename, int target_does_save)
>        fprintf (fp, ";username:%s", buf);
>        xfree (buf);
>      }
> +  if (ts->stop_desc && ts->stop_desc[0] != 0)
> +    {
> +      char *buf = xmalloc (strlen (ts->stop_desc) * 2 + 1);
> +
> +      bin2hex ((gdb_byte *) ts->stop_desc, buf, 0);
> +      fprintf (fp, ";stop-notes:%s", buf);
> +      xfree (buf);
> +    }
>
>    fprintf (fp, "\n");
>
> @@ -3992,6 +4002,14 @@ Status line: '%s'\n"), p, line);
>  	  ts->notes[end] = '\0';
>  	  p = p3;
>  	}
> +      else if (strncmp (p, "stop-notes", p1 - p) == 0)
> +	{
> +	  ++p1;
> +	  ts->stop_desc = xmalloc (strlen (p) / 2);
> +	  end = hex2bin (p1, ts->stop_desc, (p3 - p1) / 2);
> +	  ts->stop_desc[end] = '\0';

This leaks the ts->stop_desc string that was set above, while handling
the tstop:

      else if (strncmp (p, stop_reason_names[tstop_command], p1 - p) == 0)
	{
...
	  else if (p2 != p1)
	    {
	      ts->stop_desc = xmalloc (strlen (line));
	      end = hex2bin (p1, ts->stop_desc, (p2 - p1) / 2);
	      ts->stop_desc[end] = '\0';
	    }
	  else
	    ts->stop_desc = xstrdup ("");

So write:

	  xfree (ts->stop_desc);
	  ts->stop_desc[end] = '\0';

> commit 4ce80946bb684913900b5e4db3c627b2acd91399
> Author: Dmitry Kozlov <dmitry_kozlov@mentor.com>
> Date:   Fri Oct 12 11:32:25 2012 +0400
>
>     Extend tsave to save trace start time, stop time and notes.
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 27ad6ab..3e2a818 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,9 @@
>  2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
>
> +	* tracepoint.c (trace_save): Add saving starttime, stoptime, user and notes.
> +
> +2012-10-12  Dmitry Kozlov  <ddk@codesourcery.com>
> +
>  	* tracepoint.c (trace_status_command): Fix type of printf arg to
>  	prevent improper type conversion.
>  	(trace_status_mi): Likewise.
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 1577ad3..8f4f18c 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 = 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,33 @@ 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:%s",
> +    		  phex_nz (ts->start_time, sizeof (ts->start_time)));
> +    }
> +  if (ts->stop_time)

Can we assume that ts->start_time and ts->stop_time never wrap?  If not,
we'd better have separate ts->start_time_p ts->stop_time_p boolean flags,
to indicate we have a start and stop time at all, rather than checking for 0.

> +    {
> +      fprintf (fp, ";stoptime:%s",
> +    		phex_nz (ts->stop_time, sizeof (ts->stop_time)));
> +    }
> +  if (ts->notes && ts->notes[0] != 0)
> +    {
> +      char *buf = xmalloc (strlen (ts->notes) * 2 + 1);
> +
> +      bin2hex ((gdb_byte *) ts->notes, buf, 0);
> +      fprintf (fp, ";notes:%s", buf);
> +      xfree (buf);
> +    }
> +  if (ts->user_name && ts->user_name[0] != 0)
> +    {
> +      char *buf = xmalloc (strlen (ts->user_name) * 2 + 1);
> +
> +      bin2hex ((gdb_byte *) ts->user_name, buf, 0);
> +      fprintf (fp, ";username:%s", buf);
> +      xfree (buf);
> +    }
> +
>    fprintf (fp, "\n");
>
>    /* Note that we want to upload tracepoints and save those, rather

-- 
Pedro Alves