Re: [patch] Fix PR 13392 : check offset of JMP insn

[Pedro Alves <palves@redhat.com>]

On 03/07/2012 08:24 AM, Yao Qi wrote:

> On 03/07/2012 01:03 AM, Pedro Alves wrote:

>> > send a clearer error back to GDB, and we end up with:

>> > 
>> > Target returns error code '.Tracepoint 2 at 0x7ffff67c2579 already exists'.
> It is OK for me to let remote target to reports a duplicated tracepoint.


I don't know what you mean then.  It already does, but with a cryptic E01.

> In this version, some changes compared with last one,
> 
>   - move `tracepoint already exist' patch out, which can be a separate one,

>   - remove downloaded tracepoint in target when failed to install,

We should also check what happens when we do "enable TRACEPOINT_FOO", and
that tracepoints fails to be likewise inserted.  Following this direction,
we should make sure it stays disabled in the target, but _not_ deleted.

> +/* Remove TPOINT from global list.  */

> +
> +static void
> +remove_tracepoint (struct tracepoint *tpoint)
> +{
> +  struct tracepoint *tp, *tp_prev;
> +
> +  for (tp = tracepoints, tp_prev = NULL; tp && tp != tpoint;
> +       tp_prev = tp, tp = tp->next)
> +    ;
> +
> +  if (tp)
> +    {
> +      if (tp_prev)
> +	tp_prev->next = tp->next;
> +      else
> +	tracepoints->next = tp->next;


This doesn't look correct.  If there's no tp_prev, then TP must be
TRACEPOINTS, and so you need to do 'tracepoints = tp->next;'.

Or just rewrite this as:

static int
remove_tracepoint (struct tracepoint *todel)
{
  struct tracepoint *tp, **tp_link;

  tp = tracepoints;
  tp_link = &tracepoints;

  while (tp != NULL)
    {
      if (tp == todel)
	{
	  *tp_link = tp->next;

	  xfree (tp);
	  return;
	}

	tp_link = &tp->next;
	tp = *tp_link;
    }
}


>  /* There may be several tracepoints with the same number (because they
>     are "locations", in GDB parlance); return the next one after the
>     given tracepoint, or search from the beginning of the list if the
> @@ -2391,6 +2413,8 @@ cmd_qtdp (char *own_buf)
>  
>        download_tracepoint (tpoint);
>        install_tracepoint (tpoint, own_buf);
> +      if (strncmp (own_buf, "OK", 2) != 0)
> +	remove_tracepoint (tpoint);


own_buf is a \0 terminated string.  You can make that a straight strcmp(own_buf, "OK").
(Or change install_tracepoint's return type to int, to signal error.)

> +	-re "Target returns error code .* too far .*$gdb_prompt $" {
> +            if [string equal $trace_type "ftrace"] {
> +		# The target was unable to install the fast tracepoint
> +		# (e.g., jump pad too far from tracepoint).
> +		pass "$test (too far)"
> +		# Skip the rest of the tests.
> +                return


Can you check tab/spaces here, and in the similar places, please?  I may
have messed that up myself.

Otherwise okay.

-- 
Pedro Alves