[PATCH/commit] Handle errors in tracepoint target agent

[PATCH/commit] Handle errors in tracepoint target agent

[Stan Shebs]

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

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.


[-- Attachment #2: terror-patch-2 --]
[-- Type: text/plain, Size: 13342 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	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"

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Pedro Alves]

On Friday 26 March 2010 01:55:50, Stan Shebs wrote:
> For the record, here's what I ended up committing.  

(For the record, if a patch ends up different to how it was
being discussed, a chance for commenting before checking in
would be really appreciated.)

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

I don't find that hex2bin throwing is a good argument to not
do it the way it was suggested.  It's should be quite easy
to check if the string is hexencoded even without hex2bin,
before calling it.  There's a limited set of hex
characters.  :-)

I don't find that hex2bin throwing is a good argument to not
do it the way it was suggested.  It's should be quite easy
to check if the string is hexencoded even without hex2bin,
before calling it.  There's a limited set of hex
characters.  :-)

Here's a patch that works against our tree:

---
 gdb/tracepoint.c |   34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2010-03-26 09:34:34.000000000 +0000
+++ src/gdb/tracepoint.c	2010-03-26 10:35:13.000000000 +0000
@@ -3301,9 +3301,37 @@ Packet: '%s'\n"), p, line);
       else if (strncmp (p, stop_reason_names[tracepoint_error], p1 - p) == 0)
 	{
 	  p2 = strchr (++p1, ':');
-	  ts->error_desc = (char *) xmalloc (p2 - p1 + 1);
-	  memcpy (ts->error_desc, p1, p2 - p1);
-	  ts->error_desc[p2 - p1] = '\0';
+
+	  if (p2 != p1)
+	    {
+	      char *p;
+	      int end;
+
+	      ts->error_desc = xmalloc (p2 - p1 + 1);
+
+	      /* In the original implementation of this optional
+		 field, the string was not hex encoded.  Allow that
+		 for compatibility.  New targets should always hex
+		 encode.  */
+	      for (p = p1; p < p2; p++)
+		if (!((*p >= '0' && *p <= '9')
+		      || (*p >= 'a' && *p <= 'f')
+		      || (*p >= 'A' && *p <= 'F')))
+		  break;
+
+	      if (p != p2)
+		{
+		  memcpy (ts->error_desc, p1, p2 - p1);
+		  end = p2 - p1;
+		}
+	      else
+		{
+		  end = hex2bin (p1, ts->error_desc, (p2 - p1) / 2);
+		}
+
+	      ts->error_desc[end] = '\0';
+	    }
+
 	  p = unpack_varlen_hex (++p2, &val);
 	  ts->stopping_tracepoint = val;
 	  ts->stop_reason = tracepoint_error;


I tried that both against the stub that didn't hex
encode, and then made it hex encode and confirmed it
still worked fine.


If you really insist in handling this in FSF gdb as well,
then I'd like to merge this patch above to head, otherwise, 
I'd rather just remove all the plain string handling from
FSF gdb, and put that patch in our tree only.  (I don't
really see the point in carrying that workaround forever in
FSF gdb).

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

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

We should never be advertizing the broken version in the
manual.  This doesn't even say which version _should_ preferably
be used.  "what's" special characters?  (Why can't now an error
string start with X?)  Why give users/stub writers a way to shoot
their own feet?

I'd much rather remove the description of the non-hex encoded
string form from the manual as well.  Here's a patch for that.

-- 
Pedro Alves

2010-03-26  Pedro Alves  <pedro@codesourcery.com>

	gdb/doc/
	* gdb.texinfo (Tracepoint Packets): Remove mention that
	terror:string may be plain text, and drop mention of X prefix.

---
 gdb/doc/gdb.texinfo |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2010-03-26 10:51:57.000000000 +0000
+++ src/gdb/doc/gdb.texinfo	2010-03-26 10:54:39.000000000 +0000
@@ -31366,10 +31366,7 @@ The trace stopped because tracepoint @va
 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.
+@var{text} is hex encoded.
 
 @item tunknown:0
 The trace stopped for some other reason.

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Pedro Alves]

On Friday 26 March 2010 01:55:50, Stan Shebs wrote:
>  The error message is never NULL, I added the MI 
> status bit overlooked before, 

But now, `error_desc' is leaking in the case you xmalloc it.
When you fix that leak, you'll stumble of the fact that
in some cases you have this:

 ts->error_desc = ""

while in others you have this:

 ts->error_desc = (char *) xmalloc (p2 - p1 + 1);

How can you reliably know if you may xfree
ts->error_desc that way?  xfree ("") is a no go.
Comparing a pointer with "" is also not good.

-- 
Pedro Alves

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Stan Shebs]

Pedro Alves wrote:
> On Friday 26 March 2010 01:55:50, Stan Shebs wrote:
>   
>> For the record, here's what I ended up committing.  
>>     
>
> (For the record, if a patch ends up different to how it was
> being discussed, a chance for commenting before checking in
> would be really appreciated.)
>   
It seemed uncontroversial, but I guess not. :-)
> +	      for (p = p1; p < p2; p++)
> +		if (!((*p >= '0' && *p <= '9')
> +		      || (*p >= 'a' && *p <= 'f')
> +		      || (*p >= 'A' && *p <= 'F')))
> +		  break;
>   
Oh triple yuck.  Let's just make a new utility function - an ishex-type 
test shouldn't be coded more than once, or maybe twice, in a program.
> If you really insist in handling this in FSF gdb as well,
> then I'd like to merge this patch above to head, otherwise, 
> I'd rather just remove all the plain string handling from
> FSF gdb, and put that patch in our tree only.  (I don't
> really see the point in carrying that workaround forever in
> FSF gdb).
>   
Alright alright.  My interest in the subject is now totally exhausted, 
we'll just do the hex strings here.

Stan

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Stan Shebs]

Pedro Alves wrote:
> On Friday 26 March 2010 01:55:50, Stan Shebs wrote:
>   
>>  The error message is never NULL, I added the MI 
>> status bit overlooked before, 
>>     
>
> But now, `error_desc' is leaking in the case you xmalloc it.
>   
It was leaking before. :-)  It needs to stick around indefinitely; it 
guess it would work to free an old one just before allocating a new one.
> When you fix that leak, you'll stumble of the fact that
> in some cases you have this:
>
>  ts->error_desc = ""
>
> while in others you have this:
>
>  ts->error_desc = (char *) xmalloc (p2 - p1 + 1);
>   
No, because the p2 != p1 test prior ensures we only allocate non-empty 
strings.
> How can you reliably know if you may xfree
> ts->error_desc that way?  xfree ("") is a no go.
> Comparing a pointer with "" is also not good.
>   
strlen.

Stan

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Pedro Alves]

On Friday 26 March 2010 12:40:42, Stan Shebs wrote:

> My interest in the subject is now totally exhausted, 
> we'll just do the hex strings here.

Thanks.  I've applied the patch below.

-- 
Pedro Alves

2010-03-26  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* tracepoint.c (parse_trace_status): Don't allow plain strings in
	the terror description.  Don't expect an X prefix.

	gdb/doc/
	* gdb.texinfo (Tracepoint Packets): Remove mention that
	terror:string may be plain text, and drop mention of X prefix.

---
 gdb/doc/gdb.texinfo |    5 +----
 gdb/tracepoint.c    |   15 +++------------
 2 files changed, 4 insertions(+), 16 deletions(-)

Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2010-03-26 12:54:25.000000000 +0000
+++ src/gdb/doc/gdb.texinfo	2010-03-26 12:54:30.000000000 +0000
@@ -31366,10 +31366,7 @@ The trace stopped because tracepoint @va
 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.
+@var{text} is hex encoded.
 
 @item tunknown:0
 The trace stopped for some other reason.
Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2010-03-26 12:54:33.000000000 +0000
+++ src/gdb/tracepoint.c	2010-03-26 12:57:00.000000000 +0000
@@ -3197,18 +3197,9 @@ Status line: '%s'\n"), p, line);
 	  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 = xmalloc ((p2 - p1) / 2 + 1);
+	      end = hex2bin (p1, ts->error_desc, (p2 - p1) / 2);
 	      ts->error_desc[end] = '\0';
 	    }
 	  p = unpack_varlen_hex (++p2, &val);

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Pedro Alves]

On Friday 26 March 2010 13:38:36, Pedro Alves wrote:
> On Friday 26 March 2010 12:40:42, Stan Shebs wrote:
> 
> > My interest in the subject is now totally exhausted, 
> > we'll just do the hex strings here.
> 
> Thanks.  I've applied the patch below.

I'm applying this one too.  I didn't notice these, because
I ran the tests against gdbserver, and tfile.exp is skipped
if nofileio is set.

-- 
Pedro Alves
2010-03-26  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* tracepoint.c (trace_save): Remove X from tracepoint error
	description.

	gdb/testsuite/
	* gdb.trace/tfile.c (tohex, bin2hex): New.
	(write_error_trace_file): Hexify error description.

---
 gdb/testsuite/gdb.trace/tfile.c |   35 +++++++++++++++++++++++++++++++++--
 gdb/tracepoint.c                |    3 +--
 2 files changed, 34 insertions(+), 4 deletions(-)

Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2010-03-26 13:59:21.000000000 +0000
+++ src/gdb/tracepoint.c	2010-03-26 14:01:16.000000000 +0000
@@ -2484,12 +2484,11 @@ trace_save (const char *filename, int ta
   /* 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, ":%s", buf);
     }
   fprintf (fp, ":%x", ts->stopping_tracepoint);
   if (ts->traceframe_count >= 0)
Index: src/gdb/testsuite/gdb.trace/tfile.c
===================================================================
--- src.orig/gdb/testsuite/gdb.trace/tfile.c	2010-03-26 14:04:23.000000000 +0000
+++ src/gdb/testsuite/gdb.trace/tfile.c	2010-03-26 14:10:28.000000000 +0000
@@ -105,10 +105,38 @@ write_basic_trace_file (void)
   finish_trace_file (fd);
 }
 
+/* Convert number NIB to a hex digit.  */
+
+static int
+tohex (int nib)
+{
+  if (nib < 10)
+    return '0' + nib;
+  else
+    return 'a' + nib - 10;
+}
+
+int
+bin2hex (const char *bin, char *hex, int count)
+{
+  int i;
+
+  for (i = 0; i < count; i++)
+    {
+      *hex++ = tohex ((*bin >> 4) & 0xf);
+      *hex++ = tohex (*bin++ & 0xf);
+    }
+  *hex = 0;
+  return i;
+}
+
 void
 write_error_trace_file (void)
 {
   int fd;
+  const char made_up[] = "made-up error";
+  int len = sizeof (made_up) - 1;
+  char *hex = alloca (len * 2 + 1);
 
   fd = start_trace_file ("error.tf");
 
@@ -120,11 +148,14 @@ write_error_trace_file (void)
   snprintf (spbuf, sizeof spbuf, "R %x\n", 500 /* FIXME get from arch */);
   write (fd, spbuf, strlen (spbuf));
 
+  bin2hex (made_up, hex, len);
+
   /* 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");
+	    "terror:%s:1;"
+	    "tframes:0;tcreated:0;tfree:100;tsize:1000\n",
+	    hex);
   write (fd, spbuf, strlen (spbuf));
 
   /* Dump tracepoint definitions, in syntax similar to that used

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Pedro Alves]

(could you please remember to put an empty line between
quotes and replies, so everyone else's eyes don't get
hurt?  Thanks in advance.  :-) )

On Friday 26 March 2010 12:50:23, Stan Shebs wrote:
> > But now, `error_desc' is leaking in the case you xmalloc it.
> >   
> It was leaking before. :-)  It needs to stick around indefinitely; it 
> guess it would work to free an old one just before allocating a new one.
> > When you fix that leak, you'll stumble of the fact that

I meant now, compared to before any patch was applied, of
course.  Word games are unfair!  :-)

> > while in others you have this:
> >
> >  ts->error_desc = (char *) xmalloc (p2 - p1 + 1);
> >   
> No, because the p2 != p1 test prior ensures we only allocate non-empty 
> strings.
> > How can you reliably know if you may xfree
> > ts->error_desc that way?  xfree ("") is a no go.
> > Comparing a pointer with "" is also not good.
> >   
> strlen.

My turn to triple yuck.  :-)

Would you object to this instead?

error_desc should only ever be accessed if the stopping reason
is tracepoint_error.

I've checked that all callers of parse_trace_status
currently only pass it the global status object.  If we
end up building an object of this type on the stack, we'll
need to memset it.  That would also be a requirement if we
checked for strlen instead..

-- 
Pedro Alves
2010-03-26  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* tracepoint.c (current_trace_status): Don't make sure error_desc
	is non-NULL here.
	(parse_trace_status): Release a previous error_desc string, and
	set it to NULL by default.  If stop reason is tracepoint_error, make
	sure error_desc is not left NULL.

---
 gdb/tracepoint.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2010-03-26 14:32:01.000000000 +0000
+++ src/gdb/tracepoint.c	2010-03-26 14:38:31.000000000 +0000
@@ -202,9 +202,6 @@ char *stop_reason_names[] = {
 struct trace_status *
 current_trace_status ()
 {
-  /* Ensure this is never NULL.  */
-  if (!trace_status.error_desc)
-    trace_status.error_desc = "";
   return &trace_status;
 }
 
@@ -3157,7 +3154,8 @@ parse_trace_status (char *line, struct t
   ts->running_known = 1;
   ts->running = (*p++ == '1');
   ts->stop_reason = trace_stop_reason_unknown;
-  ts->error_desc = "";
+  xfree (ts->error_desc);
+  ts->error_desc = NULL;
   ts->traceframe_count = -1;
   ts->traceframes_created = -1;
   ts->buffer_free = -1;
@@ -3201,6 +3199,9 @@ Status line: '%s'\n"), p, line);
 	      end = hex2bin (p1, ts->error_desc, (p2 - p1) / 2);
 	      ts->error_desc[end] = '\0';
 	    }
+	  else
+	    ts->error_desc = xstrdup ("");
+
 	  p = unpack_varlen_hex (++p2, &val);
 	  ts->stopping_tracepoint = val;
 	  ts->stop_reason = tracepoint_error;

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Stan Shebs]

Pedro Alves wrote:
> (could you please remember to put an empty line between
> quotes and replies, so everyone else's eyes don't get
> hurt?  Thanks in advance.  :-) )
>   
(I wasn't?)
> Would you object to this instead?
>   
This is fine with me, and thanks for keeping an eye out on all this!

Stan

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Eli Zaretskii]

> Date: Fri, 26 Mar 2010 08:19:50 -0700
> From: Stan Shebs <stan@codesourcery.com>
> CC: Stan Shebs <stan@codesourcery.com>, gdb-patches@sourceware.org
> 
> Pedro Alves wrote:
> > (could you please remember to put an empty line between
> > quotes and replies, so everyone else's eyes don't get
> > hurt?  Thanks in advance.  :-) )
> >   
> (I wasn't?)

No, and it seems to be the side effect of using Thunderbird (I had
similar experience with another user of it).

Re: [PATCH/commit] Handle errors in tracepoint target agent

[Daniel Jacobowitz]

On Fri, Mar 26, 2010 at 08:19:50AM -0700, Stan Shebs wrote:
> Pedro Alves wrote:
> >(could you please remember to put an empty line between
> >quotes and replies, so everyone else's eyes don't get
> >hurt?  Thanks in advance.  :-) )
> (I wasn't?)
> >Would you object to this instead?
> This is fine with me, and thanks for keeping an eye out on all this!
> 

You still aren't...

-- 
Daniel Jacobowitz
CodeSourcery