Re: [RFA/RFC] tracepoint gdbrsp: add -1 introduce for QTFrame:@var{n}

[Hui Zhu <teawater at gmail dot com>]

On Thu, Aug 19, 2010 at 15:43, Hui Zhu <teawater@gmail.com> wrote:
> On Mon, Aug 16, 2010 at 07:09, Pedro Alves <pedro@codesourcery.com> wrote:
>> On Sunday 15 August 2010 20:05:16, Michael Snyder wrote:
>>> Pedro Alves wrote:
>>> > On Sunday 15 August 2010 08:16:34, Hui Zhu wrote:
>>> >> Sending packet: $QTFrame:ffffffff#fa...Packet received: OK
>>> >
>>> > I think it is a bug that this is assuming 32-bit, two's complement
>>> > on both client/server sides. ?(not sure it that was what you were
>>> > referring to).
>>>
>>> It's not really assuming that -- it's just assuming that no
>>> legitimate frame ID will ever be as high as ffffffff.
>>
>> I don't know where you're getting that from. ?If you believe that
>> to be true, then you also have to believe Hui's patch is wrong,
>> as it documents "-1", not the magic hard limit of "0xffffffff".
>>
>> Start here:
>>
>> /* tfind end */
>> static void
>> trace_find_end_command (char *args, int from_tty)
>> {
>> ?trace_find_command ("-1", from_tty);
>> ? ? ? ? ? ? ? ? ? ? ? ^^
>> }
>>
>> through here:
>>
>> /* Worker function for the various flavors of the tfind command. ?*/
>> void
>> tfind_1 (enum trace_find_type type, int num,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^^^^^
>> ? ? ? ? ULONGEST addr1, ULONGEST addr2,
>> ? ? ? ? int from_tty)
>> {
>> ?int target_frameno = -1, target_tracept = -1;
>> ?struct frame_id old_frame_id = null_frame_id;
>> ?struct breakpoint *tp;
>>
>> ?/* Only try to get the current stack frame if we have a chance of
>> ? ? succeeding. ?In particular, if we're trying to get a first trace
>> ? ? frame while all threads are running, it's not going to succeed,
>> ? ? so leave it with a default value and let the frame comparison
>> ? ? below (correctly) decide to print out the source location of the
>> ? ? trace frame. ?*/
>> ?if (!(type == tfind_number && num == -1)
>> ? ? ?&& (has_stack_frames () || traceframe_number >= 0))
>> ? ?old_frame_id = get_frame_id (get_current_frame ());
>>
>> ?target_frameno = target_trace_find (type, num, addr1, addr2,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&target_tracept);
>>
>> ?if (type == tfind_number
>> ? ? ?&& num == -1
>> ? ? ? ? ^^^^^^^^^
>> ? ? ?&& target_frameno == -1)
>> ? ?{
>> ? ? ?/* We told the target to get out of tfind mode, and it did. ?*/
>> ? ?}
>> ?else if (target_frameno == -1)
>> ? ?{
>> ? ? ?/* A request for a non-existent trace frame has failed.
>> ? ? ? ? Our response will be different, depending on FROM_TTY:
>>
>> ? ? ? ? If FROM_TTY is true, meaning that this command was
>> ? ? ? ? typed interactively by the user, then give an error
>> ? ? ? ? and DO NOT change the state of traceframe_number etc.
>>
>> ? ? ? ? However if FROM_TTY is false, meaning that we're either
>> ? ? ? ? in a script, a loop, or a user-defined command, then
>> ? ? ? ? DON'T give an error, but DO change the state of
>> ? ? ? ? traceframe_number etc. to invalid.
>>
>> ? ? ? ? The rationalle is that if you typed the command, you
>> ? ? ? ? might just have committed a typo or something, and you'd
>> ? ? ? ? like to NOT lose your current debugging state. ?However
>> ? ? ? ? if you're in a user-defined command or especially in a
>> ? ? ? ? loop, then you need a way to detect that the command
>> ? ? ? ? failed WITHOUT aborting. ?This allows you to write
>> ? ? ? ? scripts that search thru the trace buffer until the end,
>> ? ? ? ? and then continue on to do something else. ?*/
>>
>> ? ? ?if (from_tty)
>> ? ? ? ?error (_("Target failed to find requested trace frame."));
>>
>>
>> Notice how you get to the "else if" branch. ?Choosing any other
>> "high enough" random frame id that doesn't exist errors
>> out. ?Only the special case of 0xffffffff being parsed as -1 on
>> both target and host actually gets out of tfind mode.
>>
>> Continuing, the target_trace_find call ends up here:
>>
>> static int
>> remote_trace_find (enum trace_find_type type, int num,
>> ? ? ? ? ? ? ? ? ? ULONGEST addr1, ULONGEST addr2,
>> ? ? ? ? ? ? ? ? ? int *tpp)
>> {
>> ?struct remote_state *rs = get_remote_state ();
>> ?char *p, *reply;
>> ?int target_frameno = -1, target_tracept = -1;
>>
>> ?p = rs->buf;
>> ?strcpy (p, "QTFrame:");
>> ?p = strchr (p, '\0');
>> ?switch (type)
>> ? ?{
>> ? ?case tfind_number:
>> ? ? ?sprintf (p, "%x", num);
>> ? ? ? ? ? ? ? ? ?^^^^^^^^^
>> ? ? ?break;
>>
>> ... and here, NUM, is an int, and, is printed with %x. ?Assuming
>> that prints 0xffffffff is actually host dependent (e.g., consider
>> ILP64). ?Granted, likely not a concern on all hosts we care
>> about. ?Still a needless assumption.
>>
>> The gdbserver side does:
>>
>> ...
>> ? ? ?unpack_varlen_hex (packet, &frame);
>> ? ? ?tfnum = (int) frame;
>> ? ? ?if (tfnum == -1)
>> ? ? ? ?{
>> ? ? ? ? ?trace_debug ("Want to stop looking at traceframes");
>> ? ? ? ? ?current_traceframe = -1;
>> ? ? ? ? ?write_ok (own_buf);
>> ? ? ? ? ?return;
>> ? ? ? ?}
>>
>> Again would break if the target is ILP64, and host is not.
>>
>> And below that bit of code you'll see that trying to select any
>> other high enough frame number that does not exist
>> does not get out of tfind mode, but instead returns an error.
>>
>>> That might also be iffy, but less so....
>>
>> I should have mentioned that I consider this a _design_ bug we
>> get to live with. ?Fixing it like I suggested would be incompatible
>> with current targets, so, best leave this alone until it actually
>> becomes a problem.
>>
>> Though, this raises the point that Hui's docs don't
>> actually match what GDB sends. ?Not sure what to do. ?I guess
>> I'll just pretend I didn't spot this. ?;-)
>>
>>> ? IMO, negatives should have an explicit '-' encoding; in
>>> > this case, "$QTFrame:-1". ?Note sure if the RSP docs mention something
>>> > about this. ?We are careful in some cases (passing thread id's,
>>> > I think, is one case), though clearly not everywhere.
>>
>> --
>> Pedro Alves
>>
>
> Hi,
>
> I am not sure we need keep this bug in the GDB. ?Make gdb and
> gdbserver support the old version is not a very hard thing.
>
> I make 3 patch for gdb, gdbserver and doc.
> For the gdbserver, it can support the old gdb that use 0xffffffff and
> new gdb that use -1.
> For the gdb, if you think we ?need, I can add a switch or something to
> make it send 0xffffffff.
>
> What do you think about it?
>
> Thanks,
> Hui
>
> 2010-08-19 ?Hui Zhu ?<teawater@gmail.com>
>
> ? ? ? ?* remote.c(remote_trace_find): Handle the negative.
>
> 2010-08-19 ?Hui Zhu ?<teawater@gmail.com>
>
> ? ? ? ?* tracepoint.c(cmd_qtframe): Handle the negative.
>
> 2010-08-19 ?Hui Zhu ?<teawater@gmail.com>
>
> ? ? ? ?* gdb.texinfo (Tracepoint Packets): add introduce for -1.
>
> ---
> ?remote.c | ? ?5 ++++-
> ?1 file changed, 4 insertions(+), 1 deletion(-)
>
> --- a/remote.c
> +++ b/remote.c
> @@ -9979,7 +9979,10 @@ remote_trace_find (enum trace_find_type
> ? switch (type)
> ? ? {
> ? ? case tfind_number:
> - ? ? ?sprintf (p, "%x", num);
> + ? ? ?if (num < 0)
> + ? ? ? ?sprintf (p, "-%x", -num);
> + ? ? ?else
> + ? ? ? ?sprintf (p, "%x", num);
> ? ? ? break;
> ? ? case tfind_pc:
> ? ? ? sprintf (p, "pc:%s", phex_nz (addr1, 0));
>
>
> ---
> ?gdbserver/tracepoint.c | ? ?9 +++++++++
> ?1 file changed, 9 insertions(+)
>
> --- a/gdbserver/tracepoint.c
> +++ b/gdbserver/tracepoint.c
> @@ -3157,8 +3157,17 @@ cmd_qtframe (char *own_buf)
> ? ? }
> ? else
> ? ? {
> + ? ? ?int is_negative = 0;
> +
> + ? ? ?if (*packet == '-')
> + ? ? ? ?{
> + ? ? ? ? ?is_negative = 1;
> + ? ? ? ? ?++packet;
> + ? ? ? ?}
> ? ? ? unpack_varlen_hex (packet, &frame);
> ? ? ? tfnum = (int) frame;
> + ? ? ?if (is_negative)
> + ? ? ? ?tfnum = -tfnum;
> ? ? ? if (tfnum == -1)
> ? ? ? ?{
> ? ? ? ? ?trace_debug ("Want to stop looking at traceframes");
>
>
> ---
> ?doc/gdb.texinfo | ? 12 ++++++++++++
> ?1 file changed, 12 insertions(+)
>
> --- a/doc/gdb.texinfo
> +++ b/doc/gdb.texinfo
> @@ -33133,6 +33133,18 @@ The selected trace frame records a hit o
>
> ?@end table
>
> +If @var{n} is -1, it mean that stop debugging trace snapshots,
> +resume live debugging.
> +
> +Replies:
> +@table @samp
> +@item OK
> +The packet was understood and carried out.
> +@item E @var{NN}
> +A badly formed request was detected, or an error was encountered while
> +relocating the instruction.
> +@end table
> +
> ?@item QTFrame:pc:@var{addr}
> ?Like @samp{QTFrame:@var{n}}, but select the first tracepoint frame after the
> ?currently selected frame whose PC is @var{addr};
>

Sorry the doc patch is not the right version. following is the current version.

Thanks,
Hui

2010-08-19  Hui Zhu  <teawater@gmail.com>

	* gdb.texinfo (Tracepoint Packets): add introduce for -1.


---
 doc/gdb.texinfo |    9 +++++++++
 1 file changed, 9 insertions(+)

--- a/doc/gdb.texinfo
+++ b/doc/gdb.texinfo
@@ -33155,6 +33155,15 @@ The selected trace frame records a hit o

 @end table

+If @var{n} is -1, it mean that stop debugging trace snapshots,
+resume live debugging.
+
+Replies:
+@table @samp
+@item OK
+The packet was understood and carried out.
+@end table
+
 @item QTFrame:pc:@var{addr}
 Like @samp{QTFrame:@var{n}}, but select the first tracepoint frame after the
 currently selected frame whose PC is @var{addr};