[PATCH] Handle errors in tracepoint target agent
Stan Shebs
stan@codesourcery.com
Thu Mar 25 22:30:00 GMT 2010
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
More information about the Gdb-patches
mailing list