Re: [PATCH] Handle errors in tracepoint target agent

[Stan Shebs <stan@codesourcery.com>]

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