* Re: [PATCH] Extend tsave: save start time, stop time, user and notes
2012-10-11 17:10 ` Pedro Alves
@ 2012-10-12 8:44 ` Dmitry Kozlov
2012-10-12 8:46 ` Dmitry Kozlov
1 sibling, 0 replies; 8+ messages in thread
From: Dmitry Kozlov @ 2012-10-12 8:44 UTC (permalink / raw)
To: Pedro Alves
Cc: Yao Qi, gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus
[-- 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Extend tsave: save start time, stop time, user and notes
2012-10-11 17:10 ` Pedro Alves
2012-10-12 8:44 ` Dmitry Kozlov
@ 2012-10-12 8:46 ` Dmitry Kozlov
2012-10-16 16:36 ` Pedro Alves
1 sibling, 1 reply; 8+ messages in thread
From: Dmitry Kozlov @ 2012-10-12 8:46 UTC (permalink / raw)
To: Pedro Alves
Cc: Yao Qi, gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus
[-- 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Extend tsave: save start time, stop time, user and notes
2012-10-12 8:46 ` Dmitry Kozlov
@ 2012-10-16 16:36 ` Pedro Alves
0 siblings, 0 replies; 8+ messages in thread
From: Pedro Alves @ 2012-10-16 16:36 UTC (permalink / raw)
To: Dmitry Kozlov
Cc: Yao Qi, gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus
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
^ permalink raw reply [flat|nested] 8+ messages in thread