This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Handle errors in tracepoint target agent


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]