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

[Pedro Alves <pedro at codesourcery dot com>]

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