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

[Pedro Alves <pedro at codesourcery dot com>]

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.