Re: [PATCH 4/5] range stepping: gdbserver (x86 GNU/Linux)

[Pedro Alves <palves@redhat.com>]

On 05/14/2013 09:14 PM, Tom Tromey wrote:
> Pedro> +  /* Note that all addresses are always "out the step range" when
> Pedro> +     there's no range to begin with.  */
> 
> I think "out of the step range".

Thanks.  I fixed that one.

> Pedro> +      else if (p[0] == 'r')
> Pedro> +	{
> Pedro> +	  char *p1;
> Pedro> +
> Pedro> +	  p = p + 1;
> Pedro> +	  p1 = strchr (p, ',');
> Pedro> +	  decode_address (&resume_info[i].step_range_start, p, p1 - p);
> Pedro> +
> Pedro> +	  p = p1 + 1;
> Pedro> +	  p1 = strchr (p, ':');
> Pedro> +	  decode_address (&resume_info[i].step_range_end, p, p1 - p);
> Pedro> +
> Pedro> +	  p = p1;
> Pedro> +	}
> 
> I don't know much about gdbserver, but reading this made me wonder if it
> needs to do any kind of error-checking on its input.

Yeah, it doesn't tend to do that much validation.  Perhaps we should.

> Like - what if the wrong format is sent, 

Crash in many different places most likely.  When looking at
validation, I'm more looking at making it easier for possible
future packet extensions not break older gdbserver.

In this case, I do believe this bit:

+	  p1 = strchr (p, ':');
+	  decode_address (&resume_info[i].step_range_end, p, p1 - p);

should not expect the ':' to be there.  An action
without a ptid is valid. I means it applies to all and
is handled as the default action, further below:

      if (p[0] == 0)
	{
	  resume_info[i].thread = minus_one_ptid;
	  default_action = resume_info[i];

	  /* Note: we don't increment i here, we'll overwrite this entry
	     the next time through.  */
	}
      else if (p[0] == ':')

I'll fix it in a follow up.

> or if the range end is before the range start?

Yeah, considering that MIPS has signed addresses, I'm
leaving this one open.

-- 
Pedro Alves