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

[Pedro Alves <palves at redhat dot com>]

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