From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18628 invoked by alias); 25 Mar 2010 22:30:48 -0000 Received: (qmail 18616 invoked by uid 22791); 25 Mar 2010 22:30:46 -0000 X-SWARE-Spam-Status: No, hits=-2.4 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 22:30:39 +0000 Received: (qmail 20065 invoked from network); 25 Mar 2010 22:30:37 -0000 Received: from unknown (HELO macbook-2.local) (stan@127.0.0.2) by mail.codesourcery.com with ESMTPA; 25 Mar 2010 22:30:37 -0000 Message-ID: <4BABE407.30609@codesourcery.com> Date: Thu, 25 Mar 2010 22:30:00 -0000 From: Stan Shebs User-Agent: Thunderbird 2.0.0.24 (Macintosh/20100228) MIME-Version: 1.0 To: Pedro Alves CC: gdb-patches@sourceware.org, Stan Shebs Subject: Re: [PATCH] Handle errors in tracepoint target agent References: <4BAAB2E9.7020708@codesourcery.com> <201003251027.14862.pedro@codesourcery.com> In-Reply-To: <201003251027.14862.pedro@codesourcery.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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/msg00865.txt.bz2 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