* PATCH fix start-time and stop-time in trace-status
@ 2012-09-27 12:22 Dmitry Kozlov
2012-09-27 15:14 ` Tom Tromey
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Kozlov @ 2012-09-27 12:22 UTC (permalink / raw)
To: gdb-patches; +Cc: 'Stan_Shebs@mentor.com', Vladimir Prus
[-- Attachment #1: Type: text/plain, Size: 499 bytes --]
Hello,
I noticed that trace-status MI command reports incorrect start-time and
stop-time, sometimes even negative. Investigation showed that gdbserver
reports start-time and stop-time in decimal format, while gdb tries to
interpret it as hex adn then broke it during type conversion. I think
keeping decimal format for communication is useful because itis
human-readable in remote log, so the attached patch fixes this problem
on the gdb side.
Please consider applying it.
Thank you,
Dmitry
[-- Attachment #2: gdb_dates.diff --]
[-- Type: text/x-patch, Size: 4049 bytes --]
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0a1ccdf..b43928d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2012-09-25 Dmitry Kozlov <ddk@mentor.com>
+
+ * tracepoint.c (trace_status_command): Fix type of printf arg.
+ (trace_status_mi): Likewise.
+ * remote.c (unpack_ulongest): New.
+
2012-09-24 Siddhesh Poyarekar <siddhesh@redhat.com>
* m2-typeprint.c (m2_enum): Expand LASTVAL to LONGEST.
diff --git a/gdb/remote.c b/gdb/remote.c
index 1750bee..cc871e5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1892,6 +1892,8 @@ struct gdb_ext_thread_info
char *unpack_varlen_hex (char *buff, ULONGEST *result);
+char *unpack_ulongest (char *buff, ULONGEST *result);
+
static char *unpack_nibble (char *buf, int *val);
static char *pack_nibble (char *buf, int nibble);
@@ -2079,6 +2081,21 @@ stub_unpack_int (char *buff, int fieldlength)
}
char *
+unpack_ulongest (char *buff, ULONGEST *result)
+{
+ ULONGEST retval = 0;
+
+ while (isdigit (*buff))
+ {
+ retval *= 10;
+ retval += *buff - '0';
+ buff++;
+ }
+ *result = retval;
+ return buff;
+}
+
+char *
unpack_varlen_hex (char *buff, /* packet to parse */
ULONGEST *result)
{
diff --git a/gdb/remote.h b/gdb/remote.h
index 3adc54e..7dbe263 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -39,6 +39,8 @@ extern int putpkt (char *buf);
extern char *unpack_varlen_hex (char *buff, ULONGEST *result);
+extern char *unpack_ulongest (char *buff, ULONGEST *result);
+
extern void async_remote_interrupt_twice (void *arg);
void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index bdd6f50..6ff3a2f 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2039,20 +2039,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 ();
@@ -2167,12 +2167,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);
}
}
@@ -3938,12 +3938,12 @@ Status line: '%s'\n"), p, line);
}
else if (strncmp (p, "starttime", p1 - p) == 0)
{
- p = unpack_varlen_hex (++p1, &val);
+ p = unpack_ulongest (++p1, &val);
ts->start_time = val;
}
else if (strncmp (p, "stoptime", p1 - p) == 0)
{
- p = unpack_varlen_hex (++p1, &val);
+ p = unpack_ulongest (++p1, &val);
ts->stop_time = val;
}
else if (strncmp (p, "username", p1 - p) == 0)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH fix start-time and stop-time in trace-status
2012-09-27 12:22 PATCH fix start-time and stop-time in trace-status Dmitry Kozlov
@ 2012-09-27 15:14 ` Tom Tromey
2012-10-01 13:41 ` Dmitry Kozlov
2012-10-08 13:54 ` Dmitry Kozlov
0 siblings, 2 replies; 9+ messages in thread
From: Tom Tromey @ 2012-09-27 15:14 UTC (permalink / raw)
To: Dmitry Kozlov; +Cc: gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus
>>>>> "Dmitry" == Dmitry Kozlov <dmitry_kozlov@mentor.com> writes:
Dmitry> +char *unpack_ulongest (char *buff, ULONGEST *result);
I think you could use the existing strtoulst rather than introducing a
new function.
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH fix start-time and stop-time in trace-status
2012-09-27 15:14 ` Tom Tromey
@ 2012-10-01 13:41 ` Dmitry Kozlov
2012-10-15 19:11 ` Tom Tromey
2012-10-08 13:54 ` Dmitry Kozlov
1 sibling, 1 reply; 9+ messages in thread
From: Dmitry Kozlov @ 2012-10-01 13:41 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus
[-- Attachment #1: Type: text/plain, Size: 326 bytes --]
Tom,
I have updated the patch.
Thank you,
Dmitry
On 09/27/2012 07:14 PM, Tom Tromey wrote:
>>>>>> "Dmitry" == Dmitry Kozlov <dmitry_kozlov@mentor.com> writes:
> Dmitry> +char *unpack_ulongest (char *buff, ULONGEST *result);
>
> I think you could use the existing strtoulst rather than introducing a
> new function.
>
> Tom
[-- Attachment #2: gdb_dates_v2.diff --]
[-- Type: text/x-patch, Size: 2843 bytes --]
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 20631b5..eeaf805 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2012-10-01 Dmitry Kozlov <ddk@mentor.com>
+
+ * tracepoint.c (trace_status_command): Fix type of printf arg.
+ (trace_status_mi): Likewise.
+
2012-10-01 Andrew Burgess <aburgess@broadcom.com>
* target.c (simple_search_memory): Include access length in
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index cce8d00..0f94150 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);
}
}
@@ -3940,12 +3940,12 @@ Status line: '%s'\n"), p, line);
}
else if (strncmp (p, "starttime", p1 - p) == 0)
{
- p = unpack_varlen_hex (++p1, &val);
+ val = strtoulst(++p1, (const char **) &p, 10);
ts->start_time = val;
}
else if (strncmp (p, "stoptime", p1 - p) == 0)
{
- p = unpack_varlen_hex (++p1, &val);
+ val = strtoulst(++p1, (const char **) &p, 10);
ts->stop_time = val;
}
else if (strncmp (p, "username", p1 - p) == 0)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH fix start-time and stop-time in trace-status
2012-09-27 15:14 ` Tom Tromey
2012-10-01 13:41 ` Dmitry Kozlov
@ 2012-10-08 13:54 ` Dmitry Kozlov
1 sibling, 0 replies; 9+ messages in thread
From: Dmitry Kozlov @ 2012-10-08 13:54 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus
Hi Tom,
I have posted revised patch
(http://sourceware.org/ml/gdb-patches/2012-10/msg00005.html), could you
approve it if you don't have any other objections.
Thank you,
Dmitry
On 09/27/2012 07:14 PM, Tom Tromey wrote:
>>>>>> "Dmitry" == Dmitry Kozlov <dmitry_kozlov@mentor.com> writes:
> Dmitry> +char *unpack_ulongest (char *buff, ULONGEST *result);
>
> I think you could use the existing strtoulst rather than introducing a
> new function.
>
> Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH fix start-time and stop-time in trace-status
2012-10-01 13:41 ` Dmitry Kozlov
@ 2012-10-15 19:11 ` Tom Tromey
2012-10-16 14:59 ` Dmitry Kozlov
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2012-10-15 19:11 UTC (permalink / raw)
To: Dmitry Kozlov; +Cc: gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus
>>>>> "Dmitry" == Dmitry Kozlov <dmitry_kozlov@mentor.com> writes:
Dmitry> +2012-10-01 Dmitry Kozlov <ddk@mentor.com>
Dmitry> +
Dmitry> + * tracepoint.c (trace_status_command): Fix type of printf arg.
Dmitry> + (trace_status_mi): Likewise.
Looks pretty good.
Dmitry> + val = strtoulst(++p1, (const char **) &p, 10);
Space before open paren.
Dmitry> + val = strtoulst(++p1, (const char **) &p, 10);
Likewise.
This is ok with these fixed.
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH fix start-time and stop-time in trace-status
2012-10-15 19:11 ` Tom Tromey
@ 2012-10-16 14:59 ` Dmitry Kozlov
2012-10-16 15:15 ` Pedro Alves
2012-10-16 16:51 ` Tom Tromey
0 siblings, 2 replies; 9+ messages in thread
From: Dmitry Kozlov @ 2012-10-16 14:59 UTC (permalink / raw)
To: Tom Tromey
Cc: gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus, Pedro Alves
Hi Tom,
Pedro already asked to rewrite this patch at gdbserver-side and use hex
instead of decimal.
See http://sourceware.org/ml/gdb-patches/2012-10/msg00193.html
Could you setup one common opinion about what way is right?
It would very kind of you if you can look at other 2 related patches.
Thank you,
Dmitry
On 10/15/2012 11:11 PM, Tom Tromey wrote:
>>>>>> "Dmitry" == Dmitry Kozlov <dmitry_kozlov@mentor.com> writes:
> Dmitry> +2012-10-01 Dmitry Kozlov <ddk@mentor.com>
> Dmitry> +
> Dmitry> + * tracepoint.c (trace_status_command): Fix type of printf arg.
> Dmitry> + (trace_status_mi): Likewise.
>
> Looks pretty good.
>
> Dmitry> + val = strtoulst(++p1, (const char **) &p, 10);
>
> Space before open paren.
>
> Dmitry> + val = strtoulst(++p1, (const char **) &p, 10);
>
> Likewise.
>
> This is ok with these fixed.
>
> Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH fix start-time and stop-time in trace-status
2012-10-16 14:59 ` Dmitry Kozlov
@ 2012-10-16 15:15 ` Pedro Alves
2012-10-16 17:07 ` Stan Shebs
2012-10-16 16:51 ` Tom Tromey
1 sibling, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2012-10-16 15:15 UTC (permalink / raw)
To: Dmitry Kozlov
Cc: Tom Tromey, gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus
On 10/16/2012 03:59 PM, Dmitry Kozlov wrote:
> Hi Tom,
> Pedro already asked to rewrite this patch at gdbserver-side and use hex instead of decimal.
> See http://sourceware.org/ml/gdb-patches/2012-10/msg00193.html
> Could you setup one common opinion about what way is right?
Since the RSP uses hex everywhere, the argument for keeping decimal format because
it's human-readable in the remote log doesn't really hold. Instead, having different
formats makes reading the logs _worse_ (forcing one to wonder "is this in hex, or
in decimal"). Given that there's no backwards compatibility issue here, I much
prefer fixing gdbserver.
> It would very kind of you if you can look at other 2 related patches.
I'm looking at them now.
--
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH fix start-time and stop-time in trace-status
2012-10-16 14:59 ` Dmitry Kozlov
2012-10-16 15:15 ` Pedro Alves
@ 2012-10-16 16:51 ` Tom Tromey
1 sibling, 0 replies; 9+ messages in thread
From: Tom Tromey @ 2012-10-16 16:51 UTC (permalink / raw)
To: Dmitry Kozlov
Cc: gdb-patches, 'Stan_Shebs@mentor.com', Vladimir Prus, Pedro Alves
>>>>> "Dmitry" == Dmitry Kozlov <dmitry_kozlov@mentor.com> writes:
Dmitry> Pedro already asked to rewrite this patch at gdbserver-side and use
Dmitry> hex instead of decimal.
Dmitry> See http://sourceware.org/ml/gdb-patches/2012-10/msg00193.html
Dmitry> Could you setup one common opinion about what way is right?
Sorry about that.
I must not have realized the threads were linked.
Please do what Pedro asks.
Dmitry> It would very kind of you if you can look at other 2 related patches.
IIRC one was a gdbserver patch -- I don't know much about gdbserver and
generally don't review those.
I don't remember why I skipped the other patch but if you can point me
at it I will look.
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH fix start-time and stop-time in trace-status
2012-10-16 15:15 ` Pedro Alves
@ 2012-10-16 17:07 ` Stan Shebs
0 siblings, 0 replies; 9+ messages in thread
From: Stan Shebs @ 2012-10-16 17:07 UTC (permalink / raw)
To: gdb-patches
On 10/16/12 8:15 AM, Pedro Alves wrote:
> On 10/16/2012 03:59 PM, Dmitry Kozlov wrote:
>> Hi Tom,
>> Pedro already asked to rewrite this patch at gdbserver-side and use hex instead of decimal.
>> See http://sourceware.org/ml/gdb-patches/2012-10/msg00193.html
>> Could you setup one common opinion about what way is right?
> Since the RSP uses hex everywhere, the argument for keeping decimal format because
> it's human-readable in the remote log doesn't really hold. Instead, having different
> formats makes reading the logs _worse_ (forcing one to wonder "is this in hex, or
> in decimal"). Given that there's no backwards compatibility issue here, I much
> prefer fixing gdbserver.
>
Yes, it looks like it was simply a brain cramp on my part, I was
probably thinking plongest put out hex digits and didn't notice because
the times printed on the host looked plausible.
(Ironically, a decimal-point encoding would probably be the most
futureproof, right now the protocol assumes timestamps are in
microseconds and a target reporting in nanoseconds would either lose
data or cause all kinds of confusion.)
Stan
stan@codesourcery.com
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-10-16 17:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-27 12:22 PATCH fix start-time and stop-time in trace-status Dmitry Kozlov
2012-09-27 15:14 ` Tom Tromey
2012-10-01 13:41 ` Dmitry Kozlov
2012-10-15 19:11 ` Tom Tromey
2012-10-16 14:59 ` Dmitry Kozlov
2012-10-16 15:15 ` Pedro Alves
2012-10-16 17:07 ` Stan Shebs
2012-10-16 16:51 ` Tom Tromey
2012-10-08 13:54 ` Dmitry Kozlov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox