From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18528 invoked by alias); 25 Mar 2010 10:27:24 -0000 Received: (qmail 18518 invoked by uid 22791); 25 Mar 2010 10:27:23 -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; Thu, 25 Mar 2010 10:27:20 +0000 Received: (qmail 21626 invoked from network); 25 Mar 2010 10:27:18 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 25 Mar 2010 10:27:18 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [PATCH] Handle errors in tracepoint target agent Date: Thu, 25 Mar 2010 10:27:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-20-generic; KDE/4.3.2; x86_64; ; ) Cc: Stan Shebs References: <4BAAB2E9.7020708@codesourcery.com> In-Reply-To: <4BAAB2E9.7020708@codesourcery.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201003251027.14862.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/msg00844.txt.bz2 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