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

[Hui Zhu <teawater@gmail.com>]

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