[PATCH] Handle errors in tracepoint target agent

[PATCH] Handle errors in tracepoint target agent

[Stan Shebs]

[-- 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"

Re: [PATCH] Handle errors in tracepoint target agent

[Eli Zaretskii]

> Date: Wed, 24 Mar 2010 17:48:41 -0700
> From: Stan Shebs <stan@codesourcery.com>
> 
>     * gdb.texinfo (Tracepoint Packets): Document trace error status.

This part is fine.  Thanks.

Re: [PATCH] Handle errors in tracepoint target agent

[Pedro Alves]

On Thursday 25 March 2010 00:48:41, Stan Shebs wrote:

> 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.

IMO, we should hex encode the string, like we do for other
protocol transfered user visible strings.

> 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?

IIRC, we've had similar issues before, and the decision tends to be to
fix the mistake in the FSF version, and worry about backwards compatility
in the local versions (Ericsson's gdb, in this case).  I think this should
work: try to hex2bin the string, and if that doesn't consume the whole
buffer up to ':', then we have a non-hex encoded string.  We're still
adding features for Ericsson, so with time there's good chance the
old version will disappear from their systems.

> +         if (ts->stopping_tracepoint)
> +           printf_filtered (_("Trace stopped by an error (%s, tracepoint %d).\n"),
> +                            (ts->error_desc ? ts->error_desc : ""),

This doesn't assume error_desc is always non-NULL,

> !   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 : ""),

But this seems to.  The parsing code (below) seems to cope with a non-existent
description, and leaves the field NULL in that case, so it seems that
the "status %c..." print above should cope with it as well.

> +       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';


>   void
> + write_error_trace_file ()
                          ^^^^

"(void)" please.

> +   /* 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));

FIXME?  If not important, or if this just need to be as big enough
to cope with all supported archs, then we should drop the FIXME note,
and explain that instead.

> +   /* 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);

This will fail to compile with "warning: cast to integer from pointer
of different size" on some archs.

> +   /* Write end of tracebuffer marker.  */
> +   *((short *) trptr) = 0;
> +   trptr += sizeof (short);
> +   *((int *) trptr) = 0;
> +   trptr += sizeof (int);

Please don't add code like this.  These casts and
dereferences tend to work on x86, because that arch can
write to unaligned addresses.  Most others can't.  This should
do memset instead.  Assuming short is 2 bytes and int is 4
isn't supper portable either, but it will be a while longer
to trip on that.

>   # Make sure we are starting fresh.
>   remote_exec build {sh -xc rm\ -f\ basic.tf}
> + remote_exec build {sh -xc rm\ -f\ error.tf}

These should be doing "remote_file host delete ${filefoo}"
instead.  As is won't work with remote host testing, where
the .tf files are produced on host, not on build.

-- 
Pedro Alves

Re: [PATCH] Handle errors in tracepoint target agent

[Stan Shebs]

Pedro Alves wrote:
> On Thursday 25 March 2010 00:48:41, Stan Shebs wrote:
>
>   
>> 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.
>>     
>
> IMO, we should hex encode the string, like we do for other
> protocol transfered user visible strings.
>   
Yeah, it's the safe thing to do.  It would be a minor convenience to 
have the info be readable text, so you could understand it at a glance 
when looking at the raw packet or trace file.
>> 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?
>>     
>
> IIRC, we've had similar issues before, and the decision tends to be to
> fix the mistake in the FSF version, and worry about backwards compatility
> in the local versions (Ericsson's gdb, in this case).  I think this should
> work: try to hex2bin the string, and if that doesn't consume the whole
> buffer up to ':', then we have a non-hex encoded string.  We're still
> adding features for Ericsson, so with time there's good chance the
> old version will disappear from their systems.
>   
And hopefully no one returns "deadbeef" as their error message. :-)
>   
>> +         if (ts->stopping_tracepoint)
>> +           printf_filtered (_("Trace stopped by an error (%s, tracepoint %d).\n"),
>> +                            (ts->error_desc ? ts->error_desc : ""),
>>     
>
> This doesn't assume error_desc is always non-NULL,
>   
A last-minute waffle - alway non-NULL is a reasonable assumption in this 
context, but I always worry that a devious programmer will get around it 
somehow. :-)
>> +   /* 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));
>>     
>
> FIXME?  If not important, or if this just need to be as big enough
> to cope with all supported archs, then we should drop the FIXME note,
> and explain that instead.
>   
I was thinking somebody might know of a clever way for the compiler to 
get it into the test program...
>> +   /* Write end of tracebuffer marker.  */
>> +   *((short *) trptr) = 0;
>> +   trptr += sizeof (short);
>> +   *((int *) trptr) = 0;
>> +   trptr += sizeof (int);
>>     
>
> Please don't add code like this.  These casts and
> dereferences tend to work on x86, because that arch can
> write to unaligned addresses.  Most others can't.  This should
> do memset instead.  Assuming short is 2 bytes and int is 4
> isn't supper portable either, but it will be a while longer
> to trip on that.
>   
The file format prescribes exact sizes for its elements, so memset is a 
good idea.

Stan

Re: [PATCH] Handle errors in tracepoint target agent

[Pedro Alves]

On Thursday 25 March 2010 22:30:31, Stan Shebs wrote:
> > FIXME?  If not important, or if this just need to be as big enough
> > to cope with all supported archs, then we should drop the FIXME note,
> > and explain that instead.
> >   
> I was thinking somebody might know of a clever way for the compiler to 
> get it into the test program...

You could pass it down to gdb_compile as an extra #define
from the .exp, using additional_flags for example:

 additional_flags=-DREGBLOCK_SIZE=$regblocksize

Is there a way to query GDB for this size, so the
.exp can get at it?  If not, maybe we can add a maintenance
command for this?  A fixed size per target doesn't
quite cut it, considering extended register sets, and
target descriptions.

-- 
Pedro Alves

Re: [PATCH] Handle errors in tracepoint target agent

[Stan Shebs]

Pedro Alves wrote:
> On Thursday 25 March 2010 22:30:31, Stan Shebs wrote:
>   
>>> FIXME?  If not important, or if this just need to be as big enough
>>> to cope with all supported archs, then we should drop the FIXME note,
>>> and explain that instead.
>>>   
>>>       
>> I was thinking somebody might know of a clever way for the compiler to 
>> get it into the test program...
>>     
>
> You could pass it down to gdb_compile as an extra #define
> from the .exp, using additional_flags for example:
>
>  additional_flags=-DREGBLOCK_SIZE=$regblocksize
>
> Is there a way to query GDB for this size, so the
> .exp can get at it?  If not, maybe we can add a maintenance
> command for this?  A fixed size per target doesn't
> quite cut it, considering extended register sets, and
> target descriptions.
>   
Ugh. Given that this is a file format, and more particularly a type of 
file that we could get handed by users, I'm thinking I want to go back 
and tweak GDB to not care about its value much, and only require it to 
match the size of any fixed-size register blocks present in the trace file.

Stan