[PATCH] Handle errors in tracepoint target agent

[Stan Shebs <stan@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

One of the downsides of the fancy-schmancy expression evaluation for 
tracepoints is that it could have plain old computational errors, such 
as a divide by zero, stack underflow, invalid memory reference, etc.  
Although it's conceivable that one might somehow want tracing to 
struggle on, more likely it's a sign of a mistake in the experimental 
setup, and the right thing is to simply give up.  This patch adds 
machinery for the target to report that it stopped due to an error and 
display any available info in tstatus.

But before committing, I want to get feedback on what to do about a 
protocol mistake I made with this one; the descriptive string is 
included verbatim in the status packet, which means that it can't have 
colons embedded, and probably other characters.  There are a couple ways 
to fix - encode the string in hex, or add a length prefix.  The problem 
is that this feature has been in Ericsson's hands for awhile, bolted 
into their application already I think, and so there is a compatibility 
issue.  We do have the list of strings that their agent returns, and we 
could say for instance that a leading numeric digit is assumed to 
indicate a length field, otherwise it's handled verbatim.  On the flip 
side, this all is getting rather arcane, so maybe I'm worrying too much 
about it.  What do people think?

Stan

2010-03-24  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.
    (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.
    * gdb.trace/tfile.exp: Test trace file with an error stop.


[-- Attachment #2: terror-patch-1 --]
[-- Type: text/plain, Size: 9267 bytes --]

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	25 Mar 2010 00:28:27 -0000
*************** char *stop_reason_names[] = {
*** 195,201 ****
    "tstop",
    "tfull",
    "tdisconnected",
!   "tpasscount"
  };
  
  struct trace_status *
--- 195,202 ----
    "tstop",
    "tfull",
    "tdisconnected",
!   "tpasscount",
!   "terror"
  };
  
  struct trace_status *
*************** trace_status_command (char *args, int fr
*** 1570,1575 ****
--- 1571,1585 ----
  	  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->error_desc : ""),
+ 			     ts->stopping_tracepoint);
+ 	  else
+ 	    printf_filtered (_("Trace stopped by an error (%s).\n"),
+ 			     (ts->error_desc ? ts->error_desc : ""));
+ 	  break;
  	case trace_stop_reason_unknown:
  	  printf_filtered (_("Trace stopped for an unknown reason.\n"));
  	  break;
*************** 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)
--- 2473,2484 ----
    fprintf (fp, "R %x\n", trace_regblock_size);
  
    /* Write out status of the tracing run (aka "tstatus" info).  */
!   fprintf (fp, "status %c;%s%s%s:%x",
  	   (ts->running ? '1' : '0'),
!  	   stop_reason_names[ts->stop_reason],
!  	   (ts->stop_reason == tracepoint_error ? ":" : ""),
!  	   (ts->stop_reason == tracepoint_error ? ts->error_desc : ""),
!  	   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;
--- 3139,3151 ----
  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 = NULL;
    ts->traceframe_count = -1;
    ts->traceframes_created = -1;
    ts->buffer_free = -1;
*************** Status line: '%s'\n"), p, line);
*** 3164,3169 ****
--- 3178,3196 ----
  	  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)
+ 	    {
+ 	      ts->error_desc = (char *) xmalloc (p2 - p1 + 1);
+ 	      memcpy (ts->error_desc, p1, p2 - p1);
+ 	      ts->error_desc[p2 - p1] = '\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	25 Mar 2010 00:28:27 -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	25 Mar 2010 00:28:28 -0000
*************** The trace stopped because @value{GDBN} d
*** 31362,31367 ****
--- 31362,31373 ----
  @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 not contain any colon characters.
+ 
  @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	25 Mar 2010 00:28:28 -0000
*************** write_basic_trace_file ()
*** 100,105 ****
--- 100,146 ----
  }
  
  void
+ write_error_trace_file ()
+ {
+   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.  */
+   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);
+ 
+   /* Write end of tracebuffer marker.  */
+   *((short *) trptr) = 0;
+   trptr += sizeof (short);
+   *((int *) trptr) = 0;
+   trptr += sizeof (int);
+ 
+   write (fd, trbuf, trptr - trbuf);
+ 
+   finish_trace_file (fd);
+ }
+ 
+ void
  done_making_trace_files (void)
  {
  }
*************** main (int argc, char **argv, char **envp
*** 109,114 ****
--- 150,157 ----
  {
    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	25 Mar 2010 00:28:28 -0000
*************** gdb_reinitialize_dir $srcdir/$subdir
*** 48,53 ****
--- 48,54 ----
  
  # Make sure we are starting fresh.
  remote_exec build {sh -xc rm\ -f\ basic.tf}
+ remote_exec build {sh -xc rm\ -f\ 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"