From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17429 invoked by alias); 26 Mar 2010 11:13:50 -0000 Received: (qmail 17420 invoked by uid 22791); 26 Mar 2010 11:13:49 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 26 Mar 2010 11:13:45 +0000 Received: (qmail 15044 invoked from network); 26 Mar 2010 11:13:43 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 26 Mar 2010 11:13:43 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [PATCH/commit] Handle errors in tracepoint target agent Date: Fri, 26 Mar 2010 11:13:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-20-generic; KDE/4.3.2; x86_64; ; ) Cc: Stan Shebs References: <4BAC1426.5050003@codesourcery.com> In-Reply-To: <4BAC1426.5050003@codesourcery.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201003261113.40865.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-03/txt/msg00876.txt.bz2 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 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.