This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
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