Re: [PATCH] Handle errors in tracepoint target agent

[Pedro Alves <pedro@codesourcery.com>]

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.

> 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.

> +         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,

> !   fprintf (fp, "status %c;%s%s%s:%x",
>            (ts->running ? '1' : '0'),
> !          stop_reason_names[ts->stop_reason],
> !          (ts->stop_reason == tracepoint_error ? ":" : ""),
> !          (ts->stop_reason == tracepoint_error ? ts->error_desc : ""),

But this seems to.  The parsing code (below) seems to cope with a non-existent
description, and leaves the field NULL in that case, so it seems that
the "status %c..." print above should cope with it as well.

> +       else if (strncmp (p, stop_reason_names[tracepoint_error], p1 - p) == 0)
> +       {
> +         p2 = strchr (++p1, ':');
> +         if (p2 != p1)
> +           {
> +             ts->error_desc = (char *) xmalloc (p2 - p1 + 1);
> +             memcpy (ts->error_desc, p1, p2 - p1);
> +             ts->error_desc[p2 - p1] = '\0';


>   void
> + write_error_trace_file ()
                          ^^^^

"(void)" please.

> +   /* 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.

> +   /* Dump tracepoint definitions, in syntax similar to that used
> +      for reconnection uploads.  */
> +   snprintf (spbuf, sizeof spbuf, "tp T1:%lx:E:0:0\n",
> +           (long) &write_basic_trace_file);

This will fail to compile with "warning: cast to integer from pointer
of different size" on some archs.

> +   /* 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.

>   # Make sure we are starting fresh.
>   remote_exec build {sh -xc rm\ -f\ basic.tf}
> + remote_exec build {sh -xc rm\ -f\ error.tf}

These should be doing "remote_file host delete ${filefoo}"
instead.  As is won't work with remote host testing, where
the .tf files are produced on host, not on build.

-- 
Pedro Alves