[PATCH/commit] Handle errors in tracepoint target agent

[Stan Shebs <stan at codesourcery dot com>]

For the record, here's what I ended up committing.  The hex2bin sniffing 
idea is not a winner, because it calls error() if it sees a non-hex 
character, so I just added a prefix character 'X' that signals that the 
error message is hex-encoded instead of plain text; targets can do 
either as they prefer.  The error message is never NULL, I added the MI 
status bit overlooked before, and improved the trace file generator's 
portability, although it could probably use more.  (A bit of a tricky 
program, but a useful investment, as independent implementation of file 
format.)

Stan

2010-03-25  Stan Shebs  <stan@codesourcery.com>

   * tracepoint.h (trace_stop_reason): Add tracepoint_error.
   (struct trace_status): New field error_desc.
   * tracepoint.c (stop_reason_names): Add terror.
   (current_trace_status): Ensure non-NULL error description.
   (trace_status_command): Add error report.
   (trace_status_mi): Ditto.
   (trace_save): Add special case for error description.
   (parse_trace_status): Add case for errors.

   * gdb.texinfo (Tracepoint Packets): Document trace error status.

   * gdb.trace/tfile.c: Generate an additional trace file, improve
   portability.
   * gdb.trace/tfile.exp: Test trace file with an error stop, delete
   files in a better way.

Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.155
diff -p -r1.155 tracepoint.c
*** tracepoint.c	24 Mar 2010 21:12:18 -0000	1.155
--- tracepoint.c	26 Mar 2010 01:43:59 -0000
*************** char *stop_reason_names[] = {
*** 195,206 ****
    "tstop",
    "tfull",
    "tdisconnected",
!   "tpasscount"
  };
  
  struct trace_status *
  current_trace_status ()
  {
    return &trace_status;
  }
  
--- 195,210 ----
    "tstop",
    "tfull",
    "tdisconnected",
!   "tpasscount",
!   "terror"
  };
  
  struct trace_status *
  current_trace_status ()
  {
+   /* Ensure this is never NULL.  */
+   if (!trace_status.error_desc)
+     trace_status.error_desc = "";
    return &trace_status;
  }
  
*************** trace_status_command (char *args, int fr
*** 1570,1575 ****
--- 1574,1587 ----
  	  printf_filtered (_("Trace stopped by tracepoint %d.\n"),
  			   ts->stopping_tracepoint);
  	  break;
+ 	case tracepoint_error:
+ 	  if (ts->stopping_tracepoint)
+ 	    printf_filtered (_("Trace stopped by an error (%s, tracepoint %d).\n"),
+ 			     ts->error_desc, ts->stopping_tracepoint);
+ 	  else
+ 	    printf_filtered (_("Trace stopped by an error (%s).\n"),
+ 			     ts->error_desc);
+ 	  break;
  	case trace_stop_reason_unknown:
  	  printf_filtered (_("Trace stopped for an unknown reason.\n"));
  	  break;
*************** trace_status_mi (int on_stop)
*** 1684,1689 ****
--- 1696,1705 ----
  	      stop_reason = "passcount";
  	      stopping_tracepoint = ts->stopping_tracepoint;
  	      break;
+ 	    case tracepoint_error:
+ 	      stop_reason = "error";
+ 	      stopping_tracepoint = ts->stopping_tracepoint;
+ 	      break;
  	    }
  	  
  	  if (stop_reason)
*************** trace_status_mi (int on_stop)
*** 1692,1697 ****
--- 1708,1716 ----
  	      if (stopping_tracepoint != -1)
  		ui_out_field_int (uiout, "stopping-tracepoint",
  				  stopping_tracepoint);
+ 	      if (ts->stop_reason == tracepoint_error)
+ 		ui_out_field_string (uiout, "error-description",
+ 				     ts->error_desc);
  	    }
  	}
      }
*************** trace_save (const char *filename, int ta
*** 2463,2471 ****
    fprintf (fp, "R %x\n", trace_regblock_size);
  
    /* Write out status of the tracing run (aka "tstatus" info).  */
!   fprintf (fp, "status %c;%s:%x",
! 	   (ts->running ? '1' : '0'),
!  	   stop_reason_names[ts->stop_reason], ts->stopping_tracepoint);
    if (ts->traceframe_count >= 0)
      fprintf (fp, ";tframes:%x", ts->traceframe_count);
    if (ts->traceframes_created >= 0)
--- 2482,2497 ----
    fprintf (fp, "R %x\n", trace_regblock_size);
  
    /* Write out status of the tracing run (aka "tstatus" info).  */
!   fprintf (fp, "status %c;%s",
! 	   (ts->running ? '1' : '0'), stop_reason_names[ts->stop_reason]);
!   /* Encode the error message in hex, might have weird chars.  */
!   if (ts->stop_reason == tracepoint_error)
!     {
!       char *buf = (char *) alloca (strlen (ts->error_desc) * 2 + 1);
!       bin2hex ((gdb_byte *) ts->error_desc, buf, 0);
!       fprintf (fp, ":X%s", buf);
!     }
!   fprintf (fp, ":%x", ts->stopping_tracepoint);
    if (ts->traceframe_count >= 0)
      fprintf (fp, ";tframes:%x", ts->traceframe_count);
    if (ts->traceframes_created >= 0)
*************** extern char *unpack_varlen_hex (char *bu
*** 3126,3137 ****
  void
  parse_trace_status (char *line, struct trace_status *ts)
  {
!   char *p = line, *p1, *p_temp;
    ULONGEST val;
  
    ts->running_known = 1;
    ts->running = (*p++ == '1');
    ts->stop_reason = trace_stop_reason_unknown;
    ts->traceframe_count = -1;
    ts->traceframes_created = -1;
    ts->buffer_free = -1;
--- 3152,3164 ----
  void
  parse_trace_status (char *line, struct trace_status *ts)
  {
!   char *p = line, *p1, *p2, *p_temp;
    ULONGEST val;
  
    ts->running_known = 1;
    ts->running = (*p++ == '1');
    ts->stop_reason = trace_stop_reason_unknown;
+   ts->error_desc = "";
    ts->traceframe_count = -1;
    ts->traceframes_created = -1;
    ts->buffer_free = -1;
*************** Status line: '%s'\n"), p, line);
*** 3164,3169 ****
--- 3191,3220 ----
  	  p = unpack_varlen_hex (++p1, &val);
  	  ts->stop_reason = tstop_command;
  	}
+       else if (strncmp (p, stop_reason_names[tracepoint_error], p1 - p) == 0)
+ 	{
+ 	  p2 = strchr (++p1, ':');
+ 	  if (p2 != p1)
+ 	    {
+ 	      int end;
+ 	      ts->error_desc = (char *) xmalloc (p2 - p1 + 1);
+ 	      /* See if we're doing plain text or hex encoding.  */
+ 	      if (*p1 == 'X')
+ 		{
+ 		  ++p1;
+ 		  end = hex2bin (p1, ts->error_desc, (p2 - p1) / 2);
+ 		}
+ 	      else
+ 		{
+ 		  memcpy (ts->error_desc, p1, p2 - p1);
+ 		  end = p2 - p1;
+ 		}
+ 	      ts->error_desc[end] = '\0';
+ 	    }
+ 	  p = unpack_varlen_hex (++p2, &val);
+ 	  ts->stopping_tracepoint = val;
+ 	  ts->stop_reason = tracepoint_error;
+ 	}
        else if (strncmp (p, "tframes", p1 - p) == 0)
  	{
  	  p = unpack_varlen_hex (++p1, &val);
Index: tracepoint.h
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.h,v
retrieving revision 1.29
diff -p -r1.29 tracepoint.h
*** tracepoint.h	23 Mar 2010 22:05:45 -0000	1.29
--- tracepoint.h	26 Mar 2010 01:43:59 -0000
*************** enum trace_stop_reason
*** 73,79 ****
      tstop_command,
      trace_buffer_full,
      trace_disconnected,
!     tracepoint_passcount
    };
  
  struct trace_status
--- 73,80 ----
      tstop_command,
      trace_buffer_full,
      trace_disconnected,
!     tracepoint_passcount,
!     tracepoint_error
    };
  
  struct trace_status
*************** struct trace_status
*** 89,98 ****
  
    enum trace_stop_reason stop_reason;
  
!   /* If stop_reason == tracepoint_passcount, the on-target number
!      of the tracepoint which caused the stop.  */
    int stopping_tracepoint;
  
    /* Number of traceframes currently in the buffer.  */
  
    int traceframe_count;
--- 90,104 ----
  
    enum trace_stop_reason stop_reason;
  
!   /* If stop_reason is tracepoint_passcount or tracepoint_error, this
!      is the (on-target) number of the tracepoint which caused the
!      stop.  */
    int stopping_tracepoint;
  
+   /* If stop_reason is tracepoint_error, this is a human-readable
+      string that describes the error that happened on the target.  */
+   char *error_desc;
+ 
    /* Number of traceframes currently in the buffer.  */
  
    int traceframe_count;
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.683
diff -p -r1.683 gdb.texinfo
*** doc/gdb.texinfo	24 Mar 2010 21:24:08 -0000	1.683
--- doc/gdb.texinfo	26 Mar 2010 01:44:00 -0000
*************** The trace stopped because @value{GDBN} d
*** 31362,31367 ****
--- 31362,31376 ----
  @item tpasscount:@var{tpnum}
  The trace stopped because tracepoint @var{tpnum} exceeded its pass count.
  
+ @item terror:@var{text}:@var{tpnum}
+ The trace stopped because tracepoint @var{tpnum} had an error.  The
+ string @var{text} is available to describe the nature of the error
+ (for instance, a divide by zero in the condition expression).
+ @var{text} may take either of two forms; it may be plain text, but
+ with the restriction that no colons or other special characters are
+ allowed, or it may be an @code{X} followed by hex digits encoding the
+ text string.
+ 
  @item tunknown:0
  The trace stopped for some other reason.
  
Index: testsuite/gdb.trace/tfile.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/tfile.c,v
retrieving revision 1.1
diff -p -r1.1 tfile.c
*** testsuite/gdb.trace/tfile.c	15 Jan 2010 22:37:20 -0000	1.1
--- testsuite/gdb.trace/tfile.c	26 Mar 2010 01:44:00 -0000
*************** finish_trace_file (int fd)
*** 41,49 ****
  }
  
  void
! write_basic_trace_file ()
  {
!   int fd;
  
    fd = start_trace_file ("basic.tf");
  
--- 41,51 ----
  }
  
  void
! write_basic_trace_file (void)
  {
!   int fd, int_x;
!   short short_x;
!   long long ll_x;
  
    fd = start_trace_file ("basic.tf");
  
*************** write_basic_trace_file ()
*** 61,66 ****
--- 63,69 ----
  
    /* Dump tracepoint definitions, in syntax similar to that used
       for reconnection uploads.  */
+   /* FIXME need a portable way to print function address in hex */
    snprintf (spbuf, sizeof spbuf, "tp T1:%lx:E:0:0\n",
  	    (long) &write_basic_trace_file);
    write (fd, spbuf, strlen (spbuf));
*************** write_basic_trace_file ()
*** 71,98 ****
    write (fd, "\n", 1);
  
    /* Make up a simulated trace buffer.  */
!   /* (Encapsulate better if we're going to do lots of this.) */
    trptr = trbuf;
!   *((short *) trptr) = 1;
!   trptr += sizeof (short);
    tfsizeptr = trptr;
!   trptr += sizeof (int);
    *((char *) trptr) = 'M';
    trptr += 1;
!   *((long long *) trptr) = (long) &testglob;
    trptr += sizeof (long long);
!   *((short *) trptr) = sizeof (testglob);
!   trptr += sizeof (short);
!   *((int *) trptr) = testglob;
    trptr += sizeof (testglob);
    /* Go back and patch in the frame size.  */
!   *((int *) tfsizeptr) = trptr - tfsizeptr - sizeof (int);
  
    /* Write end of tracebuffer marker.  */
!   *((short *) trptr) = 0;
!   trptr += sizeof (short);
!   *((int *) trptr) = 0;
!   trptr += sizeof (int);
  
    write (fd, trbuf, trptr - trbuf);
  
--- 74,146 ----
    write (fd, "\n", 1);
  
    /* Make up a simulated trace buffer.  */
!   /* (Encapsulate better if we're going to do lots of this; note that
!      buffer endianness is the target program's enddianness.) */
    trptr = trbuf;
!   short_x = 1;
!   memcpy (trptr, &short_x, 2);
!   trptr += 2;
    tfsizeptr = trptr;
!   trptr += 4;
    *((char *) trptr) = 'M';
    trptr += 1;
!   ll_x = (long) &testglob;
!   memcpy (trptr, &ll_x, sizeof (long long));
    trptr += sizeof (long long);
!   short_x = sizeof (testglob);
!   memcpy (trptr, &short_x, 2);
!   trptr += 2;
!   memcpy (trptr, &testglob, sizeof (testglob));
    trptr += sizeof (testglob);
    /* Go back and patch in the frame size.  */
!   int_x = trptr - tfsizeptr - sizeof (int);
!   memcpy (tfsizeptr, &int_x, 4);
! 
!   /* Write end of tracebuffer marker.  */
!   memset (trptr, 0, 6);
!   trptr += 6;
! 
!   write (fd, trbuf, trptr - trbuf);
! 
!   finish_trace_file (fd);
! }
! 
! void
! write_error_trace_file (void)
! {
!   int fd;
! 
!   fd = start_trace_file ("error.tf");
! 
!   /* The next part of the file consists of newline-separated lines
!      defining status, tracepoints, etc.  The section is terminated by
!      an empty line.  */
! 
!   /* Dump the size of the R (register) blocks in traceframes.  */
!   snprintf (spbuf, sizeof spbuf, "R %x\n", 500 /* FIXME get from arch */);
!   write (fd, spbuf, strlen (spbuf));
! 
!   /* Dump trace status, in the general form of the qTstatus reply.  */
!   snprintf (spbuf, sizeof spbuf, "status 0;terror:made-up error:1;tframes:0;tcreated:0;tfree:100;tsize:1000\n");
!   write (fd, spbuf, strlen (spbuf));
! 
!   /* Dump tracepoint definitions, in syntax similar to that used
!      for reconnection uploads.  */
!   /* FIXME need a portable way to print function address in hex */
!   snprintf (spbuf, sizeof spbuf, "tp T1:%lx:E:0:0\n",
! 	    (long) &write_basic_trace_file);
!   write (fd, spbuf, strlen (spbuf));
!   /* (Note that we would only need actions defined if we wanted to
!      test tdump.) */
! 
!   /* Empty line marks the end of the definition section.  */
!   write (fd, "\n", 1);
! 
!   trptr = trbuf;
  
    /* Write end of tracebuffer marker.  */
!   memset (trptr, 0, 6);
!   trptr += 6;
  
    write (fd, trbuf, trptr - trbuf);
  
*************** main (int argc, char **argv, char **envp
*** 109,114 ****
--- 157,164 ----
  {
    write_basic_trace_file ();
  
+   write_error_trace_file ();
+ 
    done_making_trace_files ();
  
    return 0;
Index: testsuite/gdb.trace/tfile.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/tfile.exp,v
retrieving revision 1.3
diff -p -r1.3 tfile.exp
*** testsuite/gdb.trace/tfile.exp	24 Mar 2010 21:11:06 -0000	1.3
--- testsuite/gdb.trace/tfile.exp	26 Mar 2010 01:44:00 -0000
*************** if { [gdb_compile "$srcdir/$subdir/$srcf
*** 47,53 ****
  gdb_reinitialize_dir $srcdir/$subdir
  
  # Make sure we are starting fresh.
! remote_exec build {sh -xc rm\ -f\ basic.tf}
  
  gdb_load $binfile
  
--- 47,54 ----
  gdb_reinitialize_dir $srcdir/$subdir
  
  # Make sure we are starting fresh.
! remote_file host delete basic.tf
! remote_file host delete error.tf
  
  gdb_load $binfile
  
*************** Trace buffer has 256 bytes of 4096 bytes
*** 83,89 ****
--- 84,102 ----
  Looking at trace frame 0, tracepoint .*" \
      "tstatus on trace file"
  
+ # Now start afresh, using only a trace file.
  
+ gdb_exit
+ gdb_start
  
+ gdb_load $binfile
  
+ gdb_test "target tfile error.tf" "Created tracepoint.*" "target tfile"
  
+ gdb_test "tstatus" \
+     "Using a trace file.*
+ Trace stopped by an error \\(made-up error, tracepoint 1\\).*
+ Collected 0 trace frame.*
+ Trace buffer has 256 bytes of 4096 bytes free \\(93% full\\).*
+ Not looking at any trace frame.*" \
+     "tstatus on error trace file"