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

[Hui Zhu <teawater@gmail.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};