Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Eli Zaretskii <eliz at gnu dot org>]

> From: Luis Machado <luisgpm@linux.vnet.ibm.com>
> Date: Thu, 16 Aug 2007 18:02:58 -0300
> 
> This is a first try on the implementation of the additional "thread
> <thread_num>" parameter for the watchpoint command in order to make GDB
> stop only on specific threads when a watchpoint is triggered.

Thanks.

> Basically i started parsing the arguments backwards, trying to locate
> tokens that identify a "thread <thread_num>" command. After that i hand
> all the left parameters over to the default expression parser.

Your code assumes that <thread_num> is a literal number.  Do we want
to allow an integral expression there, or is a literal number good
enough?

In any case, this change, if and when accepted, should be accompanied
by a suitable change to the user manual.

Allow me a few comments on your code:

> +    toklen = strlen (arg); /* Size of argument list  */
> +
> +    /* Points tok to the end of the argument list  */
> +    tok = arg + toklen;

Did you try your code when the argument has trailing whitespace?
Unless I'm missing something, the last line above make `tok' point to
the null character that ends the argument, so the while loop after
that will end immediately, even if there is whitespace after the
thread ID number, and the whole parsing business will not work
correctly.

> +    while (strlen (tok) < toklen && (*tok == ' ' || *tok == '\t'))
> +      tok--;
> +    while (strlen (tok) < toklen && (*tok != ' ' && *tok != '\t'))
> +      tok--;
> +
> +    /* Go backwards in the parameters list. Skip one more parameter.
> +       If we're expecting a 'thread <thread_num>' parameter, we should
> +       reach a "thread" token.  */
> +    while (strlen (tok) < toklen && (*tok == ' ' || *tok == '\t'))
> +      tok--;
> +
> +    end_tok = tok;
> +
> +    while (strlen (tok) < toklen && (*tok != ' ' && *tok != '\t'))
> +      tok--;

You don't need to compute `strlen (tok)' every time through the loops,
because each iteration you go back exactly one character.  So you can
compute it only once, and then decrement its value inside the loop
bodies.

Alternatively, just check that you didn't yet get to the first
character of the argument:

	  while (tok > arg && (*tok != ' ' && *tok != '\t'))
	    tok--;

I like this latter alternative better.

Actually, given the trailing whitespace problem I mentioned above,
this is even better:

	  while (tok > arg && (tok[-1] != ' ' && tok[-1] != '\t'))
	    tok--;

(You will need to adjust the rest of code to bump `tok' one position
forward after you exit the loop, to account for the -1 above.)

> +      tmptok = tok;
> +      thread = strtol (tok, &tok, 0);
> +
> +      if (tok == tmptok)
> +        error (_("Incorrect parameter after thread keyword."));

There's a much better way of checking whether `tok' is a literal
number: look at the pointer returned by `strtol' in its 2nd arg, and
if it points to something other than a whitespace character, err out:

     char *endp;
     thread = strtol (tok, &endp, 0);
     if (*endp != ' ' && *endp != '\t')
       error (_("Invalid thread ID specification %s."), tok);

Thanks again for working on this.